Closed Bug 1461695 Opened 6 years ago Closed 6 years ago

tabs.onUpdated filtering uses isarticle, details uses isArticle.

Categories

(WebExtensions :: Untriaged, defect, P1)

58 Branch
defect

Tracking

(firefox61 wontfix, firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

We can add isArticle to the schema and support both, then just document one.
Assignee: nobody → mixedpuppy
Priority: -- → P1
Docs should be updated to use "isArticle" when this change lands on release.
Keywords: dev-doc-needed
Blocks: 1465520
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?
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.
Attachment #8981952 - Flags: review?(aswan)
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?
Product: Toolkit → WebExtensions
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
https://hg.mozilla.org/mozilla-central/rev/425137dedfd6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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)
Flags: needinfo?(mixedpuppy) → qe-verify-
Might be worth uplifting to 62.
Flags: needinfo?(mixedpuppy)
I don't think its necessary.  Can you make an argument for it?
Flags: needinfo?(mixedpuppy)
(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.
Flags: needinfo?(mixedpuppy)
How about we just document it correctly?
Flags: needinfo?(mixedpuppy)
(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)
Feel free to make the request if you feel that strongly about it.
Flags: needinfo?(mixedpuppy)
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?
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+
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: