983475 - Remove shouldShowProgress from browser.js

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Having shouldShowProgress at both [1] and [2] is redundant. We should clean this up by doing the following:

1) Remove the method at [1]
2) Remove the call to this method at [3], and rename `showProgress` to `restoring`.
3) Also update the names at [4] and [5].
4) Finally, change the setState at [5] to check for both `restoring && shouldShowProgress`.

[1] http://hg.mozilla.org/mozilla-central/file/02506bdd5bd8/mobile/android/chrome/content/browser.js#l244
[2] http://hg.mozilla.org/mozilla-central/file/02506bdd5bd8/mobile/android/base/Tab.java#l643
[3] http://hg.mozilla.org/mozilla-central/file/02506bdd5bd8/mobile/android/chrome/content/browser.js#l3870
[4] http://hg.mozilla.org/mozilla-central/file/02506bdd5bd8/mobile/android/base/Tabs.java#l449
[5] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#649

Comment 1

[Lucas Rocha (:lucasr)]

Makes sense. We might want to fix bug 945714 while at it.

Comment 2

[Vlad Tanase]

Attachment: patch

I made a first attempt at fixing this.

I developed the fix on top of the patch for bug 976144, that should probably be checked in way before this. What I'm relying on is the fact that the shouldShowProgress method in Tab.java returns correctly.

Let me know if there are any problems with this.

Comment 3

[Brian Nicholson (:bnicholson)]

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

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

Thanks, looks good! r+ with the minor change below.

::: mobile/android/chrome/content/browser.js
@@ +3877,5 @@
>        }
>  
>        // Check to see if we restoring the content from a previous presentation (session)
>        // since there should be no real network activity
>        let restoring = aStateFlags & Ci.nsIWebProgressListener.STATE_RESTORING;

Nit: Change this to (aStateFlags & Ci.nsIWebProgressListener.STATE_RESTORING) > 0

@@ +3884,5 @@
>          type: "Content:StateChange",
>          tabID: this.id,
>          uri: uri,
>          state: aStateFlags,
> +        restoring: (restoring ? true : false),

Then just use restoring here directly without the ternary operator.

Comment 4

[Vlad Tanase]

Attachment: 983475.patch

You are right, the code is a lot more readable in the version you proposed.

I attached a new patch with the change included. Let me know if you spot any other issues with this.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8394011 [details] [diff] [review]
983475.patch

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

Thanks, looks good with just a minor spacing issue :)

::: mobile/android/chrome/content/browser.js
@@ +3877,5 @@
>        }
>  
>        // Check to see if we restoring the content from a previous presentation (session)
>        // since there should be no real network activity
> +        let restoring = (aStateFlags & Ci.nsIWebProgressListener.STATE_RESTORING) > 0;

Please fix the indentation here so that "let" is aligned under the lines above.

Comment 6

[Vlad Tanase]

Attachment: patch v3

You're right, Emacs really got me there and auto indented that.

Here is a new patch with the spacing fixed, hopefully everything is ok now.

Comment 7

[Vlad Tanase]

Hi Brian,

I see that everything is ok with the review. What would be the next step for this?

I cannot add the check-in needed flag since the issue is not assigned to me.

Comment 8

[Brian Nicholson (:bnicholson)]

Thanks for the reminder -- I updated the assignee and set checkin-needed for you. Thanks again for fixing this!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ff53816ea39