590127 - Remove toolkit implementation of about:addons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

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

Comment 1

[[:fabrice] Fabrice Desré]

This about: page is added in the toolkit AboutRedirector component :
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsAboutRedirector.cpp?force=1

Comment 2

[Marko Pellikka]

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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 4

[Marko Pellikka]

I started working on this

Comment 5

[Marko Pellikka]

Attachment: Patch proposal to prevent opening of about:addons on mobile

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?

Comment 6

[Marko Pellikka]

Attachment: Corrected patch

Comment 7

[[:fabrice] Fabrice Desré]

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

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 9

[Marko Pellikka]

Just to be sure, should about:addons be removed totally? (not only for mobiles)

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> Just to be sure, should about:addons be removed totally? (not only for mobiles)

No, only for mobile

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Litmus test created
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=12965

Needs update once this bug is fixed.

Comment 12

[Marko Pellikka]

(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?

Comment 13

[Marko Pellikka]

Attachment: 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

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.

Comment 14

[[:fabrice] Fabrice Desré]

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?)

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 16

[Marko Pellikka]

Attachment: Patch to prevent opening about:addons from mobile OS (v4)

Now using MOZ_GFX_OPTIMIZE_MOBILE

Comment 17

[Oleg Romashin (:romaxa)]

can we just load fennec mobile extensions page which is kind of usable and provide good user experience ?

Comment 19

[Makoto Kato [:m_kato]]

(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.

Comment 20

[Doug Turner (:dougt)]

MOZ_GFX_OPTIMIZE_MOBILE is the wrong #define to be using.

Comment 21

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Patch to exclude about:addons from mobile builds

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.

Comment 22

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Patch to exclude about:addons from mobile builds

Better patch with a fix for configure.in where MOZ_BUILD_APP_standalone had the wrong case.

Comment 23

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 536735 [details] [diff] [review]
Patch to exclude about:addons from mobile builds

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

Comment 24

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 25

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Ok. How about simply setting MOZ_MOBILE from mobile/confvars.sh?

Comment 26

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: remove-extensionsui v0.1

Updated to Native Fennec. We have our own about:addons, and just need to not include the toolkit version in Fennec.

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: remove-extensionsui v0.2

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

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(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 30

[:Margaret Leibovic]

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.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/027e623f8f26

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/027e623f8f26