Fix Eclipse build support for new tablet UI code/resources

RESOLVED FIXED in Firefox 37

Status

()

Firefox for Android
Build Config & IDE Support
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: lucasr, Assigned: nalexander)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 37
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Likely broken after bug 1065494 lands.
(Reporter)

Updated

3 years ago
No longer depends on: 1065494
Unfortunately, I think this is basically impossible with Eclipse.  We've maintained a fortuitous hack to allow multiple resource directories for a long time, but this might be the end of the road.

The issue is that Eclipse projects can only reference a single resource directory.  What we do is make a new project per resource directory, and make the projects depend on each other.  That works when there are no cyclic dependencies.  We have cyclic dependencies here.  I see no way forward, but I will let it rattle around for a bit.
lucasr: it appears that you only add resources in newtablet/res.  It might be awkward, but how would you feel about just prefixing the entirety of newtablet/res with new_tablet_ (most already are) and copying them into resources/?

I'm having a tough time getting anything with these merged resource directories to work in Android Studio, although Gradle handles this easily.
Flags: needinfo?(lucasr.at.mozilla)
Created attachment 8501444 [details] [diff] [review]
DO NOT LAND - Merge newtablet/res into resources.

This works around the newtablet resource issue for Eclipse users.

I'm throwing this up so that others can easily point impacted
developers to the ticket and so that folks can easily apply the
work-around.
(Reporter)

Comment 4

3 years ago
(In reply to Nick Alexander :nalexander from comment #2)
> lucasr: it appears that you only add resources in newtablet/res.  It might
> be awkward, but how would you feel about just prefixing the entirety of
> newtablet/res with new_tablet_ (most already are) and copying them into
> resources/?

I'd fine with that. The only reason I created the separate resource directory was because that seemed to be only way we have right now to optionally include resources in the build i.e. ANDROID_RES_DIRS += ...

Maybe we could have something like 'exclude_patterns' for resources? This way we'd be able to exclude any resources prefixed with new_tablet.

With that said, I'm a bit surprised that this is not working with Eclipse given that the CrashReporter seems to use a very similar pattern. How is it different than the new tablet stuff?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #4)
> (In reply to Nick Alexander :nalexander from comment #2)
> > lucasr: it appears that you only add resources in newtablet/res.  It might
> > be awkward, but how would you feel about just prefixing the entirety of
> > newtablet/res with new_tablet_ (most already are) and copying them into
> > resources/?
> 
> I'd fine with that. The only reason I created the separate resource
> directory was because that seemed to be only way we have right now to
> optionally include resources in the build i.e. ANDROID_RES_DIRS += ...

OK, good to know.

> Maybe we could have something like 'exclude_patterns' for resources? This
> way we'd be able to exclude any resources prefixed with new_tablet.

I'd be fine with this, as a temporary measure.  I'm not that concerned about temporarily shipping 50k of resources on Nightly and Aurora.

> With that said, I'm a bit surprised that this is not working with Eclipse
> given that the CrashReporter seems to use a very similar pattern. How is it
> different than the new tablet stuff?

It's delicate but it works because CR depends on the static resources (and not vice versa).  The issue here is that newtablet and the static resources have cyclic dependencies (at least, right now): newtablet relies on styles and strings (and some other things); but in turn, the static resources rely on newtablet.  I think it would be possible to extract shared resources into a common dependency, or to extract everything into newtablet, but that doesn't fit your optional include approach above.
Duplicate of this bug: 1088959
(In reply to Nick Alexander :nalexander from comment #3)
> I'm throwing this up so that others can easily point impacted
> developers to the ticket and so that folks can easily apply the
> work-around.

Please add to
https://wiki.mozilla.org/Mobile/Fennec/Android/Eclipse
Created attachment 8525799 [details]
MozReview Request: bz://1075797/nalexander
Attachment #8525799 - Flags: review?(lucasr.at.mozilla)
/r/819 - Bug 1075797 - Move new tablet resources into regular resources. r=lucasr
/r/821 - Bug 1075797 - Build system changes. r=lucasr

Pull down these commits:

hg pull review -r b33d9f554f0b5cef8b68a4ad4004d2ebea6d8a89
lucasr: I can't figure out how to comment on my review request.  I talked to you about doing this in IRC, and you were against because you wanted to minimize risk.  I don't see the risk here: none of the files in newtablet/res were present in resources.  The build changes are tiny.  Are you concerned that this will break in some way I don't understand if we flip the switch back to off?

Are you concerned that a move of this sort will make uplift hard?  I didn't think there was a realistic expectation of uplifting.
Attachment #8525799 - Flags: review?(rnewman)
/r/819 - Bug 1075797 - Move new tablet resources into regular resources. r=lucasr
/r/821 - Bug 1075797 - Build system changes. r=lucasr
/r/825 - Bug 1102339 - Don't generate widget/Themed*.java. r=rnewman

Pull down these commits:

hg pull review -r adc5346c5ad3821840ee9d4299bc1b0d53ca0499
https://reviewboard.mozilla.org/r/825/#review377

::: mobile/android/base/moz.build
(Diff revision 1)
> +    'widget/ThemedEditText.java',

Add a comment here to indicate that these are generated files? Or add them in a separate += clause?

::: mobile/android/base/widget/ThemedEditText.java
(Diff revision 1)
> +

I'd like a header at the top of each of these files that warns about them being generated from fragments.

::: mobile/android/base/widget/generate_themed_views.py
(Diff revision 1)
> +        

Trailing whitespace.

I'm in favor of flattening these into version control. My only concern is that we make it abundantly clear that they're generated files, so we don't get quiet regressions when someone edits the output and someone else overwrites it.
(Reporter)

Comment 13

3 years ago
Hey Nick, doing this now would mean we won't be able to exclude new tablet UI resources from the build if we, for some reason, decide to postpone the new UI to 37. Yes, this is unlikely to happen but I'd prefer to only commit to the new UI for real once this cycle is over. We can land this in the first day of the 37 cycle.
(Reporter)

Comment 14

3 years ago
Comment on attachment 8525799 [details]
MozReview Request: bz://1075797/nalexander

Patch looks fine to me. Please test this with tablet ui disabled.
Attachment #8525799 - Flags: review?(lucasr.at.mozilla) → review+
(Reporter)

Updated

3 years ago
Blocks: 1084523
See Also: → bug 1106935
Let's just call it a soft dependency.
Component: General → Build Config & IDE Support
Depends on: 1106935
See Also: bug 1106935
Attachment #8525799 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/integration/fx-team/rev/a616f915715d
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/a616f915715d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment on attachment 8525799 [details]
MozReview Request: bz://1075797/nalexander
Attachment #8525799 - Attachment is obsolete: true
Attachment #8618383 - Flags: review+
Attachment #8618384 - Flags: review+
Attachment #8618385 - Flags: review+
Created attachment 8618383 [details]
MozReview Request: Bug 1102339 - Don't generate widget/Themed*.java. r=rnewman
Created attachment 8618384 [details]
MozReview Request: Bug 1075797 - Move new tablet resources into regular resources. r=lucasr
Created attachment 8618385 [details]
MozReview Request: Bug 1075797 - Build system changes. r=lucasr
You need to log in before you can comment on or make changes to this bug.