Bug 100079 – [chromium] use relative paths for includes in the TestRunner's public API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] use relative paths for includes in the TestRunner's public API

Created attachment 170068 [details]
Patch

[Adam Barth]

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.

[James Robinson]

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/

[James Robinson]

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

[Adam Barth]

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.

[Adam Barth]

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?

[Adam Barth]

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

[James Robinson]

Comment on attachment 170255 [details]
Patch

Looks good to me!

Comment on attachment 170255 [details]
Patch

James ack'd via chat

[WebKit Review Bot]

Comment on attachment 170255 [details]
Patch

Clearing flags on attachment: 170255

Committed r132370: <http://trac.webkit.org/changeset/132370>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.