Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29331700: Issue 3223 - Implement context menu fallback for SeaMonkey Mail and Thunderbird (Closed)

Created:
Dec. 1, 2015, 2:48 p.m. by Wladimir Palant
Modified:
Dec. 2, 2015, 12:23 p.m.
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : Changed notification name #

Total comments: 4

Patch Set 3 : Don`t send real event, minimal object will do #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -12 lines) Patch
M lib/child/contextMenu.js View 1 1 chunk +6 lines, -7 lines 0 comments Download
M lib/ui.js View 1 2 1 chunk +14 lines, -5 lines 0 comments Download

Messages

Total messages: 10
Wladimir Palant
Dec. 1, 2015, 2:48 p.m. (2015-12-01 14:48:31 UTC) #1
Erik
On 2015/12/01 14:48:31, Wladimir Palant wrote: I can't seem to figure out what this patch ...
Dec. 1, 2015, 5:57 p.m. (2015-12-01 17:57:53 UTC) #2
Erik
On 2015/12/01 17:57:53, Erik wrote: > On 2015/12/01 14:48:31, Wladimir Palant wrote: > > I ...
Dec. 1, 2015, 5:58 p.m. (2015-12-01 17:58:15 UTC) #3
Erik
trying this publish feature out.
Dec. 1, 2015, 6:01 p.m. (2015-12-01 18:01:12 UTC) #4
Erik
On 2015/12/01 17:57:53, Erik wrote: > On 2015/12/01 14:48:31, Wladimir Palant wrote: > > I ...
Dec. 1, 2015, 6:50 p.m. (2015-12-01 18:50:07 UTC) #5
Wladimir Palant
On 2015/12/01 18:50:07, Erik wrote: > Ah ok this review depends and is based on ...
Dec. 1, 2015, 7:16 p.m. (2015-12-01 19:16:36 UTC) #6
Wladimir Palant
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: ...
Dec. 2, 2015, 11:21 a.m. (2015-12-02 11:21:46 UTC) #7
tschuster
https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMenu.js File lib/child/contextMenu.js (right): https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMenu.js#newcode36 lib/child/contextMenu.js:36: if (target.localName == "menupopup" && target.triggerNode) Is this still ...
Dec. 2, 2015, 12:14 p.m. (2015-12-02 12:14:17 UTC) #8
Wladimir Palant
https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMenu.js File lib/child/contextMenu.js (right): https://codereview.adblockplus.org/29331700/diff/29331724/lib/child/contextMenu.js#newcode36 lib/child/contextMenu.js:36: if (target.localName == "menupopup" && target.triggerNode) On 2015/12/02 12:14:17, ...
Dec. 2, 2015, 12:20 p.m. (2015-12-02 12:20:54 UTC) #9
tschuster
Dec. 2, 2015, 12:21 p.m. (2015-12-02 12:21:44 UTC) #10
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

Powered by Google App Engine
This is Rietveld