Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation
59 bytes, text/x-review-board-request
2.12 MB, application/x-zip-compressed
536.97 KB, application/x-zip-compressed
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.
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.
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.
Summary: [BrowserAction] default_icon on android should warn rather than prevent installation → [BrowserAction] properties supported on desktop but not android should warn rather than prevent installation
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 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...
Attachment #8877695 - Flags: review?(aswan) → review+
(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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/ed85f2ec9f2a Warn on properties supported on desktop but not android instead of preventing installation r=aswan
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.
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 ?
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?
I'll take that as a no, then. Thx, bbl :-(
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).
Flags: needinfo?(matthewjwein) → needinfo?(cosmin.badescu)
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
Attachment #8877695 - Flags: approval-mozilla-beta?
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)
Attachment #8877695 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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…
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.
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. 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.  https://github.com/gorhill/uBlock/wiki/Firefox-WebExtensions
Sorry, wish we'd known earlier. But its too late to uplift to 55 which is being merged to release today(ish).
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 on attachment 8877695 [details] Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation comments 17 and 24
Attachment #8877695 - Flags: approval-mozilla-release?
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
Attachment #8877695 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.