Closed
Bug 1044334
Opened 10 years ago
Closed 10 years ago
Fix resource dependency issues
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: rnewman, Assigned: rnewman)
References
Details
Attachments
(6 files, 5 obsolete files)
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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!
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
I'm hazy about how this worked at all.
Attachment #8483955 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Same again.
Attachment #8483960 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 7•10 years ago
|
||
Same approach, but for prefs.
Attachment #8483961 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
Made a couple of improvements.
Attachment #8483968 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
> java.lang.IllegalStateException: Circular dependencies cannot exist in RelativeLayout
Guess there's an invalid shortcut in my stubs…
Assignee | ||
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
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.
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8484429 [details] [diff] [review]
Part 1: move HistoryListView style. v3
Goddamnit, bzapi.
Attachment #8484429 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8483955 -
Attachment is obsolete: true
Attachment #8483955 -
Flags: feedback?(lucasr.at.mozilla)
Assignee | ||
Comment 18•10 years ago
|
||
Tested.
Assignee | ||
Updated•10 years ago
|
Attachment #8483959 -
Attachment is obsolete: true
Attachment #8483959 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8483968 -
Attachment is obsolete: true
Attachment #8483968 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8483960 -
Attachment is obsolete: true
Attachment #8483960 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
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
Assignee | ||
Comment 22•10 years ago
|
||
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•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8483962 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 24•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8484430 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8484466 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8484469 -
Flags: review?(lucasr.at.mozilla)
Comment 25•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8484469 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 26•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
Verified that appearance is the same as Nightly for history view in portrait and landscape on Nexus 10 and Kindle Fire.
Assignee | ||
Comment 28•10 years ago
|
||
Comment 29•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•