Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1883873002:
Do not block document.written scripts in page reloads (Closed)
|
Created:
4 years, 8 months ago by shivanisha Modified:
4 years, 8 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Yoav Weiss Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTo reduce the possibility of a page break by blocking a document.written
script,this patch includes a check that if the user is reloading the same
page then do not consider the document.written scripts on this page for
blocking.
BUG=602644
Committed: https://crrev.com/f6ea16af4915e7080af6ca022363e75d8d9b05f9
Cr-Commit-Position: refs/heads/master@{#389480}
Patch Set 1 #Patch Set 2 : test modifications to include the blink setting for slow connections #
Total comments: 2
Patch Set 3 : corrected if condition #
Total comments: 10
Patch Set 4 : Feedback incorporated #
Total comments: 9
Patch Set 5 : Simplified tests, Used js-loaded with a different query parameter than other tests. #
Total comments: 4
Patch Set 6 : Feedback incorporated. #
Total comments: 14
Patch Set 7 : Feedback incorporated. #
Total comments: 15
Patch Set 8 : Feedback incorporated. #
Total comments: 16
Patch Set 9 : Removed comments from test and added spaces between parameters. #
Total comments: 3
Messages
Total messages: 35 (6 generated)
shivanisha@chromium.org changed reviewers: + bmcquade@chromium.org, japhet@chromium.org, jkarlin@chromium.org
https://codereview.chromium.org/1883873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1883873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:100: There is also a FrameLoadType value: FrameLoadTypeSame which implies user loads same URL again (but not reload button). I think its fine to not consider that case. WDYT?
https://codereview.chromium.org/1883873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1883873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:90: if (document.frame()) Changing this to if (!document.frame())
https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload-no-cache.html (right): https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload-no-cache.html:28: document.write('<scr' + 'ipt src="' + src + '" onload="loadSuccess=true" onError="internals.forceReload(true)"></scr' + 'ipt>'); This whole file is a copy of the other except for a boolean value. Why not write it once in a .js file that you load from both html files? https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:18: var crossOrigin = 'http://localhost:8000' semicolon https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:19: var filePath = '/loading/resources/js-loaded.js' semicolon https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:30: document.write('<scr' + 'ipt src="' + src + '" onload="loadSuccess=true" onError="internals.forceReload(false)"></scr' + 'ipt>'); If you write something to local storage on error then you can read it on success (during the reload) in order to verify that the load failed before it succeeded. Be sure to then clear the local storage. https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:97: const bool isReload = (loadType == FrameLoadTypeReload || loadType == FrameLoadTypeReloadBypassingCache); re FrameLoadTypeSame: So the user essentially goes to the navigation bar and hits enter to load the same page again? Seems like a reload signal to me. I think we should block intervention in that case.
Incorporated review comments. Thanks to Josh's feedback of adding a local storage value to determine if the script did get blocked the first time, I am observing that the tests in isolation are running great but when run in a batch these 2 tests are not blocking the first time and thus no reload is happening as well. This looks like a race with another feature that may be caching the script thus not allowing it to be blocked but surprisingly the other existing tests are not failing. Charles' preload feature is now not enabled in layout tests so looks like some other feature that could be racing with this. Any thoughts on what could that be? https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload-no-cache.html (right): https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload-no-cache.html:28: document.write('<scr' + 'ipt src="' + src + '" onload="loadSuccess=true" onError="internals.forceReload(true)"></scr' + 'ipt>'); On 2016/04/13 at 18:38:36, jkarlin wrote: > This whole file is a copy of the other except for a boolean value. Why not write it once in a .js file that you load from both html files? done. https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:18: var crossOrigin = 'http://localhost:8000' On 2016/04/13 at 18:38:36, jkarlin wrote: > semicolon done https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:19: var filePath = '/loading/resources/js-loaded.js' On 2016/04/13 at 18:38:36, jkarlin wrote: > semicolon done https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:30: document.write('<scr' + 'ipt src="' + src + '" onload="loadSuccess=true" onError="internals.forceReload(false)"></scr' + 'ipt>'); On 2016/04/13 at 18:38:36, jkarlin wrote: > If you write something to local storage on error then you can read it on success (during the reload) in order to verify that the load failed before it succeeded. Be sure to then clear the local storage. Added https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1883873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:97: const bool isReload = (loadType == FrameLoadTypeReload || loadType == FrameLoadTypeReloadBypassingCache); On 2016/04/13 at 18:38:36, jkarlin wrote: > re FrameLoadTypeSame: So the user essentially goes to the navigation bar and hits enter to load the > same page again? Seems like a reload signal to me. I think we should block > intervention in that case. Sounds good to me. Yes, if user goes to the navigation bar and hits enter to load the same page again then its treated as FrameLoadTypeSame. Adding FrameLoadTypeSame as well. Not able to add a layout test for that condition, though, because this intervention works only for main frame and I didn't see a way to repeat the main frame URL without calling reload.
Likely the tests are running in parallel but writing to the same cache-storage object. Give each test its own object-name.
https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js (right): https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js:1: // This script sets the flags for blocking doc.write script and then tests the File should be named test-reload.js https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js:3: Thanks for making this file! Nicer to review it once. https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js:31: if (window.localStorage.getItem("blocked-on-first-load") == "true") { Use a different item name for each test so they don't interfere with each other. https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js:32: window.localStorage.clear(); localStorage is storage per-origin. So if other tests are running clear() would wipe them out as well. So instead, it's best to delete just the item. Also, you want to clear this at the end of the test regardless of success or failure.
https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:7: var reloadFromCache = false; Instead, how about wrapping the code in testreload.js in a function and passing this as a parameter?
Simplified tests to be in 1 file. Also used a different query parameter than other tests. The layout test is now running successfully in batch as well. https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:7: var reloadFromCache = false; On 2016/04/14 at 18:27:01, jkarlin wrote: > Instead, how about wrapping the code in testreload.js in a function and passing this as a parameter? Changed the reload code for both variants to be in 1 file now, so this variable is no longer needed. https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js (right): https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js:1: // This script sets the flags for blocking doc.write script and then tests the On 2016/04/14 at 18:24:39, jkarlin wrote: > File should be named test-reload.js done https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js:31: if (window.localStorage.getItem("blocked-on-first-load") == "true") { On 2016/04/14 at 18:24:39, jkarlin wrote: > Use a different item name for each test so they don't interfere with each other. I have changed this file such that both reloads now happen in the same file. Easier to see everything in one go. https://codereview.chromium.org/1883873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/testreload.js:32: window.localStorage.clear(); On 2016/04/14 at 18:24:38, jkarlin wrote: > localStorage is storage per-origin. So if other tests are running clear() would wipe them out as well. So instead, it's best to delete just the item. > > Also, you want to clear this at the end of the test regardless of success or failure. done.
Looks good! Only a couple of comments. https://codereview.chromium.org/1883873002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/resources/test-reload.js (right): https://codereview.chromium.org/1883873002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/test-reload.js:3: Now that both tests are in this one js file, can you move this code back into the html file? https://codereview.chromium.org/1883873002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/test-reload.js:21: var filePath = '/loading/resources/js-loaded.js?10'; if you need each test to have a cache query, you might consider user the test's name
https://codereview.chromium.org/1883873002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/loading/resources/test-reload.js (right): https://codereview.chromium.org/1883873002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/test-reload.js:3: On 2016/04/15 at 11:21:26, jkarlin wrote: > Now that both tests are in this one js file, can you move this code back into the html file? done. https://codereview.chromium.org/1883873002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/loading/resources/test-reload.js:21: var filePath = '/loading/resources/js-loaded.js?10'; On 2016/04/15 at 11:21:26, jkarlin wrote: > if you need each test to have a cache query, you might consider user the test's name Good idea. Modified existing tests as well.
lgtm
Description was changed from ========== To reduce the possibility of a page break by blocking a document.written script, this patch includes a check that if the user is reloading the same page then do not consider the document.written scripts on this page for blocking. BUG=602644 ========== to ========== To reduce the possibility of a page break by blocking a document.written script, this patch includes a check that if the user is reloading the same page then do not consider the document.written scripts on this page for blocking. BUG=602644 ==========
Description was changed from ========== To reduce the possibility of a page break by blocking a document.written script, this patch includes a check that if the user is reloading the same page then do not consider the document.written scripts on this page for blocking. BUG=602644 ========== to ========== To reduce the possibility of a page break by blocking a document.written script,this patch includes a check that if the user is reloading the same page then do not consider the document.written scripts on this page for blocking. BUG=602644 ==========
https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html (right): https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:53: var filePath = '/loading/resources/js-loaded.js'; For consistency, it seems this one should be js-loaded.js?cross-origin and the one below js-loaded.js?cross-origin2 https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:69: filePath = '/loading/resources/js-loaded.js?cross-origin'; Perhaps cross-origin2? https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:18: // Remove localStorage items , just in case they haven't been s/ ,/,/ https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:29: // On reload the script should not be blocked Missing period https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:30: // This tests does 2 reloads: first reload will bypass the cache and This sentence is rough. How about, "This tests two types of reload, one with and one without cache bypass." https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:32: // Script should not be blocked in both cases. s/Script/The script/ https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:50: window.localStorage.setItem("blocked-on-first-load", "true") How do you know this was the first load that failed? If you instead counted you could verify like this (note this isn't valid js): assert_eq(0, window.localStorage.getItem("errCount")); assert_eq(0, window.localStorage.getItem("succCount")); Then, in succTest you could have: assert_eq(1, window.localStorage.getItem("errCount")); assert_lt(2, window.localStorage.getItem("succCount")); if (window.localStorage.getItem("succCount") == 0) { ... } else if (window.localStorage.getItem("succCount") == 1) { ... } else { NOTREACHED(); }
https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html (right): https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:53: var filePath = '/loading/resources/js-loaded.js'; On 2016/04/18 at 14:00:43, jkarlin wrote: > For consistency, it seems this one should be js-loaded.js?cross-origin and the one below js-loaded.js?cross-origin2 done. https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:69: filePath = '/loading/resources/js-loaded.js?cross-origin'; On 2016/04/18 at 14:00:43, jkarlin wrote: > Perhaps cross-origin2? done. https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:18: // Remove localStorage items , just in case they haven't been On 2016/04/18 at 14:00:43, jkarlin wrote: > s/ ,/,/ done. https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:29: // On reload the script should not be blocked On 2016/04/18 at 14:00:43, jkarlin wrote: > Missing period Added. https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:30: // This tests does 2 reloads: first reload will bypass the cache and On 2016/04/18 at 14:00:43, jkarlin wrote: > This sentence is rough. How about, "This tests two types of reload, one with and one without cache bypass." Updated. https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:32: // Script should not be blocked in both cases. On 2016/04/18 at 14:00:43, jkarlin wrote: > s/Script/The script/ done.
https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:50: window.localStorage.setItem("blocked-on-first-load", "true") On 2016/04/18 at 14:00:43, jkarlin wrote: > How do you know this was the first load that failed? > > If you instead counted you could verify like this (note this isn't valid js): > > assert_eq(0, window.localStorage.getItem("errCount")); > assert_eq(0, window.localStorage.getItem("succCount")); > > Then, in succTest you could have: > > assert_eq(1, window.localStorage.getItem("errCount")); > assert_lt(2, window.localStorage.getItem("succCount")); > > if (window.localStorage.getItem("succCount") == 0) { > ... > } else if (window.localStorage.getItem("succCount") == 1) { > ... > } else { > NOTREACHED(); > } Updated.
Thanks, just a few more! https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:7: This script won't work without window.internals. You might as well assert_true(window.internals); up here and not check for it below. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:48: function succTest() { Layout tests have looser style, but can you use full names here instead of abbreviations to be consistent with the rest of the file? That's for succTest, succCount, errTest, and errCount. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:49: console.assert(window.localStorage.getItem("errCount") == 1,"errCount is not one"); I've not seen console.assert used in tests much. Can you use assert_equals(window.localStorage.getItem("errCount"), 1, "errCount is not one"); instead? That's provided by testharness.js. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:52: console.assert(succCount >= 0,"succCount is not >= zero"); assert_greather_than_equal https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:62: else if (succCount == 1) { It's an error if succCount isn't 1. So either return early if succCount == 0 and remove the condition here (preferred) or make this an else { assert_equals(succCount, 1); ... } https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:70: console.assert(window.localStorage.getItem("errCount") == 0,"errCount is not zero"); assert_equals https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:71: console.assert(window.localStorage.getItem("succCount") == 0,"succCount is not zero"); assert_equals
https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:62: else if (succCount == 1) { On 2016/04/18 17:42:26, jkarlin wrote: > It's an error if succCount isn't 1. So either return early if succCount == 0 and > remove the condition here (preferred) or make this an else { > assert_equals(succCount, 1); ... } The assert_equals(succCount, 1) should be there in either approach.
https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:7: On 2016/04/18 at 17:42:27, jkarlin wrote: > This script won't work without window.internals. > > You might as well assert_true(window.internals); up here and not check for it below. done. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:48: function succTest() { On 2016/04/18 at 17:42:26, jkarlin wrote: > Layout tests have looser style, but can you use full names here instead of abbreviations to be consistent with the rest of the file? > > That's for succTest, succCount, errTest, and errCount. done. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:49: console.assert(window.localStorage.getItem("errCount") == 1,"errCount is not one"); On 2016/04/18 at 17:42:26, jkarlin wrote: > I've not seen console.assert used in tests much. Can you use assert_equals(window.localStorage.getItem("errCount"), 1, "errCount is > not one"); instead? That's provided by testharness.js. done. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:52: console.assert(succCount >= 0,"succCount is not >= zero"); On 2016/04/18 at 17:42:26, jkarlin wrote: > assert_greather_than_equal done. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:62: else if (succCount == 1) { On 2016/04/18 at 17:42:26, jkarlin wrote: > It's an error if succCount isn't 1. So either return early if succCount == 0 and remove the condition here (preferred) or make this an else { assert_equals(succCount, 1); ... } Adding the return. It will never be reachable because of the preceeding forceReload but makes it clear that the subsequent code is only reachable if successCount is not 0. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:70: console.assert(window.localStorage.getItem("errCount") == 0,"errCount is not zero"); On 2016/04/18 at 17:42:26, jkarlin wrote: > assert_equals done. https://codereview.chromium.org/1883873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:71: console.assert(window.localStorage.getItem("succCount") == 0,"succCount is not zero"); On 2016/04/18 at 17:42:26, jkarlin wrote: > assert_equals done.
lgtm with comments https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:51: assert_equals(+window.localStorage.getItem("errorCount"),1,"errorCount is not one"); What does the + do before window? You use it below as well. https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:54: assert_greater_than_equal(successCount,0,"successCount is not >= zero"); spaces between parameters https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:63: // This code path will only be followed for the second reload. Comment not necessary, the assert makes that clear https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:71: assert_equals(+window.localStorage.getItem("errorCount"),0,"errorCount is not zero"); spaces between parameters https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:72: assert_equals(+window.localStorage.getItem("successCount"),0,"successCount is not zero"); spaces between parameters
https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:51: assert_equals(+window.localStorage.getItem("errorCount"),1,"errorCount is not one"); no need for the comment https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:54: assert_greater_than_equal(successCount,0,"successCount is not >= zero"); no need for the comment https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:64: assert_equals(successCount, 1, "successCount is not one"); The comment "successCount is not one" doesn't convey any extra information here. Since the comment is optional, you might as well remove it. https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:71: assert_equals(+window.localStorage.getItem("errorCount"),0,"errorCount is not zero"); no need for the comment https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:72: assert_equals(+window.localStorage.getItem("successCount"),0,"successCount is not zero"); no need for the comment
Thanks! Modified the reload test as per feedback. https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html (right): https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:51: assert_equals(+window.localStorage.getItem("errorCount"),1,"errorCount is not one"); On 2016/04/19 at 11:00:34, jkarlin wrote: > What does the + do before window? You use it below as well. This is to convert string "1" to integer 1. Removed the comment. https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:54: assert_greater_than_equal(successCount,0,"successCount is not >= zero"); On 2016/04/19 at 14:23:59, jkarlin wrote: > no need for the comment Added spaces. removed comment. https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:63: // This code path will only be followed for the second reload. On 2016/04/19 at 11:00:34, jkarlin wrote: > Comment not necessary, the assert makes that clear removed. https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:64: assert_equals(successCount, 1, "successCount is not one"); On 2016/04/19 at 14:23:59, jkarlin wrote: > The comment "successCount is not one" doesn't convey any extra information here. Since the comment is optional, you might as well remove it. Agree the comments are redundant. done. https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:71: assert_equals(+window.localStorage.getItem("errorCount"),0,"errorCount is not zero"); On 2016/04/19 at 14:23:59, jkarlin wrote: > no need for the comment done. https://codereview.chromium.org/1883873002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload.html:72: assert_equals(+window.localStorage.getItem("successCount"),0,"successCount is not zero"); On 2016/04/19 at 14:23:59, jkarlin wrote: > no need for the comment done.
LGTM. one question but it's up to you how you want to handle it. if you do treat the two settings differently, we should include a comment to explain why we decided to do so. https://codereview.chromium.org/1883873002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1883873002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:96: const FrameLoadType loadType = document.frame()->loader().loadType(); something to consider: do we only want to block on reloads if the disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections setting is true? if disallowFetchForDocWrittenScriptsInMainFrame is true that's more likely because we're trying to test this feature out (either through finch trial to canary or people have enabled it in about:flags) and in that case i wonder if it'd be better to perform the intended behavior consistently, regardless of reload or same page nav. i could imagine a googler testing this feature out for example, loading a page, seeing that it was faster, then reloading over and over and seeing that the feature seemed to be doing nothing / wasn't making the page faster. i could go either way here but want to make sure we take this into account as part of this change. i am supportive of whatever you and josh decide.
https://codereview.chromium.org/1883873002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1883873002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:96: const FrameLoadType loadType = document.frame()->loader().loadType(); If the only purpose of disallowFetchForDocWrittenScriptsInMainFrame is to allow manual testing via about:flags , giving a consistent experience irrespective of a reload sounds fine for ease of testing. I am assuming the finch trial will still be based on disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections? That's because we would want the blocking to be disabled if a user reloads the page to allow for breaking pages to recover. Is reaching a small population because of 2G a concern for finch trial?
https://codereview.chromium.org/1883873002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1883873002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:96: const FrameLoadType loadType = document.frame()->loader().loadType(); On 2016/04/22 15:35:34, shivanisha wrote: > If the only purpose of disallowFetchForDocWrittenScriptsInMainFrame is to allow > manual testing via about:flags , giving a consistent experience irrespective of > a reload sounds fine for ease of testing. > > I am assuming the finch trial will still be based on > disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections? That's because we > would want the blocking to be disabled if a user reloads the page to allow for > breaking pages to recover. Is reaching a small population because of 2G a > concern for finch trial? Yes, we're likely to want to expand to other network types, so we should avoid blocking on reloads in those cases as well. Further, it's nice that the flag mirrors the experiment so we can see how it will behave. I see your point though Bryan, perhaps another flag that blocks even on reloads would be beneficial.
Nate: PTAL, thanks!
lgtm
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/1883873002/#ps160001 (title: "Removed comments from test and added spaces between parameters.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883873002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883873002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== To reduce the possibility of a page break by blocking a document.written script,this patch includes a check that if the user is reloading the same page then do not consider the document.written scripts on this page for blocking. BUG=602644 ========== to ========== To reduce the possibility of a page break by blocking a document.written script,this patch includes a check that if the user is reloading the same page then do not consider the document.written scripts on this page for blocking. BUG=602644 Committed: https://crrev.com/f6ea16af4915e7080af6ca022363e75d8d9b05f9 Cr-Commit-Position: refs/heads/master@{#389480} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f6ea16af4915e7080af6ca022363e75d8d9b05f9 Cr-Commit-Position: refs/heads/master@{#389480} |
