[Shadow DOM] ShadowRoot should be constructable

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

I'd like to keep this separated from Bug 76353
because the constructor needs to care about some nontrivial stuff like nesting.
I don't think we need to support nesting at first, but at least we need to guard against it.

[Hayato Ito]

I agree that we don't need to support nesting at first, but could you tell me what is the issue of nesting? Is it related to the rendering or something else?


(In reply to comment #0)
> I'd like to keep this separated from Bug 76353
> because the constructor needs to care about some nontrivial stuff like nesting.
> I don't think we need to support nesting at first, but at least we need to guard against it.

[Hayato Ito]

Created attachment 123537 [details]
wip. based on dominic's patch

[Hajime Morrita]

Comment on attachment 123537 [details]
wip. based on dominic's patch

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

> Source/WebCore/dom/ShadowRoot.idl:32
> +        CustomConstructFunction

I guess we don't need custom constructor and can use CallWith=ScriptExceptionContext  instead.
ScriptExecutionContext is actually a Document object, which provides frame() to retrieve the frame pointer.

> Source/WebCore/page/DOMWindow.idl:401
> +        attribute [CustomGetter, Conditional=SHADOW_DOM] ShadowRootConstructor ShadowRoot

EnabledAtRuntime?

[Hayato Ito]

Thank you for the review.

(In reply to comment #3)
> (From update of attachment 123537 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123537&action=review
> 
> > Source/WebCore/dom/ShadowRoot.idl:32
> > +        CustomConstructFunction
> 
> I guess we don't need custom constructor and can use CallWith=ScriptExceptionContext  instead.
> ScriptExecutionContext is actually a Document object, which provides frame() to retrieve the frame pointer.

Could you clarify how we can use SecriptExecutionContext here if possible? I am not strongly familiar with how to use it.

> 
> > Source/WebCore/page/DOMWindow.idl:401
> > +        attribute [CustomGetter, Conditional=SHADOW_DOM] ShadowRootConstructor ShadowRoot
> 
> EnabledAtRuntime?

OK. Let me fix.

Two more things:
- I'll rename ShadowRoot to WebKitShadowRoot as discussed in another thread.
- Do you think we need to support JavaScriptCore in the same patch?

[Hajime Morrita]

> > I guess we don't need custom constructor and can use CallWith=ScriptExceptionContext  instead.
> > ScriptExecutionContext is actually a Document object, which provides frame() to retrieve the frame pointer.
> 
> Could you clarify how we can use SecriptExecutionContext here if possible? I am not strongly familiar with how to use it.

I'm sorry for the lack of clarity...

The WebKItIDL support "CallWith" attribute for method invocation (and for the constructor IIRC).
You can see an example in page/History.idl.
Specifying the attribute, the script generates the code which passes
ScriptExceptionContext as a parameter of the native method call.
The given ScriptExceptionContext is actually a document.
So we can retrieve a Fame object associated with the document.

> 
> > 
> > > Source/WebCore/page/DOMWindow.idl:401
> > > +        attribute [CustomGetter, Conditional=SHADOW_DOM] ShadowRootConstructor ShadowRoot
> > 
> > EnabledAtRuntime?
> 
> OK. Let me fix.
> 
> Two more things:
> - I'll rename ShadowRoot to WebKitShadowRoot as discussed in another thread.
Thanks!

> - Do you think we need to support JavaScriptCore in the same patch?
I think it can be done in a separate patch.
But I expect we don't need any custom code and it's done automatically.

CC-ing Haraken may know something about this.

[Kentaro Hara]

Comment on attachment 123537 [details]
wip. based on dominic's patch

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

>>> Source/WebCore/dom/ShadowRoot.idl:32
>>> +        CustomConstructFunction
>> 
>> I guess we don't need custom constructor and can use CallWith=ScriptExceptionContext  instead.
>> ScriptExecutionContext is actually a Document object, which provides frame() to retrieve the frame pointer.
> 
> Could you clarify how we can use SecriptExecutionContext here if possible? I am not strongly familiar with how to use it.

EventSource.idl is a good example. It uses CallWith=ScriptExecutionContext for the constructor. You can just write

interface [Conditional=SHADOW_DOM, EnabledAtRuntime=shadowDOM, V8Constructor, CallWith=ScriptExecutionContext] ShadowRoot : Node { ... }

Then, the following code will be generated. (See src/out/Debug/obj/gen/webcore/bindings/V8ShadowRoot.cpp.)

    RefPtr<ShadowRoot> impl = ShadowRoot::create(context);
    ....

You can retrieve the frame pointer from |context| in ShadowRoot::create().

[WebKit Review Bot]

Comment on attachment 123537 [details]
wip. based on dominic's patch

Attachment 123537 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11344018

New failing tests:
fast/dom/dom-constructors.html
fast/js/constructor.html
fast/dom/Window/window-lookup-precedence.html
fast/dom/global-constructors.html

[Hayato Ito]

Created attachment 123892 [details]
Add ShadowRoot consturctor as WebKitShadowRoot

[Hayato Ito]

Thank you for the review.

(In reply to comment #5)
> > > I guess we don't need custom constructor and can use CallWith=ScriptExceptionContext  instead.
> > > ScriptExecutionContext is actually a Document object, which provides frame() to retrieve the frame pointer.
> > 
> > Could you clarify how we can use SecriptExecutionContext here if possible? I am not strongly familiar with how to use it.
> 
> I'm sorry for the lack of clarity...
> 
> The WebKItIDL support "CallWith" attribute for method invocation (and for the constructor IIRC).
> You can see an example in page/History.idl.
> Specifying the attribute, the script generates the code which passes
> ScriptExceptionContext as a parameter of the native method call.
> The given ScriptExceptionContext is actually a document.
> So we can retrieve a Fame object associated with the document.

Thank you for the explanation.

After I implemented ShadowRoot::create(ScriptExecutionContext*, Element*), I noticed that we don't need ScriptExecutionContext because we can retrieve Document object which is required for ShadowRoot's constructor, from the given shadowHost Element. Aha!

> > > EnabledAtRuntime?

Done.

[Kentaro Hara]

Comment on attachment 123892 [details]
Add ShadowRoot consturctor as WebKitShadowRoot

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

> Source/WebCore/dom/ShadowRoot.cpp:63
> +    ASSERT(!ec);

Is this OK? If setShadowRoot() can raise an exception, you should add the [ConstructorRaisesException] IDL to handle the exception. Then the signature of ShadowRoot::create() would be ShadowRoot::create(Element*, ExceptionCode&). Please look at EventSource.idl and EventSource::create() for reference.

[Hayato Ito]

Thank you for the review.

(In reply to comment #10)
> (From update of attachment 123892 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123892&action=review
> 
> > Source/WebCore/dom/ShadowRoot.cpp:63
> > +    ASSERT(!ec);
> 
> Is this OK? If setShadowRoot() can raise an exception, you should add the [ConstructorRaisesException] IDL to handle the exception. Then the signature of ShadowRoot::create() would be ShadowRoot::create(Element*, ExceptionCode&). Please look at EventSource.idl and EventSource::create() for reference.

Element::setShadowRoot() should not raise Exception in this code path since validateShadowRoot in Element.cpp always return true in this case.

But I agree we should change the signature since we might change the semantic of Element::setShadowRoot in the future. It becomes robust if we return ExceptionCode as is.

Thank you! Let me update the patch.

[Hayato Ito]

Created attachment 123919 [details]
Use ExceptionCode

[Hayato Ito]

I've found the generated code doesn't check null-pointer for in-parameter.
'new WebKitShadowRoot(undefined) ' simply crashes.
Let me handle this case.

[Hayato Ito]

Created attachment 123924 [details]
Validate input Element

[Kentaro Hara]

Comment on attachment 123924 [details]
Validate input Element

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

> Source/WebCore/dom/ShadowRoot.cpp:59
> +        ec = TYPE_MISMATCH_ERR;

This should be HIERARCHY_REQUEST_ERR.
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-object

> LayoutTests/fast/dom/shadow/shadow-root-js-api.html:15
> +shouldThrow("new WebKitShadowRoot()");
> +shouldThrow("new WebKitShadowRoot(undefined)");
> +shouldThrow("new WebKitShadowRoot(1)");

Can you add more test cases around 'might-be Nullable' data, like WebKitShadowRoot(''), WebKitShadowRoot(null), WebKitShadowRoot('null'), WebKitShadowRoot('undefined')

[Hayato Ito]

(In reply to comment #15)
> (From update of attachment 123924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123924&action=review
> 
> > Source/WebCore/dom/ShadowRoot.cpp:59
> > +        ec = TYPE_MISMATCH_ERR;
> 
> This should be HIERARCHY_REQUEST_ERR.
> https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-object

Oh. Nice catch. I'll fix it in the next patch. But I think TYPE_MISMATCH_ERR is more appropriate here. I might file a bug for the spec.

> 
> > LayoutTests/fast/dom/shadow/shadow-root-js-api.html:15
> > +shouldThrow("new WebKitShadowRoot()");
> > +shouldThrow("new WebKitShadowRoot(undefined)");
> > +shouldThrow("new WebKitShadowRoot(1)");
> 
> Can you add more test cases around 'might-be Nullable' data, like WebKitShadowRoot(''), WebKitShadowRoot(null), WebKitShadowRoot('null'), WebKitShadowRoot('undefined')

Such tests don't have much meaning here because it proves only our IDL's ability of converting 'nullable' data to null. I think there are existing tests which cover such cases.

I think the following 3 tests are enough here as error cases of ShadowRoot' constructor API:
   new WebKitShadowRoot(); // Test for args' length 0
   new WebKitShadowRoot(null); // Test for null
   new WebKitShadowRoot(1); // Test for incompatible type.

[Kentaro Hara]

Comment on attachment 123924 [details]
Validate input Element

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

>>> Source/WebCore/dom/ShadowRoot.cpp:59
>>> +        ec = TYPE_MISMATCH_ERR;
>> 
>> This should be HIERARCHY_REQUEST_ERR.
>> https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-object
> 
> Oh. Nice catch. I'll fix it in the next patch. But I think TYPE_MISMATCH_ERR is more appropriate here. I might file a bug for the spec.

Makes sense to file and discuss the error type. I think SYNTAX_ERR is another candidate. Actually, what error should be returned in such cases is different from spec to spec (e.g. exceptions in Indexed DB: http://www.w3.org/TR/IndexedDB/#exceptions).

>>> LayoutTests/fast/dom/shadow/shadow-root-js-api.html:15
>>> +shouldThrow("new WebKitShadowRoot(1)");
>> 
>> Can you add more test cases around 'might-be Nullable' data, like WebKitShadowRoot(''), WebKitShadowRoot(null), WebKitShadowRoot('null'), WebKitShadowRoot('undefined')
> 
> Such tests don't have much meaning here because it proves only our IDL's ability of converting 'nullable' data to null. I think there are existing tests which cover such cases.
> 
> I think the following 3 tests are enough here as error cases of ShadowRoot' constructor API:
>    new WebKitShadowRoot(); // Test for args' length 0
>    new WebKitShadowRoot(null); // Test for null
>    new WebKitShadowRoot(1); // Test for incompatible type.

OK. But let's add WebKitShadowRoot(undefined). Basically, I guess it is important to clarify (1) the behavior when the argument is missing (it depends on [Optional]), (2) the behavior when the argument is null (it depends on [ConvertNullToNullString]), and (3) the behavior when the argument is undefined (it depends on [ConvertNullOrUndefinedToNullString]).

[Adam Barth]

Comment on attachment 123924 [details]
Validate input Element

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

> Source/WebCore/dom/ShadowRoot.cpp:65
> +    if (!element->document()) {
> +        ec = WRONG_DOCUMENT_ERR;
> +        return 0;
> +    }

Are you sure this can happen?  Do you have a test that demonstrates it?

[Hajime Morrita]

Comment on attachment 123924 [details]
Validate input Element

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

> Source/WebCore/dom/ShadowRoot.cpp:56
> +PassRefPtr<ShadowRoot> ShadowRoot::create(Element* element, ExceptionCode& ec)

Please ensure that |element| has no shadow or throw an exception.
At this time we don't support nested shadow.

>>>> Source/WebCore/dom/ShadowRoot.cpp:59
>>>> +        ec = TYPE_MISMATCH_ERR;
>>> 
>>> This should be HIERARCHY_REQUEST_ERR.
>>> https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-object
>> 
>> Oh. Nice catch. I'll fix it in the next patch. But I think TYPE_MISMATCH_ERR is more appropriate here. I might file a bug for the spec.
> 
> Makes sense to file and discuss the error type. I think SYNTAX_ERR is another candidate. Actually, what error should be returned in such cases is different from spec to spec (e.g. exceptions in Indexed DB: http://www.w3.org/TR/IndexedDB/#exceptions).

Also, |element| should be always isElementNode().

[Hayato Ito]

Created attachment 124059 [details]
Update

[Hayato Ito]

Thank you for the reviews, guys.

(In reply to comment #17)
> OK. But let's add WebKitShadowRoot(undefined). Basically, I guess it is important to clarify (1) the behavior when the argument is missing (it depends on [Optional]), (2) the behavior when the argument is null (it depends on [ConvertNullToNullString]), and (3) the behavior when the argument is undefined (it depends on [ConvertNullOrUndefinedToNullString]).

Okay, I added 'undefined' case and 'null' case. But should we take care such converting rules of IDL here? Since we declare input parameter as Element, generated code always passes correctly-typed Element* (nullable) object and never passes String object, doesn't it?

I am afraid that if we add tests for each rule, tests will be tightly coupled with a particular IDL implementation. I mean even if IDL adds a new rule, such as 'NumberZeroToEmptyString', in the future, we don't want to update all existing JavaScript API tests to test for '0', I guess.


(In reply to comment #18)
> (From update of attachment 123924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123924&action=review
> 
> > Source/WebCore/dom/ShadowRoot.cpp:65
> > +    if (!element->document()) {
> > +        ec = WRONG_DOCUMENT_ERR;
> > +        return 0;
> > +    }
> 
> Are you sure this can happen?  Do you have a test that demonstrates it?

Done. Never happens as per Node::document()'s commet. We can assume that Element is never DocmentType node.

(In reply to comment #19)
> (From update of attachment 123924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123924&action=review
> 
> > Source/WebCore/dom/ShadowRoot.cpp:56
> > +PassRefPtr<ShadowRoot> ShadowRoot::create(Element* element, ExceptionCode& ec)
> 
> Please ensure that |element| has no shadow or throw an exception.
> At this time we don't support nested shadow.

If element has shadowRoot, the old shadowRoot is simply replaced with new ShadowRoot in Element::setShadowRoot, isn't it?

I guess you mean 'Hosting Multiple-Shadow Subtrees' in the spec by 'nesting', right?
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#nested-shadow-subtrees

I cannot read how to host multiple shadow subtree in the spec.
I guess the following is an intended way to host multiple shadow subtree. But the spec must mention it more clearly.
  var div = document.createElement('div');
  var sr1 = new WebKitShadowRoot(div);
  var sr2 = new WebKitShadowRoot(div);  // (*) sr2 is younger ShadowRoot. sr1 is now older ShadowRoot.

It this is an intended usage, we have to decide the behavior until we support hosting multiple ShadowRoot when (*) happens. I think we have two options:

1. Replace oldShadowRoot with newShadowRoot as the previous patch does it.
2. Reject and raises DOMException.

2) looks safer for me. So I've updated the patch.


> Also, |element| should be always isElementNode().

Done. I've removed isElementNode() check.

[Hayato Ito]

The correct link to (Hosting Multiple Shadow Subtrees) is here:
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#multiple-shadow-subtrees

(In reply to comment #21)
> Thank you for the reviews, guys.
> 
> (In reply to comment #17)
> > OK. But let's add WebKitShadowRoot(undefined). Basically, I guess it is important to clarify (1) the behavior when the argument is missing (it depends on [Optional]), (2) the behavior when the argument is null (it depends on [ConvertNullToNullString]), and (3) the behavior when the argument is undefined (it depends on [ConvertNullOrUndefinedToNullString]).
> 
> Okay, I added 'undefined' case and 'null' case. But should we take care such converting rules of IDL here? Since we declare input parameter as Element, generated code always passes correctly-typed Element* (nullable) object and never passes String object, doesn't it?
> 
> I am afraid that if we add tests for each rule, tests will be tightly coupled with a particular IDL implementation. I mean even if IDL adds a new rule, such as 'NumberZeroToEmptyString', in the future, we don't want to update all existing JavaScript API tests to test for '0', I guess.
> 
> 
> (In reply to comment #18)
> > (From update of attachment 123924 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=123924&action=review
> > 
> > > Source/WebCore/dom/ShadowRoot.cpp:65
> > > +    if (!element->document()) {
> > > +        ec = WRONG_DOCUMENT_ERR;
> > > +        return 0;
> > > +    }
> > 
> > Are you sure this can happen?  Do you have a test that demonstrates it?
> 
> Done. Never happens as per Node::document()'s commet. We can assume that Element is never DocmentType node.
> 
> (In reply to comment #19)
> > (From update of attachment 123924 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=123924&action=review
> > 
> > > Source/WebCore/dom/ShadowRoot.cpp:56
> > > +PassRefPtr<ShadowRoot> ShadowRoot::create(Element* element, ExceptionCode& ec)
> > 
> > Please ensure that |element| has no shadow or throw an exception.
> > At this time we don't support nested shadow.
> 
> If element has shadowRoot, the old shadowRoot is simply replaced with new ShadowRoot in Element::setShadowRoot, isn't it?
> 
> I guess you mean 'Hosting Multiple-Shadow Subtrees' in the spec by 'nesting', right?
> https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#nested-shadow-subtrees
> 
> I cannot read how to host multiple shadow subtree in the spec.
> I guess the following is an intended way to host multiple shadow subtree. But the spec must mention it more clearly.
>   var div = document.createElement('div');
>   var sr1 = new WebKitShadowRoot(div);
>   var sr2 = new WebKitShadowRoot(div);  // (*) sr2 is younger ShadowRoot. sr1 is now older ShadowRoot.
> 
> It this is an intended usage, we have to decide the behavior until we support hosting multiple ShadowRoot when (*) happens. I think we have two options:
> 
> 1. Replace oldShadowRoot with newShadowRoot as the previous patch does it.
> 2. Reject and raises DOMException.
> 
> 2) looks safer for me. So I've updated the patch.
> 
> 
> > Also, |element| should be always isElementNode().
> 
> Done. I've removed isElementNode() check.

[Hayato Ito]

Created attachment 124063 [details]
makes sure no side-effects

[Kentaro Hara]

(In reply to comment #21)
> (In reply to comment #17)
> > OK. But let's add WebKitShadowRoot(undefined). Basically, I guess it is important to clarify (1) the behavior when the argument is missing (it depends on [Optional]), (2) the behavior when the argument is null (it depends on [ConvertNullToNullString]), and (3) the behavior when the argument is undefined (it depends on [ConvertNullOrUndefinedToNullString]).
> 
> Okay, I added 'undefined' case and 'null' case. But should we take care such converting rules of IDL here? Since we declare input parameter as Element, generated code always passes correctly-typed Element* (nullable) object and never passes String object, doesn't it?

Ah, yes, right, sorry. As you mentioned before, a missing argument, null and 1 would be enough test cases. Thank you very much for the clarification!

[Dimitri Glazkov (Google)]

(In reply to comment #24)
> (In reply to comment #21)
> > (In reply to comment #17)
> > > OK. But let's add WebKitShadowRoot(undefined). Basically, I guess it is important to clarify (1) the behavior when the argument is missing (it depends on [Optional]), (2) the behavior when the argument is null (it depends on [ConvertNullToNullString]), and (3) the behavior when the argument is undefined (it depends on [ConvertNullOrUndefinedToNullString]).
> > 
> > Okay, I added 'undefined' case and 'null' case. But should we take care such converting rules of IDL here? Since we declare input parameter as Element, generated code always passes correctly-typed Element* (nullable) object and never passes String object, doesn't it?
> 
> Ah, yes, right, sorry. As you mentioned before, a missing argument, null and 1 would be enough test cases. Thank you very much for the clarification!

Haraken, you wanna give your new reviewership a try? :)

[Hajime Morrita]

(In reply to comment #21)
> 1. Replace oldShadowRoot with newShadowRoot as the previous patch does it.
> 2. Reject and raises DOMException.
> 
> 2) looks safer for me. So I've updated the patch.

I think same. Replacing existing shadow will break stuff, especially build-in shadows like one of <input>.

[Hajime Morrita]

Comment on attachment 124063 [details]
makes sure no side-effects

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

> Source/WebCore/dom/ShadowRoot.cpp:65
> +        return 0;

Could you add a test for this path. I guess creating shadow from different document (typically from another frame) would raise an exception.

[Hayato Ito]

Thank you for the review.

(In reply to comment #27)
> (From update of attachment 124063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124063&action=review
> 
> > Source/WebCore/dom/ShadowRoot.cpp:65
> > +        return 0;
> 
> Could you add a test for this path. I guess creating shadow from different document (typically from another frame) would raise an exception.

That never happens. See the previous comment for the reason I added this:
https://bugs.webkit.org/show_bug.cgi?id=76354#c11

Since we use host's document in creating ShadowRoot, host->document() and shadowRoot->document() is always same.

I guess you mean that JavaScript running in outer iframe context calls 'new ShadowRoot(elementInInnerIframe)', right?
We don't have clear spec for that case, right? In current implementation, we don't have a logic to reject such a case.

We might want to file a bug for the spec for clarity.

[Hajime Morrita]

(In reply to comment #28)
> Thank you for the review.
> 
> (In reply to comment #27)
> > (From update of attachment 124063 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=124063&action=review
> > 
> > > Source/WebCore/dom/ShadowRoot.cpp:65
> > > +        return 0;
> > 
> > Could you add a test for this path. I guess creating shadow from different document (typically from another frame) would raise an exception.
> 
> That never happens. See the previous comment for the reason I added this:
> https://bugs.webkit.org/show_bug.cgi?id=76354#c11
Ah, got it.

[Hajime Morrita]

Comment on attachment 124063 [details]
makes sure no side-effects

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

> LayoutTests/ChangeLog:1
> [phone]  Hayato Ito  <[e-mail]>

We need to skip this for non-Chromium ports.

[Hayato Ito]

fast/dom/shadow/shadow-root-js-api.html is already skipped on non-chromium ports in https://bugs.webkit.org/show_bug.cgi?id=76353. Is there any mission ports?

EWS looks good so let me try cq+.

(In reply to comment #30)
> (From update of attachment 124063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124063&action=review
> 
> > LayoutTests/ChangeLog:1
> > [phone]  Hayato Ito  <[e-mail]>
> 
> We need to skip this for non-Chromium ports.

[WebKit Review Bot]

Comment on attachment 124063 [details]
makes sure no side-effects

Clearing flags on attachment: 124063

Committed r106208: <http://trac.webkit.org/changeset/106208>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.