Implement document.currentScript

Implement document.currentScript

http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#dom-document-currentscript

document.currentScript reflects the script that is currently being executed.

BUG=240876
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152230

[arv (Not doing code reviews), 2013-05-14 22:36:07]

needs tests

[esprehn, 2013-05-14 22:50:09]

YAY, this looks great. Is that the only entry point to script? I thought there
were other places we could end up executing it.

https://codereview.chromium.org/14852011/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/14852011/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3926: m_currentScript.clear();
We usually prefer to do these in separate methods like clearCurrentScript() and
setCurrentScript() with an ASSERT in the setter.

https://codereview.chromium.org/14852011/diff/1/Source/core/dom/ScriptElement...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14852011/diff/1/Source/core/dom/ScriptElement...
Source/core/dom/ScriptElement.cpp:324: document->setCurrentScript(0);
I think you want to check that document->currentScript() == this, otherwise we
could skip the block above and then this would clear the current script even
though it's not us?

[dglazkov, 2013-05-14 22:52:32]

Don't forget a test :)

[arv (Not doing code reviews), 2013-05-15 15:55:07]

Ready for review now

[esprehn, 2013-05-15 16:08:16]

You have some left over code in setCurrentScript. Also what happens when you
appendChild() a <script> from inside another one? This code looks like it's
going to clear() the currentScript so doing:

<script id="a">
// document.currentScript is #a
document.body.appendChild(document.createElement('script'));
// document.currentScript is now null ?
</script>

https://codereview.chromium.org/14852011/diff/7001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/14852011/diff/7001/Source/core/dom/Document.c...
Source/core/dom/Document.cpp:3928: }
Please remove, you can add ASSERT(newCurrentScript);

https://codereview.chromium.org/14852011/diff/7001/Source/core/dom/ScriptElem...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14852011/diff/7001/Source/core/dom/ScriptElem...
Source/core/dom/ScriptElement.cpp:324: document->clearCurrentScript();
You're sure this is safe without checking if (document->currentScript() == this)
before it?

[arv (Not doing code reviews), 2013-05-15 16:18:11]

https://codereview.chromium.org/14852011/diff/7001/Source/core/dom/ScriptElem...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/14852011/diff/7001/Source/core/dom/ScriptElem...
Source/core/dom/ScriptElement.cpp:324: document->clearCurrentScript();
On 2013/05/15 16:08:16, esprehn wrote:
> You're sure this is safe without checking if (document->currentScript() ==
this)
> before it?

There is a case where this will fail. When you doc.write inline script it id
evaluated synchronously. I'll have to add tests and a stack to handle this case
correctly.

[arv (Not doing code reviews), 2013-05-15 16:56:24]

PTAL

Thanks for catching the nested script case.

[esprehn, 2013-05-23 17:49:37]

On 2013/05/15 16:56:24, arv wrote:
> PTAL
> 
> Thanks for catching the nested script case.

LGTM.

[commit-bot: I haz the power, 2013-05-23 17:49:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/14852011/14001

[commit-bot: I haz the power, 2013-05-23 19:25:40]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-05-23 19:27:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/14852011/14001

[commit-bot: I haz the power, 2013-05-23 21:21:47]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-11 16:01:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/14852011/36001

[commit-bot: I haz the power, 2013-06-11 17:25:27]

Change committed as 152230

[abarth-chromium, 2013-06-11 17:45:20]

https://chromiumcodereview.appspot.com/14852011/diff/36001/Source/core/dom/Sc...
File Source/core/dom/ScriptElement.cpp (right):

https://chromiumcodereview.appspot.com/14852011/diff/36001/Source/core/dom/Sc...
Source/core/dom/ScriptElement.cpp:280: return element->isHTMLElement() &&
element->hasTagName(HTMLNames::scriptTag);
The isHTMLElement check isn't redundant with hasTagName(HTMLNames::scriptTag)?

https://chromiumcodereview.appspot.com/14852011/diff/36001/Source/core/dom/Sc...
Source/core/dom/ScriptElement.cpp:308:
document->pushCurrentScript(static_cast<HTMLScriptElement*>(m_element));
We should have a toHTMLScriptElement that contains an
ASSERT_WITH_SECURITY_IMPLICATIONS that the element has the right tag name.

[arv (Not doing code reviews), 2013-06-11 18:12:58]

Adam, please take a look at https://codereview.chromium.org/16682004/