Bug 91311 - [Chromium] Move GamepadController into TestRunner.a
Summary: [Chromium] Move GamepadController into TestRunner.a
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: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 91308
  Show dependency treegraph
 
Reported: 2012-07-13 22:13 PDT by Adam Barth
Modified: 2012-07-16 17:43 PDT (History)
4 users (show)

See Also:


Attachments
Patch (106.47 KB, patch)
2012-07-13 22:16 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (105.77 KB, patch)
2012-07-16 15:57 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-07-13 22:13:50 PDT
[Chromium] Move GamepadController into TestRunner.a
Comment 1 Adam Barth 2012-07-13 22:16:19 PDT
Created attachment 152407 [details]
Patch
Comment 2 jochen 2012-07-14 13:39:08 PDT
Comment on attachment 152407 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestRunner/GamepadController.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

nit 2012 (and other files as well)

> Tools/DumpRenderTree/chromium/TestRunner/GamepadController.cpp:75
> +    webkit_support::SetGamepadData(internalData);

This won't work with content_shell, so ideally, we would have a delegate for setting the gamepad data.

> Tools/DumpRenderTree/chromium/TestRunner/GamepadController.cpp:180
> +           "GamepadController\n");

i'm not sure whether stdout/stderr is correctly captured from renderers across all platforms. We might want to have some kind of printf on the delegate as well, wdyt?
Comment 3 Adam Barth 2012-07-14 15:45:23 PDT
(In reply to comment #2)
> (From update of attachment 152407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152407&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/GamepadController.cpp:2
> > + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> nit 2012 (and other files as well)

I'm not changing any of these files.  WebKit policy is not to update the copyright.

> > Tools/DumpRenderTree/chromium/TestRunner/GamepadController.cpp:75
> > +    webkit_support::SetGamepadData(internalData);
> 
> This won't work with content_shell, so ideally, we would have a delegate for setting the gamepad data.

That's a problem to solve once we have this code linked into ContentShell.

> > Tools/DumpRenderTree/chromium/TestRunner/GamepadController.cpp:180
> > +           "GamepadController\n");
> 
> i'm not sure whether stdout/stderr is correctly captured from renderers across all platforms. We might want to have some kind of printf on the delegate as well, wdyt?

I don't think we should change any of this yet.  These are problems to solve in the future when we're smarter.
Comment 4 jochen 2012-07-15 04:15:30 PDT
Comment on attachment 152407 [details]
Patch

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

>>> Tools/DumpRenderTree/chromium/TestRunner/GamepadController.cpp:75
>>> +    webkit_support::SetGamepadData(internalData);
>> 
>> This won't work with content_shell, so ideally, we would have a delegate for setting the gamepad data.
> 
> That's a problem to solve once we have this code linked into ContentShell.

Ok. Although I'm still curious to see how you intend to make the delegate interface look and where you hook it up.

Btw, by "won't work" I meant segfault
Comment 5 Adam Barth 2012-07-15 09:54:28 PDT
(In reply to comment #4)
> (From update of attachment 152407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152407&action=review
> 
> >>> Tools/DumpRenderTree/chromium/TestRunner/GamepadController.cpp:75
> >>> +    webkit_support::SetGamepadData(internalData);
> >> 
> >> This won't work with content_shell, so ideally, we would have a delegate for setting the gamepad data.
> > 
> > That's a problem to solve once we have this code linked into ContentShell.
> 
> Ok. Although I'm still curious to see how you intend to make the delegate interface look and where you hook it up.

I'm curious too!  We'll see when we get there.  :)

> Btw, by "won't work" I meant segfault

webkit_support does a bunch of work for LayoutTestController and friends.  We should probably look at all that stuff to see what all we need.

I can continue to work on my branch, but this patch is blocking further progress on trunk.
Comment 6 jochen 2012-07-15 10:39:14 PDT
sure, no need to block this patch, it looks good
Comment 7 Tony Chang 2012-07-16 10:04:59 PDT
Comment on attachment 152407 [details]
Patch

Please land via the cq or get an EWS run that applies.
Comment 8 Adam Barth 2012-07-16 15:57:52 PDT
Created attachment 152632 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-07-16 17:43:37 PDT
Comment on attachment 152632 [details]
Patch for landing

Clearing flags on attachment: 152632

Committed r122783: <http://trac.webkit.org/changeset/122783>
Comment 10 WebKit Review Bot 2012-07-16 17:43:42 PDT
All reviewed patches have been landed.  Closing bug.