Doc.write intervention warning and error messages fixed for clarity. (issue 2640163002 by shivanisha@chromium.org)

1 view
Skip to first unread message

shiva...@chromium.org

unread,
Jan 18, 2017, 4:07:32 PM1/18/17
to bmcq...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
Reviewers: Bryan McQuade, Nate Chapin
CL: https://codereview.chromium.org/2640163002/

Message:
PTAL, Thanks!

Trying to land it today for branch cut.

Description:
Making the warning message more clear and adding an error message when the
script actually gets blocked and a warning message if the script is not blocked
due to cache hit.

Affected files (+83, -37 lines):
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block-all-conn-types-expected.txt
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block-effectively-2g-expected.txt
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block-expected.txt
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type-expected.txt
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload-expected.txt
M third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
M third_party/WebKit/Source/core/loader/FrameFetchContext.cpp


bmcq...@chromium.org

unread,
Jan 18, 2017, 4:13:29 PM1/18/17
to shiva...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws

jap...@chromium.org

unread,
Jan 18, 2017, 5:15:03 PM1/18/17
to shiva...@chromium.org, bmcq...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws

bmcq...@chromium.org

unread,
Jan 18, 2017, 6:47:05 PM1/18/17
to shiva...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws

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

https://codereview.chromium.org/2640163002/diff/1/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp#newcode96
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:96: "for
more details. If the script is indeed blocked, you should see an "
thinking about this more i'm a little concerned that devs might
interpret this last sentence as "if i don't see the subsequent error, my
page isn't affected by this issue" and they won't fix their page, so I'm
inclined to omit this last "If the script is indeed blocked..." text. We
can make it clear in the linked documentation that this message is a
warning and only if you see the additional 'script was blocked' message
was there an actual issue.

Given this, can we remove this last sentence?

https://codereview.chromium.org/2640163002/

shiva...@chromium.org

unread,
Jan 18, 2017, 6:59:40 PM1/18/17
to bmcq...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
Retrying the failed bots. If not successful, will try for a merge after branch
cut.

https://codereview.chromium.org/2640163002/

shiva...@chromium.org

unread,
Jan 19, 2017, 1:04:59 PM1/19/17
to bmcq...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws

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

https://codereview.chromium.org/2640163002/diff/1/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp#newcode96
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:96: "for
more details. If the script is indeed blocked, you should see an "
On 2017/01/18 at 23:47:04, Bryan McQuade wrote:
> thinking about this more i'm a little concerned that devs might
interpret this last sentence as "if i don't see the subsequent error, my
page isn't affected by this issue" and they won't fix their page, so I'm
inclined to omit this last "If the script is indeed blocked..." text. We
can make it clear in the linked documentation that this message is a
warning and only if you see the additional 'script was blocked' message
was there an actual issue.
>
> Given this, can we remove this last sentence?

I hear your concern. Omitting the last sentence might still leave users
unsure whether it actually was blocked or not, in case they do not read
the link very carefully. How about something like this:

"A Parser-blocking, cross site (i.e. different eTLD+1) script
http://ajax.cloudflare.com/cdn-cgi/nexp/dok3v=f2befc48d1/cloudflare.min.js,
is invoked via document.write. This MAY be blocked by the browser in
this or a future page load due to poor network connectivity. If blocked
in this page load, it will be confirmed in a subsequent ERROR message.
See https://www.chromestatus.com/feature/5718547946799104 for more
details."

https://codereview.chromium.org/2640163002/

Bryan McQuade

unread,
Jan 19, 2017, 1:08:04 PM1/19/17
to shiva...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
That sounds good, thanks! Only suggestion would be to drop 'ERROR' as I am not sure users will know what that means - just 'subsequent message' seems sufficient here. Or maybe 'subsequent console message'.

Thanks!

shiva...@chromium.org

unread,
Jan 24, 2017, 11:16:09 AM1/24/17
to bmcq...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
On 2017/01/19 at 18:08:04, bmcquade wrote:
> That sounds good, thanks! Only suggestion would be to drop 'ERROR' as I am
> not sure users will know what that means - just 'subsequent message' seems
> sufficient here. Or maybe 'subsequent console message'.
>
> Thanks!

Done. Thanks!
> --
> You received this message because you are subscribed to the Google Groups
"Blink Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to blink-review...@chromium.org.
>



https://codereview.chromium.org/2640163002/

bmcq...@chromium.org

unread,
Jan 24, 2017, 11:39:14 AM1/24/17
to shiva...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
Thanks! 2 small changes, otherwise LGTM.

https://codereview.chromium.org/2640163002/

bmcq...@chromium.org

unread,
Jan 24, 2017, 11:45:26 AM1/24/17
to shiva...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
Sorry, my earlier comments did not go through. Hope they come through this time.


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

https://codereview.chromium.org/2640163002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp#newcode272
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:272:
"connectivity. See "
do we already show the chromestatus link in the previous message? if so,
i think we can drop it here.

https://codereview.chromium.org/2640163002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp#newcode287
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:287:
"See https://www.chromestatus.com/feature/5718547946799104 "
same

https://codereview.chromium.org/2640163002/diff/140001/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
(right):

https://codereview.chromium.org/2640163002/diff/140001/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp#newcode96
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:96:
"subsequent message."
nit: maybe 'subsequent console message'? reading this now, message isn't
totally clear - as a dev i might wonder what kind of message we are
referring to.

https://codereview.chromium.org/2640163002/

shiva...@chromium.org

unread,
Jan 24, 2017, 12:22:38 PM1/24/17
to bmcq...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws

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

https://codereview.chromium.org/2640163002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp#newcode272
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:272:
"connectivity. See "
On 2017/01/24 at 16:45:26, Bryan McQuade wrote:
> do we already show the chromestatus link in the previous message? if
so, i think we can drop it here.

Done.
On 2017/01/24 at 16:45:26, Bryan McQuade wrote:
> same

Done.
On 2017/01/24 at 16:45:26, Bryan McQuade wrote:
> nit: maybe 'subsequent console message'? reading this now, message
isn't totally clear - as a dev i might wonder what kind of message we
are referring to.

shiva...@chromium.org

unread,
Jan 24, 2017, 1:00:29 PM1/24/17
to bmcq...@chromium.org, jap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
Some of the bot failures being tracked via
https://bugs.chromium.org/p/chromium/issues/detail?id=684573
Once they are fixed, hopefully commit will proceed.

https://codereview.chromium.org/2640163002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 25, 2017, 3:27:57 PM1/25/17
to shiva...@chromium.org, bmcq...@chromium.org, jap...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
This CL has an open dependency (Issue 2474943002 Patch 120001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

https://codereview.chromium.org/2640163002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 30, 2017, 11:24:45 AM1/30/17
to shiva...@chromium.org, bmcq...@chromium.org, jap...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 30, 2017, 1:37:52 PM1/30/17
to shiva...@chromium.org, bmcq...@chromium.org, jap...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, gavinp...@chromium.org, jap...@chromium.org, kinuko...@chromium.org, loading...@chromium.org, loading-rev...@chromium.org, tyoshin...@chromium.org, yo...@yoav.ws
Reply all
Reply to author
Forward
0 new messages