Bug 78453 - Needs an internal flag to accept multiple shadow roots for the purpose of tests
Summary: Needs an internal flag to accept multiple shadow roots for the purpose of tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 77499 77931
  Show dependency treegraph
 
Reported: 2012-02-12 20:28 PST by Hayato Ito
Modified: 2012-02-16 16:57 PST (History)
9 users (show)

See Also:


Attachments
add flag (7.92 KB, patch)
2012-02-12 22:13 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
fix build on mac, gtk, win (hopefully) (12.53 KB, patch)
2012-02-13 19:50 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
turn flag off after a test. (12.59 KB, patch)
2012-02-13 22:47 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch (12.12 KB, patch)
2012-02-15 18:38 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Test (7.32 KB, patch)
2012-02-15 23:40 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (9.61 KB, patch)
2012-02-16 00:17 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2012-02-16 00:19 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2012-02-16 01:17 PST, Shinya Kawanaka
morrita: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 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.
Comment 1 Hayato Ito 2012-02-12 22:13:09 PST
Created attachment 126715 [details]
add flag
Comment 2 Philippe Normand 2012-02-12 22:19:58 PST
Comment on attachment 126715 [details]
add flag

Attachment 126715 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11508667
Comment 3 Hayato Ito 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.
Comment 4 Hayato Ito 2012-02-13 00:21:30 PST
windows build bot seems down.
Comment 5 Hayato Ito 2012-02-13 19:50:48 PST
Created attachment 126894 [details]
fix build on mac, gtk, win (hopefully)
Comment 6 Shinya Kawanaka 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.
Comment 7 Hayato Ito 2012-02-13 22:47:29 PST
Created attachment 126911 [details]
turn flag off after a test.
Comment 8 WebKit Review Bot 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.
Comment 9 Hayato Ito 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.
Comment 10 Hajime Morrita 2012-02-13 23:35:15 PST
Do we really need this ?
Comment 11 Hayato Ito 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.
Comment 12 Shinya Kawanaka 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.
Comment 13 Shinya Kawanaka 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.
Comment 14 Shinya Kawanaka 2012-02-15 18:38:42 PST
Created attachment 127290 [details]
Patch
Comment 15 Shinya Kawanaka 2012-02-15 23:40:41 PST
Created attachment 127320 [details]
Test
Comment 16 Shinya Kawanaka 2012-02-16 00:17:27 PST
Created attachment 127322 [details]
Patch
Comment 17 Shinya Kawanaka 2012-02-16 00:19:29 PST
Created attachment 127323 [details]
Patch
Comment 18 Early Warning System Bot 2012-02-16 01:05:49 PST
Comment on attachment 127323 [details]
Patch

Attachment 127323 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11530990
Comment 19 Shinya Kawanaka 2012-02-16 01:17:42 PST
Created attachment 127328 [details]
Patch
Comment 20 Shinya Kawanaka 2012-02-16 16:57:47 PST
Committed r108003: <http://trac.webkit.org/changeset/108003>