|
|
Created:
4 years, 4 months ago by lfg Modified:
4 years, 3 months ago CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable navigations in the unload handler.
Blink intent:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VfItzNe3WO0
BUG=613244
Committed: https://crrev.com/8fb70277dba468aac9d2eae51e432d76667a79db
Cr-Commit-Position: refs/heads/master@{#414588}
Patch Set 1 #
Total comments: 1
Patch Set 2 : addressing comment #Patch Set 3 : add layouttest #
Total comments: 1
Patch Set 4 : testharness.js #
Messages
Total messages: 43 (22 generated)
Description was changed from ========== WIP ========== to ========== Disable navigations in the unload handler. BUG=613244 ==========
Description was changed from ========== Disable navigations in the unload handler. BUG=613244 ========== to ========== Disable navigations in the unload handler. Blink intent: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VfItzNe3WO0 BUG=613244 ==========
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lfg@chromium.org changed reviewers: + japhet@chromium.org
Please take a look.
ping.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2206843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/NavigationScheduler.h (right): https://codereview.chromium.org/2206843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/NavigationScheduler.h:112: class CORE_EXPORT NavigationDisablerForUnload { Can we just use NavigationDisablerForBeforeUnload, but rename it?
On 2016/08/16 22:32:03, dcheng wrote: > https://codereview.chromium.org/2206843003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/loader/NavigationScheduler.h (right): > > https://codereview.chromium.org/2206843003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/loader/NavigationScheduler.h:112: class > CORE_EXPORT NavigationDisablerForUnload { > Can we just use NavigationDisablerForBeforeUnload, but rename it? We could, the only side effect is that javascript navigations would be scheduled, but deleted as the frame is about to commit. WDYT?
On 2016/08/16 22:35:32, lfg wrote: > On 2016/08/16 22:32:03, dcheng wrote: > > > https://codereview.chromium.org/2206843003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/loader/NavigationScheduler.h (right): > > > > > https://codereview.chromium.org/2206843003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/loader/NavigationScheduler.h:112: class > > CORE_EXPORT NavigationDisablerForUnload { > > Can we just use NavigationDisablerForBeforeUnload, but rename it? > > We could, the only side effect is that javascript navigations would be > scheduled, but deleted as the frame is about to commit. WDYT? javascript: URLs are currently synchronously loaded, not scheduled. It's also not a problem for our specific bug, I think. Note there is a tentative plan to schedule these as well, though I don't think anyone is working on this yet. Also, have we tried providing feedback to the spec to add these restrictions?
On 2016/08/16 22:44:46, dcheng wrote: > > javascript: URLs are currently synchronously loaded, not scheduled. It's also > not a problem for our specific bug, I think. Note there is a tentative plan to > schedule these as well, though I don't think anyone is working on this yet. I did some investigation on this. It depends on how js urls are loaded. If they are loaded by window.location.href = 'javascript:', then the code goes through LocalFrame::navigate and the navigation is scheduled. If the js url is loaded by iframeElement.src = 'javascript:' then a navigation to about:blank is scheduled, and the javascript is executed synchronously in HTMLFrameElementBase::openURL. For this particular bug, in the first case the navigation would be scheduled, and deleted when the frame is about to commit. For the second case, there's no change since the navigation is executed synchronously, but the scheduled navigation to about:blank still exists, and is deleted when the frame is also about to commit. In general, I don't think it's a good idea to be scheduling navigations that will never complete. I'll work on merging both navigation disablers with a parameter so that we can avoid that case. > Also, have we tried providing feedback to the spec to add these restrictions? Yes, I've opened a spec bug about this, and will follow up as we land this.
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/23 17:08:38, lfg wrote: > In general, I don't think it's a good idea to be scheduling navigations that > will never complete. I'll work on merging both navigation disablers with a > parameter so that we can avoid that case. This adds too much complexity to the code. I ended up just merging them both, the scheduled navigations are deleted when the initial navigation is being committed (no different than what currently happens). Please take another look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The code change l-g-t-m, but I'm wondering if this is testable?
On 2016/08/24 18:46:08, dcheng wrote: > The code change l-g-t-m, but I'm wondering if this is testable? There's a test in the bug that I was planning on adding it in a follow-up patch (since it's a browser test and not a blink test).
On 2016/08/24 18:48:04, lfg wrote: > On 2016/08/24 18:46:08, dcheng wrote: > > The code change l-g-t-m, but I'm wondering if this is testable? > > There's a test in the bug that I was planning on adding it in a follow-up > patch (since it's a browser test and not a blink test). That feels like it's more about observing the side effects rather than directly testing the change though. Is it possible to just verify that the hash didn't change after trying to navigate in unload?
On 2016/08/24 20:57:38, dcheng wrote: > > That feels like it's more about observing the side effects rather than directly > testing the change though. Is it possible to just verify that the hash didn't > change after trying to navigate in unload? That's right. Since with this patch the original navigation will commit, regardless of the hash navigation in the unload handler, I could add a test that verifies that the original navigation commits, but it seems that it would also be observing the side effects, but a bit closer to what's actually happening. Would that be ok?
On 2016/08/24 21:11:56, lfg wrote: > On 2016/08/24 20:57:38, dcheng wrote: > > > > That feels like it's more about observing the side effects rather than > directly > > testing the change though. Is it possible to just verify that the hash didn't > > change after trying to navigate in unload? > > That's right. Since with this patch the original navigation will commit, > regardless > of the hash navigation in the unload handler, I could add a test that verifies > that > the original navigation commits, but it seems that it would also be observing > the > side effects, but a bit closer to what's actually happening. Would that be ok? Fragment navigations are never scheduled, AFAIK, so I think we can just observe that the fragment navigation doesn't actually change.
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/25 03:03:06, dcheng wrote: > > Fragment navigations are never scheduled, AFAIK, so I think we can just observe > that the fragment navigation doesn't actually change. Added a layouttest. Please take another look.
Sorry, one more test question. Also, I think there's a typo in the test name (I guess it should be in-unload rather than un-unload) https://codereview.chromium.org/2206843003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/navigation/hash-navigation-un-unload-handler.html (right): https://codereview.chromium.org/2206843003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/navigation/hash-navigation-un-unload-handler.html:19: document.writeln("TEST PASSED"); I think we have a pretty strong preference to writing all new tests using testharness.js - would that be possible here?
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
On 2016/08/25 18:51:40, dcheng wrote: > Sorry, one more test question. Also, I think there's a typo in the test name (I > guess it should be in-unload rather than un-unload) Done. > https://codereview.chromium.org/2206843003/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/navigation/hash-navigation-un-unload-handler.html > (right): > > https://codereview.chromium.org/2206843003/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/navigation/hash-navigation-un-unload-handler.html:19: > document.writeln("TEST PASSED"); > I think we have a pretty strong preference to writing all new tests using > testharness.js - would that be possible here? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by lfg@chromium.org
The CQ bit was checked by lfg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lfg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Disable navigations in the unload handler. Blink intent: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VfItzNe3WO0 BUG=613244 ========== to ========== Disable navigations in the unload handler. Blink intent: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VfItzNe3WO0 BUG=613244 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Disable navigations in the unload handler. Blink intent: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VfItzNe3WO0 BUG=613244 ========== to ========== Disable navigations in the unload handler. Blink intent: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VfItzNe3WO0 BUG=613244 Committed: https://crrev.com/8fb70277dba468aac9d2eae51e432d76667a79db Cr-Commit-Position: refs/heads/master@{#414588} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8fb70277dba468aac9d2eae51e432d76667a79db Cr-Commit-Position: refs/heads/master@{#414588} |