Bug 87732 – [EFL] Enable SHADOW_DOM flag

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

A lot of test cases require the shadow dom to be enabled:
  editing/shadow
  fast/dom/shadow

The feature is already enabled by default for GTK port.

[Chris Dumez]

Created attachment 145035 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 145035 [details]
Patch

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

> LayoutTests/platform/efl/test_expectations.txt:97
> +BUGWK86623 : fast/dom/shadow/drop-event-for-input-in-shadow.html = TEXT

I think it is good to list up by alphabetical sorting.

[Chris Dumez]

Created attachment 145291 [details]
Patch

Attempt to make the GTK bot happy.

[Chris Dumez]

Created attachment 145297 [details]
Patch

Simplify patch thanks to dependency bugs.

Does anyone know why it does not build for the GTK port?
I added "JSGenerateToJSObject" extended attribute to ShadowRoot.idl so that a toJS() method is generated for JSShadowRoot. This works fine on EFL port but the GTK port gets a linking error for the new toJS() method. I'm assuming some Makefile needs to be tweaked somehow?

[Hajime Morrita]

I guess code generator for JSC isn't aware of ShadowRoot. 
We need to teach it about ShadowRoot.
(We did it for V8 when we started working on that at trac.webkit.org/changeset/108035.)


Filed Bug 88211.
Haraken, do you have any ideas?

[Kentaro Hara]

(In reply to comment #5)
> I guess code generator for JSC isn't aware of ShadowRoot. 
> We need to teach it about ShadowRoot.
> (We did it for V8 when we started working on that at trac.webkit.org/changeset/108035.)
> 
> 
> Filed Bug 88211.
> Haraken, do you have any ideas?

Do you mean that the corresponding change to this (http://trac.webkit.org/changeset/108035/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm) would be needed in CodeGeneratorJS.pm to support ShadowRoot?

If my understanding is correct, the corresponding change is not needed in CodeGeneratorJS.pm since JS bindings do not distinguish between DOMNode, DOMObject and ActiveDOMObject etc.

[Hajime Morrita]

(In reply to comment #6)
> (In reply to comment #5)
> > I guess code generator for JSC isn't aware of ShadowRoot. 
> > We need to teach it about ShadowRoot.
> > (We did it for V8 when we started working on that at trac.webkit.org/changeset/108035.)
> > 
> > 
> > Filed Bug 88211.
> > Haraken, do you have any ideas?
> 
> Do you mean that the corresponding change to this (http://trac.webkit.org/changeset/108035/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm) would be needed in CodeGeneratorJS.pm to support ShadowRoot?
> 
> If my understanding is correct, the corresponding change is not needed in CodeGeneratorJS.pm since JS bindings do not distinguish between DOMNode, DOMObject and ActiveDOMObject etc.
Good point.
The compilation error is telling us that we need toJS(ShadowRoot*)
or something like that. But I don't understand the logic behind that.

[Chris Dumez]

(In reply to comment #5)
> I guess code generator for JSC isn't aware of ShadowRoot. 
> We need to teach it about ShadowRoot.
> (We did it for V8 when we started working on that at trac.webkit.org/changeset/108035.)
> 
> 
> Filed Bug 88211.
> Haraken, do you have any ideas?

I'm not sure what you mean. The EFL port is using JSC as well and it compiles just fine after the small IDL fix I made. The shadow root tests are passing as well.
The toJS() method for shadow root is generated correctly for EFL port.

[Hajime Morrita]

> 
> I'm not sure what you mean. The EFL port is using JSC as well and it compiles just fine after the small IDL fix I made. The shadow root tests are passing as well.
> The toJS() method for shadow root is generated correctly for EFL port.
Oh really? that's great news!
Then we can just add the missing symbol to the symbols.filter file.
http://trac.webkit.org/browser/trunk/Source/autotools/symbols.filter
We need to figure out the symbols name, though.

[Philippe Normand]

Maybe a clean/full GTK build is required for this patch. It wouldn't be the first time.

[Chris Dumez]

(In reply to comment #10)
> Maybe a clean/full GTK build is required for this patch. It wouldn't be the first time.

Ok, I'll make a local build of the GTK port and make sure this is the issue.

[Chris Dumez]

(In reply to comment #9)
> > 
> > I'm not sure what you mean. The EFL port is using JSC as well and it compiles just fine after the small IDL fix I made. The shadow root tests are passing as well.
> > The toJS() method for shadow root is generated correctly for EFL port.
> Oh really? that's great news!
> Then we can just add the missing symbol to the symbols.filter file.
> http://trac.webkit.org/browser/trunk/Source/autotools/symbols.filter
> We need to figure out the symbols name, though.

Thanks, this was the problem. I'll reupload a patch that should build on GTK port.

[Chris Dumez]

Created attachment 145531 [details]
Patch

This patch fixes the GTK build by adding the right symbol to Source/autotools/symbols.filter. A clean rebuild may still be required though.

[Chris Dumez]

Green GTK bubble, yay :) Now, we just need to land the dependency patch at Bug 88029.

[Chris Dumez]

Created attachment 145577 [details]
Patch

Update test expectations to indicate why remaining Shadow DOM tests are failing on EFL port.

[Gyuyoung Kim]

It seems to me there is no critical problems now. By the way, why don't you also add "WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)" for other ports which are using cmake ?

[Chris Dumez]

(In reply to comment #16)
> It seems to me there is no critical problems now. By the way, why don't you also add "WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)" for other ports which are using cmake ?

Because it is already there:

$ cat Source/cmake/WebKitFeatures.cmake | grep SHADOW_DOM
    WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)

[Gyuyoung Kim]

(In reply to comment #17)
> (In reply to comment #16)
> > It seems to me there is no critical problems now. By the way, why don't you also add "WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)" for other ports which are using cmake ?
> 
> Because it is already there:
> 
> $ cat Source/cmake/WebKitFeatures.cmake | grep SHADOW_DOM
>     WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)

If so, there is no problem. LGTM.

[Chris Dumez]

Created attachment 145973 [details]
Patch

Rebase on master. Could someone review the patch please?

[Chris Dumez]

Created attachment 145984 [details]
Patch

Unskip an additional Shadow DOM test by enabling focus cycling in DRT.

[Gyuyoung Kim]

Comment on attachment 145984 [details]
Patch

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

> Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp:192
> +static Eina_Bool onFocusCanCycle(Ewk_View_Smart_Data *sd, Ewk_Focus_Direction direction)

Style nit : s/sd/smartData/g. Or, you can omit parameter name until it is used.

[Chris Dumez]

Created attachment 146004 [details]
Patch

Take Gyuyoung's feedback into consideration, thanks.

[Martin Robinson]

Comment on attachment 146004 [details]
Patch

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

> LayoutTests/fast/dom/shadow/form-in-shadow-expected.txt:10
> +PASS obj.hidden is 'hidden'
> +PASS obj.text is 'text'
> +PASS obj.checkbox1 is 'on'
> +PASS obj.checkbox2 is undefined.
> +PASS obj.range is '50'
> +PASS obj.textarea is 'textarea'
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +

Should this result go in the EFL directory?

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:199
> +#if PLATFORM(EFL)
> +bool RuntimeEnabledFeatures::isShadowDOMEnabled = true;
> +#else
>  bool RuntimeEnabledFeatures::isShadowDOMEnabled = false;
>  #endif
> +#endif

This seems suspicious.  Is this really something that should be turned on by default for all embedders? Perhaps add a setting in the EFL API and have the DRT flip it?

[Chris Dumez]

Comment on attachment 146004 [details]
Patch

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

>> LayoutTests/fast/dom/shadow/form-in-shadow-expected.txt:10
>> +
> 
> Should this result go in the EFL directory?

I can move it, find by me. I just find it strange that there is no global expectation for this test. The test does not look platform specific.

>> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:199
>> +#endif
> 
> This seems suspicious.  Is this really something that should be turned on by default for all embedders? Perhaps add a setting in the EFL API and have the DRT flip it?

Sounds like a good idea. I'll do that.

[Chris Dumez]

Created attachment 146227 [details]
Patch

Take Martin Robinson's feedback into consideration.

[Gyuyoung Kim]

Comment on attachment 146227 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_settings.cpp:258
> +    return FALSE;

s/FALSE/false/g

> Source/WebKit/efl/ewk/ewk_settings.cpp:268
> +#endif

I think you need to add below code.

#else
    return false;
#endif.

> Source/WebKit/efl/ewk/ewk_settings.h:288
> + * By default, Shadow DOM is disabled.

Do you enable shadow dom in OptionEfl.cmake ?

> Source/WebKit/efl/ewk/ewk_settings.h:303
> + * By default, Shadow DOM is disabled.

ditto.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 146227 [details]
Patch

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

It looks mostly OK except for the missing #else clause Gyuyoung mentioned. I'm not very familiar with the work on Shadow DOM to know whether it makes sense to enable it by default, or even if it should always be enabled. Does it make the most sense to only enable it in DRT by default?

>> Source/WebKit/efl/ewk/ewk_settings.h:288
>> + * By default, Shadow DOM is disabled.
> 
> Do you enable shadow dom in OptionEfl.cmake ?

He enabled the feature at build-time in OptionsEfl.cmake; that's different from calling ewk_settings_shadow_dom_enable_set(EINA_TRUE) from ewk_main.cpp, for example.

[Chris Dumez]

Created attachment 146481 [details]
Patch

- Fix FALSE -> false
- Return false in #else case

Gyuyoung, as rakuco said, the feature is enabled at compile time but disabled by default at runtime.
For now, I'm enabling it only for DRT, as the chromium port is doing.

[Gyuyoung Kim]

(In reply to comment #28)
> Created an attachment (id=146481) [details]
> Patch
> 
> - Fix FALSE -> false
> - Return false in #else case
> 
> Gyuyoung, as rakuco said, the feature is enabled at compile time but disabled by default at runtime.
> For now, I'm enabling it only for DRT, as the chromium port is doing.

If so, ok.

[Martin Robinson]

Comment on attachment 146481 [details]
Patch

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

Looks sane to me, but it'd be good for someone else to double-check the change to the IDL file.

> Source/WebCore/dom/ShadowRoot.idl:33
> -        ConstructorRaisesException
> +        ConstructorRaisesException,
> +        JSGenerateToJSObject

I don't really feel confident enough to review to particular part of the change.

[Chris Dumez]

(In reply to comment #30)
> (From update of attachment 146481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146481&action=review
> 
> Looks sane to me, but it'd be good for someone else to double-check the change to the IDL file.
> 
> > Source/WebCore/dom/ShadowRoot.idl:33
> > -        ConstructorRaisesException
> > +        ConstructorRaisesException,
> > +        JSGenerateToJSObject
> 
> I don't really feel confident enough to review to particular part of the change.

haraken: I believe this is something you could help with?

Without this extra extended attribute, the ShadowRoot object looks like a DocumentFragment (its parent) on JS side and we cannot access its properties / methods.

[Kentaro Hara]

(In reply to comment #31)
> > > Source/WebCore/dom/ShadowRoot.idl:33
> > > -        ConstructorRaisesException
> > > +        ConstructorRaisesException,
> > > +        JSGenerateToJSObject
> > 
> > I don't really feel confident enough to review to particular part of the change.
> 
> haraken: I believe this is something you could help with?
> 
> Without this extra extended attribute, the ShadowRoot object looks like a DocumentFragment (its parent) on JS side and we cannot access its properties / methods.

IDL change looks OK. Since ShadowRoot inherits DocumentFragment, by default toJS() is not generated for ShadowRoot. Consequently, the ShadowRoot object is wrapped by inherited toJS() of DocumentFragment, and thus the object looks like a DocumentFragment object. JSGenerateToJSObject prevents the situation by generating toJS() for ShadowRoot.

Let me mark r+ based on mrobinson's review on other parts.

[WebKit Review Bot]

Comment on attachment 146481 [details]
Patch

Clearing flags on attachment: 146481

Committed r120144: <http://trac.webkit.org/changeset/120144>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

*** Bug 80086 has been marked as a duplicate of this bug. ***