Fix resource dependency issues

RESOLVED FIXED in Firefox 35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 35
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 5 obsolete attachments)

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
(Assignee)

Description

4 years ago
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

4 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

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

4 years ago
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8483955 [details] [diff] [review]
Part 1: move HistoryListView style. v1

I'm hazy about how this worked at all.
Attachment #8483955 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 5

4 years ago
Created attachment 8483959 [details] [diff] [review]
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.
Attachment #8483959 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 6

4 years ago
Created attachment 8483960 [details] [diff] [review]
Part 3: fix access to tabs_panel_footer in TabsPanel. v1

Same again.
Attachment #8483960 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 7

4 years ago
Created attachment 8483961 [details] [diff] [review]
Part 4: provide stub preference headers. v1

Same approach, but for prefs.
Attachment #8483961 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 8

4 years ago
Created attachment 8483962 [details] [diff] [review]
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.
Attachment #8483962 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 9

4 years ago
Created attachment 8483964 [details] [diff] [review]
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.
(Assignee)

Comment 11

4 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

4 years ago
Created attachment 8483968 [details] [diff] [review]
Part 2: conditionalize access to Tools and Page menus that only exist in v11+. v2

Made a couple of improvements.
Attachment #8483968 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 14

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

Guess there's an invalid shortcut in my stubs…
(Assignee)

Comment 15

4 years ago
Created attachment 8484429 [details] [diff] [review]
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.
(Assignee)

Comment 16

4 years ago
Created attachment 8484430 [details] [diff] [review]
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.
(Assignee)

Comment 17

4 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

4 years ago
Attachment #8483955 - Attachment is obsolete: true
Attachment #8483955 - Flags: feedback?(lucasr.at.mozilla)
(Assignee)

Comment 18

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

Tested.
(Assignee)

Updated

4 years ago
Attachment #8483959 - Attachment is obsolete: true
Attachment #8483959 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

4 years ago
Attachment #8483968 - Attachment is obsolete: true
Attachment #8483968 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 19

4 years ago
Created attachment 8484469 [details] [diff] [review]
Part 3: fix access to tabs_panel_footer in TabsPanel. v2
(Assignee)

Updated

4 years ago
Attachment #8483960 - Attachment is obsolete: true
Attachment #8483960 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 21

4 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

4 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 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

4 years ago
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+
(Assignee)

Updated

4 years ago
Attachment #8484430 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

4 years ago
Attachment #8484466 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

4 years ago
Attachment #8484469 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

4 years ago
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+

Updated

4 years ago
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+
(Assignee)

Comment 27

4 years ago
Verified that appearance is the same as Nightly for history view in portrait and landscape on Nexus 10 and Kindle Fire.
(Assignee)

Updated

4 years ago
Blocks: 1073474
You need to log in before you can comment on or make changes to this bug.