Closed
Bug 1339416
Opened 7 years ago
Closed 7 years ago
contextMenus.OnClickData should have the frameId (can be used with tabs.sendMessage)
Categories
(WebExtensions :: Frontend, defect, P5)
WebExtensions
Frontend
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: kernp25, Assigned: kernp25, Mentored)
References
Details
(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [contextMenus], triaged)
Attachments
(1 file, 5 obsolete files)
5.33 KB,
patch
|
mixedpuppy
:
review+
|
Details | Diff | Splinter Review |
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
Updated•7 years ago
|
Flags: needinfo?(kernp25)
Attachment #8847177 -
Flags: feedback?(hrishikeshbman)
(In reply to hrishikeshbman from comment #1) > any work been done on this? Yes! See the patch. > can I start on it? Yes!
Attachment #8847177 -
Attachment is obsolete: true
Attachment #8847177 -
Flags: feedback?(hrishikeshbman)
Attachment #8850063 -
Flags: review?(mixedpuppy)
Comment 5•7 years ago
|
||
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
Attachment #8850063 -
Flags: review?(mixedpuppy) → feedback+
(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?
Attachment #8850063 -
Attachment is obsolete: true
Attachment #8851064 -
Flags: review?(kmaglione+bmo)
Comment 8•7 years ago
|
||
Comment on attachment 8851064 [details] [diff] [review] bug-1339416-fix-v2.patch Still needs tests I requested
Attachment #8851064 -
Flags: review?(kmaglione+bmo) → review-
Attachment #8851341 -
Flags: feedback?(scaraveo)
Assignee | ||
Comment 10•7 years ago
|
||
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?
Attachment #8851341 -
Flags: feedback?(scaraveo) → feedback?(mixedpuppy)
Updated•7 years ago
|
Mentor: mixedpuppy
Comment 11•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8851064 -
Attachment is obsolete: true
Attachment #8851341 -
Attachment is obsolete: true
Attachment #8851341 -
Flags: feedback?(mixedpuppy)
Attachment #8854375 -
Flags: review?(mixedpuppy)
Comment 14•7 years ago
|
||
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.
Flags: needinfo?(kernp25)
Attachment #8854375 -
Flags: review?(mixedpuppy) → review+
Updated•7 years ago
|
Assignee: nobody → kernp25
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e275ab87d0bada5fd2c05bfc31ea3c7ce678a44
Comment 17•7 years ago
|
||
Just updating the commit message in the patch. This is ready to go.
Attachment #8854375 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8855837 -
Flags: review+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 18•7 years ago
|
||
kernp25: (no idea what your name is) on it's way to landing \o/ thank you!
Comment 19•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ebebeede8340 contextMenus.OnClickData should have the frameId. r=mixedpuppy
Keywords: checkin-needed
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ebebeede8340
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 21•7 years ago
|
||
The doc is missing here? https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/OnClickData "frameId" is not listed here!
Flags: needinfo?(wbamberg)
Comment 22•7 years ago
|
||
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.
Flags: needinfo?(wbamberg)
Keywords: dev-doc-needed
Comment 23•7 years ago
|
||
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.
Flags: needinfo?(kernp25)
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 25•6 years ago
|
||
Still not have frameId in schema file at least Firefox 58.0.2 /browser/components/extensions/schemas/menus.json
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•