WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(9.72 KB, patch)
2011-04-03 04:23 PDT
,
Eric Seidel (no email)
no flags
Details
Formatted Diff
Diff
Patch
(9.73 KB, patch)
2011-04-03 04:30 PDT
,
Eric Seidel (no email)
no flags
Details
Formatted Diff
Diff
Patch for landing
(9.84 KB, patch)
2011-04-03 05:08 PDT
,
Eric Seidel (no email)
abarth
: review-
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
2011-04-03 01:12:40 PDT
Created
attachment 88002
[details]
Patch
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
Attachment 88002
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/8321690
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
Created
attachment 88004
[details]
Patch
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
Created
attachment 88005
[details]
Patch
Early Warning System Bot
Comment 15
2011-04-03 04:33:43 PDT
Attachment 88004
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8321698
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
Attachment 88005
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/8328018
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
http://trac.webkit.org/changeset/82828
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug