Bug 78473

Created attachment 127343 [details] WIP I wish to implement this. Attaching WIP for the same.

(In reply to comment #1) > Created an attachment (id=127343) [details] > WIP > > I wish to implement this. Attaching WIP for the same. Great!!! Could you consider to extract existing implementation and share it with this? Having two similar code tend to have a trouble in future change.

Created attachment 127555 [details] Patch Update patch as per Morita's suggestion to share common code between HTMLElement & ShadowRoot. Moved code to markup as its relevant code and can be re-factored to reside there. Can u review once?

You need to write tests when adding APIs. Could you write Layout tests for new APIs? (In reply to comment #3) > Created an attachment (id=127555) [details] > Patch > > Update patch as per Morita's suggestion to share common code between HTMLElement & ShadowRoot. Moved code to markup as its relevant code and can be re-factored to reside there. Can u review once?

Created attachment 127573 [details] Patch with layout tests Added test case.

Comment on attachment 127573 [details] Patch with layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=127573&action=review > LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:20 > +root = new WebKitShadowRoot(div); 'new WebKitShadowRoot()' is enabled only if SHADOW_DOM flag is enabled. Actually this flag is currently enabled in chromium-port. So you add this test in skipped lists. See LayoutTests/platform/<port>/Skipped and add test name in it. <port> will be 'efl', 'gtk', 'qt', 'mac', 'win', 'wincairo', and 'wk2'.

Comment on attachment 127573 [details] Patch with layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=127573&action=review > Source/WebCore/dom/ShadowRoot.cpp:156 > + replaceChildrenWithFragment(host(), fragment.release(), ec); No, this is not what we need. we need to inject fragment to this ShadowRoot, not the host. > LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:24 > +shouldBeDefined("root.innerHTML"); This is not enough. We need to test - the resulted string of innerHTML after setting innerHTML. - the resulted DOM tree after setting innerHTML - the innerHTML value for existing shadow DOM tree.

Created attachment 127944 [details] Updated Patch Updated patch with fixed test case. Added test case to Skipped for other platforms.

Comment on attachment 127944 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127944&action=review > Source/WebCore/dom/ShadowRoot.cpp:157 > + replaceChildrenWithFragment(toElement(this), fragment.release(), ec); Does this really work? > LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:36 > + I'm confused. Why is div2 available even after setting another innerHTML?

(In reply to comment #9) > (From update of attachment 127944 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127944&action=review > > > Source/WebCore/dom/ShadowRoot.cpp:157 > > + replaceChildrenWithFragment(toElement(this), fragment.release(), ec); > > Does this really work? > I am assuming this works as DocumentFragment is derived from ContainerNode. > > LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:36 > > + > > I'm confused. Why is div2 available even after setting another innerHTML? Its only to show that it resides in the dom tree of the ShadowRoot. After setting innerHTML, the children gets replaced with the fragment.

Comment on attachment 127944 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127944&action=review >>> Source/WebCore/dom/ShadowRoot.cpp:157 >>> + replaceChildrenWithFragment(toElement(this), fragment.release(), ec); >> >> Does this really work? > > I am assuming this works as DocumentFragment is derived from ContainerNode. Even so this is beyond fragile. It only happens to work because currently replaceChildrenWithFragment uses ContainerNode methods only. I suggest making the parameter a ContainerNode* and do away with toElement(). Then any changes to to replaceChildrenWithFragment will break at compile time rather than runtime. > Source/WebCore/editing/markup.cpp:940 > +PassRefPtr<DocumentFragment> createFragmentFromSource(const String& markup, Element* element, ExceptionCode& ec) editing/ doesn't seem the right place to me for these methods. IMHO they'd better be moved to dom/ - either into ContainerNode or, better yet, into a separate file. >>> LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:36 >>> + >> >> I'm confused. Why is div2 available even after setting another innerHTML? > > Its only to show that it resides in the dom tree of the ShadowRoot. After setting innerHTML, the children gets replaced with the fragment. I have to say I'm likewise confused - as you say, it should get replaced and no longer be in the tree. So why is it still accessible via getElementById?

Comment on attachment 127944 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127944&action=review >> Source/WebCore/editing/markup.cpp:940 >> +PassRefPtr<DocumentFragment> createFragmentFromSource(const String& markup, Element* element, ExceptionCode& ec) > > editing/ doesn't seem the right place to me for these methods. IMHO they'd better be moved to dom/ - either into ContainerNode or, better yet, into a separate file. I think this is OK for this time if we have a bug to move this to appropriate place. >>>> LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:36 >>>> + >>> >>> I'm confused. Why is div2 available even after setting another innerHTML? >> >> Its only to show that it resides in the dom tree of the ShadowRoot. After setting innerHTML, the children gets replaced with the fragment. > > I have to say I'm likewise confused - as you say, it should get replaced and no longer be in the tree. So why is it still accessible via getElementById? I tried this locally and it triggered an assertion failure on the debug build.

Created attachment 128447 [details] Updated Patch updated replaceChildrenWithFragment to use ContainerNode. Added few more use cases in test.

Comment on attachment 128447 [details] Updated Patch Almost looks good! Could you add another test case which ensures that setting a HTML fragment not only results a innerHTML value change, but also actually creates DOM nodes? we can test it even without (currently broken) getElementId() or something. querySelector() or just traversing the tree by firstChild etc. will be sufficient.

Created attachment 128662 [details] Updated Patch Added test case to check actual dom tree getting created after setting innerHTML fragment to ShadowRoot.

Comment on attachment 128662 [details] Updated Patch Looks good! let's land this.

(In reply to comment #16) > (From update of attachment 128662 [details]) > Looks good! let's land this. Thanks Morrita for immediate review :)

Comment on attachment 128662 [details] Updated Patch Rejecting attachment 128662 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: .html patching file LayoutTests/platform/gtk/Skipped patching file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/qt/Skipped Hunk #1 succeeded at 166 (offset 1 line). patching file LayoutTests/platform/win/Skipped patching file LayoutTests/platform/wincairo/Skipped patching file LayoutTests/platform/wk2/Skipped Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Hajime Mor..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11603939

Comment on attachment 128662 [details] Updated Patch Aah..Morrita can u make c+ again? Guess commit bot failed. Retrying.

Comment on attachment 128662 [details] Updated Patch Rejecting attachment 128662 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: le LayoutTests/platform/gtk/Skipped.rej patching file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/qt/Skipped Hunk #1 succeeded at 167 with fuzz 1 (offset 2 lines). patching file LayoutTests/platform/win/Skipped patching file LayoutTests/platform/wincairo/Skipped patching file LayoutTests/platform/wk2/Skipped Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Hajime Mor..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11627819

Comment on attachment 128662 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128662&action=review Tried this again. And found that this doesn't work. Please test this using debug build so that you can see the assertion failure. > Source/WebCore/dom/ShadowRoot.cpp:154 > + RefPtr<DocumentFragment> fragment = createFragmentFromSource(markup, toElement(this), ec); You cannot use toElement() here.

Created attachment 129239 [details] Updated Patch For creation of DocumentFragmet I can use host() as HTMLDocumentParser currently take Element as reference to create HTMLTree. The injected fragment will be replaced on actual ShadowRoot only. Tested on debug build. Morrita can u review once? Thanks.

Created attachment 129397 [details] Updated Patch Guess the patch didn't made it. Updated patch

Comment on attachment 129397 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129397&action=review > Source/WebCore/editing/markup.h:30 > +#include "ExceptionCode.h" You can just typedef int ExceptionCode. instead of including this. Same for ContainerNode.h. Less includes are better. > LayoutTests/platform/mac/Skipped:426 > +fast/dom/shadow Please don do this unless you are a maintainer of this port. There some test which can run without ENABLE_SHADOW_DOM under shadow/ We should've splited these from one which needs API. But it's another story. > LayoutTests/platform/qt/Skipped:162 > +fast/dom/shadow Ditto. > LayoutTests/platform/win/Skipped:1452 > +fast/dom/shadow Ditto. > LayoutTests/platform/wincairo/Skipped:1968 > Ditto. > LayoutTests/platform/wk2/Skipped:1066 > +fast/dom/shadow Ditto.

Created attachment 129404 [details] Updated Patch

Attachment 129404 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/editing/markup.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 129404 [details] Updated Patch Looks good! thanks for your cooperation. It's time to land...

The commit-queue encountered the following flaky tests while processing attachment 129404 [details]: css3/filters/effect-invert-hw.html bug 79639 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 129404 [details] Updated Patch Rejecting attachment 129404 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 164. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9 Updating OpenSource RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/11695257

Comment on attachment 129404 [details] Updated Patch svn.webkit.org was down :(

Comment on attachment 129404 [details] Updated Patch Clearing flags on attachment: 129404 Committed r109251: <http://trac.webkit.org/changeset/109251>

All reviewed patches have been landed. Closing bug.