Closed
Bug 1440700
Opened 7 years ago
Closed 6 years ago
frameId missing from onClickData in menus.json
Categories
(WebExtensions :: General, enhancement, P5)
Tracking
(firefox65 fixed)
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: mixedpuppy, Assigned: dpino, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file, 1 obsolete file)
1.38 KB,
patch
|
mixedpuppy
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•7 years ago
|
Mentor: mixedpuppy
Reporter | ||
Comment 1•7 years ago
|
||
frameId should be added to the contextMenus.OnClickData details object in /browser/components/extensions/schemas/menus.json
Comment 2•7 years ago
|
||
If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started.
Mentor: :mixedpuppy
Comment 3•7 years ago
|
||
Hi, I am new to this, Can I take this up?
Comment 4•7 years ago
|
||
Go for it, surbhianand! If you have any questions, please feel free to needinfo :mixedpuppy, the mentor for this bug.
Updated•7 years ago
|
Product: Toolkit → WebExtensions
Comment 5•7 years ago
|
||
Hi , Is this bug still open? I am new here.
Comment 6•7 years ago
|
||
Hi Sneha! The bug is still open. If you would like to work on it, feel free to assign yourself and needinfo :mixedpuppy if you need any help!
Hi all, I'm taking a look at this. I'm assuming frameId can either be a string or an integer and that it is optional. Is this correct? Also, where can I go in the browser to validate the fix?
Flags: needinfo?(mixedpuppy)
Reporter | ||
Comment 8•6 years ago
|
||
frameId in all other apis is define as an integer in the schema, marking it as optional is good as there may be context menus on non-frame elements.
You should be able to just add a new test or maybe modify an existing test to specifically check frameId.
https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/browser/components/extensions/test/browser/browser_ext_contextMenus_onclick.js
Flags: needinfo?(mixedpuppy)
Comment 9•6 years ago
|
||
Hi,
I am a new contributer. Is this bug still open for the fix?
Comment 10•6 years ago
|
||
(In reply to hariniks19 from comment #9)
>
> I am a new contributer. Is this bug still open for the fix?
Welcome, hariniks19! Yes, this bug is open and I'm assigning it to you.
You'll want to read the Contributor Onramp before getting started: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp
Please needinfo the bug's mentor if you need any help. Happy bug fixing!
Assignee: nobody → hariniks19
Comment 11•6 years ago
|
||
Hey hariniks19, since we haen't heard from you in a few weeks, I am going to open this bug up for other contributors to work on.
If you want to jump back in, please leave us a comment and we'll reassign you!
Assignee: hariniks19 → nobody
Assignee | ||
Comment 12•6 years ago
|
||
I was fiddling with this bug and end up coding a patch. I tested the property in an existing test. Although perhaps it's better to have a separated test which actually uses a frame.
Attachment #9027223 -
Flags: review?(cneiman)
Comment 13•6 years ago
|
||
Awesome, thanks Diego!
Shane, could you take a look and advise on next steps?
Assignee: nobody → dpino
Flags: needinfo?(mixedpuppy)
Reporter | ||
Comment 14•6 years ago
|
||
Comment on attachment 9027223 [details] [diff] [review]
Bug-1440700-Add-frameId-in-onClickData-in-menus.json.patch
Rob has his head wrapped around menus more than anyone, so r? him as well before landing.
The test should use browser.test.assertTrue and include a message string.
browser.test.assertTrue(info.frameId >= 0, "frameId is set");
It would be nice to also have a test that has a subframe so we get specific on a frameId being other than zero.
Flags: needinfo?(mixedpuppy)
Attachment #9027223 -
Flags: review?(rob)
Attachment #9027223 -
Flags: review?(cneiman)
Attachment #9027223 -
Flags: review+
Comment 15•6 years ago
|
||
In this special case, tests are not needed because we already have tests for these scenario (including the one that was requested in comment 14):
https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/browser/components/extensions/test/browser/browser_ext_menus.js#312,314-315
I'll file a new bug to deal with the fact that we do currently not check whether the implementation is consistent with the schema.
Reporter | ||
Comment 16•6 years ago
|
||
Comment on attachment 9027223 [details] [diff] [review]
Bug-1440700-Add-frameId-in-onClickData-in-menus.json.patch
based on comment 15, remove the test change then add a checkin-needed keyword on the bug. Thanks!
Attachment #9027223 -
Flags: review?(rob)
Assignee | ||
Comment 17•6 years ago
|
||
Removed test change.
Attachment #9027223 -
Attachment is obsolete: true
Attachment #9027827 -
Flags: review?(mixedpuppy)
Assignee | ||
Comment 18•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•6 years ago
|
Attachment #9027827 -
Flags: review?(mixedpuppy) → review+
Reporter | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 19•6 years ago
|
||
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cdc29645ee
Add frameId in onClickData in menus.json r=mixedpuppy
Keywords: checkin-needed
Comment 20•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 21•6 years ago
|
||
Woohoo! Thanks so much for the patch, Diego. Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition
Would you be interested in creating a profile on mozillians.org? I'd be happy to vouch for you!
Assignee | ||
Comment 22•6 years ago
|
||
Thank you Caitlin. It seems I already had a profile at mozillians.org. Here it is: https://mozillians.org/en-GB/u/dpino/
Comment 23•6 years ago
|
||
Awesome! Your profile is vouched. :) Thank you again for all of your contributions!
Comment 24•6 years ago
|
||
Will this bug require manual validation from the QA team? if so please help with some steps on howto properly test, otherwise set the 'qe-verify-' flag, thank you!
Flags: needinfo?(dpino)
Assignee | ||
Comment 25•6 years ago
|
||
I think the patch doesn't need validation from the QA team (qe-verify-), since the feature is covered by a test and a new bug was filed to do further validation of this feature (https://bugzilla.mozilla.org/show_bug.cgi?id=1440700#c15).
OTOH, I tried to edit the bug and set qe-verify- but I couldn't because the option was disabled (likely I don't have enough permissions to do that change).
Flags: needinfo?(dpino) → needinfo?(cneiman)
You need to log in
before you can comment on or make changes to this bug.
Description
•