Bug 76863

Summary: ENABLE_SHADOW_DOM should be available via build-webkit --shadow-dom
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, gustavo, hayato, mrobinson, ossy, pnormand, rakuco, vestbo, webkit.review.bot, xan.lopez, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 63601    
Attachments:
Description Flags
WIP
none
WIP v2. Added Qt and CMake flags
none
Patch
none
Took care of vsprops files
none
Patch for landing
none
Patch for landing none

Description Hajime Morrita 2012-01-23 14:42:24 PST
Shadow DOM change should be available for all platforms behind the flag.
Comment 1 Hajime Morrita 2012-01-23 14:46:34 PST
As a side note, here is for chromium: http://code.google.com/p/chromium/issues/detail?id=111112
Comment 2 Hajime Morrita 2012-01-23 15:48:26 PST
Created attachment 123642 [details]
WIP
Comment 3 Hajime Morrita 2012-01-23 15:49:48 PST
This patch intentionally ENABLE_SHADOW_DOM to see it can be built. Land-able version would disable it by default.
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-01-24 05:04:20 PST
Note that for CMake-based ports you should add the respective WEBKIT_FEATURE() call in Source/cmake/Options{Efl,Blackberry,WinCE}.cmake.
Comment 5 Tor Arne Vestbø 2012-01-24 05:08:14 PST
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).
Comment 6 Hajime Morrita 2012-01-24 09:49:55 PST
Created attachment 123756 [details]
WIP v2. Added Qt and CMake flags
Comment 7 Tor Arne Vestbø 2012-01-24 10:18:07 PST
Thanks. Looks fine from a Qt perspective.
Comment 8 Hajime Morrita 2012-01-24 10:22:22 PST
(In reply to comment #7)
> Thanks. Looks fine from a Qt perspective.
Thank you too for your feedback!
Comment 9 WebKit Review Bot 2012-01-24 10:49:14 PST
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
Comment 10 Dimitri Glazkov (Google) 2012-01-24 11:41:41 PST
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.
Comment 11 Hajime Morrita 2012-01-24 14:39:01 PST
Created attachment 123812 [details]
Patch
Comment 12 Hajime Morrita 2012-01-24 14:41:53 PST
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?
Comment 13 Dimitri Glazkov (Google) 2012-01-24 15:54:45 PST
Comment on attachment 123812 [details]
Patch

are you missing the vsprops file changes by any chance?
Comment 14 Hajime Morrita 2012-01-24 17:00:38 PST
Created attachment 123846 [details]
Took care of vsprops files
Comment 15 Gustavo Noronha (kov) 2012-01-25 03:30:45 PST
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
Comment 16 Hajime Morrita 2012-01-25 09:08:37 PST
Created attachment 123950 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-01-25 09:25:54 PST
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
Comment 18 Hajime Morrita 2012-01-25 11:00:22 PST
Created attachment 123975 [details]
Patch for landing
Comment 19 WebKit Review Bot 2012-01-25 13:47:03 PST
Comment on attachment 123975 [details]
Patch for landing

Clearing flags on attachment: 123975

Committed r105922: <http://trac.webkit.org/changeset/105922>
Comment 20 WebKit Review Bot 2012-01-25 13:47:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Csaba Osztrogonác 2012-01-26 00:16:06 PST
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. :-/
Comment 22 Nikolas Zimmermann 2012-01-26 01:23:32 PST
(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.
Comment 23 Nikolas Zimmermann 2012-01-26 01:47:21 PST
 <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.
Comment 24 Hajime Morrita 2012-01-26 08:55:43 PST
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,
Comment 25 Hajime Morrita 2012-01-26 08:57:24 PST
And Zimmermann, thanks much for your kind follow up.
Comment 26 Nikolas Zimmermann 2012-01-26 12:57:23 PST
(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.