1044334 - Fix resource dependency issues

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Richard Newman [:rnewman]]

When building with limited SDK versions, we'll run into issues like

mobile/android/base/resources/layout/home_history_list.xml:17: error: Error: No resource found that matches the given name (at 'style' with value '@style/Widget.Home.HistoryListView').

This is because we have version-independent layouts (like home_history_list) that depend on version-dependent styles and images.

We'll need to fix this in order to build correctly with some resource directories excluded.

As far as I understand it, the fixes will include:

* If the feature is only used on some API levels, move the layout into a version-dependent directory.

* If the feature is used everywhere, then this is a legitimate error; move or duplicate the style.

Other thoughts welcome from folks more familiar with this than me!

Comment 1

[:Margaret Leibovic]

It sounds to me like we just have a bug in our styles.xml files. You can specify different styles.xml files for different API levels, so we should audit what we're currently doing to make sure we're not trying to use platform styles that don't exist.

The build error you included actually sounds like a bug that we're missing a style declaration somewhere (Widget.Home.HistoryListView is a style we define ourselves, so it doesn't necessarily need to depend on any API level). I actually only see that style declared for large devices, which is odd, since we definitely use that layout on all devices.

Comment 2

[Richard Newman [:rnewman]]

I should clarify what I'm doing here: building with all -v11 and up resources automatically excluded from the build. This isn't a platform limitation; it's our bug.

Comment 3

[Lucas Rocha (:lucasr)]

It would be nice if whatever solution we end up using here doesn't involve having copies of the same file just to enable us to make a clearer cut between pre and post ICS.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 1: move HistoryListView style. v1

I'm hazy about how this worked at all.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 2: conditionalize access to Tools and Page menus that only exist in v11+. v1

The approach I took here, in short: conditionalize any code that wasn't already, and establish fake IDs.

The fake IDs are necessary because aapt won't include resources that aren't applicable to its build constraints, but our code uses R.*, which is statically compiled. If we refer to R.id.foo, even within a version conditional, we'll get a compilation error due to a missing identifier.

Rather than preprocess files, I opted to introduce minimal stub resources to act as placeholders for the IDs.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Part 3: fix access to tabs_panel_footer in TabsPanel. v1

Same again.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 4: provide stub preference headers. v1

Same approach, but for prefs.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 5: expose firefox_settings_alert. v1

This is another case of "how does this work now?". The code isn't limited to v11+, but the images were only in v11 directories. I simply moved them.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Demo build patch. v1

For reference, this is how to specify a fixed version range to aapt. With this patch applied and no others, the build doesn't complete. With the other patches added, the build completes.

Testing will be via try (shortly), and manual tomorrow. Try *might* be inadequate, if we run into version compatibility nonsense; we'll see.

Comment 10

[Richard Newman [:rnewman]]

https://tbpl.mozilla.org/?tree=Try&rev=7b2af734fb6a

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8483955 [details] [diff] [review]
Part 1: move HistoryListView style. v1

Hmm, there's a pretty good chance, on self-review, that these changes are inverted. Too many buffers open. So let's set f? for now, and I'll get the details right in the morning!

Comment 12

[Richard Newman [:rnewman]]

Attachment: Part 2: conditionalize access to Tools and Page menus that only exist in v11+. v2

Made a couple of improvements.

Comment 13

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=659abbc1375d

Comment 14

[Richard Newman [:rnewman]]

> java.lang.IllegalStateException: Circular dependencies cannot exist in RelativeLayout

Guess there's an invalid shortcut in my stubs…

Comment 15

[Richard Newman [:rnewman]]

Attachment: Part 1: move HistoryListView style. v3

In order to match appearance on my test device (Kindle Fire), I had to alter the style. Which is strange. Without the alteration I got a 1px gap at the top of the history list in landscape mode.

Comment 16

[Richard Newman [:rnewman]]

Attachment: Part 1: move HistoryListView style. v3

In order to match appearance on my test device (Kindle Fire), I had to alter the style. Which is strange. Without the alteration I got a 1px gap at the top of the history list in landscape mode.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8484429 [details] [diff] [review]
Part 1: move HistoryListView style. v3

Goddamnit, bzapi.

Comment 18

[Richard Newman [:rnewman]]

Attachment: Part 2: conditionalize access to, and stub out, Tools and Page menus that only exist in v11+. v2

Tested.

Comment 19

[Richard Newman [:rnewman]]

Attachment: Part 3: fix access to tabs_panel_footer in TabsPanel. v2

Comment 20

[Richard Newman [:rnewman]]

https://tbpl.mozilla.org/?tree=Try&rev=fb7d333bed5d

Comment 21

[Richard Newman [:rnewman]]

OK, tested this both with and without version restriction. With, I got non-nested menus on my Kindle (correctly -- doesn't ship the v11+ resources!). Without, everything looks the same.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6548c6d1f93c

Comment 22

[Richard Newman [:rnewman]]

Testing this change alone -- instructing aapt to not ship anything inaccessible to v9 devices, with no other version-related pruning -- knocks 100KB off the APK size generated by my machine: 34096481 -> 33996093 bytes.

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 8483961 [details] [diff] [review]
Part 4: provide stub preference headers. v1

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

This is a bit hard to follow. The patch feels 'incomplete' somehow. I can understand the part where you're stubbing pre-v11 files but I don't see where the removed code is going (for v11+). Maybe a bit more context would help?

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 8483961 [details] [diff] [review]
Part 4: provide stub preference headers. v1

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

Looks good after realizing the patch applies to copies of the original pre-v11 files.

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 8484466 [details] [diff] [review]
Part 2: conditionalize access to, and stub out, Tools and Page menus that only exist in v11+. v2

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

Ok.

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 8484430 [details] [diff] [review]
Part 1: move HistoryListView style. v3

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

Not sure I understand the issue you described. I don't think this is worth an extra round of reviews though. Please test this patch on small and big tablets.

Comment 27

[Richard Newman [:rnewman]]

Verified that appearance is the same as Nightly for history view in portrait and landscape on Nexus 10 and Kindle Fire.

Comment 28

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/6111341ef0b0
https://hg.mozilla.org/integration/fx-team/rev/183dfd418acb
https://hg.mozilla.org/integration/fx-team/rev/a2963a88961b
https://hg.mozilla.org/integration/fx-team/rev/8c9289f2a35f
https://hg.mozilla.org/integration/fx-team/rev/e831725d77ce

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6111341ef0b0
https://hg.mozilla.org/mozilla-central/rev/183dfd418acb
https://hg.mozilla.org/mozilla-central/rev/a2963a88961b
https://hg.mozilla.org/mozilla-central/rev/8c9289f2a35f
https://hg.mozilla.org/mozilla-central/rev/e831725d77ce