944537 - Only use gecko to decide whether to show Tab progress or not

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Right now, we have code in Java and Gecko to decide whether to show Tab progress or not. This is prone to confusion. For instance, they don't necessarily overlap e.g. Gecko disables progress for restored tabs, and about:home/about:reader pages; and Java only disables progress for about:home/about:reader.

AFAIK, there's no need to keep the Java side.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: No need to store showProgress as a Tab state/param (r=mfinkle)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Use message data to decide whether to show Tab progress or not (r=mfinkle)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: No need to check for showProgress in BrowserToolbar (r=mfinkle)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Don't send showMessage boolean with Tabs.START event (r=mfinkle)

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8340406 [details] [diff] [review]
No need to store showProgress as a Tab state/param (r=mfinkle)

Storing it as a tab state/param is prone to brokenness. The decision of whether to show progress or not can be made based solely on the uri + whether we're restoring a session page or not.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8340407 [details] [diff] [review]
Use message data to decide whether to show Tab progress or not (r=mfinkle)

This also fixes something that has apparently been broken for ages. We were showing progress even when we were simply restoring a page from session history e.g. when pressing back.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8340408 [details] [diff] [review]
No need to check for showProgress in BrowserToolbar (r=mfinkle)

The Tab(.java) already does that for us through its state i.e. it won't set state to LOADING if showProgress was false. No need to check for it again here.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8340409 [details] [diff] [review]
Don't send showMessage boolean with Tabs.START event (r=mfinkle)

No need to send the 'showProgress' data with the START message as the Tab will handle it properly through its state.

Comment 9

[Lucas Rocha (:lucasr)]

Turns out I had to keep the java version of shouldShowProgress (in Tab.java) because we need to decide whether the tab should start showing progress immediately even before Gecko 'sees' the tab e.g. when creating a new tab from the menu or from an external app.

Comment 10

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

Comment on attachment 8340406 [details] [diff] [review]
No need to store showProgress as a Tab state/param (r=mfinkle)

It looks like this won't regress bug 718846

Comment 11

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

Comment on attachment 8340408 [details] [diff] [review]
No need to check for showProgress in BrowserToolbar (r=mfinkle)

This does have the side-effect of calling the code in the "else" block of setProgressVisibility where it was not being called before, since we are only calling setProgressVisibility for the "true" case.

We should keep an eye out for any regressions.

Comment 12

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

Just a note. I have always felt the using mState (STATE_SUCCESS or STATE_LOADING) as a way to control the progress visibility is a bit hacky. Maybe we can figure out a better way someday.

CC'ing rnewman since he has been looking into cleaning up some favicon/toolbar/tab-change code. There is a little overlap. See bug 941868 and bug 944550.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 8340408 [details] [diff] [review]
> No need to check for showProgress in BrowserToolbar (r=mfinkle)
> 
> This does have the side-effect of calling the code in the "else" block of
> setProgressVisibility where it was not being called before, since we are
> only calling setProgressVisibility for the "true" case.

Good catch. I think I can keep the previous behaviour given this is code handling the START event. Done.

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Just a note. I have always felt the using mState (STATE_SUCCESS or
> STATE_LOADING) as a way to control the progress visibility is a bit hacky.
> Maybe we can figure out a better way someday.

You mean the fact that tweak the tab state to represent the 'showProgress' behaviour? I agree, this is hacky. This is a bit out of the scope of this bug though. I filed bug 945714 to track this.

Comment 15

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/51cb8e90b819
https://hg.mozilla.org/integration/fx-team/rev/a1445386e051
https://hg.mozilla.org/integration/fx-team/rev/918596edaa77
https://hg.mozilla.org/integration/fx-team/rev/8f06133698b4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51cb8e90b819
https://hg.mozilla.org/mozilla-central/rev/a1445386e051
https://hg.mozilla.org/mozilla-central/rev/918596edaa77
https://hg.mozilla.org/mozilla-central/rev/8f06133698b4