Closed Bug 1223526 Opened 9 years ago Closed 9 years ago

APK size increased by 100KB after toolkit theming change

Categories

(Toolkit :: Themes, defect)

Product:
Toolkit
Component:
Themes
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox44 --- unaffected
firefox45 --- fixed
fennec 45+ ---

People

(Reporter: wlach, Assigned: mfinkle)

References

Details

Attachments

(1 file, 2 obsolete files)

nothemes v0.1
84.99 KB, patch
mossop
: review+
gbrown
: review+
Margaret
: review+
Details | Diff | Splinter Review 
We do package toolkit code in our APK, and I wonder if with this change we're accidentally packaging things we don't need.

Nick, do you have an understanding of how these toolkit themes are packaged on Android? Do we need some additional build logic to exclude these things?
Flags: needinfo?(nalexander)
See our old bugbear Bug 901059.

We happily include stuff like:

@BINPATH@/chrome/toolkit@JAREXT@
@BINPATH@/chrome/toolkit.manifest

so that leads to the following situation (with my old frontend build, so the numbers will vary):

objdir-frontend]$ find . -name 'global'
...
./dist/bin/chrome/toolkit/content/global
./dist/bin/chrome/toolkit/skin/classic/global
./toolkit/themes/faststripe/global
./toolkit/themes/windows/global

and

objdir-frontend]$ du -hs dist/bin/chrome/toolkit/**/global
2.3M	dist/bin/chrome/toolkit/content/global
952K	dist/bin/chrome/toolkit/skin/classic/global
Blocks: fatfennec
OS: Unspecified → Android
Hardware: Unspecified → All
The particular fatfennec bug that tracks this kind of thing is Bug 1044079.
Blocks: 1044079
No longer blocks: fatfennec
The change in question caused a bunch of new theme files to be added to the Android build:

omni-small is the previous changeset omnijar and omni-large is the omnijar from the changeset in question

mfinkle-macmini:Downloads mfinkle$ diff -rq omni-large/ omni-small/
Files omni-large/chrome/chrome.manifest and omni-small/chrome/chrome.manifest differ
Only in omni-large/chrome/toolkit/skin/classic/global/console: console-toolbar-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/dirListing: folder-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/dirListing: local-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/dirListing: remote-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/dirListing: up-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: Error-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: Landscape-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: Portrait-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: Print-preview-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: Question-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: Search-close-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: Search-glass-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: Warning-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: autoscroll-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: blacklist_favicon-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: blacklist_large-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: error-16-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: error-64-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: folder-item-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: information-16-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: information-24-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: information-32-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: question-16-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: question-64-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: sslWarning-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: warning-16-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: warning-64-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: warning-large-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/icons: windowControls-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/printpreview: arrow-left-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/printpreview: arrow-left-end-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/printpreview: arrow-right-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/printpreview: arrow-right-end-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/toolbar: spring-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/tree: sort-asc-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/tree: sort-dsc-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/tree: twisty-clsd-XP.png
Only in omni-large/chrome/toolkit/skin/classic/global/tree: twisty-open-XP.png
Only in omni-large/chrome/toolkit/skin/classic: help
Only in omni-large/chrome/toolkit/skin/classic/mozapps/downloads: downloadButtons-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/downloads: downloadIcon-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/extensions: category-available-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/extensions: category-discover-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/extensions: category-plugins-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/extensions: category-recent-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/extensions: extensionGeneric-16-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/extensions: localeGeneric-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/extensions: themeGeneric-16-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/extensions: themeGeneric-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/plugins: pluginBlocked-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/plugins: pluginGeneric-16-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/plugins: pluginGeneric-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/profile: profileicon-XP.png
Only in omni-large/chrome/toolkit/skin/classic/mozapps/update: downloadButtons-XP.png
(In reply to :Margaret Leibovic from comment #1)
> We do package toolkit code in our APK, and I wonder if with this change
> we're accidentally packaging things we don't need.
> 
> Nick, do you have an understanding of how these toolkit themes are packaged
> on Android? Do we need some additional build logic to exclude these things?

Such logic looks like https://hg.mozilla.org/integration/fx-team/rev/98befa2ee036#l3.65 and other conditional includes.  Unfortunately there is no magic here.  I don't think we'll every convince folks that non-XUL apps consume toolkit/ and don't want Desktop XUL theming.  I guess we could try not shipping toolkit/skin in Android, but teasing out dependencies here is basically impossible.
(In reply to Nick Alexander :nalexander from comment #5)
> I don't think we'll ever convince folks that non-XUL apps consume toolkit/ and don't want
> Desktop XUL theming.

I think we should totally try to do so.
tracking-fennec: --- → ?
status-firefox44: --- → unaffected
status-firefox45: --- → affected
Summary: APK size jumped after theming change → APK size increased by 100KB after toolkit theming change
(In reply to Richard Newman [:rnewman] from comment #6)
> (In reply to Nick Alexander :nalexander from comment #5)
> > I don't think we'll ever convince folks that non-XUL apps consume toolkit/ and don't want
> > Desktop XUL theming.
> 
> I think we should totally try to do so.

I think you'll find that many folks including the toolkit module owner are already convinced that non-XUL apps consume toolkit.

Do we need any of the skin files in Fennec?
(In reply to Dave Townsend [:mossop] from comment #7)

> Do we need any of the skin files in Fennec?

The answer *should* be none -- even stuff like aboutReader.css are already forked into mobile/android/themes/core.

It'd be nice to find out for sure; I have my doubts around about:config icons and stuff like netError background images.
Attached patch nothemes WIP (obsolete) — — Splinter Review 
Simple, brute-force patch to exclude all theme files from a Fennec build

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6fb8e48b3ee
FWIW, I'm sorry my patches did this thing; I understand you don't want toolkit themes - though I do also wonder how you weren't shipping toolkit/themes things before today? Is this "just" an exacerbation of an already-bad situation, or if not, what specifically is now new (is that comment #4 ? I'm having a hard time telling) and why, exactly.

Let me know if I can help in any practical sense, like reviewing patches!
(In reply to :Gijs Kruitbosch from comment #10)

> Is this "just" an exacerbation of an
> already-bad situation, or if not, what specifically is now new (is that
> comment #4 ? I'm having a hard time telling) and why, exactly.

The difference here, indeed, is Comment 4 -- those are the new files that ended up in omni.ja after Bug 1210703.

I would call this an exacerbation: our packaging has always been imprecise (read: terrible and lazy; see Bug 901059), and this change apparently happened to shift a bunch of resources from toolkit directories that Fennec managed to exclude into places that it includes.

I trust you and Mossop to figure out if there's anything more to learn from this in the way resources are laid out or #defined out of toolkit!


> Let me know if I can help in any practical sense, like reviewing patches!

Thanks! I reckon mfinkle'll take you up on that if his try build succeeds…
By the way, we discovered this via Perfherder, which now shows truly useful graphs and stats for parts of our installers. You can see this jump in the graph for omni.ja here:

https://treeherder.mozilla.org/perf.html#/graphs?series=[fx-team,1bd21db54104c371a26f98d62338b934223b3390,1]

I'd love to see looking at those graphs be as routine as looking at treeherder for test failures. 

Eventually we'll get to the point of automation, at which point this change would have failed CI for regressing installer size and been automatically backed out, but we're not there yet.
(In reply to :Gijs Kruitbosch from comment #10)
> Is this "just" an exacerbation of an
> already-bad situation, or if not, what specifically is now new (is that
> comment #4 ? I'm having a hard time telling) and why, exactly.

You removed the XP_WIN ifdef from toolkit/themes/windows/global/jar.mn and hence we're shipping Windows-only files on platforms that aren't Gtk, Qt, Windows or OS X. We should revert this.
Flags: needinfo?(gijskruitbosch+bugs)
Component: General → Themes
Product: Firefox for Android → Toolkit
(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Is this "just" an exacerbation of an
> > already-bad situation, or if not, what specifically is now new (is that
> > comment #4 ? I'm having a hard time telling) and why, exactly.
> 
> You removed the XP_WIN ifdef from toolkit/themes/windows/global/jar.mn and
> hence we're shipping Windows-only files on platforms that aren't Gtk, Qt,
> Windows or OS X. We should revert this.

Should we? :-)

TBH, I don't think we have any tier-1 or even tier-2 platforms (I believe the beos port is dead, but someone else gets to confirm that) that fall in that category besides fennec.

I would therefore prefer to make another elif clause in https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/moz.build and package only the things fennec actually needs, with a specific jar.mn file. I can't think of much that would fall in that category besides scrollbars.css (see trypush), videocontrols.css and whatever else is necessary for that.

IMO, this idea that themes/windows/ doesn't actually mean windows but "windows and everything else" needs to die. It is counterintuitive and makes maintaining this code a lot harder than it needs to be. In all the time I've touched it I never thought "gosh, I wonder if doing this breaks fennec".

Do you feel this is a bad idea? Why?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dao)
(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > (In reply to :Gijs Kruitbosch from comment #10)
> > > Is this "just" an exacerbation of an
> > > already-bad situation, or if not, what specifically is now new (is that
> > > comment #4 ? I'm having a hard time telling) and why, exactly.
> > 
> > You removed the XP_WIN ifdef from toolkit/themes/windows/global/jar.mn and
> > hence we're shipping Windows-only files on platforms that aren't Gtk, Qt,
> > Windows or OS X. We should revert this.
> 
> Should we? :-)
> 
> TBH, I don't think we have any tier-1 or even tier-2 platforms (I believe
> the beos port is dead, but someone else gets to confirm that) that fall in
> that category besides fennec.

Sounds like you don't really know that, nor do I. And who knows what other ports might be added or resurrected later.

> "windows and everything else" needs to die. It is counterintuitive and makes
> maintaining this code a lot harder than it needs to be.

From my experience I don't think it makes anything measurably harder.

> In all the time I've
> touched it I never thought "gosh, I wonder if doing this breaks fennec".

Exactly. Contrary to what you said above, it's usually just not a concern. So we should fix this regression, keep Windows as a fallback for obscure platforms and probably be smarter about what we do on Android regardless.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #14)
> > (In reply to Dão Gottwald [:dao] from comment #13)
> > > (In reply to :Gijs Kruitbosch from comment #10)
> > > > Is this "just" an exacerbation of an
> > > > already-bad situation, or if not, what specifically is now new (is that
> > > > comment #4 ? I'm having a hard time telling) and why, exactly.
> > > 
> > > You removed the XP_WIN ifdef from toolkit/themes/windows/global/jar.mn and
> > > hence we're shipping Windows-only files on platforms that aren't Gtk, Qt,
> > > Windows or OS X. We should revert this.
> > 
> > Should we? :-)
> > 
> > TBH, I don't think we have any tier-1 or even tier-2 platforms (I believe
> > the beos port is dead, but someone else gets to confirm that) that fall in
> > that category besides fennec.
> 
> Sounds like you don't really know that, nor do I.

I'm actually fairly sure beos is dead, but I can find no official "it's dead" confirmation because of our stereotypical laziness in making explicit that "thing X is dead". Last useful things on the internet are from 2007/2008, though. The ifdefs were useful for linux, but that's now been factored out.

This is our official list:

https://developer.mozilla.org/en/docs/Supported_build_configurations

I don't see anything on there that would benefit from the windows/ code that isn't actually windows - darwin and osxppc presumably use osx/, all the *bsd ports use linux/ because they use gtk/qt.

> And who knows what other
> ports might be added or resurrected later.

Leaving code around "just in case" is not a good argument. We have hg's history to resurrect from, and we're not even actually removing much besides an ifdef (though I'd like to get rid of the os=winnt checks in the overrides, too). On top of that, shipping extra files isn't exactly breaking things.

> > "windows and everything else" needs to die. It is counterintuitive and makes
> > maintaining this code a lot harder than it needs to be.
> 
> From my experience I don't think it makes anything measurably harder.

Because we essentially do nothing about it. This is a recipe for problems such as these: we don't consider <unknown consumer>, we make those people sad at some point when it starts mattering. Best to be explicit about "this is a windows theme, if you want to use it, you do the work" instead of the half-baked support we have now.

> > In all the time I've
> > touched it I never thought "gosh, I wonder if doing this breaks fennec".
> 
> Exactly. Contrary to what you said above, it's usually just not a concern.
> So we should fix this regression, keep Windows as a fallback for obscure
> platforms

I really don't buy that we have to do this. If people want this for their new port/special-snowflake-OS they can opt in explicitly by copying the relevant jar.mn stuff and/or files.
(In reply to :Gijs Kruitbosch from comment #16)
> Leaving code around "just in case" is not a good argument. We have hg's
> history to resurrect from, and we're not even actually removing much besides
> an ifdef (though I'd like to get rid of the os=winnt checks in the
> overrides, too). On top of that, shipping extra files isn't exactly breaking
> things.

We're not leaving any code around, we're talking about reusing what's already there and using one ifdef to single out files that definitely make only sense on Windows. This discussion and your research on BeOS has already consumed more time than we had spent on this in the last couple of years...

> > > "windows and everything else" needs to die. It is counterintuitive and makes
> > > maintaining this code a lot harder than it needs to be.
> > 
> > From my experience I don't think it makes anything measurably harder.
> 
> Because we essentially do nothing about it. This is a recipe for problems
> such as these: we don't consider <unknown consumer>, we make those people
> sad at some point when it starts mattering. Best to be explicit about "this
> is a windows theme, if you want to use it, you do the work" instead of the
> half-baked support we have now.

Yep, we did nothing about it, never promised any particular support beyond the fallback, and we weren't asked for more. Those people usually have dealt with any related issues themselves. Let's not construct a problem where there's no none.
(In reply to Dão Gottwald [:dao] from comment #17)
> (In reply to :Gijs Kruitbosch from comment #16)
> > Leaving code around "just in case" is not a good argument. We have hg's
> > history to resurrect from, and we're not even actually removing much besides
> > an ifdef (though I'd like to get rid of the os=winnt checks in the
> > overrides, too). On top of that, shipping extra files isn't exactly breaking
> > things.
> 
> We're not leaving any code around, we're talking about reusing what's
> already there and using one ifdef to single out files that definitely make
> only sense on Windows. This discussion and your research on BeOS has already
> consumed more time than we had spent on this in the last couple of years...

It wasn't just 1 ifdef. I spent more time on getting these seemingly-redundant nested ifdefs right in bug 1150417 and friends, as well as bug 1210703, than I have spent in this discussion. Based on my experience, I disagree that keeping this is almost-zero-cost.

We should fix android to use only what it needs and leave it at that.
The changes in bug 1150417 seem completely agnostic about whether or not windows/ is used as a fallback. Bug 1210703 obviously caused this bug, so no surprise there. Changing how we build linux/ in relation to windows/ isn't something we do regularly, though.

I absolutely support fixing Android because there's a larger size win waiting. At the same time, we should just restore that erroneously removed ifdef; had I noticed that during review I would have directly asked that you don't make that change in the first place.
(In reply to Dão Gottwald [:dao] from comment #19)
> The changes in bug 1150417 seem completely agnostic about whether or not
> windows/ is used as a fallback.

No. The extra ifdefs I added there had to be duplicated in windows/ because of the XP_WIN ifdef, and that got exacerbated with the seamonkey/thunderbird support in followup bugs.

In any case, my point in comment #14 is largely conceptual rather than concrete. Having files under windows/ which we use on non-windows is counterintuitive. Enumerating which changes in those files have been hindered is the smaller problem. New contributors aren't going to know the history of this stuff, and it is just confusing that the themes work the way they do ("Why do we ifdef XP_WIN in this file that is already in toolkit/themes/windows ?"), so I think we should get rid of this confusion. The only reasons I've heard from you to keep it are
- you believe the confusion and practical impact are not a large enough problem to warrant getting rid of it
- hypothetical ports that might be resurrected later

I don't think either of those reasons should outweigh code clarity and organization. If I misunderstand or misrepresent your argument, then please elaborate.

Now that we have a shared windows/linux manifest, I would actually prefer moving the files it sources to shared/ (or lindows/ or nonmac/ or something if you prefer not getting this confused with things also in use on OS X) to further reduce confusion. I can file a bug on that, but it sounds like you would be opposed to that, too.

> At the same time, we should just restore that erroneously removed
> ifdef; had I noticed that during review I would have directly asked that you
> don't make that change in the first place.

And I would have asked to keep them there - it was no accident that I removed them. :-)
tracking-fennec: ? → 45+
Trying to just skip 'theme' for Android resulted in a crash at startup. We try to force load scrollbars.css:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsLayoutStylesheetCache.cpp#73

I then tried to just use 'faststripe' and only that theme for Android. That had better results, but we hit a lot of assertion miscounts:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee5ebe65b5f

I think the assertion miscounts are due to a missing 'global.css' file. Note that running the build on my device doesn't show any obvious problems.

Next, I copy 'faststripe' to 'mobile' and add an empty 'global.css' file. Let's see how this one works:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf4fcd76831
Clearing NI since mfinkle is running with this.  Thrilled to see Desktop team is interested in pulling XUL definitions out of toolkit \o/
Flags: needinfo?(nalexander)
Attached patch nothemes WIP 2 (obsolete) — — Splinter Review 
This patch continues to add some CSS files to 'mobile/global' in order to stop the assertion debug test failures.

I added two images (Error.png and key-16.png) purely to get two mochitests to pass.

I also disabled the 'native-theme' and 'xul' related reftests for Android.

The results are very good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8995d98ee68

Failures are:
1. layout/style/crashtests/1221902.html crashing with some strange looping CSS file loading
  I don't know what's happening here

2. layout/generic/crashtests/400768-1.xhtml | assertion count 1 is more than expected 0 assertions
  might just need preferences.css added to the theme
Attachment #8685637 - Attachment is obsolete: true
(In reply to Mark Finkle (:mfinkle) from comment #23)

> The results are very good:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8995d98ee68
> 
> Failures are:
> 1. layout/style/crashtests/1221902.html crashing with some strange looping
> CSS file loading
>   I don't know what's happening here
> 
> 2. layout/generic/crashtests/400768-1.xhtml | assertion count 1 is more than
> expected 0 assertions
>   might just need preferences.css added to the theme

Yes. Adding 'preferences.css' has cleared this failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f09cee26454
(In reply to Mark Finkle (:mfinkle) from comment #23)

> The results are very good:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8995d98ee68
> 
> Failures are:
> 1. layout/style/crashtests/1221902.html crashing with some strange looping
> CSS file loading
>   I don't know what's happening here

This crashtest was backed out!!!! 
https://bugzilla.mozilla.org/show_bug.cgi?id=1221902#c11
Attached patch nothemes v0.1 — — Splinter Review 
OK, let's get this in review.

Mossop - Take a look at the way I setup the 'mobile' theme. I created an 'empty.css' and use it for all the XUL CSS files. I didn't see a need to make separate empty files. The use of the 'mobile' theme is behind a NIGHTLY_BUILD flag until we are sure nothing is broken, which could take some time. I package real assets for plugins and media contrls. I also threw two PNGs in there to keep a chrome:// test from failing. It seemed like a good test to keep, but I didn't want to fix it.

Geoff - I disable some tests in this patch. Many XUL reftests, which make little sense for an app that never shows any XUL UI, but also break since we are not shipping a real theme anymore. I switched some newly busted tests from "fail-if" to "skip-if" just to make it clear that we do not expect these to be run at all. I am shipping two (2) PNGs just to make a test not fail, and I didn't want to hack the test.

Margaret - You should take a quick look at be aware of the changes. I tried to make sure the 'mobile' theme used real skin for plugins and video controls. Even though we override most of the video images, I saw a few that still pointed to 'global\media' so I added them.

QA and others should be looking for in-content bustage. Things that come to mind are:
* Plugins
* Video/Audio
* in-content UI like about:addons, about:logins and about:downloads

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=112b72bf8c2e

(one crashtest seems intermittent)
Assignee: nobody → mark.finkle
Attachment #8688478 - Attachment is obsolete: true
Attachment #8689125 - Flags: review?(margaret.leibovic)
Attachment #8689125 - Flags: review?(gbrown)
Attachment #8689125 - Flags: review?(dtownsend)
Comment on attachment 8689125 [details] [diff] [review]
nothemes v0.1

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

I would not mind if you added comments beside some skip-if's. eg "# No real theme on Android", or such.
Attachment #8689125 - Flags: review?(gbrown) → review+
Comment on attachment 8689125 [details] [diff] [review]
nothemes v0.1

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

I like the look of this. If you're desperate for bytes of space then I think you might get even smaller by only packaging empty.css once and instead using chrome overrides in jar.mn to override all those chrome urls to the empty file.

You might need the build peers to sign off on the moz.build stuff, I forget what their policy is these days.

::: toolkit/themes/moz.build
@@ +22,5 @@
>  if toolkit == 'cocoa':
>      DIRS += ['osx']
>  elif toolkit in ('gtk2', 'gtk3', 'qt'):
>      DIRS += ['linux']
> +elif app == 'mobile/android':

could just "and CONFIG['NIGHTLY_BUILD']" here.
Attachment #8689125 - Flags: review?(dtownsend) → review+
Comment on attachment 8689125 [details] [diff] [review]
nothemes v0.1

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

Without knowing much about toolkit themes, seems fine to me.

::: toolkit/themes/mobile/jar.mn
@@ +41,5 @@
> +
> +   skin/classic/global/icons/Error.png                     (global/icons/Error.png)
> +
> +% skin mozapps classic/1.0 %skin/classic/mozapps/
> ++  skin/classic/mozapps/plugins/pluginProblem.css          (mozapps/plugins/pluginProblem.css)

Is this really the only real toolkit CSS file we need? How do video controls work?
Attachment #8689125 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #29)
> Comment on attachment 8689125 [details] [diff] [review]
> nothemes v0.1
> 
> Review of attachment 8689125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Without knowing much about toolkit themes, seems fine to me.
> 
> ::: toolkit/themes/mobile/jar.mn
> @@ +41,5 @@
> > +
> > +   skin/classic/global/icons/Error.png                     (global/icons/Error.png)
> > +
> > +% skin mozapps classic/1.0 %skin/classic/mozapps/
> > ++  skin/classic/mozapps/plugins/pluginProblem.css          (mozapps/plugins/pluginProblem.css)
> 
> Is this really the only real toolkit CSS file we need? How do video controls
> work?

We ship our own videocontrols.css override -> touchcontrols.css
https://hg.mozilla.org/mozilla-central/rev/67a9dd1470e7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1227997
(In reply to Mark Finkle (:mfinkle) (use needinfo?) from comment #23)
> I also disabled the 'native-theme' and 'xul' related reftests for Android.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8995d98ee68

mfinkle (or gbrown), do you recall why we needed to disable the 'native-theme' reftests here? (Apologies if it's obvious; I'm not clear on the exact scope of this bug w.r.t. theming.)

I'm asking because currently, the annotation looks like this:
> # native-theme/
> # skipping for B2G since something around radio-nonnative.html makes the whole suite hang
> skip-if(B2G||Android||Mulet) include native-theme/reftest.list
...and we're dropping the B2G annotations in bug 1307332, so it'll just be skip-if(Android) now -- and I'm wondering what we should update the comment to say.

(Right now the patch over there just substitutes in Android for B2G in the comment, but I suspect that might not be correct.)
Flags: needinfo?(mark.finkle)
Flags: needinfo?(gbrown)
To put it more clearly/directly: could someone involved with this bug suggest a concise code-comment that we can use to document why we're skipping the "native-theme" reftest directory for Android?
(In reply to Daniel Holbert [:dholbert] from comment #34)
> To put it more clearly/directly: could someone involved with this bug
> suggest a concise code-comment that we can use to document why we're
> skipping the "native-theme" reftest directory for Android?

mfinkle has left, so I'll take a stab: quite some of the stuff in that dir tests XUL things that android doesn't support (e.g. resizers, menulists, search inputs, ...). Presumably that is why. That said, I'm not convinced some of the HTML tests couldn't be made to work on android - we'd just have to ifdef out all/most of the XUL ones, I imagine. Probably best off in a separate followup bug. For further needinfo I'd probably suggest asking rnewman.

(What the value of some of the tests is is also a bit interesting, in my extremely humble opinion... checking that <input type=radio> renders something other than about:blank does test something, but not an awful lot...)
[14:48:50]	mfinkle	Gijs: thanks for the backup on bug 1223526
[14:48:50]	firebot	https://bugzil.la/1223526 — FIXED, mark.finkle@gmail.com — APK size increased by 100KB after toolkit theming change
[14:48:51]	mfinkle	you are exactly right
[14:49:17]	mfinkle	also, ..., I can't really log in
[14:59:26]	Gijs	mfinkle: heh, want me to post that for you?
[14:59:50]	mfinkle	yeah, or just remove my need-info
[15:00:01]	mfinkle	you covered it
[15:04:15]	Gijs	can do
Flags: needinfo?(mark.finkle)
Flags: needinfo?(gbrown)
Cool -- so I think the comment we want to end up with here is some form of:
  // no XUL theme on Android

(I'm just now noticing that https://hg.mozilla.org/integration/fx-team/rev/67a9dd1470e7 added that comment for other reftest skip-if(Android) annotations -- it was just missing this one.)

Thanks, everyone!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: