Bug 57722 – Split out UnicodeBidi enum into its own header (to allow use in platform)

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Split out UnicodeBidi enum into its own header (to allow use in platform)

[Eric Seidel (no email)]

Created attachment 88002 [details]
Patch

[Adam Barth]

Comment on attachment 88002 [details]
Patch

What about all the other build systems?

[Eric Seidel (no email)]

It's a header.  XCode is the only crazy one which wants headers listed.  Right?

[Adam Barth]

Nope.  WebCore.gypi lists headers.  One of GTK or Qt does as well.  As does visual studio.

[Adam Barth]

Oh, an CMake, maybe.  :)

[Eric Seidel (no email)]

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 on attachment 88002 [details]
Patch

You'll get EWS results without the R?

[Eric Seidel (no email)]

Really?  I thought the EWS bots will ignore r- patches.  It's silly our little game here...

[WebKit Review Bot]

Attachment 88002 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8321690

[Ryosuke Niwa]

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)]

Created attachment 88004 [details]
Patch

[Eric Seidel (no email)]

Turns out this change is going to be needed for my patch, so I did the work.

[Ryosuke Niwa]

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)]

Created attachment 88005 [details]
Patch

[Early Warning System Bot]

Attachment 88004 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8321698

[Eric Seidel (no email)]

Created attachment 88006 [details]
Patch for landing

[WebKit Commit Bot]

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]

Attachment 88005 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8328018

[Adam Barth]

Comment on attachment 88006 [details]
Patch for landing

You didn't change the visual studio project files.

[Eric Seidel (no email)]

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]

(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)]

(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]

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)]

(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]

> 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)]

I think you guys should send each other <3s. Your bickering is breaking mine :)

[Adam Barth]

> 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)]

http://trac.webkit.org/changeset/82828