Closed Bug 1376892 Opened 7 years ago Closed 7 years ago

Don't show the stop button for local url loads

Categories

(Firefox :: Toolbars and Customization, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: florian, Assigned: perry)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

23.04 KB, patch
jaws
: review+
Details | Diff | Splinter Review
8.55 KB, patch
jaws
: review+
Details | Diff | Splinter Review
When first loading about:home, the stop button flickers before we show the reload button.
This should at least apply to about:home and about:newtab.
Let's do this for all about: pages
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
Attachment #8882698 - Flags: review?(jaws) → review+
Comment on attachment 8882631 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loads

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

::: browser/base/content/test/about/browser_aboutDontShowStop.js
@@ +7,5 @@
> +  let reloadBtn = document.getElementById("reload-button");
> +  let reloadBtnObserver = new MutationObserver(reloadBtnMutationCallback);
> +  reloadBtnObserver.observe(reloadBtn, { attributeFilter: ["displaystop"] });
> +
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:home");

We need to pass waitForStateStop=true here to make sure that this call runs the full length of the load. This will need to be changed to use the 'options' object, like so:
`let tab = await BrowserTestUtils.openNewForegroundTab({gBrowser, opening: "about:home", waitForStateStop: true});`
Attachment #8882631 - Flags: review?(jaws) → review+
Comment on attachment 8883116 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loadsdontshowstop.diff

This patch is wrong. It fails to show Stop when navigating away from an about: page.
Attachment #8883116 - Flags: review+ → review-
(In reply to :Felipe Gomes (needinfo me!) from comment #2)
> Let's do this for all about: pages

We should expand this to chrome: pages too, which is what TART loads.
Comment on attachment 8883116 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loadsdontshowstop.diff

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

::: browser/base/content/browser.js
@@ +4942,5 @@
>        this._stopClicked = true;
>    },
>  
>    switchToStop() {
> +    if (!this._initialized || gBrowser.currentURI.schemeIs("about"))

If you rebase your patch on latest m-c tip, switchToStop now has aRequest and aWebProgress arguments. You can get the URI of the request from aRequest and check that instead of gBrowser.currentURI.

onStateChange gets the URI from aRequest by doing the following, http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#4575-4576
Attachment #8883116 - Attachment is obsolete: true
Attachment #8885357 - Flags: review?(jaws)
Attachment #8885357 - Attachment is obsolete: true
Attachment #8885358 - Attachment is obsolete: true
Attachment #8885570 - Flags: review?(jaws)
Comment on attachment 8885570 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loads.diff

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

Looking good, just a couple issues I found below.

::: browser/base/content/browser.js
@@ +5007,5 @@
>      this.reload.setAttribute("displaystop", "true");
>    },
>  
>    switchToReload(aRequest, aWebProgress) {
> +    if (!this._initialized || !this._shouldSwitch(aRequest))

When I have a tab that is still loading remote content (I use deelay.me for testing) and open a new tab, I noticed that the stop button gets stuck on and we never switch back to reload. I think this is the cause, though when I removed the call here to this._shouldSwitch I started see the animation each time I reloaded an about: URL.

@@ +5048,5 @@
> +  _shouldSwitch(aRequest) {
> +    return aRequest &&
> +           aRequest.originalURI &&
> +           !aRequest.originalURI.schemeIs("chrome") &&
> +           !aRequest.originalURI.schemeIs("about");

We should still show the stop/refresh for about:feeds and about:reader since they load remote URLs.

::: browser/base/content/test/about/browser_aboutStopReload.js
@@ +16,5 @@
> +                                                        opening: "about:home",
> +                                                        waitForStateStop: true});
> +  await BrowserTestUtils.removeTab(tab);
> +
> +  Assert.ok(true, "Test finished: stop-reload does not animate when navigating to local URI on new tab");

Please use info() for this and each one of the end-of-test messages.
Attachment #8885570 - Flags: review?(jaws) → review-
Attachment #8885573 - Flags: review?(jaws) → review+
Now switches to reload button if navigating to a local URL while the current page is loading.
Attachment #8885570 - Attachment is obsolete: true
Attachment #8885931 - Flags: review?(jaws)
Attached patch stopreload.diff (obsolete) — Splinter Review
(Small typo fix in a test case from the recently cancelled review)
Attachment #8885931 - Attachment is obsolete: true
Attachment #8885936 - Flags: review?(jaws)
Comment on attachment 8885931 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loads

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

::: browser/base/content/browser.js
@@ +4989,5 @@
>      });
>    },
>  
>    switchToStop(aRequest, aWebProgress) {
> +    if (!this._initialized || !this._shouldSwitchToStop(aRequest))

Please just use:
if (!this._initialized || !this._shouldSwitch(aRequest))

@@ +5007,5 @@
>      this.reload.setAttribute("displaystop", "true");
>    },
>  
>    switchToReload(aRequest, aWebProgress) {
> +    if (!this._initialized || !this._shouldSwitchToReload(aRequest))

Please just use:
if (!this._initialized || !this._shouldSwitch(aRequest) ||
    !this.reload.hasAttribute("displaystop"))

@@ +5056,5 @@
> +  _shouldSwitch(aRequest) {
> +    if (!aRequest ||
> +        !aRequest.originalURI ||
> +        aRequest.originalURI.scheme == "about:feeds" ||
> +        aRequest.originalURI.scheme == "about:reader")

scheme will only ever return the portion before the colon

Please use:
aRequest.originalURI.spec.startsWith("about:reader")

Testing with http://rss.cnn.com/rss/cnn_topstories.rss I'm not able to get an originalURI of about:feeds, it's just "http://rss.cnn.com/rss/cnn_topstories.rss" as the originalURI.spec.
Attachment #8885931 - Flags: review-
Comment on attachment 8885936 [details] [diff] [review]
stopreload.diff

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

See comment 18.
Attachment #8885936 - Flags: review?(jaws) → review-
Attachment #8885936 - Attachment is obsolete: true
Attachment #8885966 - Flags: review?(jaws)
Attachment #8885966 - Flags: review?(jaws) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/394e4cd341ca
Don't show stop button for local url loads. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e63be29ec4
Move browser_about* tests from 'general' to 'about' folder. r=jaws
https://hg.mozilla.org/mozilla-central/rev/394e4cd341ca
https://hg.mozilla.org/mozilla-central/rev/92e63be29ec4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Backed out for frequent failures in browser_aboutStopReload.js:

https://hg.mozilla.org/mozilla-central/rev/b4e656e5a996dd385e9af43a4d9e207553377c51
https://hg.mozilla.org/mozilla-central/rev/5a5f0b37b51f32b6931ef5e3565613f9189777a0

Failure log 1: https://treeherder.mozilla.org/logviewer.html#?job_id=114631056&repo=mozilla-inbound
[task 2017-07-15T22:39:24.274476Z] 22:39:24     INFO - Entering test bound checkDoShowStopOnNewTab
[task 2017-07-15T22:39:24.277050Z] 22:39:24     INFO - TEST-PASS | browser/base/content/test/about/browser_aboutStopReload.js | stop-reload-button should animate - true == true - 
[task 2017-07-15T22:39:24.282208Z] 22:39:24     INFO - Buffered messages logged at 22:39:23
[task 2017-07-15T22:39:24.285067Z] 22:39:24     INFO - Test finished: stop-reload animates when navigating to non-local URI on new tab
[task 2017-07-15T22:39:24.287004Z] 22:39:24     INFO - Leaving test bound checkDoShowStopOnNewTab
[task 2017-07-15T22:39:24.291140Z] 22:39:24     INFO - Entering test bound checkDoShowStopFromLocalURI
[task 2017-07-15T22:39:24.295062Z] 22:39:24     INFO - TEST-PASS | browser/base/content/test/about/browser_aboutStopReload.js | stop-reload-button should animate - true == true - 
[task 2017-07-15T22:39:24.297190Z] 22:39:24     INFO - Buffered messages finished
[task 2017-07-15T22:39:24.299225Z] 22:39:24     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/about/browser_aboutStopReload.js | uncaught exception - AbortError at chrome://browser/content/abouthome/aboutHome.js:163:22
[task 2017-07-15T22:39:24.300912Z] 22:39:24     INFO - Stack trace:
[task 2017-07-15T22:39:24.303057Z] 22:39:24     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
[task 2017-07-15T22:39:24.305371Z] 22:39:24     INFO - Console message: [JavaScript Error: "AbortError" {file: "chrome://browser/content/abouthome/aboutHome.js" line: 163}]
[task 2017-07-15T22:39:24.348993Z] 22:39:24     INFO - Test finished: stop-reload animates when navigating local URI from non-local URI

Failure log 2: https://treeherder.mozilla.org/logviewer.html#?job_id=114627845&repo=mozilla-inbound
13:51:32     INFO - Entering test bound checkDoShowStopOnNewTab
13:51:32     INFO - Buffered messages logged at 13:51:30
13:51:32     INFO - TEST-PASS | browser/base/content/test/about/browser_aboutStopReload.js | stop-reload-button should animate - true == true - 
13:51:32     INFO - Buffered messages logged at 13:51:31
13:51:32     INFO - Test finished: stop-reload animates when navigating to non-local URI on new tab
13:51:32     INFO - Leaving test bound checkDoShowStopOnNewTab
13:51:32     INFO - Entering test bound checkDoShowStopFromLocalURI
13:51:32     INFO - TEST-PASS | browser/base/content/test/about/browser_aboutStopReload.js | stop-reload-button should animate - true == true - 
13:51:32     INFO - Console message: [JavaScript Error: "TypeError: doc.head is null" {file: "resource://onboarding/onboarding.js" line: 764}]
13:51:32     INFO - Buffered messages logged at 13:51:32
13:51:32     INFO - Test finished: stop-reload animates when navigating local URI from non-local URI
13:51:32     INFO - Buffered messages finished
13:51:32     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/about/browser_aboutStopReload.js | A promise chain failed to handle a rejection: doc.head is null - stack: null
13:51:32     INFO - Rejection date: Sat Jul 15 2017 13:51:31 GMT-0700 (Pacific Standard Time) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 265
13:51:32     INFO - Stack trace:
13:51:32     INFO -     resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:265
13:51:32     INFO -     chrome://mochikit/content/browser-test.js:Tester_execTest/<:785
13:51:32     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:758:9
13:51:32     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:670:7
13:51:32     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
13:51:32     INFO - Leaving test bound checkDoShowStopFromLocalURI
Status: RESOLVED → REOPENED
Flags: needinfo?(jiangperry)
Resolution: FIXED → ---
Before the backout, these improvements were observed:

== Change summary for alert #7991 (as of July 14 2017 23:52 UTC) ==

Improvements:

 15%  cart summary linux64 pgo e10s     9.17 -> 7.81
 14%  tart summary osx-10-10 opt e10s   13.03 -> 11.26
 12%  cart summary linux64 opt e10s     9.98 -> 8.76
  9%  cart summary osx-10-10 opt e10s   13.29 -> 12.15
  8%  cart summary windows10-64 opt e10s10.34 -> 9.56
  7%  tart summary linux64 pgo e10s     5.07 -> 4.71
  6%  tart summary linux64 opt e10s     5.93 -> 5.55

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7991
when this was backed out we see a series of perf regressions:
== Change summary for alert #8013 (as of July 16 2017 10:36 UTC) ==

Regressions:

 19%  cart summary linux64 pgo e10s     7.83 -> 9.33
 16%  cart summary linux64 opt e10s     8.73 -> 10.16
 11%  cart summary windows10-64 opt e10s9.54 -> 10.58
 10%  cart summary osx-10-10 opt e10s   12.17 -> 13.38
  7%  tart summary linux64 pgo e10s     4.70 -> 5.05
  7%  tart summary linux64 opt e10s     5.55 -> 5.96

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8013

I am looking forward to this landing to show performance improvements.
perry, I think this failed because of snippets loading during the test. This is line 163 in aboutHome.js,

>       cursorRequest = db.transaction(SNIPPETS_OBJECTSTORE_NAME)

The simplest fix may be to use a different URL than about:home. Can you use about:robots instead?
In browser_aboutStopReload.js, changed about:home to about:robots, added a missing await, and fixed a typo.
Attachment #8885966 - Attachment is obsolete: true
Flags: needinfo?(jiangperry)
Attachment #8888470 - Flags: review?(jaws)
Attachment #8888470 - Attachment description: Bug 1376892 - on't show stop button for local url → Bug 1376892 - Don't show stop button for local url
Attachment #8888470 - Flags: review?(jaws) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2904560e11
Don't show stop button for local url loads. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb63faa7da3
Move browser_about* tests from 'general' to 'about' folder. r=jaws
Keywords: checkin-needed
there are some talos improvements from this bug:
== Change summary for alert #8146 (as of July 20 2017 21:24 UTC) ==

Improvements:

 14%  tart summary osx-10-10 opt e10s   14.26 -> 12.30
 10%  cart summary osx-10-10 opt e10s   13.23 -> 11.85

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8146
I have reproduced this bug with Nightly (2017-06-28) on Windows 7, 64 Bit! 

This bug's fix is verified with latest Nightly!

Build ID   : 20170731100325
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170726]
See Also: → 1395602
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: