Don't show the stop button for local url loads

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
2 months ago
17 days ago

People

(Reporter: florian, Assigned: Perry)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

23.04 KB, patch
jaws
: review+
Details | Diff | Splinter Review
8.55 KB, patch
jaws
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 months ago
When first loading about:home, the stop button flickers before we show the reload button.
(Reporter)

Comment 1

2 months ago
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
(Assignee)

Comment 3

2 months ago
Created attachment 8882631 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loads
Attachment #8882631 - Flags: review?(jaws)
(Assignee)

Comment 4

2 months ago
Created attachment 8882698 [details] [diff] [review]
Bug 1376892: Move browser_about* tests from 'general' to 'about' folder
Attachment #8882698 - Flags: review?(jaws)
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+
(Assignee)

Comment 6

2 months ago
Created attachment 8883116 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loadsdontshowstop.diff
Attachment #8882631 - Attachment is obsolete: true
Attachment #8883116 - Flags: review+

Comment 7

2 months ago
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-
Duplicate of this bug: 1379590
(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
(Assignee)

Comment 11

a month ago
Created attachment 8885357 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loads
Attachment #8883116 - Attachment is obsolete: true
Attachment #8885357 - Flags: review?(jaws)
(Assignee)

Comment 12

a month ago
Created attachment 8885358 [details] [diff] [review]
Bug 1376892: Move browser_about* tests from 'general' to 'about' folder
Attachment #8882698 - Attachment is obsolete: true
Attachment #8885358 - Flags: review?(jaws)
(Assignee)

Updated

a month ago
Attachment #8885357 - Flags: review?(jaws)
(Assignee)

Updated

a month ago
Attachment #8885358 - Flags: review?(jaws)
(Assignee)

Comment 13

a month ago
Created attachment 8885570 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loads.diff
Attachment #8885357 - Attachment is obsolete: true
Attachment #8885358 - Attachment is obsolete: true
Attachment #8885570 - Flags: review?(jaws)
(Assignee)

Comment 14

a month ago
Created attachment 8885573 [details] [diff] [review]
Bug 1376892: Move browser_about* tests from 'general' to 'about' folder
Attachment #8885573 - 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+
(Assignee)

Comment 16

a month ago
Created attachment 8885931 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url loads

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)
(Assignee)

Updated

a month ago
Attachment #8885931 - Flags: review?(jaws)
(Assignee)

Comment 17

a month ago
Created attachment 8885936 [details] [diff] [review]
stopreload.diff

(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-
(Assignee)

Comment 20

a month ago
Created attachment 8885966 [details] [diff] [review]
Bug 1376892: Don't show stop button for local url
Attachment #8885936 - Attachment is obsolete: true
Attachment #8885966 - Flags: review?(jaws)
Attachment #8885966 - Flags: review?(jaws) → review+

Comment 21

a month ago
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
Last Resolved: a month ago
status-firefox56: --- → fixed
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?
(Assignee)

Comment 27

29 days ago
Created attachment 8888470 [details] [diff] [review]
Bug 1376892 - Don't show stop button for local url

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+
Keywords: checkin-needed

Comment 28

29 days ago
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
https://hg.mozilla.org/mozilla-central/rev/bb2904560e11
https://hg.mozilla.org/mozilla-central/rev/dbb63faa7da3
Status: REOPENED → RESOLVED
Last Resolved: a month ago29 days ago
Resolution: --- → FIXED

Updated

28 days ago
Depends on: 1383117
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]
Depends on: 1386255
You need to log in before you can comment on or make changes to this bug.