RESOLVED FIXED 57722
Split out UnicodeBidi enum into its own header (to allow use in platform)
https://bugs.webkit.org/show_bug.cgi?id=57722
Summary Split out UnicodeBidi enum into its own header (to allow use in platform)
Eric Seidel (no email)
Reported 2011-04-03 01:09:50 PDT
Split out UnicodeBidi enum into its own header (to allow use in platform)
Attachments
Patch (7.84 KB, patch)
2011-04-03 01:12 PDT, Eric Seidel (no email)
no flags
Patch (9.72 KB, patch)
2011-04-03 04:23 PDT, Eric Seidel (no email)
no flags
Patch (9.73 KB, patch)
2011-04-03 04:30 PDT, Eric Seidel (no email)
no flags
Patch for landing (9.84 KB, patch)
2011-04-03 05:08 PDT, Eric Seidel (no email)
abarth: review-
commit-queue: commit-queue-
Eric Seidel (no email)
Comment 1 2011-04-03 01:12:40 PDT
Adam Barth
Comment 2 2011-04-03 01:18:01 PDT
Comment on attachment 88002 [details] Patch What about all the other build systems?
Eric Seidel (no email)
Comment 3 2011-04-03 01:20:15 PDT
It's a header. XCode is the only crazy one which wants headers listed. Right?
Adam Barth
Comment 4 2011-04-03 01:21:45 PDT
Nope. WebCore.gypi lists headers. One of GTK or Qt does as well. As does visual studio.
Adam Barth
Comment 5 2011-04-03 01:22:05 PDT
Oh, an CMake, maybe. :)
Eric Seidel (no email)
Comment 6 2011-04-03 01:23:54 PDT
Comment on attachment 88002 [details] Patch How about I just throw this change away. Adding a file ain't worth my time. I still want to see the EWS results.
Adam Barth
Comment 7 2011-04-03 01:33:34 PDT
Comment on attachment 88002 [details] Patch You'll get EWS results without the R?
Eric Seidel (no email)
Comment 8 2011-04-03 01:51:36 PDT
Really? I thought the EWS bots will ignore r- patches. It's silly our little game here...
WebKit Review Bot
Comment 9 2011-04-03 03:45:04 PDT
Ryosuke Niwa
Comment 10 2011-04-03 03:46:23 PDT
Comment on attachment 88002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88002&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22772 > + A863E2011343412000274926 /* UnicodeBidi.h in Headers */, Please sort it lexicologically.
Eric Seidel (no email)
Comment 11 2011-04-03 04:23:21 PDT
Eric Seidel (no email)
Comment 12 2011-04-03 04:23:37 PDT
Turns out this change is going to be needed for my patch, so I did the work.
Ryosuke Niwa
Comment 13 2011-04-03 04:25:43 PDT
Comment on attachment 88004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88004&action=review > Source/WebCore/WebCore.pro:1967 > + platform/text/UnicodeBidi.h You're missing \ here. > Source/WebCore/platform/text/UnicodeBidi.h:14 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE I don't think this is right copyright information. But it seems odd for Google to claim the copyright if we're merely copying it here. Who added this enum originally?
Eric Seidel (no email)
Comment 14 2011-04-03 04:30:21 PDT
Early Warning System Bot
Comment 15 2011-04-03 04:33:43 PDT
Eric Seidel (no email)
Comment 16 2011-04-03 05:08:46 PDT
Created attachment 88006 [details] Patch for landing
WebKit Commit Bot
Comment 17 2011-04-03 05:11:42 PDT
Comment on attachment 88006 [details] Patch for landing Rejecting attachment 88006 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2 Last 500 characters of output: efile.am patching file Source/WebCore/WebCore.gypi patching file Source/WebCore/WebCore.pro patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj patching file Source/WebCore/css/CSSPrimitiveValueMappings.h patching file Source/WebCore/platform/text/UnicodeBidi.h patching file Source/WebCore/rendering/style/RenderStyle.h patching file Source/WebCore/rendering/style/RenderStyleConstants.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8321708
WebKit Review Bot
Comment 18 2011-04-03 07:10:24 PDT
Adam Barth
Comment 19 2011-04-03 11:38:18 PDT
Comment on attachment 88006 [details] Patch for landing You didn't change the visual studio project files.
Eric Seidel (no email)
Comment 20 2011-04-03 11:42:38 PDT
You're just the build system nazi today, eh? The win-ews seems to like the build, and you seem to like making my life difficult.
Adam Barth
Comment 21 2011-04-03 12:13:53 PDT
(In reply to comment #20) > You're just the build system nazi today, eh? The win-ews seems to like the build, and you seem to like making my life difficult. Your change is not correct. The Visual Studio project files contain the headers. They're not required for the build to work, but they are used by the folks who use those project files. The build system is making your life difficult, not me. I'm just asking for your patch to be correct.
Eric Seidel (no email)
Comment 22 2011-04-03 12:20:16 PDT
(In reply to comment #21) > (In reply to comment #20) > > You're just the build system nazi today, eh? The win-ews seems to like the build, and you seem to like making my life difficult. > > Your change is not correct. The Visual Studio project files contain the headers. They're not required for the build to work, but they are used by the folks who use those project files. > > The build system is making your life difficult, not me. I'm just asking for your patch to be correct. I understand that I'm shooting the messenger here. The messenger is being needlessly pedantic. This will eventually be the first patch to ever correctly edit every build system, since I don't think we've ever actually seen one of those in WebKit. :)
Adam Barth
Comment 23 2011-04-03 12:28:22 PDT
I think you're just upset, and I'm not really sure why. I agree that adding files to the project is too hard, but that's not a reason to cut corners.
Eric Seidel (no email)
Comment 24 2011-04-03 12:32:03 PDT
(In reply to comment #23) > I think you're just upset, and I'm not really sure why. I agree that adding files to the project is too hard, but that's not a reason to cut corners. I am upset. :) But not unreasonably or irrationally so. This patch moves an enum to a single file. The first patch I posted compiled on *all* build systems, yet I'm up to patch 5 due to build system nits and copyright complaints. I will post an updated patch at some point. This bug is simply a stellar example of broken process.
Adam Barth
Comment 25 2011-04-03 12:37:15 PDT
> This bug is simply a stellar example of broken process. The overhead of dealing with our N build systems is most visible for trivial changes because virtually all the work you're doing is the overhead. However, that work is there for many other patches as well, which is why we need to fix the build system. Just because your patch builds doesn't make it correct. Would you want someone making as mess of a part of WebKit that you care about in the name of expediency? Your tone makes it sound like you don't care about the folks using non-Xcode project files.
Dimitri Glazkov (Google)
Comment 26 2011-04-03 12:42:07 PDT
I think you guys should send each other <3s. Your bickering is breaking mine :)
Adam Barth
Comment 27 2011-04-03 12:51:02 PDT
> I think you guys should send each other <3s. Your bickering is breaking mine :) Eric knows that I <3 him. Please don't let us break your heart!
Eric Seidel (no email)
Comment 28 2011-04-07 10:30:32 PDT
Note You need to log in before you can comment on or make changes to this bug.