976144 - Don't show progress bar on load of about: pages

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

On load of about: pages we show a progress bar. We shouldn't show a progress bar for loading about:pages.

Comment 1

[Aaron Train [:aaronmt]]

See also bug 976426 (progress bar shown on neterror)

Comment 2

[Vlad Tanase]

Hello,

I'd like to give this a try. I had a look around the code and from what I can tell the decision on whether or not to show the progress bar comes from here: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#449 and the boolean is handled here: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#651

I got a little stuck, since I was not able to tell where the message itself was coming from. Could you please confirm that I'm on the right path and maybe explain to me where the message is coming from?

I tried following the discussion on the neterror bug, but unfortunately I wasn't able to make too much of it, or find some relation to what needs to be done here.

Comment 3

[Brian Nicholson (:bnicholson)]

Hi Vlad,

You are indeed on the right track. That message is coming from Gecko, which gets set at [1]. shouldShowProgress is defined at [2]. Additionally, since tabs can be created in Java before they are updated by Gecko, this change will need to be mirrored on the Java side at [3].

Note that on the Java side, we already have a utility method for detecting whether a page is an about: page [4], so you should just be able to substitute that check for the ones at [3].

Thanks for taking this! Let me know if you have any other questions.

[1] http://hg.mozilla.org/mozilla-central/file/02506bdd5bd8/mobile/android/chrome/content/browser.js#l3870
[2] http://hg.mozilla.org/mozilla-central/file/02506bdd5bd8/mobile/android/chrome/content/browser.js#l244
[3] http://hg.mozilla.org/mozilla-central/file/02506bdd5bd8/mobile/android/base/Tab.java#l643
[4] http://hg.mozilla.org/mozilla-central/file/02506bdd5bd8/mobile/android/base/AboutPages.java#l25

Comment 4

[Vlad Tanase]

Attachment: patch

Hi Brian,

I added a patch with these changes. Could you have a look and see if everything is ok?

I changed both the Java and Javascript code, though I wasn't able to tell under what conditions does the Java code get called. Can you tell me what needs to happen in the browser so that the Java code comes into effect?

Thanks,
Vlad

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8387412 [details] [diff] [review]
patch

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

This looks great! Just minor fix I'd like you to make before this lands.

(In reply to Vlad Tanase from comment #4)
> I changed both the Java and Javascript code, though I wasn't able to tell
> under what conditions does the Java code get called. Can you tell me what
> needs to happen in the browser so that the Java code comes into effect?

The Java code here is used only when the Tab is created. When the user adds a tab, it gets created immediately so it's visible in the UI -- we call these tab stubs. When a tab stub is created, there's a delay between its creation and the Content:StateChange message from Gecko at [1]. In most cases, this delay will be very small and not noticeable.

In certain situations, however, it's possible that Gecko doesn't send a Content:StateChange until several seconds after the stub has been created. In particular, this will happen at startup, since the UI is shown immediately while we wait for Gecko to load. Here's one case where the Java check is useful:
1) In Settings, go to Customize > Tabs and select "Always restore"
2) Go to any non-about page (e.g., google.com)
3) Swipe Fennec from the Android recent apps list to close it
4) Reopen Fennec

When you reopen Fennec in step 4, you'll notice that the progress bar starts at around 10% or so. You'll see this immediately, even before Gecko is running. Now, if you repeat the steps above using an about page in step 2 instead of a non-about page, this 10% progress should not be visible at startup with your patch applied.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3872

::: mobile/android/base/Tab.java
@@ +640,5 @@
>          Tabs.getInstance().notifyListeners(this, Tabs.TabEvents.LOCATION_CHANGE, oldUrl);
>      }
>  
>      private static boolean shouldShowProgress(final String url) {
> +        return (url != null) && (AboutPages.isAboutPage(url));

Please move this null check into isAboutPage.

Comment 6

[Vlad Tanase]

Attachment: patch v2

Thanks for the explanations and the review Brian. I was able to test the scenario you described with the patch applied, and everything worked as expected.

I have made the necessary changes and attached a new patch. Hopefully everything is OK now.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8388438 [details] [diff] [review]
patch v2

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

This is looking good. I missed a bug that we have in our code the first time around, so this will require one last update -- sorry! Thanks for spending the time to fix this.

::: mobile/android/base/Tab.java
@@ +640,5 @@
>          Tabs.getInstance().notifyListeners(this, Tabs.TabEvents.LOCATION_CHANGE, oldUrl);
>      }
>  
>      private static boolean shouldShowProgress(final String url) {
> +        return AboutPages.isAboutPage(url);

Please invert this condition to make this check correct -- see my comment below.

Additionally, please update the following line:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#120

This should be `mState = shouldShowProgress(url) ? STATE_LOADING : STATE_SUCCESS;`, not the other way around. If shouldShowProgress is true, the tab should be put in the loading state (which means the progress bar is displayed).

@@ -642,5 @@
>  
>      private static boolean shouldShowProgress(final String url) {
> -        return AboutPages.isAboutHome(url) ||
> -               AboutPages.isAboutReader(url) ||
> -               AboutPages.isAboutPrivateBrowsing(url);

I didn't see this before, but our existing code looks wrong. We want to show progress if the URL is *not* an about: page. As it is now, our shouldShowProgress() does the opposite.

::: mobile/android/chrome/content/browser.js
@@ +240,5 @@
>    }
>    return aURI;
>  }
>  
>  function shouldShowProgress(url) {

We actually shouldn't need this method at all; having it both here and in Tab.java is redundant. But this change will require several other changes elsewhere that are unrelated to this bug, so I filed bug 983475 to handle this separately. Feel free to take this if you'd like!

Comment 8

[Vlad Tanase]

Attachment: patch v3

I finally managed to get around to doing the last set of changes. Hopefully this is ok now.

The other issue should be pretty interesting too. I will definitely have a look at it if I have the time.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8392746 [details] [diff] [review]
patch v3

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

Excellent, thanks. Since you don't have commit access, please add "checkin-needed" as a keyword for this bug. This will flag one of our volunteers to come by and land the patch for you.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Carsten Book [:Tomcat]]

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