1320412 - tabs.onUpdated does not fire for title changes

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[21Naown]

Related to: https://developer.chrome.com/extensions/tabs#event-onUpdated
Firefox has not "title" for example.

Moreover, if you are not restricted by Chrome's specs, what about add a filter like https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest#Parameters ?

Comment 1

[Tomislav Jovanovic :zombie]

Attachment: Bug 1320412 - Emit tabs.onUpdated event for title changes

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

Comment 2

[Tomislav Jovanovic :zombie]

There are two approaches to this bug:
 - simpler one, observing the changes of the tab's label attribute, or
 - alternatively, observe "DOMTitleChanged" messages that wouldn't include things like "Connecting..." and "New Tab" which come from the browser UI.

I opted for the first one, because it makes more sense to me semantically.  This is about _tab_ title changes, not only document's title changes.  Also, we already return tab.label as tab's title if you query a tab at just the right time [1].

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-utils.js#686

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8824166 [details]
Bug 1320412 - Emit  tabs.onUpdated event for title changes

https://reviewboard.mozilla.org/r/102702/#review103050

::: browser/components/extensions/ext-tabs.js:436
(Diff revision 1)
>            } else if (event.type == "TabUnpinned") {
>              needed.push("pinned");
>            }
>  
>            if (needed.length && !extension.hasPermission("tabs")) {
>              needed = needed.filter(attr => attr != "url" && attr != "favIconUrl");

We also need to filter out "title" here. Please add a test that it's filtered out when tabs permissions are missing.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js:185
(Diff revision 1)
> +    const url = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context_tabs_onUpdated_page.html";
> +    const tab = await browser.tabs.create({url});
> +
> +    browser.tabs.onUpdated.addListener(function onUpdated(tabId, changeInfo) {
> +      browser.test.assertEq(tabId, tab.id, "Check tab id");
> +      browser.test.log("onUpdate: " + JSON.stringify(changeInfo));

Nit: onUpdated, and please use a template string.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js:187
(Diff revision 1)
> +
> +    browser.tabs.onUpdated.addListener(function onUpdated(tabId, changeInfo) {
> +      browser.test.assertEq(tabId, tab.id, "Check tab id");
> +      browser.test.log("onUpdate: " + JSON.stringify(changeInfo));
> +      if ("title" in changeInfo) {
> +        if (changeInfo.title === url) {

if ("title" in changeInfo && changeInfo.title !== ...)

Comment 4

[Tomislav Jovanovic :zombie]

Comment on attachment 8824166 [details]
Bug 1320412 - Emit  tabs.onUpdated event for title changes

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

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8824166 [details]
Bug 1320412 - Emit  tabs.onUpdated event for title changes

https://reviewboard.mozilla.org/r/102702/#review103050

> We also need to filter out "title" here. Please add a test that it's filtered out when tabs permissions are missing.

That was sloppy of me, sorry.

> if ("title" in changeInfo && changeInfo.title !== ...)

Turns out we also get "Connectiong..." on some platforms...

Comment 6

[Tomislav Jovanovic :zombie]

Comment on attachment 8824166 [details]
Bug 1320412 - Emit  tabs.onUpdated event for title changes

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec8836909060
Emit  tabs.onUpdated event for title changes r=kmag

Comment 8

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/ec8836909060

Comment 9

[21Naown]

> Moreover, if you are not restricted by Chrome's specs, what about add a filter like
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest#Parameters
> ?

I assume you cannot check for this? Or should I create a specific report?

Comment 10

[Tomislav Jovanovic :zombie]

Yeah, this bug got renamed to just fix the existing bug.  You should file a separate one to request the enhancements, so that a design decision can be made.