Bug 78067 - [chromium] Move geometry headers in Platform API to Platform directory
Summary: [chromium] Move geometry headers in Platform API to Platform directory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 18:53 PST by James Robinson
Modified: 2012-02-08 12:02 PST (History)
6 users (show)

See Also:


Attachments
Patch (55.31 KB, patch)
2012-02-07 20:17 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (62.04 KB, patch)
2012-02-07 20:35 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch for landing (62.00 KB, patch)
2012-02-07 22:20 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2012-02-08 09:28 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-02-07 18:53:13 PST
[chromium] Move geometry headers in Platform API to Platform directory
Comment 1 James Robinson 2012-02-07 20:17:43 PST
Created attachment 125989 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-07 20:22:09 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 James Robinson 2012-02-07 20:35:19 PST
Created attachment 125991 [details]
Patch
Comment 4 WebKit Review Bot 2012-02-07 20:38:39 PST
Attachment 125991 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1

Source/Platform/chromium/src/WebFloatQuad.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Adam Barth 2012-02-07 21:20:45 PST
Comment on attachment 125991 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125991&action=review

We've already talked this over with fishd and we're not actually changing anything in the API.  It's probably safe to land without round-tripping with him.

> Source/Platform/Platform.gyp/copy_webcore_headers.py:37
> +            raise e

You just want to say "raise" without the "e"
Comment 6 James Robinson 2012-02-07 22:12:55 PST
Comment on attachment 125991 [details]
Patch

Whoops missed the comment, will update
Comment 7 James Robinson 2012-02-07 22:20:01 PST
Created attachment 126002 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-02-07 22:56:40 PST
Comment on attachment 126002 [details]
Patch for landing

Clearing flags on attachment: 126002

Committed r107042: <http://trac.webkit.org/changeset/107042>
Comment 9 WebKit Review Bot 2012-02-07 22:56:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Sadrul Habib Chowdhury 2012-02-08 09:28:51 PST
Reopening to attach new patch.
Comment 11 Sadrul Habib Chowdhury 2012-02-08 09:28:54 PST
Created attachment 126101 [details]
Patch
Comment 12 Sadrul Habib Chowdhury 2012-02-08 09:30:39 PST
(In reply to comment #10)
> Reopening to attach new patch.

My bad. I failed webkit-patch
Comment 13 Tony Chang 2012-02-08 10:29:34 PST
Comment on attachment 126002 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=126002&action=review

> Source/Platform/Platform.gyp/Platform.gyp:58
> +                # Since Platform/ can't add WebCore/* to the include path, this build step
> +                # copies these headers into the shared intermediate directory and adds that to the include path.

Copying the headers is OK, but forwarding headers are better.  Sometimes people edit the wrong file by accident when there's a copy.
Comment 14 James Robinson 2012-02-08 10:42:59 PST
(In reply to comment #13)
> (From update of attachment 126002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126002&action=review
> 
> > Source/Platform/Platform.gyp/Platform.gyp:58
> > +                # Since Platform/ can't add WebCore/* to the include path, this build step
> > +                # copies these headers into the shared intermediate directory and adds that to the include path.
> 
> Copying the headers is OK, but forwarding headers are better.  Sometimes people edit the wrong file by accident when there's a copy.

I disagree - forwarding headers run into all sorts of gross issues with relative paths and I really doubt anyone is going to accidentally edit a copy in out/Release/gen/webcore_headers/ instead of the real one.
Comment 15 Tony Chang 2012-02-08 11:44:24 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 126002 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126002&action=review
> > 
> > > Source/Platform/Platform.gyp/Platform.gyp:58
> > > +                # Since Platform/ can't add WebCore/* to the include path, this build step
> > > +                # copies these headers into the shared intermediate directory and adds that to the include path.
> > 
> > Copying the headers is OK, but forwarding headers are better.  Sometimes people edit the wrong file by accident when there's a copy.
> 
> I disagree - forwarding headers run into all sorts of gross issues with relative paths and I really doubt anyone is going to accidentally edit a copy in out/Release/gen/webcore_headers/ instead of the real one.

People who use an IDE just right click on an include an open it.  It's easy to accidentally edit the wrong copy.  It's also not obvious where copies come from.

Here's a previous discussion where it confused people (which is when I switched the third_party webkit headers to use forwarding headers from a copy):
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/97f633fea1369fa/b42a06a0b5deea90?lnk=gst
Comment 16 James Robinson 2012-02-08 11:54:40 PST
That's a little different, in that case both copies were in the source directories. In this case the copy is in the SHARED_INTERMEDIATE_DIR. We should just teach IDEs not to open files in the output directory, if we haven't already.

I don't think anyone is using the xcode UI any more (at least not if they've upgraded to 4.X).
Comment 17 Tony Chang 2012-02-08 12:02:34 PST
(In reply to comment #16)
> That's a little different, in that case both copies were in the source directories. In this case the copy is in the SHARED_INTERMEDIATE_DIR. We should just teach IDEs not to open files in the output directory, if we haven't already.

How do we teach IDEs not to open files in the output directory?

> I don't think anyone is using the xcode UI any more (at least not if they've upgraded to 4.X).

The same happens in MSVS.  If you open a header, it only has the filename, not the full path in the tab.  It's easy to edit the wrong file.