Bulk fixes to get Mac Test Shell more compile-happy.

Bulk fixes to get Mac Test Shell more compile-happy.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=1909

[Avi (use Gerrit), 2008-09-08 18:28:11]

What you've all been waiting for...

[pink (ping after 24hrs), 2008-09-08 18:54:52]

LGTM, agree with evan on some TODOs.

http://codereview.chromium.org/1818/diff/1/2
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/1818/diff/1/2#newcode85
Line 85: timebase_info.numer / timebase_info.denom;
indent?

[Amanda Walker, 2008-09-08 18:55:02]

As long as you didn't intend to include the mac implementations of WebViewHost
and WebWidgetHost, LGTM

[cesar.alaniz, 2008-09-08 19:17:07]

http://codereview.chromium.org/1818/diff/1/8
File webkit/tools/test_shell/webview_host.cc (right):

http://codereview.chromium.org/1818/diff/1/8#newcode17
Line 17: WebViewHost* WebViewHost::Create(HWND parent_window, WebViewDelegate*
delegate,
Should the first parameter be gfx::WindowHandle?

http://codereview.chromium.org/1818/diff/1/6
File webkit/tools/test_shell/webwidget_host.cc (right):

http://codereview.chromium.org/1818/diff/1/6#newcode17
Line 17: WebWidgetHost* WebWidgetHost::Create(HWND parent_window,
WebWidgetDelegate* delegate) {
Shouldnt the first parameter be gfx::WindowHandle unstead of HWND?

[Mark Mentovai, 2008-09-08 19:39:32]

This isn't a complete review, but I wanted to piggyback on one of pink's
comments.

http://codereview.chromium.org/1818/diff/1/2
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/1818/diff/1/2#newcode68
Line 68: #if defined(OS_WIN)
I'd really rather call into base's TimeTicks::Now here than redo the ifdefs.

[Avi (use Gerrit), 2008-09-09 14:20:57]

Changed; please tell me what you think.

http://codereview.chromium.org/1818/diff/1/2
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/1818/diff/1/2#newcode68
Line 68: #if defined(OS_WIN)
On 2008/09/08 19:39:33, Mark Mentovai wrote:
> I'd really rather call into base's TimeTicks::Now here than redo the ifdefs.

Sounds like a good idea, but it's gonna get ugly.

[Mark Mentovai, 2008-09-09 14:27:15]

http://codereview.chromium.org/1818/diff/1/2
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/1818/diff/1/2#newcode68
Line 68: #if defined(OS_WIN)
Avi wrote:
> On 2008/09/08 19:39:33, Mark Mentovai wrote:
> > I'd really rather call into base's TimeTicks::Now here than redo the ifdefs.
> 
> Sounds like a good idea, but it's gonna get ugly.

How is it ugly?

#include "base/time.h"

double GetCurrentEventTimeSec() {
  return (TimeTicks::Now() + time_offset_ms) / 1000.0;
}

[Avi (use Gerrit), 2008-09-09 14:33:39]

On 2008/09/09 14:27:15, Mark Mentovai wrote:
> double GetCurrentEventTimeSec() {
>   return (TimeTicks::Now() + time_offset_ms) / 1000.0;
> }

TimeTicks::Now() returns a TimeTicks. There's _no_way_ to get an actual number
out of there short of what I did with TimeDelta.

[Mark Mentovai, 2008-09-09 17:34:51]

LGTM, nice job.

http://codereview.chromium.org/1818/diff/76/221
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/1818/diff/76/221#newcode19
Line 19: #if defined(OS_WIN)
See comments later on about an early include of base/basictypes.h.

http://codereview.chromium.org/1818/diff/76/221#newcode70
Line 70: return (TimeTicks::Now().ToInternalValue() /
kMicrosecondsPerMillisecond +
Time::kMicrosecondsPerMillisecond and watch the 80 when you do this.

http://codereview.chromium.org/1818/diff/76/221#newcode350
Line 350: // reference I'm going by is http://www.meandmark.com/keycodes.html .
Yeah, doesn't this suck?  I usually go by
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/mac/nsMacEvent...
.

http://codereview.chromium.org/1818/diff/76/222
File webkit/tools/test_shell/event_sending_controller.h (right):

http://codereview.chromium.org/1818/diff/76/222#newcode19
Line 19: #include "build/build_config.h"
You don't need to separate this from the other guys, you can stick it into the
next header section.

http://codereview.chromium.org/1818/diff/76/223
File webkit/tools/test_shell/test_shell.h (right):

http://codereview.chromium.org/1818/diff/76/223#newcode165
Line 165: #if defined(OS_WIN)
It's not obvious that these macros are defined, can you #include
"base/basictypes.h" at the top?

http://codereview.chromium.org/1818/diff/76/228
File webkit/tools/test_shell/webview_host.h (right):

http://codereview.chromium.org/1818/diff/76/228#newcode30
Line 30: #if defined(OS_WIN)
base/basictypes.h here too.

http://codereview.chromium.org/1818/diff/76/229
File webkit/tools/test_shell/webwidget_host.h (right):

http://codereview.chromium.org/1818/diff/76/229#newcode30
Line 30: #if defined(OS_MACOSX)
base/basictypes.h here too please.

http://codereview.chromium.org/1818/diff/76/229#newcode63
Line 63: // These need to be called from a non-subclass, so they need to be
public.
...or can we make friends?

http://codereview.chromium.org/1818/diff/76/229#newcode64
Line 64: public:
One-space indent

http://codereview.chromium.org/1818/diff/76/229#newcode71
Line 71: protected:
One-space indent

[Avi (use Gerrit), 2008-09-09 18:22:06]

http://codereview.chromium.org/1818/diff/76/229
File webkit/tools/test_shell/webwidget_host.h (right):

http://codereview.chromium.org/1818/diff/76/229#newcode63
Line 63: // These need to be called from a non-subclass, so they need to be
public.
On 2008/09/09 17:34:51, Mark Mentovai wrote:
> ...or can we make friends?

As much as I hate to argue against Sesame Street, I'm not leaning towards
friendship here. Using inheritance here like Windows does feels unnecessarily
restrictive. They can inherit, make the native view be a subclass and get away
with things. With our view being ObjC, we need to go with "has a", and I think
this is a reasonably-clean interface.