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

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});`

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.

(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

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.

Now switches to reload button if navigating to a local URL while the current page is loading.

(Small typo fix in a test case from the recently cancelled review)

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.

Comment on attachment 8885936 [details] [diff] [review] stopreload.diff Review of attachment 8885936 [details] [diff] [review]: ----------------------------------------------------------------- See comment 18.

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

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

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.

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

https://hg.mozilla.org/mozilla-central/rev/bb2904560e11 https://hg.mozilla.org/mozilla-central/rev/dbb63faa7da3

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