Sending an async GET request for doc.written blocked scripts.

Sending an async GET request for doc.written blocked scripts.
BUG=639401
While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and
fetchScript sets the intervention info and header in the resource request.
The intervention info is used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

Also added internals.getResourcePriority() for
the layout test to test that the new request goes out with priority very low.

The code was tested using netlog to verify the second request is sent and has
the correct priority (net::IDLE).
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

Committed: https://crrev.com/57536eaf61f21cb9f6e351663ef75c3d6e09d705
Cr-Commit-Position: refs/heads/master@{#419476}

[shivanisha, 2016-08-19 19:37:13]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE)
==========

[shivanisha, 2016-08-19 19:37:35]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE)
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE)
==========

[shivanisha, 2016-08-19 19:43:22]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE)
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites is created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

The rest of this intervention has been tested with layout tests. I did not find
a way to test this part, with layout tests. If you have thoughts on how to add
tests for this change, let me know.
==========

[shivanisha, 2016-08-19 19:43:48]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites is created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

The rest of this intervention has been tested with layout tests. I did not find
a way to test this part, with layout tests. If you have thoughts on how to add
tests for this change, let me know.
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

The rest of this intervention has been tested with layout tests. I did not find
a way to test this part, with layout tests. If you have thoughts on how to add
tests for this change, let me know.
==========

[shivanisha, 2016-08-19 20:10:16]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 20:11:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-19 20:31:10]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 20:31:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-19 20:38:06]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 20:38:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-19 20:39:21]

shivanisha@chromium.org changed reviewers:
+ csharrison@chromium.org, japhet@chromium.org

[shivanisha, 2016-08-19 20:40:21]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

The rest of this intervention has been tested with layout tests. I did not find
a way to test this part, with layout tests. If you have thoughts on how to add
tests for this change, let me know.
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

The rest of this intervention has been tested with layout tests. I did not find
a way to test this part, with layout tests. If you have thoughts on how to add
tests for this change, please let me know.
==========

[shivanisha, 2016-08-19 20:50:42]

On 2016/08/19 at 20:50:23, shivanisha wrote:
> 

PTAL, Thanks!

[Charlie Harrison, 2016-08-19 21:24:27]

I'll let Nate sign off on the design portion. I left a few comments inline.

My main concern would be overloading the ScriptLoader instead of creating a new
class that initiates the fetch Are you sure you need all the logic in
ScriptLoader for this? I'm nervous that it will lead to unintended bugs.

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.h:125: bool
m_disallowedFetchForDocWrittenScript : 1; // This script will be blocked if not
present in http cache.
Can these be combined into an enum flag?

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.h:126: const bool
m_blockedDocWriteScriptAsyncFetch : 1; // Non parser-blocking lowest priority
fetch for the blocked script.
The comment here doesn't really specify what the "true" or "false" states
correspond to.

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:277: if
(isExecutingScript() && cachedResource->wasCanceled()) {
Will we ever hit this condition? Judging from the comment it seems like we
might?

[commit-bot: I haz the power, 2016-08-19 21:41:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 21:41:57]

Dry run: 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_...)

[shivanisha, 2016-08-19 22:29:51]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

The rest of this intervention has been tested with layout tests. I did not find
a way to test this part, with layout tests. If you have thoughts on how to add
tests for this change, please let me know.
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

I am currently working on adding testRunner.dumpResourceRequestPriorities() in 
the layout tests for this to test that the new request goes out with priority
very low.
==========

[shivanisha, 2016-08-22 00:56:51]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 00:57:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 01:59:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 01:59:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shivanisha, 2016-08-22 14:40:22]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 14:45:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-22 15:51:10]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 15:51:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-22 15:52:59]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with 
m_blockedDocWriteScriptAsyncFetch set to true. It then calls 
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the 
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

I am currently working on adding testRunner.dumpResourceRequestPriorities() in 
the layout tests for this to test that the new request goes out with priority
very low.
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and
fetchScript sets the intervention info and header in the resource request. 
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

Also added testRunner.dumpResourceRequestPriorities() in 
the layout test to test that the new request goes out with priority very low.
==========

[shivanisha, 2016-08-22 15:53:13]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and
fetchScript sets the intervention info and header in the resource request. 
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

Also added testRunner.dumpResourceRequestPriorities() in 
the layout test to test that the new request goes out with priority very low.
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and 
fetchScript sets the intervention info and header in the resource request. 
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

Also added testRunner.dumpResourceRequestPriorities() in 
the layout test to test that the new request goes out with priority very low.
==========

[shivanisha, 2016-08-22 16:13:46]

On 2016/08/19 at 21:24:27, csharrison wrote:
> I'll let Nate sign off on the design portion. I left a few comments inline.
> 
> My main concern would be overloading the ScriptLoader instead of creating a
new class that initiates the fetch Are you sure you need all the logic in
ScriptLoader for this? I'm nervous that it will lead to unintended bugs.

Took a second look at the logic in ScriptLoader. I think if we make it separate
from ScriptLoader and do not reuse the prepareScript and fetchScript
functionalities then we might run a risk of unintended bugs since we want this
script fetch to be like any other script fetch (except for the priority and
custom header). The main difference lies in what we do when we receive this
script(we do not execute it) vs a normal script. 
Since all the logic related to this fetch in ScriptLoader is completely
encapsulated with the use of the flag, this looks like a reasonably safe change.
Nate, Charles: Thoughts?

(We could also refactor prepare script and fetchscript and reuse parts of it,
but that seems to be a bigger change than what we intend for this change.)

> 
>
https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):
> 
>
https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/ScriptLoader.h:125: bool
m_disallowedFetchForDocWrittenScript : 1; // This script will be blocked if not
present in http cache.
> Can these be combined into an enum flag?
> 
>
https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/ScriptLoader.h:126: const bool
m_blockedDocWriteScriptAsyncFetch : 1; // Non parser-blocking lowest priority
fetch for the blocked script.
> The comment here doesn't really specify what the "true" or "false" states
correspond to.
> 
>
https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:277: if
(isExecutingScript() && cachedResource->wasCanceled()) {
> Will we ever hit this condition? Judging from the comment it seems like we
might?

[shivanisha, 2016-08-22 16:15:05]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 16:15:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-22 16:15:22]

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.h:125: bool
m_disallowedFetchForDocWrittenScript : 1; // This script will be blocked if not
present in http cache.
On 2016/08/19 at 21:24:27, Charlie Harrison wrote:
> Can these be combined into an enum flag?

Done for readability and the fact that both of these flags are mutually
exclusive. (even though it increases the size a bit from 2 booleans to enum)

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.h:126: const bool
m_blockedDocWriteScriptAsyncFetch : 1; // Non parser-blocking lowest priority
fetch for the blocked script.
On 2016/08/19 at 21:24:27, Charlie Harrison wrote:
> The comment here doesn't really specify what the "true" or "false" states
correspond to.

done.

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:277: if
(isExecutingScript() && cachedResource->wasCanceled()) {
On 2016/08/19 at 21:24:27, Charlie Harrison wrote:
> Will we ever hit this condition? Judging from the comment it seems like we
might?

Since its a canceled scenario, its fine to return from here and not send the
async GET request.

[shivanisha, 2016-08-22 17:47:22]

shivanisha@chromium.org changed reviewers:
+ haraken@chromium.org

[shivanisha, 2016-08-22 17:48:29]

Haraken@, PTAL, Thanks!

[commit-bot: I haz the power, 2016-08-22 17:53:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 17:53:04]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-08-22 19:47:03]

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (left):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:126: 
Why these deletes?

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:42: static ScriptLoader*
create(Element* element, bool createdByParser, bool isEvaluated, bool
createdDuringDocumentWrite = false, bool blockedDocWriteScriptAsyncFetch =
false)
Rather than adding another boolean parameter to a function that already has 3,
can we have a separate setter? This is a sufficiently unique case that I don't
think it belongs in the constructor.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:166: priority =
request.resourceRequest().computePriorityForInterventions(priority);
This seems like overkill at first glance. Is there a reason marking the request
as LazyLoad is insufficient?

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:297: if
(blockedDocWriteScriptAsyncFetch) {
Does this logic absolutely need to be after notifyScriptLoaded()? It adds a
bunch of complexity to have to spread it over.

[shivanisha, 2016-08-22 20:41:20]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 20:42:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-22 20:44:00]

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (left):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:126: 
On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> Why these deletes?

reverted.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:42: static ScriptLoader*
create(Element* element, bool createdByParser, bool isEvaluated, bool
createdDuringDocumentWrite = false, bool blockedDocWriteScriptAsyncFetch =
false)
On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> Rather than adding another boolean parameter to a function that already has 3,
can we have a separate setter? This is a sufficiently unique case that I don't
think it belongs in the constructor.

Done.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:166: priority =
request.resourceRequest().computePriorityForInterventions(priority);
On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> This seems like overkill at first glance. Is there a reason marking the
request as LazyLoad is insufficient?

LazyLoad results in priority ResourceLoadPriorityLow which maps to net::LOWEST
but we want a priority net::IDLE for this case.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:297: if
(blockedDocWriteScriptAsyncFetch) {
On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> Does this logic absolutely need to be after notifyScriptLoaded()? It adds a
bunch of complexity to have to spread it over.

I am not completely aware of the other threads that are going on simultaneously
but notifyScriptLoaded() does a bunch of things including
resumeParsingAfterScriptExecution() that calls 
HTMLDocumentParser::pumpTokenizer() and
HTMLDocumentParser::pumpPendingSpeculations(). If it does something that can
lead to posting tasks to other threads then it would be good to not delay its
execution. While debugging fetchBlockedDocWriteScript() using gdb I was seeing
it took a noticeable time before returning from ScriptResource::fetch() call.

Thoughts?

[Nate Chapin, 2016-08-22 20:55:12]

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:166: priority =
request.resourceRequest().computePriorityForInterventions(priority);
On 2016/08/22 20:44:00, shivanisha wrote:
> On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> > This seems like overkill at first glance. Is there a reason marking the
> request as LazyLoad is insufficient?
> 
> LazyLoad results in priority ResourceLoadPriorityLow which maps to net::LOWEST
> but we want a priority net::IDLE for this case.

Ah, I forgot about the script-specific LazyLoad priority.

Adding an Idle value to the ResourceFetcher::DeferOption enum seemed like it'd
be cleaner than plumbing this through ResourceRequest. Can we do that instead?

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:297: if
(blockedDocWriteScriptAsyncFetch) {
On 2016/08/22 20:44:00, shivanisha wrote:
> On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> > Does this logic absolutely need to be after notifyScriptLoaded()? It adds a
> bunch of complexity to have to spread it over.
> 
> I am not completely aware of the other threads that are going on
simultaneously
> but notifyScriptLoaded() does a bunch of things including
> resumeParsingAfterScriptExecution() that calls 
> HTMLDocumentParser::pumpTokenizer() and
> HTMLDocumentParser::pumpPendingSpeculations(). If it does something that can
> lead to posting tasks to other threads then it would be good to not delay its
> execution. While debugging fetchBlockedDocWriteScript() using gdb I was seeing
> it took a noticeable time before returning from ScriptResource::fetch() call.
> 
> Thoughts?

Hrm, ok. One option is to move all the new code here into a helper
(maybeFetchBlockedDocWriteScript() or something like that), have it return false
if it didn't fetch a blocked script, and let it have a
m_host->notifyScriptLoaded() call in the appropriate place in its own flow. If
it doesn't fetch anything, then it just falls through to the
notifyScriptLoaded() call here. That we at least contain this specialized logic.
WDYT?

[commit-bot: I haz the power, 2016-08-22 23:04:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 23:04:57]

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...)

[shivanisha, 2016-08-23 00:06:00]

shivanisha@chromium.org changed reviewers:
- haraken@chromium.org

[shivanisha, 2016-08-23 00:07:06]

Removing haraken@ as reviewer since ResourceRequest.* will no longer have any
code changes after incorporating Nate's comments.

[haraken, 2016-08-23 00:30:38]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, hiroshige@chromium.org

[haraken, 2016-08-23 00:30:39]

+hiroshige for MemoryCache handling.

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:91: bool
disallowedFetchForDocWrittenScript() { return m_documentWriteIntervention ==
DocumentWriteIntervention::DisallowedFetchForDocWrittenScript; }

disallowedFetchForDocWrittenScript => shouldFetchDocWrittenScript ?

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:92: void
setBlockedDocWriteScriptAsyncFetch();

"BlockedDocWrite" sounds a bit strange to me. Is there any non-blocking
document.write?

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:131:
DisallowedFetchForDocWrittenScript,

DoNotFetchDocWrittenScript ?

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:133:
AsyncLowPriorityFetchForBlockedScript,

FetchDocWrittenScriptAsynchrously ?

[shivanisha, 2016-08-23 01:02:22]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 01:03:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-23 01:04:46]

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:166: priority =
request.resourceRequest().computePriorityForInterventions(priority);
On 2016/08/22 at 20:55:12, Nate Chapin wrote:
> On 2016/08/22 20:44:00, shivanisha wrote:
> > On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> > > This seems like overkill at first glance. Is there a reason marking the
> > request as LazyLoad is insufficient?
> > 
> > LazyLoad results in priority ResourceLoadPriorityLow which maps to
net::LOWEST
> > but we want a priority net::IDLE for this case.
> 
> Ah, I forgot about the script-specific LazyLoad priority.
> 
> Adding an Idle value to the ResourceFetcher::DeferOption enum seemed like it'd
be cleaner than plumbing this through ResourceRequest. Can we do that instead?

That's a good suggestion. Done.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:297: if
(blockedDocWriteScriptAsyncFetch) {
On 2016/08/22 at 20:55:12, Nate Chapin wrote:
> On 2016/08/22 20:44:00, shivanisha wrote:
> > On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> > > Does this logic absolutely need to be after notifyScriptLoaded()? It adds
a
> > bunch of complexity to have to spread it over.
> > 
> > I am not completely aware of the other threads that are going on
simultaneously
> > but notifyScriptLoaded() does a bunch of things including
> > resumeParsingAfterScriptExecution() that calls 
> > HTMLDocumentParser::pumpTokenizer() and
> > HTMLDocumentParser::pumpPendingSpeculations(). If it does something that can
> > lead to posting tasks to other threads then it would be good to not delay
its
> > execution. While debugging fetchBlockedDocWriteScript() using gdb I was
seeing
> > it took a noticeable time before returning from ScriptResource::fetch()
call.
> > 
> > Thoughts?
> 
> Hrm, ok. One option is to move all the new code here into a helper
(maybeFetchBlockedDocWriteScript() or something like that), have it return false
if it didn't fetch a blocked script, and let it have a
m_host->notifyScriptLoaded() call in the appropriate place in its own flow. If
it doesn't fetch anything, then it just falls through to the
notifyScriptLoaded() call here. That we at least contain this specialized logic.
WDYT?

Sounds good. done

[commit-bot: I haz the power, 2016-08-23 05:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 05:19:01]

Dry run: This issue passed the CQ dry run.

[shivanisha, 2016-08-24 17:47:40]

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:91: bool
disallowedFetchForDocWrittenScript() { return m_documentWriteIntervention ==
DocumentWriteIntervention::DisallowedFetchForDocWrittenScript; }
On 2016/08/23 at 00:30:39, haraken wrote:
> disallowedFetchForDocWrittenScript => shouldFetchDocWrittenScript ?

Named it as disallowedFetchForDocWrittenScript so that it is consistent with the
function shouldDisallowFetchForMainFrameScript() which is the decision making
function. This one just returns true or false based on what decision
shouldDisallowFetchForMainFrameScript() took.

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:92: void
setBlockedDocWriteScriptAsyncFetch();
On 2016/08/23 at 00:30:39, haraken wrote:
> "BlockedDocWrite" sounds a bit strange to me. Is there any non-blocking
document.write?

Here blocking is used not in the sense of parser-blocking but whether the
intervention actually blocked the fetch of this script. There are conditions
like a reload, cache-hit where the intervention does not block the script.

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:131:
DisallowedFetchForDocWrittenScript,
On 2016/08/23 at 00:30:39, haraken wrote:
> DoNotFetchDocWrittenScript ?

Named such so that it is consistent with shouldDisallowFetchForMainFrameScript()

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:133:
AsyncLowPriorityFetchForBlockedScript,
On 2016/08/23 at 00:30:39, haraken wrote:
> FetchDocWrittenScriptAsynchrously ?

Wanted to say that it is a fetch for a script whose original fetch was blocked
by the intervention, thus "block" is part of the name.

[shivanisha, 2016-08-24 17:54:07]

Thanks all, for the feedback. I have incorporated/responded to all the feedback.
PTAL, thanks!

[haraken, 2016-08-25 01:20:59]

On 2016/08/24 17:47:40, shivanisha wrote:
>
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):
> 
>
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ScriptLoader.h:91: bool
> disallowedFetchForDocWrittenScript() { return m_documentWriteIntervention ==
> DocumentWriteIntervention::DisallowedFetchForDocWrittenScript; }
> On 2016/08/23 at 00:30:39, haraken wrote:
> > disallowedFetchForDocWrittenScript => shouldFetchDocWrittenScript ?
> 
> Named it as disallowedFetchForDocWrittenScript so that it is consistent with
the
> function shouldDisallowFetchForMainFrameScript() which is the decision making
> function. This one just returns true or false based on what decision
> shouldDisallowFetchForMainFrameScript() took.
> 
>
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ScriptLoader.h:92: void
> setBlockedDocWriteScriptAsyncFetch();
> On 2016/08/23 at 00:30:39, haraken wrote:
> > "BlockedDocWrite" sounds a bit strange to me. Is there any non-blocking
> document.write?
> 
> Here blocking is used not in the sense of parser-blocking but whether the
> intervention actually blocked the fetch of this script. There are conditions
> like a reload, cache-hit where the intervention does not block the script.
> 
>
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ScriptLoader.h:131:
> DisallowedFetchForDocWrittenScript,
> On 2016/08/23 at 00:30:39, haraken wrote:
> > DoNotFetchDocWrittenScript ?
> 
> Named such so that it is consistent with
shouldDisallowFetchForMainFrameScript()
> 
>
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ScriptLoader.h:133:
> AsyncLowPriorityFetchForBlockedScript,
> On 2016/08/23 at 00:30:39, haraken wrote:
> > FetchDocWrittenScriptAsynchrously ?
> 
> Wanted to say that it is a fetch for a script whose original fetch was blocked
> by the intervention, thus "block" is part of the name.

I now understand your point, but the enum name is really confusing. At least
please add a very good comment about what it means.

(Nate would be a good reviewer for the overall change.)

[shivanisha, 2016-08-25 15:23:05]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 15:23:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 15:34:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 15:34:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[shivanisha, 2016-08-25 15:36:17]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 15:36:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-08-25 15:37:16]

On 2016/08/25 at 01:20:59, haraken wrote:
> On 2016/08/24 17:47:40, shivanisha wrote:
> >
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):
> > 
> >
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/ScriptLoader.h:91: bool
> > disallowedFetchForDocWrittenScript() { return m_documentWriteIntervention ==
> > DocumentWriteIntervention::DisallowedFetchForDocWrittenScript; }
> > On 2016/08/23 at 00:30:39, haraken wrote:
> > > disallowedFetchForDocWrittenScript => shouldFetchDocWrittenScript ?
> > 
> > Named it as disallowedFetchForDocWrittenScript so that it is consistent with
the
> > function shouldDisallowFetchForMainFrameScript() which is the decision
making
> > function. This one just returns true or false based on what decision
> > shouldDisallowFetchForMainFrameScript() took.
> > 
> >
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/ScriptLoader.h:92: void
> > setBlockedDocWriteScriptAsyncFetch();
> > On 2016/08/23 at 00:30:39, haraken wrote:
> > > "BlockedDocWrite" sounds a bit strange to me. Is there any non-blocking
> > document.write?
> > 
> > Here blocking is used not in the sense of parser-blocking but whether the
> > intervention actually blocked the fetch of this script. There are conditions
> > like a reload, cache-hit where the intervention does not block the script.
> > 
> >
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/ScriptLoader.h:131:
> > DisallowedFetchForDocWrittenScript,
> > On 2016/08/23 at 00:30:39, haraken wrote:
> > > DoNotFetchDocWrittenScript ?
> > 
> > Named such so that it is consistent with
shouldDisallowFetchForMainFrameScript()
> > 
> >
https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/ScriptLoader.h:133:
> > AsyncLowPriorityFetchForBlockedScript,
> > On 2016/08/23 at 00:30:39, haraken wrote:
> > > FetchDocWrittenScriptAsynchrously ?
> > 
> > Wanted to say that it is a fetch for a script whose original fetch was
blocked
> > by the intervention, thus "block" is part of the name.
> 
> I now understand your point, but the enum name is really confusing. At least
please add a very good comment about what it means.
> 
> (Nate would be a good reviewer for the overall change.)

Thanks! Took another look at the names and I agree they are a little verbose and
thus may be confusing to the reader. Renamed the enum values and added a
comment, based on feedback from haraken@.

[commit-bot: I haz the power, 2016-08-25 17:12:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 17:12:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shivanisha, 2016-08-31 14:07:30]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 14:07:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 15:41:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 15:42:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shivanisha, 2016-09-01 18:58:04]

The layout tests are failing after rebasing. The test function that they used
testRunner.dumpResourceRequestPriorities has been removed and I am looking into
how to re-write the test using WebFrameTest as recommended in the removing CL:
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...

[shivanisha, 2016-09-06 16:56:17]

On 2016/09/01 at 18:58:04, shivanisha wrote:
> The layout tests are failing after rebasing. The test function that they used
testRunner.dumpResourceRequestPriorities has been removed and I am looking into
how to re-write the test using WebFrameTest as recommended in the removing CL:
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...

Nate,
Given that this test is slightly more complex (requires a blink setting and a
cross origin script for the first fetch to be blocked and requires testing the
async low priority fetch when the first fetch fails), it is probably best tested
using a layout test. Also since this CL is feature blocking and high priority,
what are your thoughts on getting the dumpResourceRequestPriorities back to
support this case?

- Shivani

[Nate Chapin, 2016-09-06 20:28:13]

LGTM, just a couple nitpicks.

https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
if (window.testRunner) {
This shouldn't be needed twice? and the dumpAsText() appears unnecessary, given
that the expectations are already text.

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:263: bool
HTMLScriptRunner::possiblyFetchBlockedDocWriteScript(Resource* cachedResource)
Nit: |cachedResource| is anachronistic. Just |resource| is preferable.

[shivanisha, 2016-09-06 20:31:25]

https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
if (window.testRunner) {
On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> This shouldn't be needed twice? and the dumpAsText() appears unnecessary,
given that the expectations are already text.

Passing this test will require putting dumpResourceRequestPriorities back ,
which was removed by you in CL
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
Just wanted to confirm if that sounds good as well.

[Nate Chapin, 2016-09-06 20:41:47]

On 2016/09/06 20:31:25, shivanisha wrote:
>
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> if (window.testRunner) {
> On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > This shouldn't be needed twice? and the dumpAsText() appears unnecessary,
> given that the expectations are already text.
> 
> Passing this test will require putting dumpResourceRequestPriorities back ,
> which was removed by you in CL
>
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> Just wanted to confirm if that sounds good as well.

Whoops, missed that. dumpResourceRequestPriorities() wasn't just removed from
that test, I ripped out the plumbing. You'd need a unit test to verify the
request priority. You can probably add a case to WebFrameTest.ScriptPriority

[haraken, 2016-09-07 01:47:36]

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:137:
FetchDocWrittenScriptDeferIdle,

FetchDocWrittenScriptInIdleTask ?

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:279:
shouldFetchBlockedDocWriteScript = true;

It's confusing to set "shouldFetch"BlockedDocWriteScript true when
"disallowedFetch"ForDocWrittenScript returns true...

Does "DoNotFetchDocWrittenScript" mean that we should not fetch the script
during loading but should fetch the script after the loading has finished? Then
how is it different from "FetchDocWrittenScriptDeferIdle"?

[shivanisha, 2016-09-12 20:17:22]

On 2016/09/06 at 20:41:47, japhet wrote:
> On 2016/09/06 20:31:25, shivanisha wrote:
> >
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> > if (window.testRunner) {
> > On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > > This shouldn't be needed twice? and the dumpAsText() appears unnecessary,
> > given that the expectations are already text.
> > 
> > Passing this test will require putting dumpResourceRequestPriorities back ,
> > which was removed by you in CL
> >
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> > Just wanted to confirm if that sounds good as well.
> 
> Whoops, missed that. dumpResourceRequestPriorities() wasn't just removed from
that test, I ripped out the plumbing. You'd need a unit test to verify the
request priority. You can probably add a case to WebFrameTest.ScriptPriority

I spent a lot of time implementing the test but I am not completely sure if
that's the best approach. This is a special case where the fetch we are trying
to test is a result of a previously blocked fetch. That was possible to test
using the layout tests but not with unit tests. With unit tests, I was hoping to
test just the second fetch request by creating a ScriptLoader and invoking
setFetchDocWrittenScriptDeferIdle() on it followed by prepareScript(). The
closest framework I found to test this is ScriptRunnerTest but there is an issue
coming in that approach:
- prepareScript requires the script document to either have a context document
or a frame. I tried using DummyPageHolder to provide a frame but that's not
working as expected. Its hitting an assertion during DummyPageHolder::create and
I am debugging why that's the case. 
- When this is fixed, I am not completely sure if I can test the fetch request's
priority. I can probably just test that prepareScript returns a success.

Any thoughts on a better way to test this?

[Nate Chapin, 2016-09-12 21:02:22]

On 2016/09/12 20:17:22, shivanisha wrote:
> On 2016/09/06 at 20:41:47, japhet wrote:
> > On 2016/09/06 20:31:25, shivanisha wrote:
> > >
>
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
> > >
>
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> > > if (window.testRunner) {
> > > On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > > > This shouldn't be needed twice? and the dumpAsText() appears
unnecessary,
> > > given that the expectations are already text.
> > > 
> > > Passing this test will require putting dumpResourceRequestPriorities back
,
> > > which was removed by you in CL
> > >
>
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> > > Just wanted to confirm if that sounds good as well.
> > 
> > Whoops, missed that. dumpResourceRequestPriorities() wasn't just removed
from
> that test, I ripped out the plumbing. You'd need a unit test to verify the
> request priority. You can probably add a case to WebFrameTest.ScriptPriority
> 
> I spent a lot of time implementing the test but I am not completely sure if
> that's the best approach. This is a special case where the fetch we are trying
> to test is a result of a previously blocked fetch. That was possible to test
> using the layout tests but not with unit tests. With unit tests, I was hoping
to
> test just the second fetch request by creating a ScriptLoader and invoking
> setFetchDocWrittenScriptDeferIdle() on it followed by prepareScript(). The
> closest framework I found to test this is ScriptRunnerTest but there is an
issue
> coming in that approach:
> - prepareScript requires the script document to either have a context document
> or a frame. I tried using DummyPageHolder to provide a frame but that's not
> working as expected. Its hitting an assertion during DummyPageHolder::create
and
> I am debugging why that's the case. 
> - When this is fixed, I am not completely sure if I can test the fetch
request's
> priority. I can probably just test that prepareScript returns a success.
> 
> Any thoughts on a better way to test this?

I think my terminology was clumsy and misled you :(

We have a way in the unit testing harness to load a webpage from
Source/web/tests/data/ and have it more or less load and run the page normally,
but you can reach into blink's internal state directly or listen to public/web/
callbacks. I was thinking you should be able to more or less copy
doc-write-sync-third-party-script-block.html to Source/web/tests/data/, create a
unit test in WebFrameTest that loads that html file, and use
TestResourcePriorityWebFrameClient or make a variant of it, which can listen to
willSendRequest() callbacks and inspect the resource priority.

Might something like that work?

[shivanisha, 2016-09-13 16:48:19]

On 2016/09/12 at 21:02:22, japhet wrote:
> On 2016/09/12 20:17:22, shivanisha wrote:
> > On 2016/09/06 at 20:41:47, japhet wrote:
> > > On 2016/09/06 20:31:25, shivanisha wrote:
> > > >
> >
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
> > > >
> >
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> > > > if (window.testRunner) {
> > > > On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > > > > This shouldn't be needed twice? and the dumpAsText() appears
unnecessary,
> > > > given that the expectations are already text.
> > > > 
> > > > Passing this test will require putting dumpResourceRequestPriorities
back ,
> > > > which was removed by you in CL
> > > >
> >
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> > > > Just wanted to confirm if that sounds good as well.
> > > 
> > > Whoops, missed that. dumpResourceRequestPriorities() wasn't just removed
from
> > that test, I ripped out the plumbing. You'd need a unit test to verify the
> > request priority. You can probably add a case to WebFrameTest.ScriptPriority
> > 
> > I spent a lot of time implementing the test but I am not completely sure if
> > that's the best approach. This is a special case where the fetch we are
trying
> > to test is a result of a previously blocked fetch. That was possible to test
> > using the layout tests but not with unit tests. With unit tests, I was
hoping to
> > test just the second fetch request by creating a ScriptLoader and invoking
> > setFetchDocWrittenScriptDeferIdle() on it followed by prepareScript(). The
> > closest framework I found to test this is ScriptRunnerTest but there is an
issue
> > coming in that approach:
> > - prepareScript requires the script document to either have a context
document
> > or a frame. I tried using DummyPageHolder to provide a frame but that's not
> > working as expected. Its hitting an assertion during DummyPageHolder::create
and
> > I am debugging why that's the case. 
> > - When this is fixed, I am not completely sure if I can test the fetch
request's
> > priority. I can probably just test that prepareScript returns a success.
> > 
> > Any thoughts on a better way to test this?
> 
> I think my terminology was clumsy and misled you :(
> 
> We have a way in the unit testing harness to load a webpage from
Source/web/tests/data/ and have it more or less load and run the page normally,
but you can reach into blink's internal state directly or listen to public/web/
callbacks. I was thinking you should be able to more or less copy
doc-write-sync-third-party-script-block.html to Source/web/tests/data/, create a
unit test in WebFrameTest that loads that html file, and use
TestResourcePriorityWebFrameClient or make a variant of it, which can listen to
willSendRequest() callbacks and inspect the resource priority.
> 
> Might something like that work?

The script blocking requires the script to be cross origin and a blink-settings
flag to be enabled to achieve blocking. Are these two conditions achievable?
The script actually gets blocked in the browser process but I am assuming the
WebFrameTest framework does not invoke the browser, does it?

[Nate Chapin, 2016-09-13 22:38:47]

On 2016/09/13 16:48:19, shivanisha wrote:
> On 2016/09/12 at 21:02:22, japhet wrote:
> > On 2016/09/12 20:17:22, shivanisha wrote:
> > > On 2016/09/06 at 20:41:47, japhet wrote:
> > > > On 2016/09/06 20:31:25, shivanisha wrote:
> > > > >
> > >
>
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
> > > > >
> > >
>
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> > > > > if (window.testRunner) {
> > > > > On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > > > > > This shouldn't be needed twice? and the dumpAsText() appears
> unnecessary,
> > > > > given that the expectations are already text.
> > > > > 
> > > > > Passing this test will require putting dumpResourceRequestPriorities
> back ,
> > > > > which was removed by you in CL
> > > > >
> > >
>
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> > > > > Just wanted to confirm if that sounds good as well.
> > > > 
> > > > Whoops, missed that. dumpResourceRequestPriorities() wasn't just removed
> from
> > > that test, I ripped out the plumbing. You'd need a unit test to verify the
> > > request priority. You can probably add a case to
WebFrameTest.ScriptPriority
> > > 
> > > I spent a lot of time implementing the test but I am not completely sure
if
> > > that's the best approach. This is a special case where the fetch we are
> trying
> > > to test is a result of a previously blocked fetch. That was possible to
test
> > > using the layout tests but not with unit tests. With unit tests, I was
> hoping to
> > > test just the second fetch request by creating a ScriptLoader and invoking
> > > setFetchDocWrittenScriptDeferIdle() on it followed by prepareScript(). The
> > > closest framework I found to test this is ScriptRunnerTest but there is an
> issue
> > > coming in that approach:
> > > - prepareScript requires the script document to either have a context
> document
> > > or a frame. I tried using DummyPageHolder to provide a frame but that's
not
> > > working as expected. Its hitting an assertion during
DummyPageHolder::create
> and
> > > I am debugging why that's the case. 
> > > - When this is fixed, I am not completely sure if I can test the fetch
> request's
> > > priority. I can probably just test that prepareScript returns a success.
> > > 
> > > Any thoughts on a better way to test this?
> > 
> > I think my terminology was clumsy and misled you :(
> > 
> > We have a way in the unit testing harness to load a webpage from
> Source/web/tests/data/ and have it more or less load and run the page
normally,
> but you can reach into blink's internal state directly or listen to
public/web/
> callbacks. I was thinking you should be able to more or less copy
> doc-write-sync-third-party-script-block.html to Source/web/tests/data/, create
a
> unit test in WebFrameTest that loads that html file, and use
> TestResourcePriorityWebFrameClient or make a variant of it, which can listen
to
> willSendRequest() callbacks and inspect the resource priority.
> > 
> > Might something like that work?
> 
> The script blocking requires the script to be cross origin and a
blink-settings
> flag to be enabled to achieve blocking. Are these two conditions achievable?
> The script actually gets blocked in the browser process but I am assuming the
> WebFrameTest framework does not invoke the browser, does it?

You should be able to emulate a cross origin load by provided a cross-origin url
to URLTestHelpers::reigsterMockedURLLoad, and a blink settings flag can be
modified directly via WebView::settings().

As for where the script gets blocked, that's trickier. Can you point me to where
that blocking happens?

[shivanisha, 2016-09-14 00:13:58]

On 2016/09/13 at 22:38:47, japhet wrote:
> On 2016/09/13 16:48:19, shivanisha wrote:
> > On 2016/09/12 at 21:02:22, japhet wrote:
> > > On 2016/09/12 20:17:22, shivanisha wrote:
> > > > On 2016/09/06 at 20:41:47, japhet wrote:
> > > > > On 2016/09/06 20:31:25, shivanisha wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
> > > > > >
> > > >
> >
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> > > > > > if (window.testRunner) {
> > > > > > On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > > > > > > This shouldn't be needed twice? and the dumpAsText() appears
> > unnecessary,
> > > > > > given that the expectations are already text.
> > > > > > 
> > > > > > Passing this test will require putting dumpResourceRequestPriorities
> > back ,
> > > > > > which was removed by you in CL
> > > > > >
> > > >
> >
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> > > > > > Just wanted to confirm if that sounds good as well.
> > > > > 
> > > > > Whoops, missed that. dumpResourceRequestPriorities() wasn't just
removed
> > from
> > > > that test, I ripped out the plumbing. You'd need a unit test to verify
the
> > > > request priority. You can probably add a case to
WebFrameTest.ScriptPriority
> > > > 
> > > > I spent a lot of time implementing the test but I am not completely sure
if
> > > > that's the best approach. This is a special case where the fetch we are
> > trying
> > > > to test is a result of a previously blocked fetch. That was possible to
test
> > > > using the layout tests but not with unit tests. With unit tests, I was
> > hoping to
> > > > test just the second fetch request by creating a ScriptLoader and
invoking
> > > > setFetchDocWrittenScriptDeferIdle() on it followed by prepareScript().
The
> > > > closest framework I found to test this is ScriptRunnerTest but there is
an
> > issue
> > > > coming in that approach:
> > > > - prepareScript requires the script document to either have a context
> > document
> > > > or a frame. I tried using DummyPageHolder to provide a frame but that's
not
> > > > working as expected. Its hitting an assertion during
DummyPageHolder::create
> > and
> > > > I am debugging why that's the case. 
> > > > - When this is fixed, I am not completely sure if I can test the fetch
> > request's
> > > > priority. I can probably just test that prepareScript returns a success.
> > > > 
> > > > Any thoughts on a better way to test this?
> > > 
> > > I think my terminology was clumsy and misled you :(
> > > 
> > > We have a way in the unit testing harness to load a webpage from
> > Source/web/tests/data/ and have it more or less load and run the page
normally,
> > but you can reach into blink's internal state directly or listen to
public/web/
> > callbacks. I was thinking you should be able to more or less copy
> > doc-write-sync-third-party-script-block.html to Source/web/tests/data/,
create a
> > unit test in WebFrameTest that loads that html file, and use
> > TestResourcePriorityWebFrameClient or make a variant of it, which can listen
to
> > willSendRequest() callbacks and inspect the resource priority.
> > > 
> > > Might something like that work?
> > 
> > The script blocking requires the script to be cross origin and a
blink-settings
> > flag to be enabled to achieve blocking. Are these two conditions achievable?
> > The script actually gets blocked in the browser process but I am assuming
the
> > WebFrameTest framework does not invoke the browser, does it?
> 
> You should be able to emulate a cross origin load by provided a cross-origin
url to URLTestHelpers::reigsterMockedURLLoad, and a blink settings flag can be
modified directly via WebView::settings().
> 
> As for where the script gets blocked, that's trickier. Can you point me to
where that blocking happens?

When shouldDisallowFetchForMainFrameScript() in FrameFetchContext.cpp returns
true, the cache policy is set to WebCachePolicy::ReturnCacheDataDontLoad and
that leads to the resource being blocked if its not in the cache. This is
checked in the browser process http cache code.

[Nate Chapin, 2016-09-14 19:36:25]

On 2016/09/14 00:13:58, shivanisha wrote:
> On 2016/09/13 at 22:38:47, japhet wrote:
> > On 2016/09/13 16:48:19, shivanisha wrote:
> > > On 2016/09/12 at 21:02:22, japhet wrote:
> > > > On 2016/09/12 20:17:22, shivanisha wrote:
> > > > > On 2016/09/06 at 20:41:47, japhet wrote:
> > > > > > On 2016/09/06 20:31:25, shivanisha wrote:
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
> > > > > > >
> > > > >
> > >
>
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> > > > > > > if (window.testRunner) {
> > > > > > > On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > > > > > > > This shouldn't be needed twice? and the dumpAsText() appears
> > > unnecessary,
> > > > > > > given that the expectations are already text.
> > > > > > > 
> > > > > > > Passing this test will require putting
dumpResourceRequestPriorities
> > > back ,
> > > > > > > which was removed by you in CL
> > > > > > >
> > > > >
> > >
>
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> > > > > > > Just wanted to confirm if that sounds good as well.
> > > > > > 
> > > > > > Whoops, missed that. dumpResourceRequestPriorities() wasn't just
> removed
> > > from
> > > > > that test, I ripped out the plumbing. You'd need a unit test to verify
> the
> > > > > request priority. You can probably add a case to
> WebFrameTest.ScriptPriority
> > > > > 
> > > > > I spent a lot of time implementing the test but I am not completely
sure
> if
> > > > > that's the best approach. This is a special case where the fetch we
are
> > > trying
> > > > > to test is a result of a previously blocked fetch. That was possible
to
> test
> > > > > using the layout tests but not with unit tests. With unit tests, I was
> > > hoping to
> > > > > test just the second fetch request by creating a ScriptLoader and
> invoking
> > > > > setFetchDocWrittenScriptDeferIdle() on it followed by prepareScript().
> The
> > > > > closest framework I found to test this is ScriptRunnerTest but there
is
> an
> > > issue
> > > > > coming in that approach:
> > > > > - prepareScript requires the script document to either have a context
> > > document
> > > > > or a frame. I tried using DummyPageHolder to provide a frame but
that's
> not
> > > > > working as expected. Its hitting an assertion during
> DummyPageHolder::create
> > > and
> > > > > I am debugging why that's the case. 
> > > > > - When this is fixed, I am not completely sure if I can test the fetch
> > > request's
> > > > > priority. I can probably just test that prepareScript returns a
success.
> > > > > 
> > > > > Any thoughts on a better way to test this?
> > > > 
> > > > I think my terminology was clumsy and misled you :(
> > > > 
> > > > We have a way in the unit testing harness to load a webpage from
> > > Source/web/tests/data/ and have it more or less load and run the page
> normally,
> > > but you can reach into blink's internal state directly or listen to
> public/web/
> > > callbacks. I was thinking you should be able to more or less copy
> > > doc-write-sync-third-party-script-block.html to Source/web/tests/data/,
> create a
> > > unit test in WebFrameTest that loads that html file, and use
> > > TestResourcePriorityWebFrameClient or make a variant of it, which can
listen
> to
> > > willSendRequest() callbacks and inspect the resource priority.
> > > > 
> > > > Might something like that work?
> > > 
> > > The script blocking requires the script to be cross origin and a
> blink-settings
> > > flag to be enabled to achieve blocking. Are these two conditions
achievable?
> > > The script actually gets blocked in the browser process but I am assuming
> the
> > > WebFrameTest framework does not invoke the browser, does it?
> > 
> > You should be able to emulate a cross origin load by provided a cross-origin
> url to URLTestHelpers::reigsterMockedURLLoad, and a blink settings flag can be
> modified directly via WebView::settings().
> > 
> > As for where the script gets blocked, that's trickier. Can you point me to
> where that blocking happens?
> 
> When shouldDisallowFetchForMainFrameScript() in FrameFetchContext.cpp returns
> true, the cache policy is set to WebCachePolicy::ReturnCacheDataDontLoad and
> that leads to the resource being blocked if its not in the cache. This is
> checked in the browser process http cache code.

Hmm, then we don't really care about the script being blocked from blink's
perspective, right? It's sufficient to make sure that the request has the proper
WebCachePolicy. That should be possible to inspect in a willSendRequest() hook,
similar to the request's priority.

[shivanisha, 2016-09-14 19:45:07]

On 2016/09/14 at 19:36:25, japhet wrote:
> On 2016/09/14 00:13:58, shivanisha wrote:
> > On 2016/09/13 at 22:38:47, japhet wrote:
> > > On 2016/09/13 16:48:19, shivanisha wrote:
> > > > On 2016/09/12 at 21:02:22, japhet wrote:
> > > > > On 2016/09/12 20:17:22, shivanisha wrote:
> > > > > > On 2016/09/06 at 20:41:47, japhet wrote:
> > > > > > > On 2016/09/06 20:31:25, shivanisha wrote:
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
> > > > > > > >
> > > > > >
> > > >
> >
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> > > > > > > > if (window.testRunner) {
> > > > > > > > On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > > > > > > > > This shouldn't be needed twice? and the dumpAsText() appears
> > > > unnecessary,
> > > > > > > > given that the expectations are already text.
> > > > > > > > 
> > > > > > > > Passing this test will require putting
dumpResourceRequestPriorities
> > > > back ,
> > > > > > > > which was removed by you in CL
> > > > > > > >
> > > > > >
> > > >
> >
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> > > > > > > > Just wanted to confirm if that sounds good as well.
> > > > > > > 
> > > > > > > Whoops, missed that. dumpResourceRequestPriorities() wasn't just
> > removed
> > > > from
> > > > > > that test, I ripped out the plumbing. You'd need a unit test to
verify
> > the
> > > > > > request priority. You can probably add a case to
> > WebFrameTest.ScriptPriority
> > > > > > 
> > > > > > I spent a lot of time implementing the test but I am not completely
sure
> > if
> > > > > > that's the best approach. This is a special case where the fetch we
are
> > > > trying
> > > > > > to test is a result of a previously blocked fetch. That was possible
to
> > test
> > > > > > using the layout tests but not with unit tests. With unit tests, I
was
> > > > hoping to
> > > > > > test just the second fetch request by creating a ScriptLoader and
> > invoking
> > > > > > setFetchDocWrittenScriptDeferIdle() on it followed by
prepareScript().
> > The
> > > > > > closest framework I found to test this is ScriptRunnerTest but there
is
> > an
> > > > issue
> > > > > > coming in that approach:
> > > > > > - prepareScript requires the script document to either have a
context
> > > > document
> > > > > > or a frame. I tried using DummyPageHolder to provide a frame but
that's
> > not
> > > > > > working as expected. Its hitting an assertion during
> > DummyPageHolder::create
> > > > and
> > > > > > I am debugging why that's the case. 
> > > > > > - When this is fixed, I am not completely sure if I can test the
fetch
> > > > request's
> > > > > > priority. I can probably just test that prepareScript returns a
success.
> > > > > > 
> > > > > > Any thoughts on a better way to test this?
> > > > > 
> > > > > I think my terminology was clumsy and misled you :(
> > > > > 
> > > > > We have a way in the unit testing harness to load a webpage from
> > > > Source/web/tests/data/ and have it more or less load and run the page
> > normally,
> > > > but you can reach into blink's internal state directly or listen to
> > public/web/
> > > > callbacks. I was thinking you should be able to more or less copy
> > > > doc-write-sync-third-party-script-block.html to Source/web/tests/data/,
> > create a
> > > > unit test in WebFrameTest that loads that html file, and use
> > > > TestResourcePriorityWebFrameClient or make a variant of it, which can
listen
> > to
> > > > willSendRequest() callbacks and inspect the resource priority.
> > > > > 
> > > > > Might something like that work?
> > > > 
> > > > The script blocking requires the script to be cross origin and a
> > blink-settings
> > > > flag to be enabled to achieve blocking. Are these two conditions
achievable?
> > > > The script actually gets blocked in the browser process but I am
assuming
> > the
> > > > WebFrameTest framework does not invoke the browser, does it?
> > > 
> > > You should be able to emulate a cross origin load by provided a
cross-origin
> > url to URLTestHelpers::reigsterMockedURLLoad, and a blink settings flag can
be
> > modified directly via WebView::settings().
> > > 
> > > As for where the script gets blocked, that's trickier. Can you point me to
> > where that blocking happens?
> > 
> > When shouldDisallowFetchForMainFrameScript() in FrameFetchContext.cpp
returns
> > true, the cache policy is set to WebCachePolicy::ReturnCacheDataDontLoad and
> > that leads to the resource being blocked if its not in the cache. This is
> > checked in the browser process http cache code.
> 
> Hmm, then we don't really care about the script being blocked from blink's
perspective, right? It's sufficient to make sure that the request has the proper
WebCachePolicy. That should be possible to inspect in a willSendRequest() hook,
similar to the request's priority.

But that's just the first leg of the scenario for which we already have layout
tests. The second leg of the scenario (this CL) is the one that needs tests
which happens when the first fetch gets blocked and then the second fetch is
done for the same script with a very low priority.

[shivanisha, 2016-09-14 20:43:03]

On 2016/09/14 at 19:45:07, shivanisha wrote:
> On 2016/09/14 at 19:36:25, japhet wrote:
> > On 2016/09/14 00:13:58, shivanisha wrote:
> > > On 2016/09/13 at 22:38:47, japhet wrote:
> > > > On 2016/09/13 16:48:19, shivanisha wrote:
> > > > > On 2016/09/12 at 21:02:22, japhet wrote:
> > > > > > On 2016/09/12 20:17:22, shivanisha wrote:
> > > > > > > On 2016/09/06 at 20:41:47, japhet wrote:
> > > > > > > > On 2016/09/06 20:31:25, shivanisha wrote:
> > > > > > > > >
> > > > > > >
> > > > >
> > >
https://codereview.chromium.org/2260303002/diff/240001/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/2260303002/diff/240001/third_party/WebKit/Lay...
> > > > > > > > >
> > > > > > >
> > > > >
> > >
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
> > > > > > > > > if (window.testRunner) {
> > > > > > > > > On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> > > > > > > > > > This shouldn't be needed twice? and the dumpAsText() appears
> > > > > unnecessary,
> > > > > > > > > given that the expectations are already text.
> > > > > > > > > 
> > > > > > > > > Passing this test will require putting
dumpResourceRequestPriorities
> > > > > back ,
> > > > > > > > > which was removed by you in CL
> > > > > > > > >
> > > > > > >
> > > > >
> > >
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297...
> > > > > > > > > Just wanted to confirm if that sounds good as well.
> > > > > > > > 
> > > > > > > > Whoops, missed that. dumpResourceRequestPriorities() wasn't just
> > > removed
> > > > > from
> > > > > > > that test, I ripped out the plumbing. You'd need a unit test to
verify
> > > the
> > > > > > > request priority. You can probably add a case to
> > > WebFrameTest.ScriptPriority
> > > > > > > 
> > > > > > > I spent a lot of time implementing the test but I am not
completely sure
> > > if
> > > > > > > that's the best approach. This is a special case where the fetch
we are
> > > > > trying
> > > > > > > to test is a result of a previously blocked fetch. That was
possible to
> > > test
> > > > > > > using the layout tests but not with unit tests. With unit tests, I
was
> > > > > hoping to
> > > > > > > test just the second fetch request by creating a ScriptLoader and
> > > invoking
> > > > > > > setFetchDocWrittenScriptDeferIdle() on it followed by
prepareScript().
> > > The
> > > > > > > closest framework I found to test this is ScriptRunnerTest but
there is
> > > an
> > > > > issue
> > > > > > > coming in that approach:
> > > > > > > - prepareScript requires the script document to either have a
context
> > > > > document
> > > > > > > or a frame. I tried using DummyPageHolder to provide a frame but
that's
> > > not
> > > > > > > working as expected. Its hitting an assertion during
> > > DummyPageHolder::create
> > > > > and
> > > > > > > I am debugging why that's the case. 
> > > > > > > - When this is fixed, I am not completely sure if I can test the
fetch
> > > > > request's
> > > > > > > priority. I can probably just test that prepareScript returns a
success.
> > > > > > > 
> > > > > > > Any thoughts on a better way to test this?
> > > > > > 
> > > > > > I think my terminology was clumsy and misled you :(
> > > > > > 
> > > > > > We have a way in the unit testing harness to load a webpage from
> > > > > Source/web/tests/data/ and have it more or less load and run the page
> > > normally,
> > > > > but you can reach into blink's internal state directly or listen to
> > > public/web/
> > > > > callbacks. I was thinking you should be able to more or less copy
> > > > > doc-write-sync-third-party-script-block.html to
Source/web/tests/data/,
> > > create a
> > > > > unit test in WebFrameTest that loads that html file, and use
> > > > > TestResourcePriorityWebFrameClient or make a variant of it, which can
listen
> > > to
> > > > > willSendRequest() callbacks and inspect the resource priority.
> > > > > > 
> > > > > > Might something like that work?
> > > > > 
> > > > > The script blocking requires the script to be cross origin and a
> > > blink-settings
> > > > > flag to be enabled to achieve blocking. Are these two conditions
achievable?
> > > > > The script actually gets blocked in the browser process but I am
assuming
> > > the
> > > > > WebFrameTest framework does not invoke the browser, does it?
> > > > 
> > > > You should be able to emulate a cross origin load by provided a
cross-origin
> > > url to URLTestHelpers::reigsterMockedURLLoad, and a blink settings flag
can be
> > > modified directly via WebView::settings().
> > > > 
> > > > As for where the script gets blocked, that's trickier. Can you point me
to
> > > where that blocking happens?
> > > 
> > > When shouldDisallowFetchForMainFrameScript() in FrameFetchContext.cpp
returns
> > > true, the cache policy is set to WebCachePolicy::ReturnCacheDataDontLoad
and
> > > that leads to the resource being blocked if its not in the cache. This is
> > > checked in the browser process http cache code.
> > 
> > Hmm, then we don't really care about the script being blocked from blink's
perspective, right? It's sufficient to make sure that the request has the proper
WebCachePolicy. That should be possible to inspect in a willSendRequest() hook,
similar to the request's priority.
> 
> But that's just the first leg of the scenario for which we already have layout
tests. The second leg of the scenario (this CL) is the one that needs tests
which happens when the first fetch gets blocked and then the second fetch is
done for the same script with a very low priority.

I and Nate discussed this over VC, and I am looking into two of the possible
testing strategies:
- Adding a test function in internals API to test this in layout tests.
(Preferred since rest of the tests for this feature already exist in layout
tests)
- Adding a unit test that mocks the failure of the first fetch using
registerMockedErrorURLLoad.

[shivanisha, 2016-09-15 20:28:57]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 20:29:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shivanisha, 2016-09-15 20:35:08]

The new patch adds a new internal.GetResourcePriority for testing and addresses
feedback.

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.h:137:
FetchDocWrittenScriptDeferIdle,
On 2016/09/07 at 01:47:36, haraken wrote:
> FetchDocWrittenScriptInIdleTask ?

used DeferIdle in the name to convey the usage of DeferOption::IdleLoad.

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:263: bool
HTMLScriptRunner::possiblyFetchBlockedDocWriteScript(Resource* cachedResource)
On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> Nit: |cachedResource| is anachronistic. Just |resource| is preferable.

done.

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:279:
shouldFetchBlockedDocWriteScript = true;
disallowedFetch"ForDocWrittenScript was set when this resource was requested and
this code is executed on receiving the response/error for the resource.
shouldFetchBlockedDocWriteScript implies if another fetch should be done for
this resource.

DoNotFetchDocWrittenScript simply means that this request should not be fetched
from the network.(set for 1st fetch request)
FetchDocWrittenScriptDeferIdle means that a script that was blocked should be
fetched again using idle priority. (set for 2nd fetch request)

[commit-bot: I haz the power, 2016-09-15 21:10:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 21:10:24]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[shivanisha, 2016-09-16 14:08:38]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 14:09:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 15:16:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 15:16:58]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-09-16 21:37:53]

LGTM with a couple of nits.

https://codereview.chromium.org/2260303002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2260303002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.cpp:419: Resource* resource =
document->fetcher()->allResources().get(URLTestHelpers::toKURL(url.utf8().data()));
KURL(ParsedURLString, url) is the canonical way to do a String->KURL conversion
if the url is known to be absolute.

https://codereview.chromium.org/2260303002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2260303002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:39: unsigned long
getResourcePriority (DOMString url, Document document);
Nit: suprious whitespace after getResourcePriority

[shivanisha, 2016-09-19 15:07:02]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and 
fetchScript sets the intervention info and header in the resource request. 
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

Also added testRunner.dumpResourceRequestPriorities() in 
the layout test to test that the new request goes out with priority very low.
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and 
fetchScript sets the intervention info and header in the resource request. 
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

Also added internals.getResourcePriority() for 
the layout test to test that the new request goes out with priority very low.
==========

[shivanisha, 2016-09-19 15:07:17]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and 
fetchScript sets the intervention info and header in the resource request. 
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

Also added internals.getResourcePriority() for 
the layout test to test that the new request goes out with priority very low.
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and 
fetchScript sets the intervention info and header in the resource request. 
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

Also added internals.getResourcePriority() for 
the layout test to test that the new request goes out with priority very low.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/
==========

[shivanisha, 2016-09-19 15:07:25]

The CQ bit was checked by shivanisha@chromium.org

[commit-bot: I haz the power, 2016-09-19 15:07:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 16:19:07]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-09-19 16:21:22]

Description was changed from

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and 
fetchScript sets the intervention info and header in the resource request. 
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

Also added internals.getResourcePriority() for 
the layout test to test that the new request goes out with priority very low.

The code was tested using netlog to verify the second request is sent and has 
the correct priority (net::IDLE). 
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/
==========

to

==========
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets a member flag.
On receiving notifyFinished for the original request, HTMLScriptRunner
creates a new ScriptLoader instance using the original Element with
another flag set to true. It then calls ScriptLoader::prepareScript which
further calls fetchScript. prepareScript sets the defer flag as LazyLoad and
fetchScript sets the intervention info and header in the resource request.
The intervention info is used to set the priority in
ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

Also added internals.getResourcePriority() for
the layout test to test that the new request goes out with priority very low.

The code was tested using netlog to verify the second request is sent and has
the correct priority (net::IDLE).
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

Committed: https://crrev.com/57536eaf61f21cb9f6e351663ef75c3d6e09d705
Cr-Commit-Position: refs/heads/master@{#419476}
==========

[commit-bot: I haz the power, 2016-09-19 16:21:24]

Patchset 16 (id:??) landed as
https://crrev.com/57536eaf61f21cb9f6e351663ef75c3d6e09d705
Cr-Commit-Position: refs/heads/master@{#419476}