1384964 - pageAction show and hide do not work on tabId 0 on Android

Status: VERIFIED FIXED

(WebExtensions :: Android, defect, P1)

Description

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

The pageAction show and hide implementation on Firefox for Android is currently unable to work on tabId 0, because of the following ternary `tabId ? tabTracker.getTab(tabId) : null;` which will return null for `tabId == 0`:

- http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/mobile/android/components/extensions/ext-pageAction.js#240,245

(also getPopup and setPopup seems to have the same issue: http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/mobile/android/components/extensions/ext-pageAction.js#250,256)

Comment 1

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

Attachment: Bug 1384964 - Fix pageAction.show/hide/getPopup/setPopup on Android for tabId 0.

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

Comment 2

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

There should be no real reason to check the tabId manually inside this API methods, the type of this parameter is already checked by the schema wrappers.

I would have liked to also change the test cases to explicitly test to guard this from regressing again, unfortunately the tabId 0 is used by the test harness in the mochitests (that is likely to be the reason why it wasn't tested with the tabId 0), and so I opted to attach a fix for the issue as soon as possible.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8890921 [details]
Bug 1384964 - Fix pageAction.show/hide/getPopup/setPopup on Android for tabId 0.

https://reviewboard.mozilla.org/r/162124/#review167436

Comment 4

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

Attachment: Bug 1384964 - Fix browserAction.setTitle/getTitle on Android for tabId 0.

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

Comment 5

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

I noticed that browserAction.setTitle/getTitle have a similar issue with tabId 0 (the main difference is that browserAction tabId parameter is actually optional and so we need to keep the ternary but fix it to detect tabId 0 as a valid tabId).

Comment 7

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

Comment on attachment 8891347 [details]
Bug 1384964 - Fix browserAction.setTitle/getTitle on Android for tabId 0.

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

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8891347 [details]
Bug 1384964 - Fix browserAction.setTitle/getTitle on Android for tabId 0.

https://reviewboard.mozilla.org/r/162544/#review167878

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60076d86bb5c
Fix pageAction.show/hide/getPopup/setPopup on Android for tabId 0. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/5c6a8f30184c
Fix browserAction.setTitle/getTitle on Android for tabId 0. r=mixedpuppy

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/60076d86bb5c
https://hg.mozilla.org/mozilla-central/rev/5c6a8f30184c

Comment 11

[CosminB]

Attachment: Bug1384964.gif

This issue is verified as fixed on Fennec 57.0a1 (2017-08-28) under Android 6.0.1.

The pageAction.show/hide, browserAction setTitle/getTitle, pageAction setPopup/getPopup are working on the web page that has tabId 0.

(On Fennec 56.0b5 under Android 6.0.1 browserAction setTitle/getTitle is not working because of the Bug 1392322 which was fixed only in Fennec 57, the pageAction.show/hide and pageAction setPopup/getPopup work as expected in Fennec 56.0b5.)

Please see the attached video.

Comment 12

[CosminB]

Attachment: 56.0b7-1384964.gif

This issue is verified as fixed on Fennec 56.0b7 under Android 6.0.1.

With the uplift of Bug 1392322 on Fennec 56.0b7,the pageAction.show/hide, browserAction setTitle/getTitle, pageAction setPopup/getPopup are working on the web page that has tabId 0.

Please see the attached video.