Closed
Bug 1461695
Opened 6 years ago
Closed 6 years ago
tabs.onUpdated filtering uses isarticle, details uses isArticle.
Categories
(WebExtensions :: Untriaged, defect, P1)
Tracking
(firefox61 wontfix, firefox62 fixed, firefox63 fixed)
RESOLVED
FIXED
mozilla63
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
aswan
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
We can add isArticle to the schema and support both, then just document one.
Updated•6 years ago
|
Assignee: nobody → mixedpuppy
Priority: -- → P1
Assignee | ||
Comment 1•6 years ago
|
||
Docs should be updated to use "isArticle" when this change lands on release.
Keywords: dev-doc-needed
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8981952 [details] Bug 1461695 change tabs.onUpdated filter name to isArticle, https://reviewboard.mozilla.org/r/247962/#review254144 As suggested on irc, this should also log a deprecation warning in the addon console if an extension uses the deprecated form. ::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:229 (Diff revision 1) > +add_task(async function test_filter_isArticle() { > + let extension = ExtensionTestUtils.loadExtension({ > + manifest: { > + permissions: ["tabs"], > + }, > + background() { > + // We expect only status updates, anything else is a failure. > + browser.tabs.onUpdated.addListener((tabId, changeInfo) => { > + browser.test.log(`got onUpdated ${JSON.stringify(changeInfo)}`); > + if ("isArticle" in changeInfo) { > + browser.test.notifyPass("isArticle"); > + } > + }, {properties: ["isArticle"]}); > + }, > + }); > + await extension.startup(); > + let ok = extension.awaitFinish("isArticle"); > + > + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/"); > + await ok; > + > + await extension.unload(); > + > + await BrowserTestUtils.removeTab(tab); > +}); isn't this covered by the test below?
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8981952 [details] Bug 1461695 change tabs.onUpdated filter name to isArticle, https://reviewboard.mozilla.org/r/247962/#review255556 ::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js:229 (Diff revision 1) > +add_task(async function test_filter_isArticle() { > + let extension = ExtensionTestUtils.loadExtension({ > + manifest: { > + permissions: ["tabs"], > + }, > + background() { > + // We expect only status updates, anything else is a failure. > + browser.tabs.onUpdated.addListener((tabId, changeInfo) => { > + browser.test.log(`got onUpdated ${JSON.stringify(changeInfo)}`); > + if ("isArticle" in changeInfo) { > + browser.test.notifyPass("isArticle"); > + } > + }, {properties: ["isArticle"]}); > + }, > + }); > + await extension.startup(); > + let ok = extension.awaitFinish("isArticle"); > + > + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/"); > + await ok; > + > + await extension.unload(); > + > + await BrowserTestUtils.removeTab(tab); > +}); No, the test below fails if it were present.
Updated•6 years ago
|
Attachment #8981952 -
Flags: review?(aswan)
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8981952 [details] Bug 1461695 change tabs.onUpdated filter name to isArticle, https://reviewboard.mozilla.org/r/247962/#review256330 ::: browser/components/extensions/parent/ext-tabs.js:303 (Diff revision 2) > for (let [name, listener] of listeners) { > windowTracker.addListener(name, listener); > } > > - if (filter.properties.has("isarticle")) { > + // TODO Bug 1465520 remove lowercase name when ready. > + let isArticle = filter.properties.has("isarticle") || filter.properties.has("isArticle"); How about just normalizing the property above when the deprecation warning is emitted so we don't need to mess with anything here later?
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8981952 [details] Bug 1461695 change tabs.onUpdated filter name to isArticle, https://reviewboard.mozilla.org/r/247962/#review263232
Attachment #8981952 -
Flags: review?(aswan) → review+
Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/425137dedfd6 change tabs.onUpdated filter name to isArticle, r=aswan
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/425137dedfd6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 11•6 years ago
|
||
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag. Thank you!
Flags: needinfo?(mixedpuppy)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mixedpuppy) → qe-verify-
Assignee | ||
Comment 13•6 years ago
|
||
I don't think its necessary. Can you make an argument for it?
Flags: needinfo?(mixedpuppy)
Comment 14•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #13) > I don't think its necessary. Can you make an argument for it? So extension developers have the deprecation message and the matching MDN docs as early as possible.
Updated•6 years ago
|
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 15•6 years ago
|
||
How about we just document it correctly?
Flags: needinfo?(mixedpuppy)
Comment 16•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #15) > How about we just document it correctly? Yeah, but between a property that works on 61+ and a property that works on 63+, people will chosse the former. So uplifting to 62 would help
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 17•6 years ago
|
||
Feel free to make the request if you feel that strongly about it.
Flags: needinfo?(mixedpuppy)
Comment 18•6 years ago
|
||
Comment on attachment 8981952 [details] Bug 1461695 change tabs.onUpdated filter name to isArticle, Approval Request Comment [Feature/Bug causing the regression]: bug 1329507 [User impact if declined]: the filter name for 'isArticle' will be wrong for two releases instead of one, which will encourage developers to use the incorrect name for greater compatibility [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: none that I'm aware of [Is the change risky?]: low risk [Why is the change risky/not risky?]: straightforward API name change with compatibility for the wrong name [String changes made/needed]:
Attachment #8981952 -
Flags: approval-mozilla-beta?
Updated•6 years ago
|
status-firefox61:
--- → wontfix
status-firefox62:
--- → affected
Comment 19•6 years ago
|
||
Comment on attachment 8981952 [details] Bug 1461695 change tabs.onUpdated filter name to isArticle, Tim is persuasive, we have a month left in 62 beta, let's give it a try. I commented in the followup bug, which we should check in Firefox 65.
Attachment #8981952 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/336b10b00c1c
Comment 21•6 years ago
|
||
I've updated https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/onUpdated#addListener_syntax and also the compat data: https://github.com/mdn/browser-compat-data/pull/2771 (although the compat data isn't deployed to the Wiki yet). Marking as dev-doc-complete, but please let me know if we need anything else here.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•