Closed Bug 1044334 Opened 5 years ago Closed 5 years ago

Fix resource dependency issues

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(6 files, 5 obsolete files)

8.79 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.24 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.13 KB, patch
Details | Diff | Splinter Review
4.84 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
5.05 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
3.87 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
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!
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.
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.
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.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
I'm hazy about how this worked at all.
Attachment #8483955 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8483959 - Flags: review?(lucasr.at.mozilla)
Same again.
Attachment #8483960 - Flags: review?(lucasr.at.mozilla)
Same approach, but for prefs.
Attachment #8483961 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8483962 - Flags: review?(lucasr.at.mozilla)
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 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!
Attachment #8483955 - Flags: review?(lucasr.at.mozilla) → feedback?(lucasr.at.mozilla)
Made a couple of improvements.
Attachment #8483968 - Flags: review?(lucasr.at.mozilla)
> java.lang.IllegalStateException: Circular dependencies cannot exist in RelativeLayout

Guess there's an invalid shortcut in my stubs…
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.
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 on attachment 8484429 [details] [diff] [review]
Part 1: move HistoryListView style. v3

Goddamnit, bzapi.
Attachment #8484429 - Attachment is obsolete: true
Attachment #8483955 - Attachment is obsolete: true
Attachment #8483955 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8483959 - Attachment is obsolete: true
Attachment #8483959 - Flags: review?(lucasr.at.mozilla)
Attachment #8483968 - Attachment is obsolete: true
Attachment #8483968 - Flags: review?(lucasr.at.mozilla)
Attachment #8483960 - Attachment is obsolete: true
Attachment #8483960 - Flags: review?(lucasr.at.mozilla)
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
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 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?
Attachment #8483962 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8483961 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #8484430 - Flags: review?(lucasr.at.mozilla)
Attachment #8484466 - Flags: review?(lucasr.at.mozilla)
Attachment #8484469 - Flags: review?(lucasr.at.mozilla)
Blocks: 1063643
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.
Attachment #8484466 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #8484469 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8484430 - Flags: review?(lucasr.at.mozilla) → review+
Verified that appearance is the same as Nightly for history view in portrait and landscape on Nexus 10 and Kindle Fire.
Blocks: 1073474
You need to log in before you can comment on or make changes to this bug.