Opened 6 months ago

Closed 2 weeks ago

#7350 closed change (duplicate)

Remove Page.prototype.sendMessage from stub code

Reported by: kzar Assignee:
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: greiner, sebastian Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by greiner)

Background

With #7271 we're getting rid of Page.prototype.sendMessage and calling browser.tabs.sendMessage directly. Therefore we can get rid of the Page.prototype.sendMessage stub from adblockplusui/background.js.

What to change

  • Remove Page.prototype.sendMessage from ext/common.js
  • Remove usage of Page.prototype.sendMessage in ext.devtools.onCreated listener in background.js

See also ui#357

Change History (6)

comment:1 Changed 6 months ago by greiner

  • Description modified (diff)

I agree that we should update adblockplusui/background.js accordingly but I'm not sure why we would need to wait for #7271. As you mentioned, Page.prototype.sendMessage is only being used in our mock and there we're not using the one provided by adblockpluschrome but our own implementation (see adblockplusui/ext/common.js).

Therefore I'd suggest removing that dependency.

comment:2 Changed 6 months ago by kzar

  • Blocked By 7271 removed

...Therefore I'd suggest removing that dependency.

Sounds good to me! Done.

comment:3 Changed 6 months ago by greiner

  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set

Thanks.

comment:4 follow-up: Changed 6 months ago by sebastian

The dependency would rather be the other way around, since we should remove Page.sendMessage() from adblockpluschrome/ext/background.js as soon as it is no longer used. However, we can do that once we update the adblockplusui dependency for this change. Please add an integration note, so that we don't forget.

Also while at it you might want to consider removing related logic from your ext mock implementation. Eventually, we want to get rid of ext entirely, and as less adblockplusui still relies on it, as easier will be the transition on our end.

comment:5 in reply to: ↑ 4 Changed 6 months ago by greiner

Replying to sebastian:

The dependency would rather be the other way around, since we should remove Page.sendMessage() from adblockpluschrome/ext/background.js as soon as it is no longer used.

As I mentioned, we're only using it in our mock environment and there we don't use adblockpluschrome's Page implementation. Therefore there's no dependency, as far as I'm aware of.

Also while at it you might want to consider removing related logic from your ext mock implementation. Eventually, we want to get rid of ext entirely, and as less adblockplusui still relies on it, as easier will be the transition on our end.

Good point. That part of the code has been causing confusion since the inception of the adblockplusui repo. Plus, there are only a few places left where we actually rely on it (i.e. ext.onMessage, ext.devtools.*). Therefore it should be somewhat easy to remove so I created #7358 to take care of that.

comment:6 Changed 2 weeks ago by greiner

  • Resolution set to duplicate
  • Status changed from new to closed

Closing this ticket in favor of ui#357.

Note: See TracTickets for help on using tickets.