Set currentScript for SVGScriptElements.

Set currentScript for SVGScriptElements.

Spec: https://html.spec.whatwg.org/#dom-document-currentscript
Mozilla already supports this.

BUG=495194
Committed: https://crrev.com/82b3400655ebb30a0099a0f1a91d42d00f26fa08
Cr-Commit-Position: refs/heads/master@{#391486}

[ramya.v, 2016-05-03 10:23:16]

ramya.v@samsung.com changed reviewers:
+ fs@opera.com, tkent@chromium.org

[ramya.v, 2016-05-03 10:23:17]

PTAL!

Thanks

[fs, 2016-05-03 10:56:01]

I'd suggest making the title/description be about what is being implemented
rather than what didn't work (I.e more along the lines of "Set currentScript for
SVGScriptElements".) It might also make sense to mention that Gecko (Firefox)
supports this already.

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4595: }
NOTREACHED()?

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:179: typedef
HTMLScriptElementOrSVGScriptElement ScriptElement;
With the below adressed I don't think this serves much of a purpose I think.

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:797: void pushCurrentScript(const
ScriptElement&);
I think I would prefer that the "internal" part of the interface just use
Element - and (d)check for the expected types when pertinent. (Now the element
is wrapped to just be unwrapped again before being pushed onto the stack.)

[ramya.v, 2016-05-04 04:01:35]

Description was changed from

==========
currentScript doesn't work for svg namespace script elements.

Spec: https://html.spec.whatwg.org/#dom-document-currentscript

BUG=495194
==========

to

==========
Set currentScript for SVGScriptElements.

Spec: https://html.spec.whatwg.org/#dom-document-currentscript
Mozilla already supports this.

BUG=495194
==========

[ramya.v, 2016-05-04 04:18:41]

PTAL!

Thanks

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4595: }
On 2016/05/03 10:56:00, fs wrote:
> NOTREACHED()?

Below test cases are failing if we put ASSERT_NOT_REACHED at this point.
https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/...

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:179: typedef
HTMLScriptElementOrSVGScriptElement ScriptElement;
On 2016/05/03 10:56:01, fs wrote:
> With the below adressed I don't think this serves much of a purpose I think.

Done.

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:797: void pushCurrentScript(const
ScriptElement&);
On 2016/05/03 10:56:01, fs wrote:
> I think I would prefer that the "internal" part of the interface just use
> Element - and (d)check for the expected types when pertinent. (Now the element
> is wrapped to just be unwrapped again before being pushed onto the stack.)

Done.

[fs, 2016-05-04 08:42:54]

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4595: }
On 2016/05/04 at 04:18:41, ramya.v wrote:
> On 2016/05/03 10:56:00, fs wrote:
> > NOTREACHED()?
> 
> Below test cases are failing if we put ASSERT_NOT_REACHED at this point.
>
https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/...

Well, the placement in PS3 is definitely wrong - currentScript() being null is a
valid case, but non-null that are not SVGScriptElement or HTMLScriptElement
would not be. So the NOTREACHED() would need to go in a final 'else' branch of
the inner if. I'm fine with just a DCHECK in pushCurrentScript though - it
should serve the same purpose/verify the same contract.

https://codereview.chromium.org/1945563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1945563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4601:
m_currentScriptStack.append(newCurrentScript);
Add a DCHECK here that |newCurrentScript| is one of the expected types.

[ramya.v, 2016-05-04 09:35:51]

PTAL!

Thanks

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1945563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4595: }
On 2016/05/04 08:42:54, fs wrote:
> On 2016/05/04 at 04:18:41, ramya.v wrote:
> > On 2016/05/03 10:56:00, fs wrote:
> > > NOTREACHED()?
> > 
> > Below test cases are failing if we put ASSERT_NOT_REACHED at this point.
> >
>
https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/...
> 
> Well, the placement in PS3 is definitely wrong - currentScript() being null is
a
> valid case, but non-null that are not SVGScriptElement or HTMLScriptElement
> would not be. So the NOTREACHED() would need to go in a final 'else' branch of
> the inner if. I'm fine with just a DCHECK in pushCurrentScript though - it
> should serve the same purpose/verify the same contract.

Got your point. Thanks.
Done.

https://codereview.chromium.org/1945563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1945563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4601:
m_currentScriptStack.append(newCurrentScript);
On 2016/05/04 08:42:54, fs wrote:
> Add a DCHECK here that |newCurrentScript| is one of the expected types.

Done.

[fs, 2016-05-04 10:35:21]

LGTM w/ nit

https://codereview.chromium.org/1945563002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1945563002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:28: #include
"bindings/core/v8/UnionTypesCore.h"
Nit: Don't think this include is needed any longer?

[ramya.v, 2016-05-04 10:41:18]

https://codereview.chromium.org/1945563002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1945563002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:28: #include
"bindings/core/v8/UnionTypesCore.h"
On 2016/05/04 10:35:21, fs wrote:
> Nit: Don't think this include is needed any longer?

Done.

[ramya.v, 2016-05-04 10:41:38]

The CQ bit was checked by ramya.v@samsung.com

[ramya.v, 2016-05-04 10:41:38]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1945563002/#ps100001
(title: "Removed unnecessary include")

[commit-bot: I haz the power, 2016-05-04 10:41:50]

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

[commit-bot: I haz the power, 2016-05-04 12:50:54]

Description was changed from

==========
Set currentScript for SVGScriptElements.

Spec: https://html.spec.whatwg.org/#dom-document-currentscript
Mozilla already supports this.

BUG=495194
==========

to

==========
Set currentScript for SVGScriptElements.

Spec: https://html.spec.whatwg.org/#dom-document-currentscript
Mozilla already supports this.

BUG=495194
==========

[commit-bot: I haz the power, 2016-05-04 12:50:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-04 12:53:47]

Description was changed from

==========
Set currentScript for SVGScriptElements.

Spec: https://html.spec.whatwg.org/#dom-document-currentscript
Mozilla already supports this.

BUG=495194
==========

to

==========
Set currentScript for SVGScriptElements.

Spec: https://html.spec.whatwg.org/#dom-document-currentscript
Mozilla already supports this.

BUG=495194

Committed: https://crrev.com/82b3400655ebb30a0099a0f1a91d42d00f26fa08
Cr-Commit-Position: refs/heads/master@{#391486}
==========

[commit-bot: I haz the power, 2016-05-04 12:53:52]

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

[Srirama, 2016-05-04 18:26:27]

srirama.m@samsung.com changed reviewers:
+ srirama.m@samsung.com

[Srirama, 2016-05-04 18:26:28]

https://codereview.chromium.org/1945563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1945563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4600: DCHECK(newCurrentScript);
I think we can remove this DCHECK now. It is taken care in the next line inside
is[HTML|SVG]ScriptElement functions.

[ramya.v, 2016-05-05 03:50:34]

https://codereview.chromium.org/1945563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1945563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4600: DCHECK(newCurrentScript);
On 2016/05/04 18:26:28, Srirama wrote:
> I think we can remove this DCHECK now. It is taken care in the next line
inside
> is[HTML|SVG]ScriptElement functions.


Removed in https://codereview.chromium.org/1943403005/. Thanks.
Done.