1266477 - Make BrowserApp simpler by delegating lifecycle events

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

BrowserApp is pretty large and growing constantly. A lot of code in there could live in its own class - it's is mostly only interested in the lifecycle events of BrowserApp.

AddToHomeScreenPromotion is already doing this partially: Instead of adding the code to BrowserApp, it is in its own class and resume/pause events are delegated to the class. If we generate a generic "delegate" class then we could move more code, for example the screenshot observer code, into its own classes and just delegate calls from BrowserApp.

This will also make it easier to fix bugs like bug 1266383 without adding even more code for a particular feature in BrowserApp.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1266477 - Introduce BrowserAppDelegate to extend the behavior of BrowserApp. r=mcomella,ahunt

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

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1266477 - Migrate AddToHomeScreenPromotion to extend BrowserAppDelegate. r=mcomella,ahunt

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

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1266477 - Migrate screenshot observer code to class extending BrowserAppDelegate. r=mcomella,ahunt

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

Comment 4

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

Comment on attachment 8743964 [details]
MozReview Request: Bug 1266477 - Introduce BrowserAppDelegate to extend the behavior of BrowserApp. r=mcomella,ahunt

https://reviewboard.mozilla.org/r/48201/#review45317

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:304
(Diff revision 1)
>      private boolean mHideWebContentOnAnimationEnd;
>  
>      private final DynamicToolbar mDynamicToolbar = new DynamicToolbar();
>      private final ScreenshotObserver mScreenshotObserver = new ScreenshotObserver();
>  
> +    private List<BrowserAppDelegate> delegates = Arrays.asList(

nit: `final`

::: mobile/android/base/java/org/mozilla/gecko/BrowserAppDelegate.java:14
(Diff revision 1)
> +
> +/**
> + * Abstract class for extending the behavior of BrowserApp without adding additional code to the
> + * already huge class.
> + */
> +public abstract class BrowserAppDelegate {

Why did you choose to make this an abstract class rather than an interface? I think I'd prefer an interface: it's more flexible (though we will quickly increase our method count! :d).

::: mobile/android/base/java/org/mozilla/gecko/BrowserAppDelegate.java:18
(Diff revision 1)
> + */
> +public abstract class BrowserAppDelegate {
> +    private BrowserApp browserApp;
> +
> +    public BrowserAppDelegate(BrowserApp browserApp) {
> +        this.browserApp = browserApp;

I'm not a fan of forcing all of the extenders to hold a reference to BrowserApp, just in-case they're ever passed to a context that outlives the Activity.

I'd prefer if this was a `WeakReference` or if the extenders had to manually take a reference to the activity.

Comment 5

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

Comment on attachment 8743966 [details]
MozReview Request: Bug 1266477 - Migrate AddToHomeScreenPromotion to extend BrowserAppDelegate. r=mcomella,ahunt

https://reviewboard.mozilla.org/r/48203/#review45319

::: mobile/android/base/java/org/mozilla/gecko/promotion/AddToHomeScreenPromotion.java:60
(Diff revision 1)
>      private int lastVisitMaximumAgeMs;
>  
> -    public AddToHomeScreenPromotion(Activity activity) {
> -        this.activity = activity;
> +    public AddToHomeScreenPromotion(BrowserApp browserApp) {
> +        super(browserApp);
>  
> +        this.activity = browserApp;

This reference is duplicated in the sub-class and this class.

Comment 6

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

Comment on attachment 8743964 [details]
MozReview Request: Bug 1266477 - Introduce BrowserAppDelegate to extend the behavior of BrowserApp. r=mcomella,ahunt

https://reviewboard.mozilla.org/r/48201/#review45321

::: mobile/android/base/java/org/mozilla/gecko/BrowserAppDelegate.java:18
(Diff revision 1)
> + */
> +public abstract class BrowserAppDelegate {
> +    private BrowserApp browserApp;
> +
> +    public BrowserAppDelegate(BrowserApp browserApp) {
> +        this.browserApp = browserApp;

Oh! If you chose to keep this reference in order to allow the delegates to access the Activity in their Activity callbacks, consider passing in the Activity in the callbacks!

Comment 7

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

Comment on attachment 8743964 [details]
MozReview Request: Bug 1266477 - Introduce BrowserAppDelegate to extend the behavior of BrowserApp. r=mcomella,ahunt

https://reviewboard.mozilla.org/r/48201/#review45323

Comment 8

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

Comment on attachment 8743967 [details]
MozReview Request: Bug 1266477 - Migrate screenshot observer code to class extending BrowserAppDelegate. r=mcomella,ahunt

https://reviewboard.mozilla.org/r/48205/#review45325

I love this – BrowserApp is going to get so squeeky clean! Maybe worth a blog post once we clean it up some more! ;)

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8743964 [details]
MozReview Request: Bug 1266477 - Introduce BrowserAppDelegate to extend the behavior of BrowserApp. r=mcomella,ahunt

https://reviewboard.mozilla.org/r/48201/#review45369

Looks good, with mcomellas fixes!

(I'll probably want to add onActivityResult() to the API, as part of Bug 1246238.)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/48201/#review45321

> Oh! If you chose to keep this reference in order to allow the delegates to access the Activity in their Activity callbacks, consider passing in the Activity in the callbacks!

That was actually my first implementation! :)

Then I was a bit annoyed that I needed to fish the reference out of onCreate() and keep it in the class in order to use it in other callbacks (ScreenshotObserver, onTabsChangedListener). Also the methods looked a bit polluted with all the BrowserApp parameters. So I changed BrowserAppDelegate to always get a reference via the constructor.
But with that every class implementing BrowserAppDelegate is forced to implement a constructor...
Right now I'm inclined to change it back.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/48201/#review45317

> Why did you choose to make this an abstract class rather than an interface? I think I'd prefer an interface: it's more flexible (though we will quickly increase our method count! :d).

When you'll write a class that implements the interface then you will be forced to implement ALL methods. This can be annoying here. Especially as we start to add more methods that are specific to BrowserApp (See bug 1266383). For this the Android framework often implements Simple* classes that are just an empty implementation of the interface (OnGestureListener / SimpleOnGestureListener). However I can't see the advantage of having the interface here - especially if everyone will extend the abstract class anyways.

> I'm not a fan of forcing all of the extenders to hold a reference to BrowserApp, just in-case they're ever passed to a context that outlives the Activity.
> 
> I'd prefer if this was a `WeakReference` or if the extenders had to manually take a reference to the activity.

(Regarding the reference see below).

The sole purpose of this class is to extend the behavior of BrowserApp. Passing and holding references of such an instance that outlive BrowserApp are a huge red flag. I do not want to add code for and optimize for a use case that is explicitly not wanted.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/48201/#review45369

Yeah, that's what I was thinking too! :)

For this first implementation I just wanted to mirror the activity lifecycle methods and let the follow-up bugs add what they need.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #11)
> > I'm not a fan of forcing all of the extenders to hold a reference to BrowserApp, just in-case they're ever passed to a context that outlives the Activity.
> > 
> > I'd prefer if this was a `WeakReference` or if the extenders had to manually take a reference to the activity.
> 
> (Regarding the reference see below).

"Below" is now actually comment 10 because I pressed the wrong buttons in reviewboard..

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8743964 [details]
MozReview Request: Bug 1266477 - Introduce BrowserAppDelegate to extend the behavior of BrowserApp. r=mcomella,ahunt

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

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8743966 [details]
MozReview Request: Bug 1266477 - Migrate AddToHomeScreenPromotion to extend BrowserAppDelegate. r=mcomella,ahunt

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

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8743967 [details]
MozReview Request: Bug 1266477 - Migrate screenshot observer code to class extending BrowserAppDelegate. r=mcomella,ahunt

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

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> Right now I'm inclined to change it back.

That's what I did now. The base delegate class does not hold any references anymore. Instead I pass the reference in as a parameter. If needed we can always create sub classes of BrowserAppDelegate that take care of the keeping a (weak) reference for you.

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8743966 [details]
MozReview Request: Bug 1266477 - Migrate AddToHomeScreenPromotion to extend BrowserAppDelegate. r=mcomella,ahunt

https://reviewboard.mozilla.org/r/48203/#review45571

Comment 19

[Andrzej Hunt :ahunt]

Comment on attachment 8743967 [details]
MozReview Request: Bug 1266477 - Migrate screenshot observer code to class extending BrowserAppDelegate. r=mcomella,ahunt

https://reviewboard.mozilla.org/r/48205/#review45573

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/0a5331a770552e879e9a5b3e0ab82d82b1e709dc
Bug 1266477 - Introduce BrowserAppDelegate to extend the behavior of BrowserApp. r=mcomella,ahunt

https://hg.mozilla.org/integration/fx-team/rev/f00621d9b0bcb469842505542292afbcc969e53f
Bug 1266477 - Migrate AddToHomeScreenPromotion to extend BrowserAppDelegate. r=mcomella,ahunt

https://hg.mozilla.org/integration/fx-team/rev/8a3cf7fdd36f09af2a7f9c2442b1ef4ec6ba975b
Bug 1266477 - Migrate screenshot observer code to class extending BrowserAppDelegate. r=mcomella,ahunt

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a5331a77055
https://hg.mozilla.org/mozilla-central/rev/f00621d9b0bc
https://hg.mozilla.org/mozilla-central/rev/8a3cf7fdd36f