Don't import or refer to Adjust classes in AppConstants.java.in

RESOLVED FIXED in Firefox 41

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

Trunk
Firefox 41
All
Android
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Not only does this classload them when AppConstants is first used, but it also means I'm spending this afternoon fixing build errors in android-sync.

See also Bug 1161195.
(Assignee)

Comment 1

3 years ago
Created attachment 8612496 [details] [diff] [review]
Move Adjust-related stuff to AdjustConstants. v1
Attachment #8612496 - Flags: review?(nalexander)
Comment on attachment 8612496 [details] [diff] [review]
Move Adjust-related stuff to AdjustConstants. v1

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

You should be able to just include the StubAdjustHelper in android-sync, and set the flags in the preprocess.in file there.

Try that before this.  If it can't be made to work, explain to me why.  (It pretty much has to, since we don't ship the Adjust code when the build flag is off.)  I really don't want to add additional preprocessed Java.
Attachment #8612496 - Flags: review?(nalexander) → feedback-
(Assignee)

Comment 3

3 years ago
I think this is the correct approach, and what we have now is wrong.

1. Importing more stuff into android-sync is as painful as doing this.

2. Having imports and instances in AppConstants is pretty wretched from a philosophical perspective, not to mention a classloader perspective.

3. Doesn't the current approach force StubAdjustHelper into GeckoView consumers? This avoids that, because only our ReferrerReceiver and GeckoApplication would import AdjustConstants.

4. What's wrong with having a second preprocessed file right next to AppConstants?


I confirmed that this builds with Mach and IntelliJ, so I really don't see any reason not to land this patch.
(In reply to Richard Newman [:rnewman] from comment #3)
> I think this is the correct approach, and what we have now is wrong.
> 
> 1. Importing more stuff into android-sync is as painful as doing this.

This is the price we pay, periodically, to maintain the artificial separation.  As we tie Gecko and services together more (FxA sign-in via the web, for example) this separation will go away.
 
> 2. Having imports and instances in AppConstants is pretty wretched from a
> philosophical perspective, not to mention a classloader perspective.

imports?  No.  instances?  Yes.  But Java's just not that flexible with such indirection.

> 3. Doesn't the current approach force StubAdjustHelper into GeckoView
> consumers? This avoids that, because only our ReferrerReceiver and
> GeckoApplication would import AdjustConstants.

Yeah, but I'm not going to lose any sleep over this.  GV already ships the entirety of Fennec; it's no more pain to separate this out, if and when the day comes.

> 4. What's wrong with having a second preprocessed file right next to
> AppConstants?

I fought really hard to have a single preprocessed file.  There used to be a comment saying something like "don't add to this list" in the moz.build.  It remains in spirit.  As the principal build maintainer, it simplifies a lot of things to know that AppConstants.java is the only special snowflake.

Food for thought: we could conditionally #include some part of AppConstants.java.in and rework some of the code around how the instance is found.  That would handle your classloader concern, at least.

Two final positions.  First, I anticipated this situation when landing Adjust, and laid a clear blueprint for how it can be arranged.  Conditional compilation in Java is hard; in some sense I want to have a discussion (if not a battle) every time we try to arrange this.

Second, Adjust is special.  I think we either strip it entirely, in which case I don't want to touch it; or we start requiring it and use more of its functionality, in which case I would favour your approach.  So I would prefer keeping the build system as is for some time.

I'd like some time to think this over, and perhaps to discuss it face to face.  Can this stay in your tree for a day?
Flags: needinfo?(rnewman)
(Assignee)

Comment 5

3 years ago
Of course!
Flags: needinfo?(rnewman)
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/73ebf6ff32f9
(Assignee)

Comment 7

3 years ago
Comment on attachment 8612496 [details] [diff] [review]
Move Adjust-related stuff to AdjustConstants. v1

Nick reviewed in person.
Attachment #8612496 - Flags: feedback- → review+
(Assignee)

Updated

3 years ago
status-firefox41: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/73ebf6ff32f9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.