Summary: | Add support for document.currentScript | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Syoichi Tsuyuhara <syoichi> | ||||||||||||
Component: | DOM | Assignee: | Kangil Han <kangil.han> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, aestes, ap, arko, arv, buildbot, cdumez, commit-queue, d-r, dtrebbien, esprehn+autocc, fmalita, haraken, jose.lejin, kangil.han, laszlo.gombos, ojan, pdr, rniwa, schenney, s.choi, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | BlinkMergeCandidate, HTML5, WebExposed | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
URL: | http://whatwg.org/html#dom-document-currentscript | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 32934 | ||||||||||||||
Attachments: |
|
Description
Syoichi Tsuyuhara
2012-12-05 22:41:46 PST
Created attachment 186725 [details]
Patch
Comment on attachment 186725 [details]
Patch
I’d also like to see some test cases that cover multiple documents, particularly subframes.
Comment on attachment 186725 [details] Patch Attachment 186725 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16393005 Comment on attachment 186725 [details]
Patch
Alos, we should test defer, async, and scripts run by nested calls to document.write. We should also test scripts running in SVG-in-HTML in all those permutations.
Comment on attachment 186725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186725&action=review > Source/WebCore/dom/Document.cpp:552 > + m_currentScript = 0; This should be part of the initializer list. > Source/WebCore/dom/Document.h:1573 > + HTMLScriptElement* m_currentScript; How do you know this pointer remains valid through the script's execution? Perhaps this should be a RefPtr. > Source/WebCore/dom/Document.idl:178 > + readonly attribute HTMLScriptElement currentScript; What happens when we're executing a non-HTML script element? > Source/WebCore/dom/ScriptElement.cpp:301 > + if (m_element->isHTMLElement()) > + document->setCurrentScript(static_cast<HTMLScriptElement*>(m_element)); Please also test the cases of nesting HTML and non-HTML script elements. > Source/WebCore/dom/ScriptElement.cpp:313 > + document->setCurrentScript(oldScriptElement); How do you know oldScriptElement still points of a valid element? Perhaps oldScriptElement needs to be a RefPtr<HTMLScriptElement> Blink supports this. https://src.chromium.org/viewvc/blink?revision=152230&view=revision https://src.chromium.org/viewvc/blink?revision=152237&view=revision I will merge those two patches. :) Please announce this feature on webkit-dev first: http://www.webkit.org/coding/adding-features.html (In reply to comment #8) > Please announce this feature on webkit-dev first: http://www.webkit.org/coding/adding-features.html Done, thanks! :) Created attachment 204815 [details]
Patch
Comment on attachment 204815 [details] Patch Attachment 204815 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/858412 Comment on attachment 204815 [details] Patch Attachment 204815 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/865197 Created attachment 204877 [details]
Patch
Comment on attachment 204877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204877&action=review > Source/WebCore/dom/ScriptElement.cpp:285 > +static bool isHTMLScriptElement(Element* element) Add inline maybe? > Source/WebCore/dom/ScriptElement.cpp:290 > +static bool isSVGScriptElement(Element* element) Ditto. > Source/WebCore/dom/ScriptElement.cpp:323 > + if (isHTMLScriptElement(m_element)) > + document->pushCurrentScript(toHTMLScriptElement(m_element)); > + Don't we need to do this in HTMLScriptRunner::executePendingScriptAndDispatchEvent? Also, can we use a RAII object to do this? Perhaps we can rename IgnoreDestructiveWriteCountIncrementer and do it there? Created attachment 205261 [details]
Patch
Took rniwa's comment into consideration. :) (In reply to comment #14) > (From update of attachment 204877 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204877&action=review > > > Source/WebCore/dom/ScriptElement.cpp:285 > > +static bool isHTMLScriptElement(Element* element) > > Add inline maybe? Done. > > > Source/WebCore/dom/ScriptElement.cpp:290 > > +static bool isSVGScriptElement(Element* element) > > Ditto. Done. > > > Source/WebCore/dom/ScriptElement.cpp:323 > > + if (isHTMLScriptElement(m_element)) > > + document->pushCurrentScript(toHTMLScriptElement(m_element)); > > + > > Don't we need to do this in HTMLScriptRunner::executePendingScriptAndDispatchEvent? > Also, can we use a RAII object to do this? Perhaps we can rename IgnoreDestructiveWriteCountIncrementer and do it there? IMHO, 1. As note describes, this is the point where script is actually run 2. IgnoreDestructiveWriteCountIncrementer is for http://www.whatwg.org/specs/web-apps/current-work/#ignore-destructive-writes-counter so document is manipulated as specification Comment on attachment 205261 [details] Patch Attachment 205261 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/980061 New failing tests: fast/media/mq-transform-03.html fast/forms/search-event-delay.html fast/media/mq-transform-02.html platform/win/accessibility/multiple-select-element-role.html Created attachment 205265 [details]
Archive of layout-test-results from APPLE-EWS-6 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-6 Port: win-future Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
(In reply to comment #17) > (From update of attachment 205261 [details]) > Attachment 205261 [details] did not pass win-ews (win): > Output: http://webkit-queues.appspot.com/results/980061 > > New failing tests: > fast/media/mq-transform-03.html > fast/forms/search-event-delay.html > fast/media/mq-transform-02.html > platform/win/accessibility/multiple-select-element-role.html I don't think this patch is related to above failed cases. :) Comment on attachment 205261 [details] Patch Clearing flags on attachment: 205261 Committed r151951: <http://trac.webkit.org/changeset/151951> All reviewed patches have been landed. Closing bug. |