Issue 3223 - Implement context menu fallback for SeaMonkey Mail and Thunderbird

Issue 3223 - Implement context menu fallback for SeaMonkey Mail and Thunderbird

[Erik, 2015-12-01 17:57:53]

On 2015/12/01 14:48:31, Wladimir Palant wrote:

I can't seem to figure out what this patch is based on.  The e10s and default
branches of the adblockplus hg repo seem to have a difference lib/ui.js file
then the one this diff is based on afaict.

Am I missing something?

[Erik, 2015-12-01 17:58:15]

On 2015/12/01 17:57:53, Erik wrote:
> On 2015/12/01 14:48:31, Wladimir Palant wrote:
> 
> I can't seem to figure out what this patch is based on.  The e10s and default
> branches of the adblockplus hg repo seem to have a difference lib/ui.js file
> then the one this diff is based on afaict.
> 
> Am I missing something?

s/difference/different

[Erik, 2015-12-01 18:01:12]

trying this publish feature out.

[Erik, 2015-12-01 18:50:07]

On 2015/12/01 17:57:53, Erik wrote:
> On 2015/12/01 14:48:31, Wladimir Palant wrote:
> 
> I can't seem to figure out what this patch is based on.  The e10s and default
> branches of the adblockplus hg repo seem to have a difference lib/ui.js file
> then the one this diff is based on afaict.
> 
> Am I missing something?

Ah ok this review depends and is based on the patches at
https://codereview.adblockplus.org/29331687/

[Wladimir Palant, 2015-12-01 19:16:36]

On 2015/12/01 18:50:07, Erik wrote:
> Ah ok this review depends and is based on the patches at
> https://codereview.adblockplus.org/29331687/

Yes, these two reviews belong together - the first one is the main change, this
is merely adding the fallback to it.

[Wladimir Palant, 2015-12-02 11:21:46]

Uploaded a new patchset, implemented the change explained below.

https://codereview.adblockplus.org/29331700/diff/29331701/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/29331700/diff/29331701/lib/ui.js#newcode1615
lib/ui.js:1615: Services.obs.notifyObservers(data, "content-contextmenu", null);
I thought a bit more about this and it's probably better to send out our own
notification here. This way, if the semantics of the (currently undocumented)
built-in content-contextmenu notification change we won't end up breaking
anybody by sending the notification incorrectly.

[tschuster, 2015-12-02 12:14:17]

https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMe...
File lib/child/contextMenu.js (right):

https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMe...
lib/child/contextMenu.js:36: if (target.localName == "menupopup" &&
target.triggerNode)
Is this still needed now?

https://codereview.adblockplus.org/29331700/diff/29331724/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/29331700/diff/29331724/lib/ui.js#newcode1608
lib/ui.js:1608: let event = new Event("contextmenu");
Isn't event = {target: popup.triggernode} enough?

[Wladimir Palant, 2015-12-02 12:20:54]

https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMe...
File lib/child/contextMenu.js (right):

https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMe...
lib/child/contextMenu.js:36: if (target.localName == "menupopup" &&
target.triggerNode)
On 2015/12/02 12:14:17, tschuster wrote:
> Is this still needed now?

Yes, it is. That's SeaMonkey sending the content-contextmenu notification,
triggered by SeaMonkey's browser code. For the mail client we are sending our
own notification - there it isn't necessary.

https://codereview.adblockplus.org/29331700/diff/29331724/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/29331700/diff/29331724/lib/ui.js#newcode1608
lib/ui.js:1608: let event = new Event("contextmenu");
On 2015/12/02 12:14:17, tschuster wrote:
> Isn't event = {target: popup.triggernode} enough?

Fair enough, no point faking the notification properly if nobody other than us
will be listening to it.

[tschuster, 2015-12-02 12:21:44]

On 2015/12/02 12:20:54, Wladimir Palant wrote:
>
https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMe...
> File lib/child/contextMenu.js (right):
> 
>
https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMe...
> lib/child/contextMenu.js:36: if (target.localName == "menupopup" &&
> target.triggerNode)
> On 2015/12/02 12:14:17, tschuster wrote:
> > Is this still needed now?
> 
> Yes, it is. That's SeaMonkey sending the content-contextmenu notification,
> triggered by SeaMonkey's browser code. For the mail client we are sending our
> own notification - there it isn't necessary.
> 
> https://codereview.adblockplus.org/29331700/diff/29331724/lib/ui.js
> File lib/ui.js (right):
> 
>
https://codereview.adblockplus.org/29331700/diff/29331724/lib/ui.js#newcode1608
> lib/ui.js:1608: let event = new Event("contextmenu");
> On 2015/12/02 12:14:17, tschuster wrote:
> > Isn't event = {target: popup.triggernode} enough?
> 
> Fair enough, no point faking the notification properly if nobody other than us
> will be listening to it.

LGTM