1269406 - Implement BrowserAppDelegate subclass with WeakReference to BrowserApp

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andrzej Hunt :ahunt]

We seem to have multiple delegates that need a reference to BrowserApp.

Sebastian in Bug 1247689:
> We are using this pattern quite often. By now it might make sense to > create a new abstract class (based on BrowserAppDelegate) that does just > the reference keeping (and adds a handy getter).
>
> As more patterns emerge we can create more helpful delegate base classes.

Comment 1

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1269406 - Pre: move BrowserAppDelegates into their own package r?sebastian

related promotions (HomeScreenPromotion, ReaderViewBookmarkPromotion)
live in org.mozilla.gecko.promotions.

BookmarkStateChangeDelegate previously lived in promotions, however
it isn't really a promotion (we're just showing snackbars, and showing
one helper UI to explain offline reader view bookmarks _after_ such
a bookmark has been created), hence it seems a better fit for delegates.

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

Comment 2

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1269406 - Implement BrowserAppDelegateWithReference r?sebastian

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

Comment 3

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1269406 - Upgrade delegates needing BrowserApp references to use BrowserAppDelegateWithReference r?sebastian

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

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8747823 [details]
MozReview Request: Bug 1269406 - Pre: move BrowserAppDelegates into their own package r?sebastian

https://reviewboard.mozilla.org/r/50031/#review46977

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8747824 [details]
MozReview Request: Bug 1269406 - Implement BrowserAppDelegateWithReference r?sebastian

https://reviewboard.mozilla.org/r/50033/#review46979

::: mobile/android/base/java/org/mozilla/gecko/delegates/BrowserAppDelegateWithReference.java:14
(Diff revision 1)
> +
> +/**
> + * BrowserAppDelegate that stores a reference to the parent BrowserApp.
> + */
> +public abstract class BrowserAppDelegateWithReference extends BrowserAppDelegate {
> +

nit: It looks like the majority of our classes do not start with an empty line.

::: mobile/android/base/java/org/mozilla/gecko/delegates/BrowserAppDelegateWithReference.java:15
(Diff revision 1)
> +/**
> + * BrowserAppDelegate that stores a reference to the parent BrowserApp.
> + */
> +public abstract class BrowserAppDelegateWithReference extends BrowserAppDelegate {
> +
> +    private WeakReference<BrowserApp> mBrowserApp;

I think we tend to not use m-prefix for new code/classes.

::: mobile/android/base/java/org/mozilla/gecko/delegates/BrowserAppDelegateWithReference.java:18
(Diff revision 1)
> +public abstract class BrowserAppDelegateWithReference extends BrowserAppDelegate {
> +
> +    private WeakReference<BrowserApp> mBrowserApp;
> +
> +    @Override
> +    @CallSuper

Great annotation usage. :)

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8747825 [details]
MozReview Request: Bug 1269406 - Upgrade delegates needing BrowserApp references to use BrowserAppDelegateWithReference r?sebastian

https://reviewboard.mozilla.org/r/50035/#review46981

Comment 7

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/38765920cd071a9a8da4c25635de8578ef5867cf
Bug 1269406 - Pre: remove unnecessary imports r=me

https://hg.mozilla.org/integration/fx-team/rev/cd67607bb88d72431af3efa689eb07723c259f2b
Bug 1269406 - Pre: move BrowserAppDelegates into their own package r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/8fd03156c2b22db7bf62ebae7180c1509885aff7
Bug 1269406 - Implement BrowserAppDelegateWithReference r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/52b494bc4c42b6aa662057a139907a29da5d73ec
Bug 1269406 - Upgrade delegates needing BrowserApp references to use BrowserAppDelegateWithReference r=sebastian

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38765920cd07
https://hg.mozilla.org/mozilla-central/rev/cd67607bb88d
https://hg.mozilla.org/mozilla-central/rev/8fd03156c2b2
https://hg.mozilla.org/mozilla-central/rev/52b494bc4c42