957992 - Document toolbar high-level architecture

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Something like a 2-3 paragraph overview on top of BrowserToolbar should be enough.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Document toolbar high-level architecture (r=margaret)

Comment 2

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

Comment on attachment 8358333 [details] [diff] [review]
Document toolbar high-level architecture (r=margaret)

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

Drive-by: The comments look pretty good and helped me figure out the code layout so ^_^!

I made some comments moreso about the code than the documentation, so if you feel it's relevant, open up some more bugs! :)

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +64,5 @@
>  
> +/**
> +* {@code BrowserToolbar} is single entry point for users of the toolbar
> +* subsystem i.e. this should be the only import outside the 'toolbar'
> +* package.

Then perhaps we should change the access levels of the other internal classes (e.g. ToolbarDisplayLayout, ToolbarEditLayout, and ToolbarEditText are all public).

@@ +70,5 @@
> +* {@code BrowserToolbar} serves at the single event bus for all
> +* sub-components in the toolbar. It tracks tab events and gecko messages
> +* and update the state of its inner components accordingly.
> +*
> +* It has two states, display and edit, which are controlled by

Regarding the code, I wonder if there shouldn't be a:

enum State { DISPLAY, EDIT }
State mState = State.<state>;

Currently, this seems to be controlled by mIsEditing, but I think mState is more explicit, and `BrowserToolbar.isEditing()` can still be easily used in both cases.

@@ +74,5 @@
> +* It has two states, display and edit, which are controlled by
> +* ToolbarEditLayout and ToolbarDisplayLayout. In display state, the toolbar
> +* displays the current state for the selected tab. In edit state, it shows
> +* a text entry for searching bookmarks/history. {@code BrowserToolbar}
> +* provides public API to enter, cancel, and commit the edit state as well

Speaking of public API, it might be good to run through the access levels of all the methods and ensure they're correct as this is the only public entry point (according to the above).

I didn't do much skimming but `public void setIsEditing` jumps out at me as strange.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +51,5 @@
> +* {@code ToolbarDisplayLayout} is the UI for when the toolbar is in
> +* display state. It's used to display the state of the currently selected
> +* tab. It should always be updated through a single entry point
> +* (updateFromTab) and should never track any tab events or gecko messages
> +* on its own to keep it as dummy as possible.

nit: "as dumb as possible."

@@ +53,5 @@
> +* tab. It should always be updated through a single entry point
> +* (updateFromTab) and should never track any tab events or gecko messages
> +* on its own to keep it as dummy as possible.
> +*
> +* The UI has two possible modes: progress and display which are triggered

Again, maybe:

enum State { PROGRESS, DISPLAY }?
State mState;

@@ +67,5 @@
>  
>      private static final String LOGTAG = "GeckoToolbarDisplayLayout";
>  
> +    // To be used with updateFromTab() to allow the caller
> +    // to give enough context for the requested state change.

nit: Maybe as javadoc?

Comment 3

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #2)

> @@ +70,5 @@
> > +* {@code BrowserToolbar} serves at the single event bus for all
> > +* sub-components in the toolbar. It tracks tab events and gecko messages
> > +* and update the state of its inner components accordingly.
> > +*
> > +* It has two states, display and edit, which are controlled by
> 
> Regarding the code, I wonder if there shouldn't be a:
> 
> enum State { DISPLAY, EDIT }
> State mState = State.<state>;
> 
> Currently, this seems to be controlled by mIsEditing, but I think mState is
> more explicit, and `BrowserToolbar.isEditing()` can still be easily used in
> both cases.

I like this suggestion.

> @@ +53,5 @@
> > +* tab. It should always be updated through a single entry point
> > +* (updateFromTab) and should never track any tab events or gecko messages
> > +* on its own to keep it as dummy as possible.
> > +*
> > +* The UI has two possible modes: progress and display which are triggered
> 
> Again, maybe:
> 
> enum State { PROGRESS, DISPLAY }?
> State mState;

Did you see UIMode and mUiMode? I think those match what you're talking about here, but maybe renaming them would make their role clearer, especially if we were to implement a similar state variable for BrowserToolbar.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8358333 [details] [diff] [review]
Document toolbar high-level architecture (r=margaret)

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

mcomella did a good job commenting on this :) I agree with the things he had to say, but they're mostly material for follow-up bugs.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #2)
> Comment on attachment 8358333 [details] [diff] [review]
> Document toolbar high-level architecture (r=margaret)
> 
> Review of attachment 8358333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by: The comments look pretty good and helped me figure out the code
> layout so ^_^!
> 
> I made some comments moreso about the code than the documentation, so if you
> feel it's relevant, open up some more bugs! :)
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +64,5 @@
> >  
> > +/**
> > +* {@code BrowserToolbar} is single entry point for users of the toolbar
> > +* subsystem i.e. this should be the only import outside the 'toolbar'
> > +* package.
> 
> Then perhaps we should change the access levels of the other internal
> classes (e.g. ToolbarDisplayLayout, ToolbarEditLayout, and ToolbarEditText
> are all public).

I agree that these classes should all be package-scoped. However, we reference them in Android layout files (which are parsed with classes that are not in the same package). Big bummer, I know.

> @@ +70,5 @@
> > +* {@code BrowserToolbar} serves at the single event bus for all
> > +* sub-components in the toolbar. It tracks tab events and gecko messages
> > +* and update the state of its inner components accordingly.
> > +*
> > +* It has two states, display and edit, which are controlled by
> 
> Regarding the code, I wonder if there shouldn't be a:
> 
> enum State { DISPLAY, EDIT }
> State mState = State.<state>;
> 
> Currently, this seems to be controlled by mIsEditing, but I think mState is
> more explicit, and `BrowserToolbar.isEditing()` can still be easily used in
> both cases.

Good point. Filed bug 959184 to track this.

> @@ +74,5 @@
> > +* It has two states, display and edit, which are controlled by
> > +* ToolbarEditLayout and ToolbarDisplayLayout. In display state, the toolbar
> > +* displays the current state for the selected tab. In edit state, it shows
> > +* a text entry for searching bookmarks/history. {@code BrowserToolbar}
> > +* provides public API to enter, cancel, and commit the edit state as well
> 
> Speaking of public API, it might be good to run through the access levels of
> all the methods and ensure they're correct as this is the only public entry
> point (according to the above).
> 
> I didn't do much skimming but `public void setIsEditing` jumps out at me as
> strange.

Indeed. Filed bug 959185 to audit API access levels in the toolbar package.

> ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
> @@ +51,5 @@
> > +* {@code ToolbarDisplayLayout} is the UI for when the toolbar is in
> > +* display state. It's used to display the state of the currently selected
> > +* tab. It should always be updated through a single entry point
> > +* (updateFromTab) and should never track any tab events or gecko messages
> > +* on its own to keep it as dummy as possible.
> 
> nit: "as dumb as possible."

Fixed.

> @@ +53,5 @@
> > +* tab. It should always be updated through a single entry point
> > +* (updateFromTab) and should never track any tab events or gecko messages
> > +* on its own to keep it as dummy as possible.
> > +*
> > +* The UI has two possible modes: progress and display which are triggered
> 
> Again, maybe:
> 
> enum State { PROGRESS, DISPLAY }?
> State mState;

This is already in as enum UIMode { DISPLAY, PROGRESS }.

Comment 6

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/f5986233db46

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5986233db46