1367494 - [BrowserAction] properties supported on desktop but not android should warn rather than prevent installation

Status: VERIFIED FIXED

(WebExtensions :: Android, defect, P2)

Description

[Matthew Wein [:mattw]]

The first version of the browser action API on android relies on the title of the browser action, instead of the icon, for displaying it in the browser's main menu - meaning that the extension should still function without default_icon.

Currently, extensions using default_icon on desktop will fail to install on android because this property is marked as unsupported. Instead, we should provide a warning when default_icon is used to allow these extensions to install successfully.

Comment 1

[Andy McKay]

Would it be possible to fix this quickly and uplift to 55? Having a scenario where the manifest.json has to change dramatically between Desktop and Android is something that we really don't have a good story for the developers or on AMO yet.

Comment 2

[Matthew Wein [:mattw]]

Yeah, it should just be a matter or adding "deprecated" with a warning message to the schema for default_icon - I'll upload a patch today.

Comment 3

[Matthew Wein [:mattw]]

Attachment: Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

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

Comment 4

[Matthew Wein [:mattw]]

Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

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

Comment 5

[sebastian.noack]

Shouldn't properties supported on desktop, but not on Android, rather be just ignored. Otherwise you would encourage people to provide different builds for desktop and Android, which seems to be unnecessary.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

https://reviewboard.mozilla.org/r/149126/#review153568

I would remove "at this time" from the deprecation message but up to you...

Comment 8

[Matthew Wein [:mattw]]

(In reply to sebastian.noack from comment #5)
> Shouldn't properties supported on desktop, but not on Android, rather be
> just ignored. Otherwise you would encourage people to provide different
> builds for desktop and Android, which seems to be unnecessary.

If we disabled default_icon on desktop, we would break many existing desktop addons which use that property. On the other hand, the first version of the browser action API on android doesn't use default_icon, but instead uses default_title to add menu items for each browser action. An addon that is designed to work on both desktop and android will fail to install on android if it uses default_icon or any other browser specific property. By marking these properties as deprecated instead of unsupported, we will instead log warnings to the console, which will allow addons designed to work both on desktop and android to install successfully on android.

Comment 9

[Matthew Wein [:mattw]]

Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed85f2ec9f2a
Warn on properties supported on desktop but not android instead of preventing installation r=aswan

Comment 11

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

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

Comment 12

[sebastian.noack]

I wasn't suggesting to unsupport these properties on desktop, but to not issue a warning on Android and instead rather silently ignore them there.

Comment 13

[CosminB]

Attachment: Video+Logs.zip

The installation of the next WebExtension https://addons-dev.allizom.org/en-US/android/addon/open-my-page/ that uses default_icon in the browser_action works, but I don’t see any warnings displayed in the remote browser console for the unsupported properties.
 
Verified on Fennec 56.0a1 (2017-06-20) under Android 7.1.2
 
Any thoughts about this Matthew ?

Comment 14

[ntninja]

Hi! I have just added a bunch of workarounds for my extension to get it to load on Firefox for Android 55 Beta.
This includes things like adding:
<link rel="stylesheet" type="text/css" href="chrome://browser/content/extension.css" />
<link rel="stylesheet" type="text/css" href="chrome://browser/content/extension-mac.css" />
to the browser_action/default_popup file or declaring the browser action icon in code instead of the manifest (which means that, on desktop Firefox, the default "puzzle piece" icon for be visible on startup in the toolbar).

Since these changes have not been uplifted to Firefox for Android 55, I take it that I'll have to maintain these workarounds for a couple of releases? Is there any chance if this still becoming part of the 55 Beta version?

Comment 15

[ntninja]

I'll take that as a no, then. Thx, bbl :-(

Comment 16

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

Hi Cosmin,
can you check also the messages logged to "adb logcat" while the extension is installing?

these "deprecated" warnings are not visible to the user, but they should have been logged in the "adb logcat" logs and in the browser console (the "Main Process" when connecting from WebIDE to the Firefox for Android instance).

Comment 17

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

Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

Approval Request Comment
[Feature/Bug causing the regression]:
- Bug 1331742 - Add support for browserAction.onClicked
  When the first pieces of the browserAction API have been implemented for Android (during the Firefox 55 cycle), the browserAction API schema has been added to Firefox for Android and, by marking the browser_action manifest properties as unsupported, we have started to prevent the installation of addons with these properties in the manifest as a unintended side-effect.

[User impact if declined]:
Firefox for Android 55 will block the installation of any extension which has these properties in the manifest, which would force the addon developer to create to different add-ons with different manifest.json files to be able to support both Firefox Desktop and Firefox for Android.

[Is this code covered by automated tests?]:
This patch doesn't contain new automated tests.

(but the related "deprecated" and "unsupported" feature of the API schemas have their own tests: http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js)

[Has the fix been verified in Nightly?]:
It has been manually verified that the installation is not prevented anymore
(testing the logged message is still ongoing) 

[Needs manual test from QE? If yes, steps to reproduce]:
Yes, we should use the same STR that we are already using to manually verify the behavior on this fix on nightly.
 
[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Risks should be pretty low.

[Why is the change risky/not risky?]:
This change is restricted to the JSON of a single API schema file, 
and it is restricted to the "WebExtension" add-ons. 

[String changes made/needed]:
None

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

I don't think this meets the bar for uplift this late in the cycle (we only have a single beta build left)

Comment 19

[ntninja]

Just wanted to mention that I currently receive an error popup about some "failed add-on installation" due to the add-on being "broken" every time I start Firefox Beta for Android that is very likely related to this backwards-incompatible change (even if it's "just" one version, it's still breaking stuff nonetheless).

ofc, Luca already gave a much more in-depth description of this, so I guess I'll be ignored like last time.

Just wanted to mention that this is indeed an issue that users (aside from me) will actually notice…

Comment 20

[CosminB]

Attachment: Warnings+Logs.zip

Thanks Luca!

The “deprecated” warnings are displayed in the WebIDE “Main Process”.
Please see the attached files.

This issue is verified as fixed on Fennec 56.0a1 (2017-07-27) under Android 7.1.2.

Comment 22

[u287251]

For what it's worth, here is my feedback re. this issue.

I was really hoping this would be fixed for Firefox for Android 55. If the fix is only available for Firefox for Android 56, this is seriously delaying the publishing the webext version of my extension which I was planning to 1st release on August 22nd on stable AMO channel[1]. My extension is one of those cases requiring an intermediate embedded-webext version in order to seamlessly migrate user data from legacy to webext storage.

[1] https://github.com/gorhill/uBlock/wiki/Firefox-WebExtensions

Comment 23

[Andy McKay]

Sorry, wish we'd known earlier. But its too late to uplift to 55 which is being merged to release today(ish).

Comment 24

[Andy McKay]

If there's a point release for 55, I'd like to nominate this patch. It's very low risk and causes lots of add-ons to have problems.

Comment 25

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

comments 17 and 24

Comment 26

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

we're going to build a rc2, so based on the impact on lots of add-ons, let's take this one on 55

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/b1eea5fe226d