Summary: | [chromium] use relative paths for includes in the TestRunner's public API | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||
Component: | New Bugs | Assignee: | jochen | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, jamesr, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
jochen
2012-10-22 23:19:42 PDT
Created attachment 170068 [details]
Patch
Comment on attachment 170068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170068&action=review > Tools/DumpRenderTree/chromium/TestRunner/public/WebEventSender.h:34 > -#include "WebDragOperation.h" > +#include "../../../../../Source/WebKit/chromium/public/WebDragOperation.h" Yuck. I think we should just add "Source" to the include path and be able to include these headers as: #include "WebKit/chromium/public/WebDragOperation.h" That would also solve our problem of including Platform/chromium/public headers in WebKit/chromium/public. Comment on attachment 170068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170068&action=review > Tools/ChangeLog:8 > + When including the headers from chromium, we need to specify the full path. Gross! And not really true. What target is including these headers? It almost certainly has quite a few things on the include path already, one of which we could definitely use Comment on attachment 170068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170068&action=review >> Tools/ChangeLog:8 >> + When including the headers from chromium, we need to specify the full path. > > Gross! And not really true. What target is including these headers? It almost certainly has quite a few things on the include path already, one of which we could definitely use content_shell_lib in chromium (i can post a patch, it's not yet landed) Note that the headers in e.g. Source/WebKit/chromium/public/platform have similar includes. (In reply to comment #4) > (From update of attachment 170068 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170068&action=review > > >> Tools/ChangeLog:8 > >> + When including the headers from chromium, we need to specify the full path. > > > > Gross! And not really true. What target is including these headers? It almost certainly has quite a few things on the include path already, one of which we could definitely use > > content_shell_lib in chromium (i can post a patch, it's not yet landed) > > Note that the headers in e.g. Source/WebKit/chromium/public/platform have similar includes. https://codereview.chromium.org/11245004/ Can you make the DumpRenderTree.gyp:TestRunner target put something reasonable on the include path of direct_dependent_settings? Created attachment 170213 [details]
Patch
(In reply to comment #6) > Can you make the DumpRenderTree.gyp:TestRunner target put something reasonable on the include path of direct_dependent_settings? that seems to be working Comment on attachment 170213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170213&action=review > Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:112 > + ['inside_chromium_build==1', { > + 'direct_dependent_settings': { > + 'include_dirs': [ > + '../../../Source/WebKit/chromium/public', > + ], > + }, > + }], Is this better than adding ../../../Source ? I guess it depends how explicit we want to be about where these headers come from. We eventually want to get rid of the "forwarding" headers in WebKit/chromium/public/platform. We should pick something here that can scale to that case too. Created attachment 170255 [details]
Patch
(In reply to comment #10) > We eventually want to get rid of the "forwarding" headers in WebKit/chromium/public/platform. We should pick something here that can scale to that case too. like this? Comment on attachment 170255 [details]
Patch
I'm happy with that. @jamesr ?
(Jochen, please feel free to land this patch if its blocking you. We can iterate based on jamesr's feedback.)
Comment on attachment 170255 [details]
Patch
Looks good to me!
Comment on attachment 170255 [details]
Patch
James ack'd via chat
Comment on attachment 170255 [details] Patch Clearing flags on attachment: 170255 Committed r132370: <http://trac.webkit.org/changeset/132370> All reviewed patches have been landed. Closing bug. |