1461695 - tabs.onUpdated filtering uses isarticle, details uses isArticle.

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Shane Caraveo (:mixedpuppy)]

We can add isArticle to the schema and support both, then just document one.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Docs should be updated to use "isArticle" when this change lands on release.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1461695 change tabs.onUpdated filter name to isArticle,

Review commit: https://reviewboard.mozilla.org/r/247962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247962/

Comment 3

[Andrew Swan [:aswan]]

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 4

[Shane Caraveo (:mixedpuppy)]

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.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8981952 [details]
Bug 1461695 change tabs.onUpdated filter name to isArticle,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247962/diff/1-2/

Comment 6

[Andrew Swan [: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?

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8981952 [details]
Bug 1461695 change tabs.onUpdated filter name to isArticle,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247962/diff/2-3/

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8981952 [details]
Bug 1461695 change tabs.onUpdated filter name to isArticle,

https://reviewboard.mozilla.org/r/247962/#review263232

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/425137dedfd6
change tabs.onUpdated filter name to isArticle, r=aswan

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/425137dedfd6

Comment 11

[Vlad Jiman]

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!

Comment 12

[Tim Nguyen :ntim]

Might be worth uplifting to 62.

Comment 13

[Shane Caraveo (:mixedpuppy)]

I don't think its necessary.  Can you make an argument for it?

Comment 14

[Tim Nguyen :ntim]

(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.

Comment 15

[Shane Caraveo (:mixedpuppy)]

How about we just document it correctly?

Comment 16

[Tim Nguyen :ntim]

(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

Comment 17

[Shane Caraveo (:mixedpuppy)]

Feel free to make the request if you feel that strongly about it.

Comment 18

[Tim Nguyen :ntim]

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]:

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 20

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

https://hg.mozilla.org/releases/mozilla-beta/rev/336b10b00c1c

Comment 21

[Will Bamberg [:wbamberg]]

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.