944533 - Re-implement toolbar's display UI state management

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Current internal API to update the state of the toolbar display UI (throbber, favicon, title, stop, page actions) is confusing. Ideally, we should have a single entry point to update the state of the toolbar based on the selected tab instead of scattering this code into multiple unrelated methods.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: No need to set progress visibility explicitly in BrowserApp (r=margaret)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Make unnecessary public API in BrowserToolbar private (r=margaret)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Inflate SiteIdentityPopup on show() and add identity setter (r=margaret)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Re-define internal API to update toolbar UI state (r=mfinkle)

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Remove unused constants from BrowserToolbar (r=margaret)

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Factor out title display prefs into separate class (r=margaret)

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Move title handling to ToolbarDisplayLayout (r=margaret)

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8341625 [details] [diff] [review]
No need to set progress visibility explicitly in BrowserApp (r=margaret)

The new Tab's state will do that for us anyway as it will be immediately selected on the Java side.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8341626 [details] [diff] [review]
Make unnecessary public API in BrowserToolbar private (r=margaret)

Seal BrowserToolbar a bit more before redoing its guts.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8341627 [details] [diff] [review]
Inflate SiteIdentityPopup on show() and add identity setter (r=margaret)

Change the SiteIdentityPopup API a bit. The core intent here is change SiteIdentityPopup to hold its necessary data. This looks cleaner now that site identity/security mode is encapsulated behind a proper API (bug 945212).

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8341628 [details] [diff] [review]
Re-define internal API to update toolbar UI state (r=mfinkle)

Here's the what the new internal API does:
- Creates a single entry point to update ToolbarDisplayLayout's state from a tab instance i.e. the updateFromTab().
- The point here is to make ToolbarDisplayLayout as dummo/stateless/functional as possible. We only hold enough state to be able to short-circuit/optimize certain code paths e.g. avoid UI unnecessary changes.
- Reduce the amount of dependencies in ToolbarDisplayLayout as much as possible. For instance, it doesn't depend on things like the Tabs API at all. It only acts on the given Tab instance in updateFromTab().

The title handling is refactored in separate patches because it requires some extra steps (see following patches).

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: Factor out title display prefs into separate class (r=margaret)

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Move title handling to ToolbarDisplayLayout (r=margaret)

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8341654 [details] [diff] [review]
Move title handling to ToolbarDisplayLayout (r=margaret)

Now ToolbarDisplayLayout manages all its state behind a single API, updateFromTab().

Comment 15

[:Margaret Leibovic]

Comment on attachment 8341653 [details] [diff] [review]
Factor out title display prefs into separate class (r=margaret)

Review of attachment 8341653 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, looks like you forgot to hg add ToolbarTitlePrefs :)

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8341653 [details] [diff] [review]
Factor out title display prefs into separate class (r=margaret)

Missing ToolbarTitlePrefs.java

If you are migrating the title handling to ToolbarDisplayLayout, maybe the prefs could just live there instead of a separate class?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8341628 [details] [diff] [review]
Re-define internal API to update toolbar UI state (r=mfinkle)

This looks OK to me. I took a quick look at all the other patches to get more context. I like the way the code is organizing. I like the way we could optimize some of the toolbar UI operations too.

Question: Is this low-risk enough to take on mozilla-central so close to a merge?

Comment 18

[Richard Newman [:rnewman]]

I love it!

I think this is moving in the direction of Bug 946330, so blocking that.

Comment 19

[Lucas Rocha (:lucasr)]

Attachment: Factor out title display prefs into separate class (r=margaret)

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 8342515 [details] [diff] [review]
Factor out title display prefs into separate class (r=margaret)

Now with all the files :-)

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> Question: Is this low-risk enough to take on mozilla-central so close to a
> merge?

This is actually risky enough that we should avoid landing it just before the merge. Besides, these patches provide no user visible benefit. So, no need to hurry.

Comment 22

[:Margaret Leibovic]

Comment on attachment 8341628 [details] [diff] [review]
Re-define internal API to update toolbar UI state (r=mfinkle)

Review of attachment 8341628 [details] [diff] [review]:
-----------------------------------------------------------------

It's hard to review patches that are based on code that hasn't landed yet, but this seems reasonable to me. Also, landing this early in the cycle makes me feel better about having time to shake out regressions.

Comment 23

[:Margaret Leibovic]

Comment on attachment 8342515 [details] [diff] [review]
Factor out title display prefs into separate class (r=margaret)

Review of attachment 8342515 [details] [diff] [review]:
-----------------------------------------------------------------

Nice reorganization.

Comment 24

[:Margaret Leibovic]

Comment on attachment 8341654 [details] [diff] [review]
Move title handling to ToolbarDisplayLayout (r=margaret)

Review of attachment 8341654 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +272,5 @@
> +                } else {
> +                    contentDescription = mActivity.getString(R.string.url_bar_default_text);
> +                }
> +
> +                setContentDescription(contentDescription);

You should add a comment about why we need to set the content description here, rather than where we actually update the title (this was a question I had when I first looked at this).

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +50,5 @@
>                                    implements Animation.AnimationListener {
>  
>      private static final String LOGTAG = "GeckoToolbarDisplayLayout";
>  
>      enum UpdateFlags {

As a follow-up (mentor bug?), it would be nice to add some docs here to explain what these flags mean in practice.

Comment 25

[Lucas Rocha (:lucasr)]

FYI: Just waiting for a final review from wesj in bug 942862 to land this. All green on try: https://tbpl.mozilla.org/?tree=Try&rev=3a33a65bf136

Comment 26

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #24)
> Comment on attachment 8341654 [details] [diff] [review]
> Move title handling to ToolbarDisplayLayout (r=margaret)
> 
> Review of attachment 8341654 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +272,5 @@
> > +                } else {
> > +                    contentDescription = mActivity.getString(R.string.url_bar_default_text);
> > +                }
> > +
> > +                setContentDescription(contentDescription);
> 
> You should add a comment about why we need to set the content description
> here, rather than where we actually update the title (this was a question I
> had when I first looked at this).

Done.

> ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
> @@ +50,5 @@
> >                                    implements Animation.AnimationListener {
> >  
> >      private static final String LOGTAG = "GeckoToolbarDisplayLayout";
> >  
> >      enum UpdateFlags {
> 
> As a follow-up (mentor bug?), it would be nice to add some docs here to
> explain what these flags mean in practice.

Working on javadocs for the toolbar high-level architecture. Filed bug 957992 to track this.

Comment 27

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/874bcd13b8b4
https://hg.mozilla.org/integration/fx-team/rev/95c0731132ae
https://hg.mozilla.org/integration/fx-team/rev/1a32a3b9dd20
https://hg.mozilla.org/integration/fx-team/rev/fa0a7a730324
https://hg.mozilla.org/integration/fx-team/rev/cd18587bd2b5
https://hg.mozilla.org/integration/fx-team/rev/1260d241f84f
https://hg.mozilla.org/integration/fx-team/rev/125b24bb78d9

Comment 28

[Carsten Book [:Tomcat]]

hi,

had to backout this change as part of https://tbpl.mozilla.org/?tree=Fx-Team&rev=e89b407be9c5 because of build bustages on android and windows like https://tbpl.mozilla.org/php/getParsedLog.php?id=32753197&tree=Fx-Team

Comment 29

[Lucas Rocha (:lucasr)]

These patches require a clobber build. Touched the CLOBBER file and re-landed:

https://hg.mozilla.org/integration/fx-team/rev/f8a1445c80d5
https://hg.mozilla.org/integration/fx-team/rev/53c8b206ca96
https://hg.mozilla.org/integration/fx-team/rev/23c6d01caed1
https://hg.mozilla.org/integration/fx-team/rev/6113af5c0724
https://hg.mozilla.org/integration/fx-team/rev/1586c7943fbd
https://hg.mozilla.org/integration/fx-team/rev/c7f22c46812d
https://hg.mozilla.org/integration/fx-team/rev/06bc8e68d4fc

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/f8a1445c80d5
https://hg.mozilla.org/mozilla-central/rev/53c8b206ca96
https://hg.mozilla.org/mozilla-central/rev/23c6d01caed1
https://hg.mozilla.org/mozilla-central/rev/6113af5c0724
https://hg.mozilla.org/mozilla-central/rev/1586c7943fbd
https://hg.mozilla.org/mozilla-central/rev/c7f22c46812d
https://hg.mozilla.org/mozilla-central/rev/06bc8e68d4fc