1461693 - tabs.json missing some items for onUpdated event

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

from bug 1329507 comment 18

* it looks as if "changeInfo" in tabs.json (https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/tabs.json#1398) would give me the list of properties that can appear in the changeInfo argument. But this omits a few properties which do appear, namely title, hidden, isArticle.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1461693 - add missing params to changeInfo for tabs.onUpdated,

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8976182 [details]
Bug 1461693 - add missing params to changeInfo for tabs.onUpdated,

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

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8976182 [details]
Bug 1461693 - add missing params to changeInfo for tabs.onUpdated,

https://reviewboard.mozilla.org/r/244364/#review251448

::: browser/components/extensions/schemas/tabs.json:1422
(Diff revision 2)
> -              "pinned": {
> +              "hidden": {
>                  "type": "boolean",
>                  "optional": true,
> -                "description": "The tab's new pinned state."
> +                "description": "The tab's new hidden state."
>                },
> -              "audible": {
> +              "isarticle": {

This seems that it should actually be in camel case (`isArticle`), e.g. this is one of our test case for that changeInfo property:

- https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/browser/components/extensions/test/browser/browser_ext_tabs_readerMode.js#61

at least if I'm not misreading or missing something from the original discussion from bug 1329507 comment 18.

(it looks that also the one in `UpdatePropertyName` should use the same case for consistency, but I think that this part of the fix will be part of Bug 1461695, am I right?)

::: browser/components/extensions/schemas/tabs.json:1425
(Diff revision 2)
> -                "description": "The tab's new pinned state."
> +                "description": "The tab's new hidden state."
>                },
> -              "audible": {
> +              "isarticle": {
>                  "type": "boolean",
>                  "optional": true,
> -                "description": "The tab's new audible state."
> +                "description": "Whether the tab supports article mode."

Nit, We could use the same description from the related tab.Tab property (https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/browser/components/extensions/schemas/tabs.json#102):

"Whether the document in the tab can be rendered in reader mode."

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8976182 [details]
Bug 1461693 - add missing params to changeInfo for tabs.onUpdated,

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

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8976182 [details]
Bug 1461693 - add missing params to changeInfo for tabs.onUpdated,

https://reviewboard.mozilla.org/r/244364/#review251448

> This seems that it should actually be in camel case (`isArticle`), e.g. this is one of our test case for that changeInfo property:
> 
> - https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/browser/components/extensions/test/browser/browser_ext_tabs_readerMode.js#61
> 
> at least if I'm not misreading or missing something from the original discussion from bug 1329507 comment 18.
> 
> (it looks that also the one in `UpdatePropertyName` should use the same case for consistency, but I think that this part of the fix will be part of Bug 1461695, am I right?)

Yeah, the other one is Bug 1461695

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8976182 [details]
Bug 1461693 - add missing params to changeInfo for tabs.onUpdated,

https://reviewboard.mozilla.org/r/244364/#review251546

::: browser/components/extensions/schemas/tabs.json:1414
(Diff revision 3)
>                  "description": "True while the tab is not loaded with content."
>                },
> -              "url": {
> +              "favIconUrl": {
>                  "type": "string",
>                  "optional": true,
> -                "description": "The tab's URL if it has changed."
> +                "permissions": ["tabs"],

Good catch, this one and the ones related to "url" and "title" were also out of sync from what the actually implemented (and tested) behavior is (I mean the "requiring the tabs permission" to be present in the changeInfo).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4958a88bd86
add missing params to changeInfo for tabs.onUpdated, r=rpl

Comment 8

[Narcis Beleuzu [:NarcisB]]

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