Closed Bug 590127 Opened 9 years ago Closed 4 years ago

Remove toolkit implementation of about:addons

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed
fennec - ---

People

(Reporter: fabrice, Assigned: mfinkle)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 7 obsolete files)

about:addons UI doesn't work well at all in fennec, so it should be removed since it creates a bad user experience.

This page is listed in about:about
This about: page is added in the toolkit AboutRedirector component :
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsAboutRedirector.cpp?force=1
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0-
Should it be removed or fixed by making fields more easily accessible? I mean, it's not too bad on my Linux Fennec, just some scrolling is needed and sometimes buttons are too close to right edge of the screen.
Fennec doesn't use about:addons, and this bug is just about removing it so users don't stumble upon it and try to use it.
Blocks: 596518
I started working on this
I wonder if this patch is enough to stop access to about:addons. This is effective only on Maemo. Should this be blocked also from other OS?
Attached patch Corrected patch (obsolete) — Splinter Review
Attachment #477101 - Attachment is obsolete: true
Marko,

We need this to be disabled for all mobile platforms, not just Maemo. The env var MOZ_APP_NAME is set to "mobile" in such cases
Another thing to consider is trying to remove the actual code shipped with the toolkit to support about:addons

We might be able to reduce our installed footprint if we can stop shipping code we don't use.
Flags: in-litmus?(nhirata.bugzilla)
Just to be sure, should about:addons be removed totally? (not only for mobiles)
(In reply to comment #9)
> Just to be sure, should about:addons be removed totally? (not only for mobiles)

No, only for mobile
Litmus test created
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=12965

Needs update once this bug is fixed.
Flags: in-litmus?(nhirata.bugzilla) → in-litmus+
(In reply to comment #7)
> We need this to be disabled for all mobile platforms, not just Maemo. The env
> var MOZ_APP_NAME is set to "mobile" in such cases

Should we create such precompiler macro/definition indicating mobile platform, because environmental variable is not checked on compilation phase?
This patch is checking that none of the following is defined:
ANDROID
MOZ_PLATFORM_MAEMO
SYMBIAN
WINCE_WINDOWS_MOBILE

What comes to removing the actual contents of chrome://mozapps/content/extensions/extensions.xul, I guess it can't be done because it's not on mobile branch.
Marko,

extensions.xul is included because it is referenced in toolkit/mozapps/extensions/jar.mn
You can use ifdefs here also (e.g. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/printing/jar.mn#4)

The last one to remove is toolkit/mozapps/extensions/content/extensions.js, included from the same jar.mn

(also, can you mark your previous patches as obsolete?)
Attachment #477102 - Attachment is obsolete: true
(In reply to comment #13)
> Created attachment 477865 [details] [diff] [review]
> Patch to prevent opening about:addons from mobile OS
> 
> This patch is checking that none of the following is defined:
> ANDROID
> MOZ_PLATFORM_MAEMO
> SYMBIAN
> WINCE_WINDOWS_MOBILE

You can use MOZ_GFX_OPTIMIZE_MOBILE instead of all of those
Now using MOZ_GFX_OPTIMIZE_MOBILE
Attachment #477865 - Attachment is obsolete: true
can we just load fennec mobile extensions page which is kind of usable and provide good user experience ?
Duplicate of this bug: 624103
(In reply to comment #16)
> Created attachment 478194 [details] [diff] [review]
> Patch to prevent opening about:addons from mobile OS (v4)
> 
> Now using MOZ_GFX_OPTIMIZE_MOBILE

When Windows/Mac/Linux fennec build (not Maemo and Android), we need --enable-mobile-optimize to use this define.

As long as I check build config for win32, this config flag isn't tuned on. (http://hg.mozilla.org/build/buildbot-configs/file/83c96267d655/mozilla2/mobile/win32-i686/mobile-browser/nightly/mozconfig)

Also, we shouldn't include desktop addon manager xul.
MOZ_GFX_OPTIMIZE_MOBILE is the wrong #define to be using.
Here is a patch to exclude about:addons from mobile builds.

I modified configure.in to introduce a define that looks like MOZ_BUILD_APP_MOBILE. We already have this info in MOZ_BUILD_APP but that cannot be used in source code since the preprocessor cannot compare strings.
Assignee: nobody → sarentz
Attachment #478194 - Attachment is obsolete: true
Better patch with a fix for configure.in where MOZ_BUILD_APP_standalone had the wrong case.
Attachment #536721 - Attachment is obsolete: true
Comment on attachment 536735 [details] [diff] [review]
Patch to exclude about:addons from mobile builds

Review of attachment 536735 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #536735 - Flags: review?(fabrice)
Comment on attachment 536735 [details] [diff] [review]
Patch to exclude about:addons from mobile builds

I don't think we want to add general purpose MOZ_BUILD_APP_* defines. There should probably be a global "mobile" define (or perhaps just rename MOZ_GFX_OPTIMIZE_MOBILE?), but it should be independent of MOZ_BUILD_APP, and maybe just be set in mobile's confvars.sh.
Attachment #536735 - Flags: feedback-
Ok. How about simply setting MOZ_MOBILE from mobile/confvars.sh?
Restarting with about:addons open will cause :
[Exception... "'JavaScript component does not have a method named: "deserializeToVariant"' when calling method: [nsIStructuredCloneContainer::deserializeToVariant]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: chrome://mozapps/content/extensions/extensions.js :: initialize :: line 123"  data: no]
chrome://mozapps/content/extensions/extensions.js
123

Error: Root node doesn't exist for 'discover' view
Source File: chrome://mozapps/content/extensions/extensions.js Line: 638
Attachment #536735 - Flags: review?(fabrice)
Assignee: sarentz → nobody
Product: Fennec Graveyard → Firefox for Android
Summary: remove about:addons → Remove toolkit implementation of about:addons
Attached patch remove-extensionsui v0.1 (obsolete) — Splinter Review
Updated to Native Fennec. We have our own about:addons, and just need to not include the toolkit version in Fennec.
Assignee: nobody → mark.finkle
Attachment #536735 - Attachment is obsolete: true
Try found a test that hardcodes xpinstallConfirm.js, so I just skipped that part for now:

http://mxr.mozilla.org/mozilla-central/source/caps/tests/mochitest/test_bug292789.html?force=1#16

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20c27fb81ad6
Attachment #8703730 - Attachment is obsolete: true
Attachment #8703739 - Flags: review?(margaret.leibovic)
(In reply to Mark Finkle (:mfinkle) from comment #28)

> Try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=20c27fb81ad6

Try looks good. We might get ~110KB reduction in APK size from this change.
Comment on attachment 8703739 [details] [diff] [review]
remove-extensionsui v0.2

Review of attachment 8703739 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we have much automated test coverage for add-ons UI interactions, so we should keep an eye out for regressions.

::: toolkit/mozapps/extensions/jar.mn
@@ +30,5 @@
>    content/mozapps/extensions/newaddon.js                        (content/newaddon.js)
>    content/mozapps/extensions/setting.xml                        (content/setting.xml)
>    content/mozapps/extensions/pluginPrefs.xul                    (content/pluginPrefs.xul)
>    content/mozapps/extensions/gmpPrefs.xul                       (content/gmpPrefs.xul)
>    content/mozapps/extensions/OpenH264-license.txt               (content/OpenH264-license.txt)

We do include the OpenH264 plugin on mobile... we don't need these files?

Looking at the item in about:addons, I do see that we don't show any license information or add-on options, but that might be its own bug.
Attachment #8703739 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/027e623f8f26
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Depends on: 1250967
You need to log in before you can comment on or make changes to this bug.