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

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 28
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 944533
(Assignee)

Comment 1

5 years ago
Created attachment 8340406 [details] [diff] [review]
No need to store showProgress as a Tab state/param (r=mfinkle)
(Assignee)

Comment 2

5 years ago
Created attachment 8340407 [details] [diff] [review]
Use message data to decide whether to show Tab progress or not (r=mfinkle)
(Assignee)

Comment 3

5 years ago
Created attachment 8340408 [details] [diff] [review]
No need to check for showProgress in BrowserToolbar (r=mfinkle)
(Assignee)

Comment 4

5 years ago
Created attachment 8340409 [details] [diff] [review]
Don't send showMessage boolean with Tabs.START event (r=mfinkle)
(Assignee)

Comment 5

5 years ago
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)
(Assignee)

Comment 6

5 years ago
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)
(Assignee)

Comment 7

5 years ago
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)
(Assignee)

Comment 8

5 years ago
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)
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 942862
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
(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.