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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Move Adjust-related stuff to AdjustConstants. v1

Comment 2

[Nick Alexander :nalexander [he/him]]

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.

Comment 3

[Richard Newman [:rnewman]]

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.

Comment 4

[Nick Alexander :nalexander [he/him]]

(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?

Comment 5

[Richard Newman [:rnewman]]

Of course!

Comment 6

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/73ebf6ff32f9

Comment 7

[Richard Newman [:rnewman]]

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

Nick reviewed in person.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73ebf6ff32f9