Wire GetWindowRect, GetRootWindowRect, and GetScreenInfo out to the UI thread.

Wire GetWindowRect, GetRootWindowRect, and GetScreenInfo out to the UI thread.

Convert GetScreenInfo to be sync and routed.

http://crbug.com/13113
R=darin@chromium.org, jam@chromium.org, amanda@chromium.org
TEST=See bug.

[Scott Hess - ex-Googler, 2009-07-02 22:50:08]

Same song seventh verse ...

[Scott Hess - ex-Googler, 2009-07-06 19:16:19]

jam suggested I ping...

[Scott Hess - ex-Googler, 2009-07-06 19:18:48]

http://codereview.chromium.org/151130/diff/1003/1017
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/151130/diff/1003/1017#newcode957
Line 957: WebKit::WebScreenInfo /* Out: results */)
Dammit, I'll go kill the Out: without any feedback needed.

[jam, 2009-07-06 19:33:27]

http://codereview.chromium.org/151130/diff/1003/1004
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/151130/diff/1003/1004#newcode645
Line 645: #if !defined(OS_MACOSX)
This should just be an "#if defined(OS_WIN)", see Antoine's change which is
about to land any minute now:
http://codereview.chromium.org/146078/diff/3077/3080

Instead of the comment referring to why certain API calls are needed on a
specific platform, it should just say that this is needed on Windows because
Windows internally does blocking calls to the child HWNDs in the plugin process,
so if the renderer making sync calls to the UI thread a deadlock can occur.

http://codereview.chromium.org/151130/diff/1003/1005
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/151130/diff/1003/1005#newcode29
Line 29: using WebKit::WebScreenInfo;
nit: why not put this with the other using WebKit:: lines below at 41-44?

http://codereview.chromium.org/151130/diff/1003/1006
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/151130/diff/1003/1006#newcode396
Line 396: void OnMsgGetScreenInfo(gfx::NativeViewId view,
Since these functions are only used on mac, what do you think about surrounding
them with #if defined(OS_MACOSX)?  Then you wouldn't need such an ifdef in each
function definition.

[darin (slow to review), 2009-07-06 19:52:47]

sorry for the delay in providing feedback

http://codereview.chromium.org/151130/diff/1003/1004
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/151130/diff/1003/1004#newcode642
Line 642: // Synchronous messages allowed on Mac so that the UI thread can
nit: "messages [are] allowed"

http://codereview.chromium.org/151130/diff/1003/1005
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/151130/diff/1003/1005#newcode27
Line 27: #include "webkit/api/public/WebScreenInfo.h"
nit: this header should be inserted above the include for webcursor.h

http://codereview.chromium.org/151130/diff/1003/1005#newcode769
Line 769: #else  // OS_WIN || OS_LINUX
nit: normally, we do not list these macros in the #else case.  it is not very
future proof since we might add more OS_ values in the future.

http://codereview.chromium.org/151130/diff/1003/1005#newcode780
Line 780: *results = gfx::Rect();
nit: is this assignment really necessary?  i would have expected the caller to
have already executed the default constructor for gfx::Rect.  i don't think you
have to worry about the given gfx::Rect having funny values, do you?

http://codereview.chromium.org/151130/diff/1003/1009
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/151130/diff/1003/1009#newcode363
Line 363: namespace {
i'm not a good reviewer for this file.  you should find a mac expert to review
this.

http://codereview.chromium.org/151130/diff/1003/1013
File chrome/browser/renderer_host/resource_message_filter_win.cc (right):

http://codereview.chromium.org/151130/diff/1003/1013#newcode7
Line 7: 
nit: no new line here

http://codereview.chromium.org/151130/diff/1003/1013#newcode41
Line 41: // TODO(darin): Change this into a routed message so that we can
eliminate
isn't this comment stale now?

http://codereview.chromium.org/151130/diff/1003/1017
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/151130/diff/1003/1017#newcode952
Line 952: // TODO(darin): Change this to be a routed message so we don't need to
pass
your changing this to be a routed message, so my TODO needs to be revised or
deleted.

http://codereview.chromium.org/151130/diff/1003/1017#newcode956
Line 956: gfx::NativeViewId /* view */,
since the message is routed, we don't really need the ViewId parameter.  perhaps
my TODO should be about killing that parameter.  that change is out-of-scope for
this CL.

[Scott Hess - ex-Googler, 2009-07-06 22:23:01]

Amanda, looping you in for the code which moved from
resource_message_filter_mac.mm to render_widget_host_view_mac.mm.  Also, since
you reviewed 57032 (added ShowPopup stuff, which is similar Mac-only wiring
through RenderWidgetHost), you perhaps have an opinion on whether there should
be defined(OS_MACOSX) in render_widget_host.{h,cc}.

http://codereview.chromium.org/151130/diff/1003/1004
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/151130/diff/1003/1004#newcode645
Line 645: #if !defined(OS_MACOSX)
On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
> This should just be an "#if defined(OS_WIN)", see Antoine's change which is
> about to land any minute now:
> http://codereview.chromium.org/146078/diff/3077/3080
> 
> Instead of the comment referring to why certain API calls are needed on a
> specific platform, it should just say that this is needed on Windows because
> Windows internally does blocking calls to the child HWNDs in the plugin
process,
> so if the renderer making sync calls to the UI thread a deadlock can occur.

Antoine's change landed, so not much more to say.  Changing to #if
defined(OS_WIN), added Mac comment - but it occurs to me that listing the
known-good variants may be the more future-proof decision.

http://codereview.chromium.org/151130/diff/1003/1005
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/151130/diff/1003/1005#newcode27
Line 27: #include "webkit/api/public/WebScreenInfo.h"
On 2009/07/06 19:52:47, darin wrote:
> nit: this header should be inserted above the include for webcursor.h

With jam's other suggestion, this is now w/in the #if defined(OS_MACOSX).

http://codereview.chromium.org/151130/diff/1003/1005#newcode29
Line 29: using WebKit::WebScreenInfo;
On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
> nit: why not put this with the other using WebKit:: lines below at 41-44?

Done.

http://codereview.chromium.org/151130/diff/1003/1005#newcode769
Line 769: #else  // OS_WIN || OS_LINUX
On 2009/07/06 19:52:47, darin wrote:
> nit: normally, we do not list these macros in the #else case.  it is not very
> future proof since we might add more OS_ values in the future.

Done.  Just following style of OnShowPopup, which I've now modified anyhow.

http://codereview.chromium.org/151130/diff/1003/1005#newcode780
Line 780: *results = gfx::Rect();
On 2009/07/06 19:52:47, darin wrote:
> nit: is this assignment really necessary?  i would have expected the caller to
> have already executed the default constructor for gfx::Rect.  i don't think
you
> have to worry about the given gfx::Rect having funny values, do you?

Done.  Was perhaps too slavishly following the previous iteration of this code,
which looked more like:

gfx::Rect rect;
if (view_) {
  rect = getTheRect();
}
writeTheRectToMessage(msg, rect);

In the new form, that doesn't make the same kind of sense.

http://codereview.chromium.org/151130/diff/1003/1006
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/151130/diff/1003/1006#newcode396
Line 396: void OnMsgGetScreenInfo(gfx::NativeViewId view,
On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
> Since these functions are only used on mac, what do you think about
surrounding
> them with #if defined(OS_MACOSX)?  Then you wouldn't need such an ifdef in
each
> function definition.

I did these along the lines of OnMsgShowPopup(), which is also Mac-specific.  I
assumed it was done that way to prevent #if defined() from sneaking into this
header file.

http://codereview.chromium.org/57032 has a comment similar to yours.  So my
guess is that your suggestion is a sensible change.  I'll modify that code along
with mine.

http://codereview.chromium.org/151130/diff/1003/1009
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/151130/diff/1003/1009#newcode363
Line 363: namespace {
On 2009/07/06 19:52:47, darin wrote:
> i'm not a good reviewer for this file.  you should find a mac expert to review
> this.

OK, I'll loop someone in.

http://codereview.chromium.org/151130/diff/1003/1013
File chrome/browser/renderer_host/resource_message_filter_win.cc (right):

http://codereview.chromium.org/151130/diff/1003/1013#newcode7
Line 7: 
On 2009/07/06 19:52:47, darin wrote:
> nit: no new line here

Done.

http://codereview.chromium.org/151130/diff/1003/1013#newcode41
Line 41: // TODO(darin): Change this into a routed message so that we can
eliminate
On 2009/07/06 19:52:47, darin wrote:
> isn't this comment stale now?

Done by adding an overall TODO(shess) around routing_id -> HWND mapping to
remove need for NativeViewId.

http://codereview.chromium.org/151130/diff/1003/1017
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/151130/diff/1003/1017#newcode952
Line 952: // TODO(darin): Change this to be a routed message so we don't need to
pass
On 2009/07/06 19:52:47, darin wrote:
> your changing this to be a routed message, so my TODO needs to be revised or
> deleted.

Done.

http://codereview.chromium.org/151130/diff/1003/1017#newcode956
Line 956: gfx::NativeViewId /* view */,
On 2009/07/06 19:52:47, darin wrote:
> since the message is routed, we don't really need the ViewId parameter. 
perhaps
> my TODO should be about killing that parameter.  that change is out-of-scope
for
> this CL.

Done.

[Amanda Walker, 2009-07-06 22:32:54]

On 2009/07/06 22:23:01, shess wrote:
> Amanda, looping you in for the code which moved from
> resource_message_filter_mac.mm to render_widget_host_view_mac.mm.

That code movement LGTM.

> Also, since
> you reviewed 57032 (added ShowPopup stuff, which is similar Mac-only wiring
> through RenderWidgetHost), you perhaps have an opinion on whether there should
> be defined(OS_MACOSX) in render_widget_host.{h,cc}.

I have no strong feelings, though I lean mildly towards putting an #if into the
.h and removing them from the .cc (makes the implementations much easier to
read).

[Scott Hess - ex-Googler, 2009-07-07 20:57:30]

Sorry about the merge-related junk yesterday.  Some code conflicted, then once I
was merged reverted and I didn't realize it so a later merge was all horrid. 
All should be good, now.

Everyone satisfied with my responses?  [I only see LGTM from Amanda.  Probably
needing one from jam or darin.]

Thanks,
scott


On 2009/07/06 22:23:01, shess wrote:
> Amanda, looping you in for the code which moved from
> resource_message_filter_mac.mm to render_widget_host_view_mac.mm.  Also, since
> you reviewed 57032 (added ShowPopup stuff, which is similar Mac-only wiring
> through RenderWidgetHost), you perhaps have an opinion on whether there should
> be defined(OS_MACOSX) in render_widget_host.{h,cc}.
> 
> http://codereview.chromium.org/151130/diff/1003/1004
> File chrome/browser/renderer_host/render_view_host.cc (right):
> 
> http://codereview.chromium.org/151130/diff/1003/1004#newcode645
> Line 645: #if !defined(OS_MACOSX)
> On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
> > This should just be an "#if defined(OS_WIN)", see Antoine's change which is
> > about to land any minute now:
> > http://codereview.chromium.org/146078/diff/3077/3080
> > 
> > Instead of the comment referring to why certain API calls are needed on a
> > specific platform, it should just say that this is needed on Windows because
> > Windows internally does blocking calls to the child HWNDs in the plugin
> process,
> > so if the renderer making sync calls to the UI thread a deadlock can occur.
> 
> Antoine's change landed, so not much more to say.  Changing to #if
> defined(OS_WIN), added Mac comment - but it occurs to me that listing the
> known-good variants may be the more future-proof decision.
> 
> http://codereview.chromium.org/151130/diff/1003/1005
> File chrome/browser/renderer_host/render_widget_host.cc (right):
> 
> http://codereview.chromium.org/151130/diff/1003/1005#newcode27
> Line 27: #include "webkit/api/public/WebScreenInfo.h"
> On 2009/07/06 19:52:47, darin wrote:
> > nit: this header should be inserted above the include for webcursor.h
> 
> With jam's other suggestion, this is now w/in the #if defined(OS_MACOSX).
> 
> http://codereview.chromium.org/151130/diff/1003/1005#newcode29
> Line 29: using WebKit::WebScreenInfo;
> On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
> > nit: why not put this with the other using WebKit:: lines below at 41-44?
> 
> Done.
> 
> http://codereview.chromium.org/151130/diff/1003/1005#newcode769
> Line 769: #else  // OS_WIN || OS_LINUX
> On 2009/07/06 19:52:47, darin wrote:
> > nit: normally, we do not list these macros in the #else case.  it is not
very
> > future proof since we might add more OS_ values in the future.
> 
> Done.  Just following style of OnShowPopup, which I've now modified anyhow.
> 
> http://codereview.chromium.org/151130/diff/1003/1005#newcode780
> Line 780: *results = gfx::Rect();
> On 2009/07/06 19:52:47, darin wrote:
> > nit: is this assignment really necessary?  i would have expected the caller
to
> > have already executed the default constructor for gfx::Rect.  i don't think
> you
> > have to worry about the given gfx::Rect having funny values, do you?
> 
> Done.  Was perhaps too slavishly following the previous iteration of this
code,
> which looked more like:
> 
> gfx::Rect rect;
> if (view_) {
>   rect = getTheRect();
> }
> writeTheRectToMessage(msg, rect);
> 
> In the new form, that doesn't make the same kind of sense.
> 
> http://codereview.chromium.org/151130/diff/1003/1006
> File chrome/browser/renderer_host/render_widget_host.h (right):
> 
> http://codereview.chromium.org/151130/diff/1003/1006#newcode396
> Line 396: void OnMsgGetScreenInfo(gfx::NativeViewId view,
> On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
> > Since these functions are only used on mac, what do you think about
> surrounding
> > them with #if defined(OS_MACOSX)?  Then you wouldn't need such an ifdef in
> each
> > function definition.
> 
> I did these along the lines of OnMsgShowPopup(), which is also Mac-specific. 
I
> assumed it was done that way to prevent #if defined() from sneaking into this
> header file.
> 
> http://codereview.chromium.org/57032 has a comment similar to yours.  So my
> guess is that your suggestion is a sensible change.  I'll modify that code
along
> with mine.
> 
> http://codereview.chromium.org/151130/diff/1003/1009
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
> http://codereview.chromium.org/151130/diff/1003/1009#newcode363
> Line 363: namespace {
> On 2009/07/06 19:52:47, darin wrote:
> > i'm not a good reviewer for this file.  you should find a mac expert to
review
> > this.
> 
> OK, I'll loop someone in.
> 
> http://codereview.chromium.org/151130/diff/1003/1013
> File chrome/browser/renderer_host/resource_message_filter_win.cc (right):
> 
> http://codereview.chromium.org/151130/diff/1003/1013#newcode7
> Line 7: 
> On 2009/07/06 19:52:47, darin wrote:
> > nit: no new line here
> 
> Done.
> 
> http://codereview.chromium.org/151130/diff/1003/1013#newcode41
> Line 41: // TODO(darin): Change this into a routed message so that we can
> eliminate
> On 2009/07/06 19:52:47, darin wrote:
> > isn't this comment stale now?
> 
> Done by adding an overall TODO(shess) around routing_id -> HWND mapping to
> remove need for NativeViewId.
> 
> http://codereview.chromium.org/151130/diff/1003/1017
> File chrome/common/render_messages_internal.h (right):
> 
> http://codereview.chromium.org/151130/diff/1003/1017#newcode952
> Line 952: // TODO(darin): Change this to be a routed message so we don't need
to
> pass
> On 2009/07/06 19:52:47, darin wrote:
> > your changing this to be a routed message, so my TODO needs to be revised or
> > deleted.
> 
> Done.
> 
> http://codereview.chromium.org/151130/diff/1003/1017#newcode956
> Line 956: gfx::NativeViewId /* view */,
> On 2009/07/06 19:52:47, darin wrote:
> > since the message is routed, we don't really need the ViewId parameter. 
> perhaps
> > my TODO should be about killing that parameter.  that change is out-of-scope
> for
> > this CL.
> 
> Done.

[darin (slow to review), 2009-07-07 21:24:17]

LGTM

The CL description should include BUG=, TEST=, and R= lines.

http://codereview.chromium.org/151130/diff/2111/2113
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/151130/diff/2111/2113#newcode137
Line 137: #endif
nit: #elif

http://codereview.chromium.org/151130/diff/2111/2113#newcode767
Line 767: #if defined(OS_MACOSX)
nit: #elif

http://codereview.chromium.org/151130/diff/2111/2114
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/151130/diff/2111/2114#newcode29
Line 29: #if defined(OS_MACOSX)
nit: no need to make this conditional.  the fewer the preprocessor directives
the better!

http://codereview.chromium.org/151130/diff/2111/2114#newcode400
Line 400: #endif
nit: #elif

[jam, 2009-07-07 21:30:28]

lgtm

On Tue, Jul 7, 2009 at 1:57 PM, <shess@chromium.org> wrote:

> Sorry about the merge-related junk yesterday.  Some code conflicted,
> then once I was merged reverted and I didn't realize it so a later merge
> was all horrid.  All should be good, now.
>
> Everyone satisfied with my responses?  [I only see LGTM from Amanda.
> Probably needing one from jam or darin.]
>
> Thanks,
> scott
>
>
> On 2009/07/06 22:23:01, shess wrote:
>
>> Amanda, looping you in for the code which moved from
>> resource_message_filter_mac.mm to render_widget_host_view_mac.mm.
>>
> Also, since
>
>> you reviewed 57032 (added ShowPopup stuff, which is similar Mac-only
>>
> wiring
>
>> through RenderWidgetHost), you perhaps have an opinion on whether
>>
> there should
>
>> be defined(OS_MACOSX) in render_widget_host.{h,cc}.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1004
>> File chrome/browser/renderer_host/render_view_host.cc (right):
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1004#newcode645
>> Line 645: #if !defined(OS_MACOSX)
>> On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
>> > This should just be an "#if defined(OS_WIN)", see Antoine's change
>>
> which is
>
>> > about to land any minute now:
>> > http://codereview.chromium.org/146078/diff/3077/3080
>> >
>> > Instead of the comment referring to why certain API calls are needed
>>
> on a
>
>> > specific platform, it should just say that this is needed on Windows
>>
> because
>
>> > Windows internally does blocking calls to the child HWNDs in the
>>
> plugin
>
>> process,
>> > so if the renderer making sync calls to the UI thread a deadlock can
>>
> occur.
>
>  Antoine's change landed, so not much more to say.  Changing to #if
>> defined(OS_WIN), added Mac comment - but it occurs to me that listing
>>
> the
>
>> known-good variants may be the more future-proof decision.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1005
>> File chrome/browser/renderer_host/render_widget_host.cc (right):
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1005#newcode27
>> Line 27: #include "webkit/api/public/WebScreenInfo.h"
>> On 2009/07/06 19:52:47, darin wrote:
>> > nit: this header should be inserted above the include for
>>
> webcursor.h
>
>  With jam's other suggestion, this is now w/in the #if
>>
> defined(OS_MACOSX).
>
>  http://codereview.chromium.org/151130/diff/1003/1005#newcode29
>> Line 29: using WebKit::WebScreenInfo;
>> On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
>> > nit: why not put this with the other using WebKit:: lines below at
>>
> 41-44?
>
>  Done.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1005#newcode769
>> Line 769: #else  // OS_WIN || OS_LINUX
>> On 2009/07/06 19:52:47, darin wrote:
>> > nit: normally, we do not list these macros in the #else case.  it is
>>
> not very
>
>> > future proof since we might add more OS_ values in the future.
>>
>
>  Done.  Just following style of OnShowPopup, which I've now modified
>>
> anyhow.
>
>  http://codereview.chromium.org/151130/diff/1003/1005#newcode780
>> Line 780: *results = gfx::Rect();
>> On 2009/07/06 19:52:47, darin wrote:
>> > nit: is this assignment really necessary?  i would have expected the
>>
> caller to
>
>> > have already executed the default constructor for gfx::Rect.  i
>>
> don't think
>
>> you
>> > have to worry about the given gfx::Rect having funny values, do you?
>>
>
>  Done.  Was perhaps too slavishly following the previous iteration of
>>
> this code,
>
>> which looked more like:
>>
>
>  gfx::Rect rect;
>> if (view_) {
>>   rect = getTheRect();
>> }
>> writeTheRectToMessage(msg, rect);
>>
>
>  In the new form, that doesn't make the same kind of sense.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1006
>> File chrome/browser/renderer_host/render_widget_host.h (right):
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1006#newcode396
>> Line 396: void OnMsgGetScreenInfo(gfx::NativeViewId view,
>> On 2009/07/06 19:33:27, John Abd-El-Malek wrote:
>> > Since these functions are only used on mac, what do you think about
>> surrounding
>> > them with #if defined(OS_MACOSX)?  Then you wouldn't need such an
>>
> ifdef in
>
>> each
>> > function definition.
>>
>
>  I did these along the lines of OnMsgShowPopup(), which is also
>>
> Mac-specific.  I
>
>> assumed it was done that way to prevent #if defined() from sneaking
>>
> into this
>
>> header file.
>>
>
>  http://codereview.chromium.org/57032 has a comment similar to yours.
>>
> So my
>
>> guess is that your suggestion is a sensible change.  I'll modify that
>>
> code along
>
>> with mine.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1009
>> File chrome/browser/renderer_host/render_widget_host_view_mac.mm
>>
> (right):
>
>  http://codereview.chromium.org/151130/diff/1003/1009#newcode363
>> Line 363: namespace {
>> On 2009/07/06 19:52:47, darin wrote:
>> > i'm not a good reviewer for this file.  you should find a mac expert
>>
> to review
>
>> > this.
>>
>
>  OK, I'll loop someone in.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1013
>> File chrome/browser/renderer_host/resource_message_filter_win.cc
>>
> (right):
>
>  http://codereview.chromium.org/151130/diff/1003/1013#newcode7
>> Line 7:
>> On 2009/07/06 19:52:47, darin wrote:
>> > nit: no new line here
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1013#newcode41
>> Line 41: // TODO(darin): Change this into a routed message so that we
>>
> can
>
>> eliminate
>> On 2009/07/06 19:52:47, darin wrote:
>> > isn't this comment stale now?
>>
>
>  Done by adding an overall TODO(shess) around routing_id -> HWND
>>
> mapping to
>
>> remove need for NativeViewId.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1017
>> File chrome/common/render_messages_internal.h (right):
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1017#newcode952
>> Line 952: // TODO(darin): Change this to be a routed message so we
>>
> don't need to
>
>> pass
>> On 2009/07/06 19:52:47, darin wrote:
>> > your changing this to be a routed message, so my TODO needs to be
>>
> revised or
>
>> > deleted.
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/151130/diff/1003/1017#newcode956
>> Line 956: gfx::NativeViewId /* view */,
>> On 2009/07/06 19:52:47, darin wrote:
>> > since the message is routed, we don't really need the ViewId
>>
> parameter.
>
>> perhaps
>> > my TODO should be about killing that parameter.  that change is
>>
> out-of-scope
>
>> for
>> > this CL.
>>
>
>  Done.
>>
>
>
>
> http://codereview.chromium.org/151130
>

[Scott Hess - ex-Googler, 2009-07-07 21:41:51]

On 2009/07/07 21:24:17, darin wrote:
> The CL description should include BUG=, TEST=, and R= lines.

I thought the http://crbug.com/13113 line was an accepted BUG= alternative
(BUG=http://crbug.com/13113 breaks linkiness).  Didn't know we used R=, I
thought that was a g4-ism.

TEST= ... wow, I wish there was a repro case I could slot in there, short of a
patch to the code.  In this case the proof of the pudding will be in the eating.
 I'll put my test html file which at least exercises these functions in the bug.

[Scott Hess - ex-Googler, 2009-07-07 21:42:03]

http://codereview.chromium.org/151130/diff/2111/2113
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/151130/diff/2111/2113#newcode137
Line 137: #endif
On 2009/07/07 21:24:17, darin wrote:
> nit: #elif

Done.

http://codereview.chromium.org/151130/diff/2111/2113#newcode767
Line 767: #if defined(OS_MACOSX)
On 2009/07/07 21:24:17, darin wrote:
> nit: #elif

Done.

http://codereview.chromium.org/151130/diff/2111/2114
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/151130/diff/2111/2114#newcode29
Line 29: #if defined(OS_MACOSX)
On 2009/07/07 21:24:17, darin wrote:
> nit: no need to make this conditional.  the fewer the preprocessor directives
> the better!

Done.

http://codereview.chromium.org/151130/diff/2111/2114#newcode400
Line 400: #endif
On 2009/07/07 21:24:17, darin wrote:
> nit: #elif

Done.  Was unsure since they aren't variants of each other, just coincidentally
near each other.

[darin (slow to review), 2009-07-08 00:01:19]

http://codereview.chromium.org/151130/diff/2111/2114
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/151130/diff/2111/2114#newcode400
Line 400: #endif
> Done.  Was unsure since they aren't variants of each other, just
coincidentally
> near each other.

That's a good point!  Maybe my suggestion was bad in this case.