1339416 - contextMenus.OnClickData should have the frameId (can be used with tabs.sendMessage)

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[kernp25]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170214030231

Steps to reproduce:

Chrome also has frameId in info
https://developer.chrome.com/extensions/contextMenus#event-onClicked

Comment 1

[Hrishikesh Barman[:geekodour08]]

any work been done on this? can I start on it?

Comment 2

[kernp25]

Attachment: Bug 1339416 - contextMenus.OnClickData should have the frameId (can be used with tabs.sendMessage)

Comment 3

[kernp25]

(In reply to hrishikeshbman from comment #1)
> any work been done on this?
Yes! See the patch.

> can I start on it?
Yes!

Comment 4

[kernp25]

Attachment: bug-1339416-fix.patch

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8850063 [details] [diff] [review]
bug-1339416-fix.patch

This looks fine, I'd like to see a test along with this.  Maybe add something in browser_ext_contextMenus_onclick.js.  There is a frame now in the context.html test file that could be used as part of that.

Also, if the context menu is on chrome, what does frameId end up as?  I'm expecting it should be -1.  You could probably add a simple check for that in some test in browser_ext_contextMenus_chrome.js

Comment 6

[kernp25]

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> Comment on attachment 8850063 [details] [diff] [review]
> bug-1339416-fix.patch
> 
> This looks fine, I'd like to see a test along with this.  Maybe add
> something in browser_ext_contextMenus_onclick.js.  There is a frame now in
> the context.html test file that could be used as part of that.
> 
> Also, if the context menu is on chrome, what does frameId end up as?  I'm
> expecting it should be -1.  You could probably add a simple check for that
> in some test in browser_ext_contextMenus_chrome.js

How to write and run those tests?

Comment 7

[kernp25]

Attachment: bug-1339416-fix-v2.patch

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8851064 [details] [diff] [review]
bug-1339416-fix-v2.patch

Still needs tests I requested

Comment 9

[kernp25]

Attachment: bug-1339416-fix-v3.patch

Comment 10

[kernp25]

113 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_contextMenus_chrome.js | click in frame - Got 4294967304, expected 1

Can you tell me what's the problem?

Comment 11

[Shane Caraveo (:mixedpuppy)]

kern25: fyi we're in a work week, I've looked at this and reproduced the problem but haven't looked at why yet.  My responses will be slow this week.

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to kernp25 from comment #10)
> 113 INFO TEST-UNEXPECTED-FAIL |
> browser/components/extensions/test/browser/test-oop-extensions/
> browser_ext_contextMenus_chrome.js | click in frame - Got 4294967304,
> expected 1
> 
> Can you tell me what's the problem?

Ok, looking into the code, we don't handle frameId like we do for tabs and windows.  frameId is actually the value of outerWindowId unless it is the top frame (it is zero).  Change the test to something like this:

isnot(info.frameId, undefined, "frame click, frameId is not undefined");
isnot(info.frameId, 0, "frame click, frameId probably okay");

The patch is a little bit rotted due to changes yesterday, so you'll need to hg pull.

Comment 13

[kernp25]

Attachment: bug-1339416-fix-v4.patch

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8854375 [details] [diff] [review]
bug-1339416-fix-v4.patch

I'd like to run this through tryserver before landing, if you don't have access to push to that, let me know I'll do it.

Comment 15

[kernp25]

I don't have access to the tryserver :/

Comment 16

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e275ab87d0bada5fd2c05bfc31ea3c7ce678a44

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: bug-1339416-fix-v4.patch

Just updating the commit message in the patch.  This is ready to go.

Comment 18

[Shane Caraveo (:mixedpuppy)]

kernp25: (no idea what your name is) on it's way to landing \o/  thank you!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebebeede8340
contextMenus.OnClickData should have the frameId. r=mixedpuppy

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ebebeede8340

Comment 21

[kernp25]

The doc is missing here?
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/OnClickData

"frameId" is not listed here!

Comment 22

[Will Bamberg [:wbamberg]]

To get documentation for new features, you need to set the dev-doc-needed keyword, which I have now done. If this keyword is not set, I will probably not see the bug at all.

Comment 23

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/OnClickData for frameId.

Please let me know if this covers it.

Comment 24

[kernp25]

Yes! Looks good :)

Comment 25

[blackgloryzh]

Still not have frameId in schema file at least Firefox 58.0.2
/browser/components/extensions/schemas/menus.json