1408585 - Simplify away session's delegates

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, enhancement, P1)

Description

[:Grisha Kruglov]

Part 1 of (many), split from Bug 1408243. Let's start with some basic simplifications.

In RecordsChannel:
> "begin" work that sessions perform is all synchronous, so the "begin delegates" could be removed

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1408585 - Remove RepositorySession begin delegates

'begin' now throws in case things go wrong.

Review commit: https://reviewboard.mozilla.org/r/189376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189376/

Comment 2

[:Grisha Kruglov]

Comment on attachment 8918502 [details]
Bug 1408585 - Remove RepositorySession begin delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189376/diff/1-2/

Comment 3

[:Grisha Kruglov]

Comment on attachment 8918502 [details]
Bug 1408585 - Remove RepositorySession begin delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189376/diff/2-3/

Comment 4

[:Grisha Kruglov]

junit4 tests pass locally, and to my slight surprise robocop tests also pass on 'try' on the first... try.

Comment 5

[:Grisha Kruglov]

Comment on attachment 8918502 [details]
Bug 1408585 - Remove RepositorySession begin delegates

https://reviewboard.mozilla.org/r/189376/#review194628

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/synchronizer/RecordsChannel.java:350
(Diff revision 3)
> -  }
> -
> -  @Override
>    public RepositorySessionFetchRecordsDelegate deferredFetchDelegate(ExecutorService executor) {
>      // Lie outright. We know that all of our fetch methods are safe.
>      return this;

Note to self: ensure this is still fine.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #4)
> junit4 tests pass locally, and to my slight surprise robocop tests also pass
> on 'try' on the first... try.

There are no Sync/Services/FxA Robocop tests :)

Comment 7

[:Grisha Kruglov]

Surely, I meant junit3 tests :)

Comment 8

[:Grisha Kruglov]

Attachment: Bug 1408585 - Remove RepositorySession createSession delegates

TODO: generally appears to work, need to re-shape the tests

Review commit: https://reviewboard.mozilla.org/r/190352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190352/

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8918502 [details]
Bug 1408585 - Remove RepositorySession begin delegates

https://reviewboard.mozilla.org/r/189376/#review198980

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/RepositorySession.java:219
(Diff revision 3)
>    /**
>     * Start the session. This is an appropriate place to initialize
>     * data access components such as database handles.
>     *
> -   * @param delegate
>     * @throws InvalidSessionTransitionException

Fix the comments, too.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ServerSyncStage.java:323
(Diff revision 3)
>            @Override
>            public void onSessionCreated(final RepositorySession session) {
>              try {
> -              session.begin(new RepositorySessionBeginDelegate() {
> +              session.begin();
> +            } catch (SyncException e) {
> +              e.printStackTrace();

Do you really want to print the stacktrace here?

::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestBookmarks.java:626
(Diff revision 3)
> +      @Override
> +      public void onSessionCreated(final RepositorySession session) {
> +        try {
> +          session.begin();
> +        } catch (SyncException e) {
> +          performNotify("Begin failed", e);

Shouldn't you `return` here?

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/WBORepository.java:180
(Diff revision 3)
>  
>      @Override
> -    public void begin(RepositorySessionBeginDelegate delegate) throws InvalidSessionTransitionException {
> +    public void begin() throws SyncException {
>        this.wbos = wboRepository.cloneWBOs();
>        stats.begun = now();
> -      super.begin(delegate);
> +      super.begin();

Set `begun` after `begin` — we can do that now!

Comment 10

[:Grisha Kruglov]

Comment on attachment 8918502 [details]
Bug 1408585 - Remove RepositorySession begin delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189376/diff/3-4/

Comment 11

[:Grisha Kruglov]

Comment on attachment 8919521 [details]
Bug 1408585 - Remove RepositorySession createSession delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190352/diff/1-2/

Comment 12

[:Grisha Kruglov]

Comment on attachment 8919521 [details]
Bug 1408585 - Remove RepositorySession createSession delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190352/diff/2-3/

Comment 13

[:Grisha Kruglov]

Comment on attachment 8919521 [details]
Bug 1408585 - Remove RepositorySession createSession delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190352/diff/3-4/

Comment 14

[:Grisha Kruglov]

Comment on attachment 8919521 [details]
Bug 1408585 - Remove RepositorySession createSession delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190352/diff/4-5/

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8919521 [details]
Bug 1408585 - Remove RepositorySession createSession delegates

https://reviewboard.mozilla.org/r/190352/#review204336

::: mobile/android/services/src/androidTest/java/org/mozilla/gecko/background/sync/TestStoreTracking.java:133
(Diff revision 5)
>        public void onStoreFailed(Exception e) {
>  
>        }
>      };
>  
> +    Runnable doStore = new Runnable() {

I know it's test code, but `final`, here and below?

Comment 16

[:Grisha Kruglov]

Comment on attachment 8918502 [details]
Bug 1408585 - Remove RepositorySession begin delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189376/diff/4-5/

Comment 17

[:Grisha Kruglov]

Comment on attachment 8919521 [details]
Bug 1408585 - Remove RepositorySession createSession delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190352/diff/5-6/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/421ecf591543
Remove RepositorySession begin delegates r=rnewman
https://hg.mozilla.org/integration/autoland/rev/526f6a68b77d
Remove RepositorySession createSession delegates r=rnewman

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/421ecf591543
https://hg.mozilla.org/mozilla-central/rev/526f6a68b77d