Closed Bug 1408585 Opened 3 years ago Closed 2 years ago

Simplify away session's delegates

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
junit4 tests pass locally, and to my slight surprise robocop tests also pass on 'try' on the first... try.
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.
See Also: → 1408710
(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 :)
Surely, I meant junit3 tests :)
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!
Attachment #8918502 - Flags: review?(rnewman) → review+
Priority: -- → P1
Summary: Simplify away session's begin delegates → Simplify away session's delegates
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?
Attachment #8919521 - Flags: review?(rnewman) → review+
Pushed by gkruglov@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/421ecf591543
https://hg.mozilla.org/mozilla-central/rev/526f6a68b77d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Target Milestone: Firefox 58 → Firefox 59
You need to log in before you can comment on or make changes to this bug.