Closed Bug 944537 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(4 files)

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.
Blocks: 944533
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.
Attachment #8340406 - Flags: review?(mark.finkle)
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.
Attachment #8340407 - Flags: review?(mark.finkle)
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.
Attachment #8340408 - Flags: review?(mark.finkle)
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.
Attachment #8340409 - Flags: review?(mark.finkle)
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 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
Attachment #8340406 - Flags: review?(mark.finkle) → review+
Attachment #8340407 - Flags: review?(mark.finkle) → review+
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.
Attachment #8340408 - Flags: review?(mark.finkle) → review+
Attachment #8340409 - Flags: review?(mark.finkle) → review+
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.
Blocks: 942862
(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.
(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.
You need to log in before you can comment on or make changes to this bug.