Bug 76863

Summary:

ENABLE_SHADOW_DOM should be available via build-webkit --shadow-dom

Product:

WebKit

Reporter:

Hajime Morrita <morrita>

Component:

DOM

Assignee:

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:

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

Hajime Morrita

Reported 2012-01-23 14:42:24 PST

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

Attachments
WIP (22.78 KB, patch) 2012-01-23 15:48 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
WIP v2. Added Qt and CMake flags (28.16 KB, patch) 2012-01-24 09:49 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (32.42 KB, patch) 2012-01-24 14:39 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Took care of vsprops files (40.08 KB, patch) 2012-01-24 17:00 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch for landing (42.01 KB, patch) 2012-01-25 09:08 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch for landing (42.08 KB, patch) 2012-01-25 11:00 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Hajime Morrita

Comment 1 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

Hajime Morrita

Comment 2 2012-01-23 15:48:26 PST

Created attachment 123642 [details] WIP

Hajime Morrita

Comment 3 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.

Raphael Kubo da Costa (:rakuco)

Comment 4 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.

Tor Arne Vestbø

Comment 5 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).

Hajime Morrita

Comment 6 2012-01-24 09:49:55 PST

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

Tor Arne Vestbø

Comment 7 2012-01-24 10:18:07 PST

Thanks. Looks fine from a Qt perspective.

Hajime Morrita

Comment 8 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!

WebKit Review Bot

Comment 9 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

Dimitri Glazkov (Google)

Comment 10 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.

Hajime Morrita

Comment 11 2012-01-24 14:39:01 PST

Created attachment 123812 [details] Patch

Hajime Morrita

Comment 12 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?

Dimitri Glazkov (Google)

Comment 13 2012-01-24 15:54:45 PST

Comment on attachment 123812 [details] Patch are you missing the vsprops file changes by any chance?

Hajime Morrita

Comment 14 2012-01-24 17:00:38 PST

Created attachment 123846 [details] Took care of vsprops files

Gustavo Noronha (kov)

Comment 15 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

Hajime Morrita

Comment 16 2012-01-25 09:08:37 PST

Created attachment 123950 [details] Patch for landing

WebKit Review Bot

Comment 17 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

Hajime Morrita

Comment 18 2012-01-25 11:00:22 PST

Created attachment 123975 [details] Patch for landing

WebKit Review Bot

Comment 19 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>

WebKit Review Bot

Comment 20 2012-01-25 13:47:13 PST

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 21 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. :-/

Nikolas Zimmermann

Comment 22 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.

Nikolas Zimmermann

Comment 23 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.

Hajime Morrita

Comment 24 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,

Hajime Morrita

Comment 25 2012-01-26 08:57:24 PST

And Zimmermann, thanks much for your kind follow up.

Nikolas Zimmermann

Comment 26 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.

Note You need to log in before you can comment on or make changes to this bug.