Dev tools warning and console warning for document.write script blocking

Dev tools warning and console warning for document.write script blocking

BUG=599875
Committed: https://crrev.com/021389e7cbc9ed30a47eb69894f9bb8d48d930ee
Cr-Commit-Position: refs/heads/master@{#401606}

[shivanisha, 2016-06-08 18:04:00]

Description was changed from

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875
==========

to

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

- Needed to add a new overloaded addConsoleMessage in FrameFetchContext since
the existing one is an overridden one and the parent class does not have access
to MessageLevel enum sue to include dependency rules for core/fetch.

- Made shouldDisallowFetchForMainFrameScript a class function so it can make use
of the addConsoleMessage without the need to pass around the document and frame
arguments.
==========

[shivanisha, 2016-06-08 18:04:51]

Description was changed from

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

- Needed to add a new overloaded addConsoleMessage in FrameFetchContext since
the existing one is an overridden one and the parent class does not have access
to MessageLevel enum sue to include dependency rules for core/fetch.

- Made shouldDisallowFetchForMainFrameScript a class function so it can make use
of the addConsoleMessage without the need to pass around the document and frame
arguments.
==========

to

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

- Needed to add a new overloaded addConsoleMessage in FrameFetchContext since
the
existing one is an overridden one and the parent class does not have access to
MessageLevel enum due to include dependency rules for core/fetch.

- Made shouldDisallowFetchForMainFrameScript a class function so it can make use
of 
the addConsoleMessage function without the need to pass around the document and
frame pointers as arguments.
==========

[shivanisha, 2016-06-08 18:06:25]

Description was changed from

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

- Needed to add a new overloaded addConsoleMessage in FrameFetchContext since
the
existing one is an overridden one and the parent class does not have access to
MessageLevel enum due to include dependency rules for core/fetch.

- Made shouldDisallowFetchForMainFrameScript a class function so it can make use
of 
the addConsoleMessage function without the need to pass around the document and
frame pointers as arguments.
==========

to

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

- Needed to add a new overloaded addConsoleMessage in FrameFetchContext since
the
existing one is an overridden one and the parent class does not have access to
MessageLevel enum due to include dependency rules for core/fetch.

- Made shouldDisallowFetchForMainFrameScript a class function so it can make use
of 
the addConsoleMessage function without the need to pass around the document and
frame pointers as arguments.  

- Used WTFLogAlways for the browser console message but it outputs it as an INFO
message and not as a warning. Do we have something that does that?
==========

[shivanisha, 2016-06-08 18:07:10]

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

[shivanisha, 2016-06-08 18:07:32]

Description was changed from

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

- Needed to add a new overloaded addConsoleMessage in FrameFetchContext since
the
existing one is an overridden one and the parent class does not have access to
MessageLevel enum due to include dependency rules for core/fetch.

- Made shouldDisallowFetchForMainFrameScript a class function so it can make use
of 
the addConsoleMessage function without the need to pass around the document and
frame pointers as arguments.  

- Used WTFLogAlways for the browser console message but it outputs it as an INFO
message and not as a warning. Do we have something that does that?
==========

to

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

- Needed to add a new overloaded addConsoleMessage in FrameFetchContext since
the
existing one is an overridden one and the parent class does not have access to
MessageLevel enum due to include dependency rules for core/fetch.

- Made shouldDisallowFetchForMainFrameScript a class function so it can make use
of 
the addConsoleMessage function without the need to pass around the document and
frame pointers as arguments.  

- Used WTFLogAlways for the browser console message but it outputs it as an INFO
message and not as a warning. Do we have something that does that?
==========

[shivanisha, 2016-06-09 19:45:42]

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

[Nate Chapin, 2016-06-09 23:44:44]

https://codereview.chromium.org/2045313003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2045313003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:79: void
FrameFetchContext::emitWarningForDocWriteScripts(const String& url) const
Maybe take the a const Document& here and keep this and
shouldDisallowFetchForMainFrameScript() file-scope?

https://codereview.chromium.org/2045313003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:82:
addConsoleMessage(message, WarningMessageLevel);
Call frame()->document()->addConsoleMessage() directly here? The overloaded
addConsoleMessage() helper doesn't seem worthwhile.

[shivanisha, 2016-06-10 14:41:01]

https://codereview.chromium.org/2045313003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2045313003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:82:
addConsoleMessage(message, WarningMessageLevel);
On 2016/06/09 at 23:44:44, Nate Chapin wrote:
> Call frame()->document()->addConsoleMessage() directly here? The overloaded
addConsoleMessage() helper doesn't seem worthwhile.

Sounds good. Made these changes.

[shivanisha, 2016-06-10 17:17:59]

Update layout tests to incorporate the new console warnings.

Its strange that the in the layout tests the console log prints for each
resource twice while dev tools does it only once. Any ideas why this could be
happening?

[jkarlin, 2016-06-10 18:38:36]

https://codereview.chromium.org/2045313003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2045313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:83: String message =
"A Parser-blocking, cross-origin script " + url + " is invoked via
document.write. This may get blocked by the browser in slow connections. It is
recommended to invoke this script asynchronously.";
Commas needed around the url.

s/may get blocked/may be blocked/

s/in slow connections/if the device has poor network connectivity/

"recommended to invoke" doesn't make sense.

https://codereview.chromium.org/2045313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:83: String message =
"A Parser-blocking, cross-origin script " + url + " is invoked via
document.write. This may get blocked by the browser in slow connections. It is
recommended to invoke this script asynchronously.";
There should also be a link to a help page explaining (a little bit) how to make
code asynchronous. That can likely wait until after field trials however. Please
create a new bug to add the link so that we don't forget.

[Nate Chapin, 2016-06-10 23:34:32]

One more code nit from me...

https://codereview.chromium.org/2045313003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2045313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:84: const_cast
<Document &>(document).addConsoleMessage(ConsoleMessage::create(JSMessageSource,
WarningMessageLevel, message));
Nit: Pass the Document non-const instead of const_cast'ing.

[shivanisha, 2016-06-20 14:28:06]

https://codereview.chromium.org/2045313003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2045313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:83: String message =
"A Parser-blocking, cross-origin script " + url + " is invoked via
document.write. This may get blocked by the browser in slow connections. It is
recommended to invoke this script asynchronously.";
On 2016/06/10 at 18:38:36, jkarlin wrote:
> There should also be a link to a help page explaining (a little bit) how to
make code asynchronous. That can likely wait until after field trials however.
Please create a new bug to add the link so that we don't forget.

Created https://bugs.chromium.org/p/chromium/issues/detail?id=621520

Updated the warning string. Removed the "recommended to invoke" part as that
would be replaced by the help link.

https://codereview.chromium.org/2045313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:84: const_cast
<Document &>(document).addConsoleMessage(ConsoleMessage::create(JSMessageSource,
WarningMessageLevel, message));
On 2016/06/10 at 23:34:32, Nate Chapin wrote:
> Nit: Pass the Document non-const instead of const_cast'ing.

done.

[Nate Chapin, 2016-06-20 21:52:45]

Code LGTM, though looks like your tests are failing on the bots?

[shivanisha, 2016-06-21 16:45:12]

Description was changed from

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

- Needed to add a new overloaded addConsoleMessage in FrameFetchContext since
the
existing one is an overridden one and the parent class does not have access to
MessageLevel enum due to include dependency rules for core/fetch.

- Made shouldDisallowFetchForMainFrameScript a class function so it can make use
of 
the addConsoleMessage function without the need to pass around the document and
frame pointers as arguments.  

- Used WTFLogAlways for the browser console message but it outputs it as an INFO
message and not as a warning. Do we have something that does that?
==========

to

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875
==========

[jkarlin, 2016-06-23 11:20:41]

lgtm with nit

https://codereview.chromium.org/2045313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2045313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:125:
emitWarningForDocWriteScripts(request.url().getString(), document);
no need to have two emitWarning calls. Can just have one above the if(isReload).

[shivanisha, 2016-06-23 13:56:04]

Thanks!

https://codereview.chromium.org/2045313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2045313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:125:
emitWarningForDocWriteScripts(request.url().getString(), document);
On 2016/06/23 at 11:20:41, jkarlin wrote:
> no need to have two emitWarning calls. Can just have one above the
if(isReload).

done.

[shivanisha, 2016-06-23 13:56:18]

The CQ bit was checked by shivanisha@chromium.org

[shivanisha, 2016-06-23 13:56:19]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2045313003/#ps100001
(title: "Only one invocation of emitWarning function")

[commit-bot: I haz the power, 2016-06-23 13:56:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2045313003/100001

[commit-bot: I haz the power, 2016-06-23 15:04:07]

Committed patchset #6 (id:100001)

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

Description was changed from

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875
==========

to

==========
Dev tools warning and console warning for document.write script blocking

BUG=599875

Committed: https://crrev.com/021389e7cbc9ed30a47eb69894f9bb8d48d930ee
Cr-Commit-Position: refs/heads/master@{#401606}
==========

[commit-bot: I haz the power, 2016-06-23 15:06:23]

Patchset 6 (id:??) landed as
https://crrev.com/021389e7cbc9ed30a47eb69894f9bb8d48d930ee
Cr-Commit-Position: refs/heads/master@{#401606}