Bug 76863 – ENABLE_SHADOW_DOM should be available via build-webkit --shadow-dom

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

[Hajime Morrita]

Shadow DOM change should be available for all platforms behind the flag.

[Hajime Morrita]

As a side note, here is for chromium: http://code.google.com/p/chromium/issues/detail?id=111112

[Hajime Morrita]

Created attachment 123642 [details]
WIP

[Hajime Morrita]

This patch intentionally ENABLE_SHADOW_DOM to see it can be built. Land-able version would disable it by default.

[Raphael Kubo da Costa (:rakuco)]

Note that for CMake-based ports you should add the respective WEBKIT_FEATURE() call in Source/cmake/Options{Efl,Blackberry,WinCE}.cmake.

[Tor Arne Vestbø]

Please add the corresponding update to Tools/qmake/mkspecs/features/features.prf (for the Qt port), and update build-webkit (adding it to the list of enable/disable-options).

[Hajime Morrita]

Created attachment 123756 [details]
WIP v2. Added Qt and CMake flags

[Tor Arne Vestbø]

Thanks. Looks fine from a Qt perspective.

[Hajime Morrita]

(In reply to comment #7)
> Thanks. Looks fine from a Qt perspective.
Thank you too for your feedback!

[WebKit Review Bot]

Comment on attachment 123756 [details]
WIP v2. Added Qt and CMake flags

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

New failing tests:
media/audio-garbage-collect.html

[Dimitri Glazkov (Google)]

Comment on attachment 123756 [details]
WIP v2. Added Qt and CMake flags

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

Use this commit as an example: http://trac.webkit.org/changeset/101323

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:109
> +ENABLE_SHADOW_DOM = ENABLE_SHADOW_DOM;

This means "on" by default when building from XCode.

> Source/WebCore/Configurations/FeatureDefines.xcconfig:109
> +ENABLE_SHADOW_DOM = ENABLE_SHADOW_DOM;

Ditto.

> Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:109
> +ENABLE_SHADOW_DOM = ENABLE_SHADOW_DOM;

Ditto.

> Source/WebKit2/Configurations/FeatureDefines.xcconfig:109
> +ENABLE_SHADOW_DOM = ENABLE_SHADOW_DOM;

Ditto.

> Tools/Scripts/build-webkit:301
> +      define => "ENABLE_SHADOW_DOM", default => 1, value => \$shadowDomSupport },

Again, this is "on" by default.

[Hajime Morrita]

Created attachment 123812 [details]
Patch

[Hajime Morrita]

Dimitri, thanks for taking a look!
I shoul've give --no-review for the last patch - It had wrong flags, no ChangeLogs.
But this new one is intend to be ready for your review.
Could you take a look again?

[Dimitri Glazkov (Google)]

Comment on attachment 123812 [details]
Patch

are you missing the vsprops file changes by any chance?

[Hajime Morrita]

Created attachment 123846 [details]
Took care of vsprops files

[Gustavo Noronha (kov)]

Comment on attachment 123846 [details]
Took care of vsprops files

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

[Hajime Morrita]

Created attachment 123950 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 123950 [details]
Patch for landing

Rejecting attachment 123950 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11344587

[Hajime Morrita]

Created attachment 123975 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 123975 [details]
Patch for landing

Clearing flags on attachment: 123975

Committed r105922: <http://trac.webkit.org/changeset/105922>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen because it broke the GTK build. GTK EWS could have report this breakage, but you didn't wait for it, but commit the patch and leave the build broken.
Breaking a build and leaving it broken isn't a fair play game. :-/

[Nikolas Zimmermann]

(In reply to comment #21)
> Reopen because it broke the GTK build. GTK EWS could have report this breakage, but you didn't wait for it, but commit the patch and leave the build broken.
> Breaking a build and leaving it broken isn't a fair play game. :-/

I admit, I have to share Ossys concerns. When I woke up today, Gtk/Win were not building, all failing with JSShadowRoot related changes. I wonder why you didn't wait for the EWS for a patch like this?

Anyhow, I've fixed Qt-win/Gtk builds in r105975. Next is windows.

[Nikolas Zimmermann]

 <UserMacro
		Name="ENABLE_SHADOW_DOM"
		Value="ENABLE_SHADOW_DOM"
		PerformEnvironmentSet="true"
	/>

This enabled the feature by default, I changed this to Value="" in r105976.
Closing this bug, next time please be more careful watching the bots and/or using EWS extensively.

[Hajime Morrita]

Ossy, Zimmermann, 
You are totally right. I apologize my lack of presence. 
I should've been there, or I shouldn't have landed patch when I cannot be available.
Especially for this kind of complex change.

Regards,

[Hajime Morrita]

And Zimmermann, thanks much for your kind follow up.

[Nikolas Zimmermann]

(In reply to comment #25)
> And Zimmermann, thanks much for your kind follow up.
Heh, no worries, it happens to any of us at some point :-) You're welcome.