Add deprecation warning for script fetches with invalid types

Add deprecation warning for script fetches with invalid types

BUG=626321
Committed: https://crrev.com/ffe177716cabfc5f8fedd62d14904390fb178a8d
Cr-Commit-Position: refs/heads/master@{#418711}

[Charlie Harrison, 2016-09-02 16:40:06]

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

[commit-bot: I haz the power, 2016-09-02 16:40:29]

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

[Charlie Harrison, 2016-09-02 17:52:18]

csharrison@chromium.org changed reviewers:
+ yoav@yoav.ws

[Charlie Harrison, 2016-09-02 17:52:19]

Hey Yoav, can you take a first pass at this? Low priority.

[commit-bot: I haz the power, 2016-09-02 18:16:42]

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

[commit-bot: I haz the power, 2016-09-02 18:16:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-09-02 19:01:15]

Looks like I will have to add the warning to a failing test.

[Yoav Weiss, 2016-09-05 15:06:25]

Looks good. Refresh my memory: Counting at preloadScanner is enough because the
regular parser just ignores these scripts, right?

https://codereview.chromium.org/2301323003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2301323003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:134: DCHECK_EQ(language,
language.lower());
Why the change here? I don't expect this function to be super-hot but the older
version seems like it would be faster. Maybe you can just add the DCHECK and
change the hash type to a StringHash?

[Charlie Harrison, 2016-09-05 17:15:06]

I mainly added the counting at the preload scanner level to be sure that we're
counting the right thing. I don't see a huge reason not to just instrument the
regular parsing if that'll be cleaner.

https://codereview.chromium.org/2301323003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2301323003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:134: DCHECK_EQ(language,
language.lower());
On 2016/09/05 15:06:25, Yoav Weiss wrote:
> Why the change here? I don't expect this function to be super-hot but the
older
> version seems like it would be faster. Maybe you can just add the DCHECK and
> change the hash type to a StringHash?

I changed this so that the function could be called from multiple threads.
HashSets are not thread safe. Not quite sure the best approach here, though.
Suggestions welcome :)

[Yoav Weiss, 2016-09-05 18:57:48]

LGTM

On 2016/09/05 17:15:06, Charlie Harrison wrote:
> I mainly added the counting at the preload scanner level to be sure that we're
> counting the right thing. I don't see a huge reason not to just instrument the
> regular parsing if that'll be cleaner.

Counting at the preloadScanner level is indeed more accurate. I'm fine with it
either way.

> 
>
https://codereview.chromium.org/2301323003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):
> 
>
https://codereview.chromium.org/2301323003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/ScriptLoader.cpp:134: DCHECK_EQ(language,
> language.lower());
> On 2016/09/05 15:06:25, Yoav Weiss wrote:
> > Why the change here? I don't expect this function to be super-hot but the
> older
> > version seems like it would be faster. Maybe you can just add the DCHECK and
> > change the hash type to a StringHash?
> 
> I changed this so that the function could be called from multiple threads.
> HashSets are not thread safe. Not quite sure the best approach here, though.
> Suggestions welcome :)

Multiple threads is reason enough. Let's keep it as is and optimize in case we'd
see benchmark regressions.

[Charlie Harrison, 2016-09-09 20:05:57]

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

[commit-bot: I haz the power, 2016-09-09 20:06:30]

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

[Charlie Harrison, 2016-09-09 20:06:36]

csharrison@chromium.org changed reviewers:
+ rbyers@chromium.org

[Charlie Harrison, 2016-09-09 20:06:37]

+rbyers, could you take a look at the deprecation warning? I wasn't sure if e.g.
I should mention the preload scanner.

[commit-bot: I haz the power, 2016-09-09 23:05:16]

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

[commit-bot: I haz the power, 2016-09-09 23:05:17]

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

[Charlie Harrison, 2016-09-10 05:49:42]

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

[commit-bot: I haz the power, 2016-09-10 05:49:46]

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-10 07:28:48]

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

[commit-bot: I haz the power, 2016-09-10 07:28:48]

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

[Rick Byers, 2016-09-14 15:26:37]

On 2016/09/09 20:06:37, Charlie Harrison wrote:
> +rbyers, could you take a look at the deprecation warning? I wasn't sure if
e.g.
> I should mention the preload scanner.

There's really not room to go into any deal in the message, but normally we
include the chromestatus.com link so that we can easily add details (links to
posts, demos, etc.) there.  Maybe replace your spec link with a chromestatus
link and add some text / spec link there?

[Charlie Harrison, 2016-09-14 17:27:22]

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

[commit-bot: I haz the power, 2016-09-14 17:27:45]

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

[Charlie Harrison, 2016-09-14 17:28:26]

I updated the deprecation message and added a chromestatus entry.

Please let me know if there's anything you think I should add to the entry.

[Rick Byers, 2016-09-14 18:15:00]

On 2016/09/14 17:28:26, Charlie Harrison wrote:
> I updated the deprecation message and added a chromestatus entry.

Looks great, thanks.

> Please let me know if there's anything you think I should add to the entry.

You said Safari already does this, right?  Then you can mark the Safari status
as "shipped" (give the entry a little green <grin>).  Otherwise it looks great,
the main thing is that it gives us an easy place to update if we start hearing
concerns/confusion from web devs.

[Charlie Harrison, 2016-09-14 18:22:36]

On 2016/09/14 18:15:00, Rick Byers wrote:
> On 2016/09/14 17:28:26, Charlie Harrison wrote:
> > I updated the deprecation message and added a chromestatus entry.
> 
> Looks great, thanks.
> 
> > Please let me know if there's anything you think I should add to the entry.
> 
> You said Safari already does this, right?  Then you can mark the Safari status
> as "shipped" (give the entry a little green <grin>).  Otherwise it looks
great,
> the main thing is that it gives us an easy place to update if we start hearing
> concerns/confusion from web devs.

No, that was a mistake on my part early in the i2d. I was just failing to
trigger Safari's preload scanner (which is only enabled when we're parser
blocked on scripts).

[Rick Byers, 2016-09-14 18:36:53]

On 2016/09/14 18:22:36, Charlie Harrison wrote:
> On 2016/09/14 18:15:00, Rick Byers wrote:
> > On 2016/09/14 17:28:26, Charlie Harrison wrote:
> > > I updated the deprecation message and added a chromestatus entry.
> > 
> > Looks great, thanks.
> > 
> > > Please let me know if there's anything you think I should add to the
entry.
> > 
> > You said Safari already does this, right?  Then you can mark the Safari
status
> > as "shipped" (give the entry a little green <grin>).  Otherwise it looks
> great,
> > the main thing is that it gives us an easy place to update if we start
hearing
> > concerns/confusion from web devs.
> 
> No, that was a mistake on my part early in the i2d. I was just failing to
> trigger Safari's preload scanner (which is only enabled when we're parser
> blocked on scripts).

Ah, ok - no worries.  Just keep your ears open for any reports of breakage and
help developers adapt - I suspect it'll be fine.

[Charlie Harrison, 2016-09-14 18:40:53]

csharrison@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Charlie Harrison, 2016-09-14 18:40:54]

Thanks for the guidance. I will certainly try to be vigilant :)

+rkaplow for histograms.xml UseCounter change.

[rkaplow, 2016-09-14 20:12:36]

lgtm

[Charlie Harrison, 2016-09-14 20:14:10]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2016-09-14 20:14:10]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-09-14 20:14:11]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/2301323003/#ps60001
(title: "Update deprecation message with chromestatus entry")

[commit-bot: I haz the power, 2016-09-14 20:15:10]

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-14 23:05:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-14 23:08:39]

Description was changed from

==========
Add deprecation warning for script fetches with invalid types

BUG=626321
==========

to

==========
Add deprecation warning for script fetches with invalid types

BUG=626321

Committed: https://crrev.com/ffe177716cabfc5f8fedd62d14904390fb178a8d
Cr-Commit-Position: refs/heads/master@{#418711}
==========

[commit-bot: I haz the power, 2016-09-14 23:08:40]

Patchset 4 (id:??) landed as
https://crrev.com/ffe177716cabfc5f8fedd62d14904390fb178a8d
Cr-Commit-Position: refs/heads/master@{#418711}