RESOLVED FIXED 78453
Needs an internal flag to accept multiple shadow roots for the purpose of tests
https://bugs.webkit.org/show_bug.cgi?id=78453
Summary Needs an internal flag to accept multiple shadow roots for the purpose of tests
Hayato Ito
Reported 2012-02-12 20:28:22 PST
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.
Attachments
add flag (7.92 KB, patch)
2012-02-12 22:13 PST, Hayato Ito
no flags
fix build on mac, gtk, win (hopefully) (12.53 KB, patch)
2012-02-13 19:50 PST, Hayato Ito
no flags
turn flag off after a test. (12.59 KB, patch)
2012-02-13 22:47 PST, Hayato Ito
no flags
Patch (12.12 KB, patch)
2012-02-15 18:38 PST, Shinya Kawanaka
no flags
Test (7.32 KB, patch)
2012-02-15 23:40 PST, Shinya Kawanaka
no flags
Patch (9.61 KB, patch)
2012-02-16 00:17 PST, Shinya Kawanaka
no flags
Patch (9.62 KB, patch)
2012-02-16 00:19 PST, Shinya Kawanaka
no flags
Patch (9.62 KB, patch)
2012-02-16 01:17 PST, Shinya Kawanaka
morrita: review+
Hayato Ito
Comment 1 2012-02-12 22:13:09 PST
Created attachment 126715 [details] add flag
Philippe Normand
Comment 2 2012-02-12 22:19:58 PST
Hayato Ito
Comment 3 2012-02-12 22:23:34 PST
We need to add symbols for some ports. Let me watch the result of build on some ports.
Hayato Ito
Comment 4 2012-02-13 00:21:30 PST
windows build bot seems down.
Hayato Ito
Comment 5 2012-02-13 19:50:48 PST
Created attachment 126894 [details] fix build on mac, gtk, win (hopefully)
Shinya Kawanaka
Comment 6 2012-02-13 22:27:47 PST
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
Comment 7 2012-02-13 22:47:29 PST
Created attachment 126911 [details] turn flag off after a test.
WebKit Review Bot
Comment 8 2012-02-13 22:49:10 PST
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
Comment 9 2012-02-13 22:51:40 PST
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
Comment 10 2012-02-13 23:35:15 PST
Do we really need this ?
Hayato Ito
Comment 11 2012-02-13 23:46:42 PST
(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
Comment 12 2012-02-13 23:58:14 PST
(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
Comment 13 2012-02-15 17:33:27 PST
Since hayato-san is off a few days, I'll take over this patch. I need this patch.
Shinya Kawanaka
Comment 14 2012-02-15 18:38:42 PST
Shinya Kawanaka
Comment 15 2012-02-15 23:40:41 PST
Shinya Kawanaka
Comment 16 2012-02-16 00:17:27 PST
Shinya Kawanaka
Comment 17 2012-02-16 00:19:29 PST
Early Warning System Bot
Comment 18 2012-02-16 01:05:49 PST
Shinya Kawanaka
Comment 19 2012-02-16 01:17:42 PST
Shinya Kawanaka
Comment 20 2012-02-16 16:57:47 PST
Note You need to log in before you can comment on or make changes to this bug.