Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(533)

Issue 1381003004: Better distinguish didFinishLoad and didStopLoading (Closed)

Created:
5 years, 2 months ago by Nate Chapin
Modified:
5 years, 1 month ago
CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, gsennton, kinuko+watch, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Better distinguish didFinishLoad and didStopLoading Currently, they mean roughly the same thing. didFinishLoad fires when a navigation successfully completes and no other navigations are in progress. didStopLoading does the same thing, but fires whether or not the navigation was successful. This changes didFinishLoad to mean "a navigation successfully completed", while didStopLoading still means "there are now no navigations in progress in this frame". For each didStartProvisionalLoad, there will be exactly one of: didFailProvisionalLoad, didFailLoad, didFinishLoad. Prior to this change, there would be at most one, but it could be that a load would complete without a matching notification. This also reverses the order of didFinishLoad and didStopLoading. didStartLoading and didStopLoading will now bracket zero to many individual navigation notifications, which may be interleaved. This requires 4 changes to consumers of these notifications: 1. http/tests/loading/progress-finished-callback.html asserted the old finish/stop ordering, and did nothing else. Remove it and its test harness logic. 2. web_navigation_api.cc assumed that didFinishLoad would not be sent if a new provisional navigation began before the finish notification. Change a DCHECK to an early exit. 3. password_autofill_agent.cc depends on stop firing before finish for determining whether all loading is done. Give it the ability to determine loading state for itself inside DidFinishLoad. 4. chrome/android/ incorrectly listens to the browser process equivalents of didStartProvisionaLoad and didFinishLoad to determine whether loading is in progress and start/stop the progress bar. Switch it to the equivalent of didStartLoading and didStopLoading. BUG=539952 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect Committed: https://crrev.com/ba1f66fc44875b25efb3faf991c0b6754793088b Cr-Commit-Position: refs/heads/master@{#357517}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 21

Patch Set 6 : address dcheng's comments #

Patch Set 7 : Fix WebFrameTest.CallbackOrdering race #

Total comments: 1

Patch Set 8 : Fix android failure #

Total comments: 4

Patch Set 9 : Don't pulse progress bar for same-document navs #

Total comments: 8

Patch Set 10 : #

Patch Set 11 : #

Total comments: 2

Patch Set 12 : Pass toDifferentDocument in onLoadStopped #

Patch Set 13 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+179 lines, -181 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/tab/EmptyTabObserver.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +20 lines, -3 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/tab/TabObserver.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -9 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +24 lines, -24 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/HistoryUITest.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/toolbar/BrandColorTest.java View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/web_navigation/web_navigation_api.cc View 1 2 3 4 5 6 7 1 chunk +10 lines, -6 lines 0 comments Download
M chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/TabLoadObserver.java View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -3 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.h View 1 2 3 4 5 6 7 3 chunks +0 lines, -7 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.cc View 1 2 3 4 5 6 7 8 4 chunks +10 lines, -20 lines 0 comments Download
M components/test_runner/test_runner.h View 1 2 3 4 5 6 3 chunks +0 lines, -10 lines 0 comments Download
M components/test_runner/test_runner.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +0 lines, -17 lines 0 comments Download
M components/test_runner/web_frame_test_proxy.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -5 lines 0 comments Download
M components/test_runner/web_test_proxy.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M components/test_runner/web_test_proxy.cc View 1 2 3 4 5 6 7 9 1 chunk +0 lines, -5 lines 0 comments Download
M components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M components/web_contents_delegate_android/web_contents_delegate_android.cc View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -3 lines 0 comments Download
D third_party/WebKit/LayoutTests/http/tests/loading/progress-finished-callback.html View 1 chunk +0 lines, -7 lines 0 comments Download
D third_party/WebKit/LayoutTests/http/tests/loading/progress-finished-callback-expected.txt View 1 chunk +0 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/loader/DocumentLoader.h View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/loader/DocumentLoader.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +31 lines, -23 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrame.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +32 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrame.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 51 (14 generated)
Nate Chapin
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc#newcode1102 components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame != render_frame()->GetWebFrame() && frame->isLoading()) { This is ...
5 years, 2 months ago (2015-10-06 18:35:36 UTC) #2
dcheng
https://codereview.chromium.org/1381003004/diff/80001/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/1381003004/diff/80001/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc#newcode404 chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:404: if (navigation_state_.GetUrl(render_frame_host) != validated_url && This first check seems ...
5 years, 2 months ago (2015-10-08 05:34:35 UTC) #3
dcheng
https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode542 third_party/WebKit/Source/core/loader/FrameLoader.cpp:542: // We might have declined to run the load ...
5 years, 2 months ago (2015-10-08 05:37:41 UTC) #4
Nate Chapin
https://codereview.chromium.org/1381003004/diff/80001/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/1381003004/diff/80001/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc#newcode404 chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:404: if (navigation_state_.GetUrl(render_frame_host) != validated_url && On 2015/10/08 05:34:35, dcheng ...
5 years, 2 months ago (2015-10-08 21:03:59 UTC) #5
dcheng
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc#newcode1102 components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame != render_frame()->GetWebFrame() && frame->isLoading()) { On 2015/10/08 ...
5 years, 2 months ago (2015-10-09 18:34:01 UTC) #6
Nate Chapin
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc#newcode1102 components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame != render_frame()->GetWebFrame() && frame->isLoading()) { On 2015/10/09 ...
5 years, 2 months ago (2015-10-09 18:39:44 UTC) #7
dcheng
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc#newcode1102 components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame != render_frame()->GetWebFrame() && frame->isLoading()) { On 2015/10/09 ...
5 years, 2 months ago (2015-10-09 18:45:26 UTC) #8
Nate Chapin
On 2015/10/09 18:45:26, dcheng wrote: > https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc#newcode1102 > ...
5 years, 2 months ago (2015-10-09 18:52:32 UTC) #9
dcheng
On 2015/10/09 at 18:52:32, japhet wrote: > On 2015/10/09 18:45:26, dcheng wrote: > > https://codereview.chromium.org/1381003004/diff/80001/components/autofill/content/renderer/password_autofill_agent.cc ...
5 years, 2 months ago (2015-10-09 18:54:18 UTC) #10
Nate Chapin
On 2015/10/09 18:54:18, dcheng wrote: > On 2015/10/09 at 18:52:32, japhet wrote: > > On ...
5 years, 2 months ago (2015-10-09 18:56:51 UTC) #11
dcheng
On 2015/10/09 at 18:56:51, japhet wrote: > On 2015/10/09 18:54:18, dcheng wrote: > > On ...
5 years, 2 months ago (2015-10-09 18:58:10 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381003004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381003004/100001
5 years, 2 months ago (2015-10-09 19:41:27 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/108280)
5 years, 2 months ago (2015-10-09 19:50:31 UTC) #16
Nate Chapin
dpranke: Please review components/test_runner/ changes isherman: Please review components/autofill/ changes nasko: Please review chrome/browser/extensions/api/web_navigation/ changes
5 years, 2 months ago (2015-10-09 21:01:32 UTC) #18
Dirk Pranke
lgtm
5 years, 2 months ago (2015-10-09 21:06:05 UTC) #19
Ilya Sherman
autofill lgtm, thanks.
5 years, 2 months ago (2015-10-10 02:40:48 UTC) #20
nasko
https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc#newcode409 chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:409: return; Does your change imply that a DidFailLoad will ...
5 years, 2 months ago (2015-10-12 23:48:24 UTC) #21
Nate Chapin
On 2015/10/12 23:48:24, nasko (slow to review) wrote: > https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc > File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): > ...
5 years, 2 months ago (2015-10-13 18:30:36 UTC) #22
nasko
On 2015/10/13 18:30:36, Nate Chapin wrote: > On 2015/10/12 23:48:24, nasko (slow to review) wrote: ...
5 years, 2 months ago (2015-10-13 20:59:38 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381003004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381003004/120001
5 years, 2 months ago (2015-10-13 21:11:34 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/81499)
5 years, 2 months ago (2015-10-14 00:00:06 UTC) #28
Nate Chapin
nednguyen: Please review the logging addition in tools/perf/measurements tedchoc: Please review the chrome/android/ changes. I ...
5 years, 2 months ago (2015-10-22 21:21:26 UTC) #31
Nate Chapin
tedchoc: Please review the chrome/android/ changes. I added context to the patch description, so hopefully ...
5 years, 2 months ago (2015-10-22 21:21:50 UTC) #33
nednguyen
On 2015/10/22 21:21:50, Nate Chapin wrote: > tedchoc: Please review the chrome/android/ changes. I added ...
5 years, 2 months ago (2015-10-22 21:27:32 UTC) #34
Nate Chapin
tedchoc: friendly ping :)
5 years, 1 month ago (2015-10-27 22:37:19 UTC) #35
Ted C
https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode1382 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1382: protected void onLoadStarted() { javadoc non-private methods https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java File ...
5 years, 1 month ago (2015-10-27 23:07:20 UTC) #36
Nate Chapin
https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode1382 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1382: protected void onLoadStarted() { On 2015/10/27 23:07:19, Ted C ...
5 years, 1 month ago (2015-10-28 20:57:39 UTC) #37
Ted C
https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java#newcode137 chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:137: if (toDifferentDocument) mProgressObserver.onProgressBarStarted(); this wasn't guarded before by toDifferentDocument, ...
5 years, 1 month ago (2015-10-28 23:41:02 UTC) #38
Nate Chapin
https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java#newcode137 chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:137: if (toDifferentDocument) mProgressObserver.onProgressBarStarted(); On 2015/10/28 23:41:02, Ted C wrote: ...
5 years, 1 month ago (2015-10-30 20:41:34 UTC) #39
Ted C
https://codereview.chromium.org/1381003004/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1381003004/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode1410 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1410: mIsLoading = false; I think you'll have to synthesize ...
5 years, 1 month ago (2015-11-02 21:08:47 UTC) #40
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381003004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381003004/220001
5 years, 1 month ago (2015-11-02 21:49:58 UTC) #42
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_dbg/builds/135286)
5 years, 1 month ago (2015-11-02 22:11:17 UTC) #44
Nate Chapin
https://codereview.chromium.org/1381003004/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1381003004/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode1410 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1410: mIsLoading = false; On 2015/11/02 21:08:47, Ted C wrote: ...
5 years, 1 month ago (2015-11-02 23:13:19 UTC) #45
Ted C
lgtm
5 years, 1 month ago (2015-11-02 23:18:29 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381003004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381003004/240001
5 years, 1 month ago (2015-11-02 23:40:21 UTC) #49
commit-bot: I haz the power
Committed patchset #13 (id:240001)
5 years, 1 month ago (2015-11-03 02:41:33 UTC) #50
commit-bot: I haz the power
5 years, 1 month ago (2015-11-03 02:42:49 UTC) #51
Message was sent while issue was closed.
Patchset 13 (id:??) landed as
https://crrev.com/ba1f66fc44875b25efb3faf991c0b6754793088b
Cr-Commit-Position: refs/heads/master@{#357517}

Powered by Google App Engine
This is Rietveld 408576698