Bug 78453 – Needs an internal flag to accept multiple shadow roots for the purpose of tests

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hayato Ito]

We need a simple internal flag on ShadowRootList by which it accepts multiple shadow tree.

We should avoid accepting multiple shadow roots until multiple shadow tree becomes stable.
At the same time, we need to test multiple shadow tree.

Therefore it might be better to have a flag (maybe static bool member variable is enough for the purpose of test) and expose the setter through window.internals.

[Hayato Ito]

Created attachment 126715 [details]
add flag

[Philippe Normand]

Comment on attachment 126715 [details]
add flag

Attachment 126715 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11508667

[Hayato Ito]

We need to add symbols for some ports. Let me watch the result of build on some ports.

[Hayato Ito]

windows build bot seems down.

[Hayato Ito]

Created attachment 126894 [details]
fix build on mac, gtk, win (hopefully)

[Shinya Kawanaka]

Comment on attachment 126894 [details]
fix build on mac, gtk, win (hopefully)

View in context: https://bugs.webkit.org/attachment.cgi?id=126894&action=review

> LayoutTests/fast/dom/shadow/shadow-root-js-api.html:62
> +   window.internals.setMultipleShadowRootsEnabled(true);

Could you make this flag OFF after the test? This has a side effect actually, and will cause flaky results.

[Hayato Ito]

Created attachment 126911 [details]
turn flag off after a test.

[WebKit Review Bot]

Attachment 126911 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Last 3072 characters of output:
fast/backgrounds/size/backgroundSize21-expected.txt
W: -empty_dir: trunk/LayoutTests/platform/qt/fast/backgrounds/size/backgroundSize22-expected.txt
r107672 = 444e12b7089743ee976e71dd7889fca2dd0c50b0 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: [chromium] Rebaseline JPEG image results after r107389
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
warning: too many files (created: 168845 deleted: 8), skipping inexact rename detection
error: refusing to lose untracked file at 'LayoutTests/fast/backgrounds/size/backgroundSize21-expected.txt'
error: refusing to lose untracked file at 'LayoutTests/fast/backgrounds/size/backgroundSize22-expected.txt'
CONFLICT (rename/rename): Rename "LayoutTests/platform/gtk/fast/backgrounds/size/backgroundSize21-expected.txt"->"LayoutTests/platform/efl/fast/backgrounds/size/backgroundSize21-expected.txt" in branch "HEAD" rename "LayoutTests/platform/gtk/fast/backgrounds/size/backgroundSize21-expected.txt"->"LayoutTests/fast/backgrounds/size/backgroundSize21-expected.txt" in "[chromium] Rebaseline JPEG image results after r107389"
CONFLICT (rename/rename): Rename "LayoutTests/platform/gtk/fast/backgrounds/size/backgroundSize22-expected.txt"->"LayoutTests/platform/efl/fast/backgrounds/size/backgroundSize22-expected.txt" in branch "HEAD" rename "LayoutTests/platform/gtk/fast/backgrounds/size/backgroundSize22-expected.txt"->"LayoutTests/fast/backgrounds/size/backgroundSize22-expected.txt" in "[chromium] Rebaseline JPEG image results after r107389"
CONFLICT (rename/delete): Rename LayoutTests/platform/mac/fast/backgrounds/size/backgroundSize21-expected.txt->LayoutTests/fast/backgrounds/size/backgroundSize19-expected.txt in HEAD and deleted in [chromium] Rebaseline JPEG image results after r107389
CONFLICT (rename/delete): Rename LayoutTests/platform/mac/fast/backgrounds/size/backgroundSize22-expected.txt->LayoutTests/fast/backgrounds/size/backgroundSize20-expected.txt in HEAD and deleted in [chromium] Rebaseline JPEG image results after r107389
CONFLICT (rename/delete): Rename LayoutTests/platform/qt/fast/backgrounds/size/backgroundSize21-expected.txt->LayoutTests/fast/backgrounds/size/backgroundSize21-expected.txt in HEAD and deleted in [chromium] Rebaseline JPEG image results after r107389
CONFLICT (rename/delete): Rename LayoutTests/platform/qt/fast/backgrounds/size/backgroundSize22-expected.txt->LayoutTests/fast/backgrounds/size/backgroundSize22-expected.txt in HEAD and deleted in [chromium] Rebaseline JPEG image results after r107389
Auto-merging LayoutTests/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [chromium] Rebaseline JPEG image results after r107389

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hayato Ito]

Nice catch. Done.
I am afraid that it is dangerous to use static variable flag. We should be careful for writing tests using this flag.

(In reply to comment #6)
> (From update of attachment 126894 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126894&action=review
> 
> > LayoutTests/fast/dom/shadow/shadow-root-js-api.html:62
> > +   window.internals.setMultipleShadowRootsEnabled(true);
> 
> Could you make this flag OFF after the test? This has a side effect actually, and will cause flaky results.

[Hajime Morrita]

Do we really need this ?

[Hayato Ito]

(In reply to comment #10)
> Do we really need this ?

As discussed with shinyak, we need this to move forward step by step.
We need to write tests using this flag for multiple shadow trees. But we don't want to accept multiple shadow roots for a while in production, which will be the root cause of crashes.

[Shinya Kawanaka]

(In reply to comment #11)
> (In reply to comment #10)
> > Do we really need this ?
> 
> As discussed with shinyak, we need this to move forward step by step.
> We need to write tests using this flag for multiple shadow trees. But we don't want to accept multiple shadow roots for a while in production, which will be the root cause of crashes.

Yeah, I agree. I think multiple shadow subtrees will be unstable for a while, and it will take time to implement it stably. It's good to have a flag for us to go forward step by step.

[Shinya Kawanaka]

Since hayato-san is off a few days, I'll take over this patch. I need this patch.

[Shinya Kawanaka]

Created attachment 127290 [details]
Patch

[Shinya Kawanaka]

Created attachment 127320 [details]
Test

[Shinya Kawanaka]

Created attachment 127322 [details]
Patch

[Shinya Kawanaka]

Created attachment 127323 [details]
Patch

[Early Warning System Bot]

Comment on attachment 127323 [details]
Patch

Attachment 127323 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11530990

[Shinya Kawanaka]

Created attachment 127328 [details]
Patch

[Shinya Kawanaka]

Committed r108003: <http://trac.webkit.org/changeset/108003>