773050 - Move Sync files to a well-thought-out location and resolve layout discrepancies viz android-sync

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Build & Test, defect)

Description

[Chris Peterson [:cpeterson]]

In an email conversation with rnewman, he recommended:

"[The Sync code] in HG is a snapshot, so it makes sense to move out of mobile.

But Sync alone makes up something like 25% of Fennec's total code size, and it's de facto a major module, so it seems weird for it to end up crunched into a subdirectory (that's not /services/, at least).

...

if you're thinking about changes that involve moving third party code out of mobile/ and into other directories, which will involve a bunch of Makefile changes, I'd be inclined to see if we can get to a situation where:

* Sync moves to a more sane place, like services/android/ (perhaps with subdirs for {sync,aitc,notifications,persona,deps})

* We consider taking the opportunity to move to a structure that matches the Git layout more closely (or even 100%).

Comment 1

[Richard Newman [:rnewman]]

CCing folks who might have thoughts on this. gps, mconnor: note that this would leave us with:

services/
  aitc/
  android/              << NEW
    ... all kinds of AITC/Sync/Persona code
  common/
  crypto/
  notifications/
  sync/

which doesn't seem ideal, but then the alternative is for chunks of Services Integration code to end up scattered widely around the tree. I also think it's better than living in mobile/android/base/sync.

[[
I'm also killing Target Milestone, because we track that for landings not targets (I know, I know).

Would someone with Bugzilla magic privs please set tracking-fennec to 17+?
]]

Comment 2

[Gregory Szorc [:gps]]

+1

Comment 3

[Nick Alexander :nalexander [he/him]]

In general, +1 to keeping Services Integration code in one place.

Cultural concern: I have heard that ServInt might function as an internal consultancy team, with ServInt devs embedded into different teams to help move Fx toward a Services-aware future.  If this is a long term goal, does separating ServInt code erect walls and/or assert code ownership counter to that goal?

Engineering concern: integration code will likely have tendrils into several parts of the greater code base.  How has that worked (with Desktop Sync) in the past?  With AITC?

Brass tacks: Java cares about path names.  How does this affect building Fennec?  Any hope for building a separate android-sync JAR that is linked into the Fennec APK?  I would love to make it faster to actually build Fennec, and having a separate sync build target that could be updated in the built APK (== signed aligned zip file) might reduce build times.

Comment 4

[Gregory Szorc [:gps]]

(In reply to Nick Alexander :nalexander from comment #3) 
> Cultural concern: I have heard that ServInt might function as an internal
> consultancy team, with ServInt devs embedded into different teams to help
> move Fx toward a Services-aware future.  If this is a long term goal, does
> separating ServInt code erect walls and/or assert code ownership counter to
> that goal?

Meh. This is more about consolidating related code for interaction with external services than it is trying to erect walls around team boundaries. We're all big one happy family. This isn't a large corporate enterprise where such political games are necessary.

> Engineering concern: integration code will likely have tendrils into several
> parts of the greater code base.  How has that worked (with Desktop Sync) in
> the past?  With AITC?

Services code is typically thought of as middleware. It's a standalone library with tentacles coming out in every direction. It can't belong elsewhere because that would be everywhere. The only important split is the divide between backend service and frontend interaction. Things in services/ are today currently backendy only. But, there's nothing that says we can't put full apps (like Android Sync) there, if warranted.

> Brass tacks: Java cares about path names.  How does this affect building
> Fennec?  Any hope for building a separate android-sync JAR that is linked
> into the Fennec APK?  I would love to make it faster to actually build
> Fennec, and having a separate sync build target that could be updated in the
> built APK (== signed aligned zip file) might reduce build times.

Patches welcome. Seriously, I doubt you will find someone else willing to do this legwork for you, unfortunately.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Nick Alexander :nalexander from comment #3)
> Cultural concern: I have heard that ServInt might function as an internal
> consultancy team, with ServInt devs embedded into different teams to help
> move Fx toward a Services-aware future.  If this is a long term goal, does
> separating ServInt code erect walls and/or assert code ownership counter to
> that goal?
> 
> Engineering concern: integration code will likely have tendrils into several
> parts of the greater code base.  How has that worked (with Desktop Sync) in
> the past?  With AITC?
> 
> Brass tacks: Java cares about path names.  How does this affect building
> Fennec?  Any hope for building a separate android-sync JAR that is linked
> into the Fennec APK?  I would love to make it faster to actually build
> Fennec, and having a separate sync build target that could be updated in the
> built APK (== signed aligned zip file) might reduce build times.

All these issues might be addressed if the Android Sync code was simply consolidated into a directory like mobile/android/sync. Sync is part of Fennec, but it is a distinct module.

Actually, mobile/android/sync already exists, but it only contains some .mn manifest files right now.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Chris Peterson (:cpeterson) from comment #5)

> > Brass tacks: Java cares about path names.  How does this affect building
> > Fennec?  Any hope for building a separate android-sync JAR that is linked
> > into the Fennec APK?  I would love to make it faster to actually build
> > Fennec, and having a separate sync build target that could be updated in the
> > built APK (== signed aligned zip file) might reduce build times.

I'm not particularly swayed about the Java path issue; source files can go pretty much anywhere. We do that already.

My first avenue for Fennec integration was actually to attempt to combine two JARs or APKs, and it didn't go so well (they need to be built together in order for Android resources to work), but interested parties are welcome to try again under less time pressure!

> Sync is part of Fennec, but it is a distinct module.

Yeah, the question is what's the best place for code that's currently primarily used in Fennec, but is a distinct module and might be part of other products in the future.

> Actually, mobile/android/sync already exists, but it only contains some .mn
> manifest files right now.

At the time we decided that source could/should go in base, and manifests would go elsewhere.

Comment 7

[Nick Alexander :nalexander [he/him]]

rnewman and I discussed deprecating the android-sync github repo today.  The motivation is to reduce development process overhead.  (For example, back-porting contributor patches from m-c to a-s.)

We agree on a few requirements:

* being able to run JUnit 3 and JUnit 4 test suites from m-c locally;
* being able to rev ch.boye and other third party libraries easily;
* keeping background services "pure" enough that other potential consumers (partners, etc) could integrate the existing android-sync code base into their products reasonably.

This ticket could form part of that work, since the first step is likely populating mobile/android/services and making the android-sync code a functioning Gradle-style project.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 773050 - Decouple base and services: TabReceivedService. r?mcomella

Bug 773050 - Decouple base and services: TabReceivedService. r?mcomella

This does a few things:

* Move TabReceivedService out of org.mozilla.gecko.sync;
* Add an <intent-filter>, so the service isn't referred to by name in
  the command processor;
* Use the existing Intent.EXTRA_TITLE instead of a custom field.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 773050 - Decouple base and services: SKIP_TAB_QUEUE_FLAG. r?mcomella

Bug 773050 - Decouple base and services: SKIP_TAB_QUEUE_FLAG. r?mcomella

This just moves a shared flag out of base and into the shared
BrowserContract to be consumed by services.

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 773050 - Decouple base and services: reflect BrowserLocaleManager. r?rnewman

Bug 773050 - Decouple base and services: reflect BrowserLocaleManager. r?rnewman

There are two pieces to this that are undesireable:

* using reflection to access BrowserLocaleManager;
* stuffing the contracts provided by base and consumed by services
  into the constants.jar.

Expedience.

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 773050 - Build services.jar to consume in gecko-browser.jar. r?rnewman

Bug 773050 - Build services.jar to consume in gecko-browser.jar. r?rnewman

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8679185 [details]
MozReview Request: Bug 773050 - Decouple base and services: reflect BrowserLocaleManager. r?rnewman

https://reviewboard.mozilla.org/r/23385/#review20917

This is fine, but I think a small amount of extra shuffling can eliminate the need for reflection here. s/constants/fundamentals and put BLM into fundamentals.

::: mobile/android/base/Locales.java:30
(Diff revision 1)
> +            final Class<?> clazz = Class.forName("org.mozilla.gecko.BrowserLocaleManager");

BLM's only significant dependency is on GeckoJarReader, and that's only for getPackagedLocaleTags, which we only use from preferences UI code, and thus wouldn't be hard to tease out.

Can we move BLM itself into a precursor package? ('constants'!) It's very glue-ey.

::: mobile/android/base/moz.build:15
(Diff revision 1)
>  constants_jar.sources = [

"constants" is now a pretty bad name… presumably fixed in a later commit.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8679186 [details]
MozReview Request: Bug 773050 - Build services.jar to consume in gecko-browser.jar. r?rnewman

https://reviewboard.mozilla.org/r/23387/#review20919

::: mobile/android/base/background/healthreport/HealthReportDatabaseStorage.java:478
(Diff revision 1)
> +    @SuppressWarnings("fallthrough")    

Trailing whitespace (throughout the entire diff).

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8679185 [details]
MozReview Request: Bug 773050 - Decouple base and services: reflect BrowserLocaleManager. r?rnewman

r+. See notes on the RB req.

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8679183 [details]
MozReview Request: Bug 773050 - Decouple base and services: TabReceivedService. r?mcomella

Bug 773050 - Decouple base and services: TabReceivedService. r?mcomella

This does a few things:

* Move TabReceivedService out of org.mozilla.gecko.sync;
* Use the existing Intent.EXTRA_TITLE instead of a custom field;
* Refer to the service by name in the command processor, to break a
  compile time dependency.

We'd like a static check that the service was launched but I don't
have a good pattern for this across module boundaries yet.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8679184 [details]
MozReview Request: Bug 773050 - Decouple base and services: SKIP_TAB_QUEUE_FLAG. r?mcomella

Bug 773050 - Decouple base and services: SKIP_TAB_QUEUE_FLAG. r?mcomella

This just moves a shared flag out of base and into the shared
BrowserContract to be consumed by services.

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8679185 [details]
MozReview Request: Bug 773050 - Decouple base and services: reflect BrowserLocaleManager. r?rnewman

Bug 773050 - Decouple base and services: reflect BrowserLocaleManager. r?rnewman

There are two pieces to this that are undesireable:

* using reflection to access BrowserLocaleManager;
* stuffing the contracts provided by base and consumed by services
  into the constants.jar.

Expedience.

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8679186 [details]
MozReview Request: Bug 773050 - Build services.jar to consume in gecko-browser.jar. r?rnewman

Bug 773050 - Build services.jar to consume in gecko-browser.jar. r?rnewman

Comment 19

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bedb42c88a54

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8679185 [details]
MozReview Request: Bug 773050 - Decouple base and services: reflect BrowserLocaleManager. r?rnewman

https://reviewboard.mozilla.org/r/23385/#review20975

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8679183 [details]
MozReview Request: Bug 773050 - Decouple base and services: TabReceivedService. r?mcomella

https://reviewboard.mozilla.org/r/23381/#review21213

::: mobile/android/base/db/BrowserContract.java:492
(Diff revision 2)
> +    public static final String TAB_RECEIVED_SERVICE_CLASS_NAME = "org.mozilla.gecko.tabqueue.TabReceivedService";

This seems really fragile. Is there any plan to improve this (e.g. preprocessing)?

::: mobile/android/base/sync/CommandProcessor.java:259
(Diff revision 2)
> -    context.startService(sendTabNotificationIntent);
> +    final ComponentName componentName = context.startService(sendTabNotificationIntent);

Unused variable?

::: mobile/android/base/tabqueue/TabReceivedService.java:5
(Diff revision 2)
> -package org.mozilla.gecko.sync;
> +package org.mozilla.gecko.tabqueue;

tabqueue still seems like a weird place to put this but with bug 1214332, wfm.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8679183 [details]
MozReview Request: Bug 773050 - Decouple base and services: TabReceivedService. r?mcomella

https://reviewboard.mozilla.org/r/23381/#review21219

> We'd like a static check that the service was launched but I don't
> have a good pattern for this across module boundaries yet.

What do you mean by this? What are you going to use it for?

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8679184 [details]
MozReview Request: Bug 773050 - Decouple base and services: SKIP_TAB_QUEUE_FLAG. r?mcomella

https://reviewboard.mozilla.org/r/23383/#review21221

Comment 24

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/e904bde0c73db52ec863f02f6136a7ee9ccb2850
Bug 773050 - Decouple base and services: TabReceivedService. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/fb54de74ddc0ebdca100b3581426a4a8121cb499
Bug 773050 - Decouple base and services: SKIP_TAB_QUEUE_FLAG. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/13c459f1f8a74dbe13b6202758a8b8ec16980fff
Bug 773050 - Decouple base and services: reflect BrowserLocaleManager. r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/36f84d4539503b3f6848f8ab49ea98b02dfef960
Bug 773050 - Build services.jar to consume in gecko-browser.jar. r=rnewman

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e904bde0c73d
https://hg.mozilla.org/mozilla-central/rev/fb54de74ddc0
https://hg.mozilla.org/mozilla-central/rev/13c459f1f8a7
https://hg.mozilla.org/mozilla-central/rev/36f84d453950