Add support for custom cursors set by windowless plugins. Windowless plugins...

Add support for custom cursors set by windowless plugins. Windowless plugins typically set the cursor in NPP_HandleEvent for WM_MOUSEMOVE.The current implementation looks for the cursor type and if the typedoes not match predefinedcursor types defaults to the pointer cursor.

The fixes are as below:-
1. Marshal the HCURSOR after copying it to ensure that it  
   remains valid. This works as a HCURSOR is a user object  
   and can be used across processes. Ideally we would 
   like  to convert it to a skia bitmap but there are issues 
   with converting monochrome cursors.

2. Added support for marshaling platform specific data in  
   the webcursor Serialize/Deserialize functions. This is in  
   the form of functions like InitPlatformData,  
   SerializePlatformData, DeserializePlatformData, etc.  
   which are stubbed out for the other platforms.

3. Mimic webkit windowless plugin behavior where it sets a  
   flag to ignore the next setCursor after HandleEvent of  
   WM_MOUSEMOVE. If  we don't do this the cursor keeps  
   changing between a pointerCursor and the cursor set by  
   the plugin which also causes flicker.

4. Fixed the WebCursor::IsEqual function to ensure that it  
   checks all fields for equality.

5. The browser(RenderWidgetHostViewWin) now maintains a 
   WebCursor instance representing the current cursor. Any 
   cursor updates received from the renderer update the 
   current cursor member maintained by the browser.

6. We intercept the SetCursor API for windowless plugins 
   like Flash and Silverlight and remember the cursor being 
   set. We don't invoke the original API as the browser UI 
   thread would do it anyways. This fixes the annoying 
   cursor flicker caused by the windowless flash plugin 
   instance constantly setting the cursor even when the tab
   is not visible.

This fixes bug http://code.google.com/p/chromium/issues/detail?id=3800.


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

[darin (slow to review), 2008-12-22 16:41:15]

http://codereview.chromium.org/15088/diff/204/44
File chrome/browser/render_widget_host_view_win.h (right):

http://codereview.chromium.org/15088/diff/204/44#newcode223
Line 223: regular_cursor,
these need to be UPPER_CASE per the style guide.

http://codereview.chromium.org/15088/diff/204/42
File webkit/glue/webcursor.h (right):

http://codereview.chromium.org/15088/diff/204/42#newcode71
Line 71: // Getter/Setters for the cursor_ member
i don't understand these new methods.  it seems like we are adding another
meaning of the term "custom".  how do they relate to IsCustom() and
GetCustomCursor()?  why do we need these methods?

it seems like GetCustomCursor could still be used.  just call CopyCursor (or
CopyIcon) in GetCustomCursor for the case of a WebCursor having a HCURSOR
member.

http://codereview.chromium.org/15088/diff/204/41
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/15088/diff/204/41#newcode906
Line 906: chrome_client->SetCursor(cursor);
why don't we just create a new method named SetCursorForPlugin?  then
ChromeClientImpl can internally manage the "ignore next set cursor" thingy.

[ananta, 2008-12-23 00:28:38]

On 2008/12/22 16:41:15, darin wrote:
> http://codereview.chromium.org/15088/diff/204/44
> File chrome/browser/render_widget_host_view_win.h (right):
> 
> http://codereview.chromium.org/15088/diff/204/44#newcode223
> Line 223: regular_cursor,
> these need to be UPPER_CASE per the style guide.

Based on our discussions to have the WebCursor object manage the cursor data,
this is no longer needed.

> 
> http://codereview.chromium.org/15088/diff/204/42
> File webkit/glue/webcursor.h (right):
> 
> http://codereview.chromium.org/15088/diff/204/42#newcode71
> Line 71: // Getter/Setters for the cursor_ member
> i don't understand these new methods.  it seems like we are adding another
> meaning of the term "custom".  how do they relate to IsCustom() and
> GetCustomCursor()?  why do we need these methods?
> 
> it seems like GetCustomCursor could still be used.  just call CopyCursor (or
> CopyIcon) in GetCustomCursor for the case of a WebCursor having a HCURSOR
> member.

The getter/setters for the HCURSOR have been removed. 

> 
> http://codereview.chromium.org/15088/diff/204/41
> File webkit/glue/webplugin_impl.cc (right):
> 
> http://codereview.chromium.org/15088/diff/204/41#newcode906
> Line 906: chrome_client->SetCursor(cursor);
> why don't we just create a new method named SetCursorForPlugin?  then
> ChromeClientImpl can internally manage the "ignore next set cursor" thingy.

Done.

Updated the CB spec based on the latest changes.

[darin (slow to review), 2008-12-23 05:00:25]

Looking good...

http://codereview.chromium.org/15088/diff/801/813
File chrome/browser/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/15088/diff/801/813#newcode86
Line 86: current_cursor_.Clear();
WebCursor should have a destructor that calls Clear so you don't have to here.

http://codereview.chromium.org/15088/diff/801/804
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/15088/diff/801/804#newcode488
Line 488: // Currently, Widget::setCursor is always called after this function
in
this feels like it is working around buggy behavior in webkit.  i suspect it
should be fixed to not call setCursor twice, but we don't have to do that now. 
maybe it would be good to file a bug upstream about this issue.

http://codereview.chromium.org/15088/diff/801/803
File webkit/glue/chrome_client_impl.h (right):

http://codereview.chromium.org/15088/diff/801/803#newcode130
Line 130: bool ignore_next_set_cursor() const {
these accessors are no longer needed right?  or at least they could be made
private.

http://codereview.chromium.org/15088/diff/801/805
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/15088/diff/801/805#newcode1135
Line 1135: if (prev_cursor != current_cursor)
please add a comment explaining this code.  also, shouldn't we just early return
from this function if prev_cursor == current_cursor?  why bother calling
InitFromCursor in that case?

http://codereview.chromium.org/15088/diff/801/805#newcode1138
Line 1138: web_current_cursor.Clear();
this call to .Clear() should be unnecessary if you define a suitable ~WebCursor.

http://codereview.chromium.org/15088/diff/801/810
File webkit/glue/webcursor.h (right):

http://codereview.chromium.org/15088/diff/801/810#newcode60
Line 60: // the HCURSOR the Clear function should be used.
it seems like we don't need to expose a Clear function.  we should just say that
the HCURSOR remains valid for the lifetime of the WebCursor.

http://codereview.chromium.org/15088/diff/801/810#newcode65
Line 65: HCURSOR GetCustomCursor();
we can delete this method now, right?

http://codereview.chromium.org/15088/diff/801/810#newcode82
Line 82: // Helper function to perform a deep copy of the WebCursor passed in.
nit: please put the platform independent methods above the platform-specific
methods.  that should help readability a bit in the presence of #ifdefs.

http://codereview.chromium.org/15088/diff/801/810#newcode107
Line 107: bool ComparePlatformData(const WebCursor& other) const ;
nit: how about calling this IsPlatformDataEqual?  that way it sounds a bit
similar to IsEqual, which is what it is used to implement.

http://codereview.chromium.org/15088/diff/801/810#newcode128
Line 128: // A custom windows HCURSOR typically created via CreateCursor or
LoadCursor.
the names and comments for these member variables is a bit confusing.  maybe it
would be good to use names like:  custom_cursor_ and external_cursor_, where
custom_cursor_ is one created from custom_data_, and external_cursor_ is one set
via InitFromCursor.  You could even rename that method InitFromExternalCursor to
help keep things clear.

http://codereview.chromium.org/15088/diff/801/811
File webkit/glue/webcursor_win.cc (right):

http://codereview.chromium.org/15088/diff/801/811#newcode216
Line 216: return
pickle->WriteInt(reinterpret_cast<int>(windows_custom_cursor_));
maybe you should use WriteIntPtr here?

http://codereview.chromium.org/15088/diff/801/811#newcode221
Line 221: reinterpret_cast<int*>(&windows_custom_cursor_));
maybe you should use ReadIntPtr here?

http://codereview.chromium.org/15088/diff/801/815
File webkit/tools/test_shell/test_webview_delegate_win.cc (right):

http://codereview.chromium.org/15088/diff/801/815#newcode39
Line 39: current_cursor_.Clear();
should just rely on ~WebCursor here.

[ananta, 2008-12-23 06:05:21]

On 2008/12/23 05:00:25, darin wrote:
> Looking good...
> 
> http://codereview.chromium.org/15088/diff/801/813
> File chrome/browser/render_widget_host_view_win.cc (right):
> 
> http://codereview.chromium.org/15088/diff/801/813#newcode86
> Line 86: current_cursor_.Clear();
> WebCursor should have a destructor that calls Clear so you don't have to here.

While this is the right thing to do in the long run, it is tricky as the
WebCursor instance does not have ownership of the underlying data like HCURSOR
at all times. I added some more comments explaining this along with a TODO to
the effect that this needs to be revisited.


> 
> http://codereview.chromium.org/15088/diff/801/804
> File webkit/glue/chrome_client_impl.cc (right):
> 
> http://codereview.chromium.org/15088/diff/801/804#newcode488
> Line 488: // Currently, Widget::setCursor is always called after this function
> in
> this feels like it is working around buggy behavior in webkit.  i suspect it
> should be fixed to not call setCursor twice, but we don't have to do that now.

> maybe it would be good to file a bug upstream about this issue.

Will do.

> 
> http://codereview.chromium.org/15088/diff/801/803
> File webkit/glue/chrome_client_impl.h (right):
> 
> http://codereview.chromium.org/15088/diff/801/803#newcode130
> Line 130: bool ignore_next_set_cursor() const {
> these accessors are no longer needed right?  or at least they could be made
> private.

Removed.

> 
> http://codereview.chromium.org/15088/diff/801/805
> File webkit/glue/plugins/webplugin_delegate_impl.cc (right):
> 
> http://codereview.chromium.org/15088/diff/801/805#newcode1135
> Line 1135: if (prev_cursor != current_cursor)
> please add a comment explaining this code.  also, shouldn't we just early
return
> from this function if prev_cursor == current_cursor?  why bother calling
> InitFromCursor in that case?

Done.

> 
> http://codereview.chromium.org/15088/diff/801/805#newcode1138
> Line 1138: web_current_cursor.Clear();
> this call to .Clear() should be unnecessary if you define a suitable
~WebCursor.

The comments above for the WebCursor destructor apply here.

> 
> http://codereview.chromium.org/15088/diff/801/810
> File webkit/glue/webcursor.h (right):
> 
> http://codereview.chromium.org/15088/diff/801/810#newcode60
> Line 60: // the HCURSOR the Clear function should be used.
> it seems like we don't need to expose a Clear function.  we should just say
that
> the HCURSOR remains valid for the lifetime of the WebCursor.

Added some more comments explaining this.

> 
> http://codereview.chromium.org/15088/diff/801/810#newcode65
> Line 65: HCURSOR GetCustomCursor();
> we can delete this method now, right?

Done.

> 
> http://codereview.chromium.org/15088/diff/801/810#newcode82
> Line 82: // Helper function to perform a deep copy of the WebCursor passed in.
> nit: please put the platform independent methods above the platform-specific
> methods.  that should help readability a bit in the presence of #ifdefs.

Done.

> 
> http://codereview.chromium.org/15088/diff/801/810#newcode107
> Line 107: bool ComparePlatformData(const WebCursor& other) const ;
> nit: how about calling this IsPlatformDataEqual?  that way it sounds a bit
> similar to IsEqual, which is what it is used to implement.

Done.

> 
> http://codereview.chromium.org/15088/diff/801/810#newcode128
> Line 128: // A custom windows HCURSOR typically created via CreateCursor or
> LoadCursor.
> the names and comments for these member variables is a bit confusing.  maybe
it
> would be good to use names like:  custom_cursor_ and external_cursor_, where
> custom_cursor_ is one created from custom_data_, and external_cursor_ is one
set
> via InitFromCursor.  You could even rename that method InitFromExternalCursor
to
> help keep things clear.

Done.

> 
> http://codereview.chromium.org/15088/diff/801/811
> File webkit/glue/webcursor_win.cc (right):
> 
> http://codereview.chromium.org/15088/diff/801/811#newcode216
> Line 216: return
> pickle->WriteInt(reinterpret_cast<int>(windows_custom_cursor_));

> maybe you should use WriteIntPtr here?

Done.

> 
> http://codereview.chromium.org/15088/diff/801/811#newcode221
> Line 221: reinterpret_cast<int*>(&windows_custom_cursor_));
> maybe you should use ReadIntPtr here?

Done.

> 
> http://codereview.chromium.org/15088/diff/801/815
> File webkit/tools/test_shell/test_webview_delegate_win.cc (right):
> 
> http://codereview.chromium.org/15088/diff/801/815#newcode39
> Line 39: current_cursor_.Clear();
> should just rely on ~WebCursor here.

The comments above for the WebCursor destructor apply here.

Thanks
Ananta

[ananta, 2008-12-23 20:50:35]

Based on our discussion, added a flag indicating whether the WebCursor has
ownership of the underlying HCURSOR. This flag is set to true when we call
CopyCursor/CopyIcon on the passed in WebCursor in Assign.

The Clear method is now private and is invoked from the destructor.
We only clean up the HCURSORS when we have ownership.

The CB has been updated with these changes.

Thanks
Ananta

On 2008/12/23 06:05:21, ananta wrote:
> On 2008/12/23 05:00:25, darin wrote:
> > Looking good...
> > 
> > http://codereview.chromium.org/15088/diff/801/813
> > File chrome/browser/render_widget_host_view_win.cc (right):
> > 
> > http://codereview.chromium.org/15088/diff/801/813#newcode86
> > Line 86: current_cursor_.Clear();
> > WebCursor should have a destructor that calls Clear so you don't have to
here.
> 
> While this is the right thing to do in the long run, it is tricky as the
> WebCursor instance does not have ownership of the underlying data like HCURSOR
> at all times. I added some more comments explaining this along with a TODO to
> the effect that this needs to be revisited.
> 
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/804
> > File webkit/glue/chrome_client_impl.cc (right):
> > 
> > http://codereview.chromium.org/15088/diff/801/804#newcode488
> > Line 488: // Currently, Widget::setCursor is always called after this
function
> > in
> > this feels like it is working around buggy behavior in webkit.  i suspect it
> > should be fixed to not call setCursor twice, but we don't have to do that
now.
> 
> > maybe it would be good to file a bug upstream about this issue.
> 
> Will do.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/803
> > File webkit/glue/chrome_client_impl.h (right):
> > 
> > http://codereview.chromium.org/15088/diff/801/803#newcode130
> > Line 130: bool ignore_next_set_cursor() const {
> > these accessors are no longer needed right?  or at least they could be made
> > private.
> 
> Removed.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/805
> > File webkit/glue/plugins/webplugin_delegate_impl.cc (right):
> > 
> > http://codereview.chromium.org/15088/diff/801/805#newcode1135
> > Line 1135: if (prev_cursor != current_cursor)
> > please add a comment explaining this code.  also, shouldn't we just early
> return
> > from this function if prev_cursor == current_cursor?  why bother calling
> > InitFromCursor in that case?
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/805#newcode1138
> > Line 1138: web_current_cursor.Clear();
> > this call to .Clear() should be unnecessary if you define a suitable
> ~WebCursor.
> 
> The comments above for the WebCursor destructor apply here.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/810
> > File webkit/glue/webcursor.h (right):
> > 
> > http://codereview.chromium.org/15088/diff/801/810#newcode60
> > Line 60: // the HCURSOR the Clear function should be used.
> > it seems like we don't need to expose a Clear function.  we should just say
> that
> > the HCURSOR remains valid for the lifetime of the WebCursor.
> 
> Added some more comments explaining this.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/810#newcode65
> > Line 65: HCURSOR GetCustomCursor();
> > we can delete this method now, right?
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/810#newcode82
> > Line 82: // Helper function to perform a deep copy of the WebCursor passed
in.
> > nit: please put the platform independent methods above the platform-specific
> > methods.  that should help readability a bit in the presence of #ifdefs.
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/810#newcode107
> > Line 107: bool ComparePlatformData(const WebCursor& other) const ;
> > nit: how about calling this IsPlatformDataEqual?  that way it sounds a bit
> > similar to IsEqual, which is what it is used to implement.
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/810#newcode128
> > Line 128: // A custom windows HCURSOR typically created via CreateCursor or
> > LoadCursor.
> > the names and comments for these member variables is a bit confusing.  maybe
> it
> > would be good to use names like:  custom_cursor_ and external_cursor_, where
> > custom_cursor_ is one created from custom_data_, and external_cursor_ is one
> set
> > via InitFromCursor.  You could even rename that method
InitFromExternalCursor
> to
> > help keep things clear.
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/811
> > File webkit/glue/webcursor_win.cc (right):
> > 
> > http://codereview.chromium.org/15088/diff/801/811#newcode216
> > Line 216: return
> > pickle->WriteInt(reinterpret_cast<int>(windows_custom_cursor_));
> 
> > maybe you should use WriteIntPtr here?
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/811#newcode221
> > Line 221: reinterpret_cast<int*>(&windows_custom_cursor_));
> > maybe you should use ReadIntPtr here?
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/801/815
> > File webkit/tools/test_shell/test_webview_delegate_win.cc (right):
> > 
> > http://codereview.chromium.org/15088/diff/801/815#newcode39
> > Line 39: current_cursor_.Clear();
> > should just rely on ~WebCursor here.
> 
> The comments above for the WebCursor destructor apply here.
> 
> Thanks
> Ananta

[darin (slow to review), 2008-12-23 20:58:02]

http://codereview.chromium.org/15088/diff/499/503
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/15088/diff/499/503#newcode1141
Line 1141: web_current_cursor.Clear();
so this .Clear call should be unnecessary now, right?

could we just call .InitFromExternalCursor(current_cursor) on
current_windowless_cursor_ avoiding the Assign call?

http://codereview.chromium.org/15088/diff/499/508
File webkit/glue/webcursor.h (right):

http://codereview.chromium.org/15088/diff/499/508#newcode66
Line 66: void Assign(const WebCursor& other);
on crap, do we have to worry about the WebCursor copy-constructor and assignment
operators?  maybe WebCursor should have DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/15088/diff/499/508#newcode123
Line 123: // A custom windows HCURSOR typically created via CreateCursor or
LoadCursor,
nit: to minimize confusion, how about "An externally generated HCURSOR..."

[ananta, 2008-12-23 21:09:39]

On 2008/12/23 20:58:02, darin wrote:
> http://codereview.chromium.org/15088/diff/499/503
> File webkit/glue/plugins/webplugin_delegate_impl.cc (right):
> 
> http://codereview.chromium.org/15088/diff/499/503#newcode1141
> Line 1141: web_current_cursor.Clear();
> so this .Clear call should be unnecessary now, right?

Done. Oddly this file did not get picked up in the initial gcl upload.

> 
> could we just call .InitFromExternalCursor(current_cursor) on
> current_windowless_cursor_ avoiding the Assign call?

Done.

> 
> http://codereview.chromium.org/15088/diff/499/508
> File webkit/glue/webcursor.h (right):
> 
> http://codereview.chromium.org/15088/diff/499/508#newcode66
> Line 66: void Assign(const WebCursor& other);
> on crap, do we have to worry about the WebCursor copy-constructor and
assignment
> operators?  maybe WebCursor should have DISALLOW_COPY_AND_ASSIGN?

We cannot use DISALLOW_COPY_AND_ASSIGN as the renderer uses the default
assignment operator. It cannot use Assign as it runs in a sandbox.

> 
> http://codereview.chromium.org/15088/diff/499/508#newcode123
> Line 123: // A custom windows HCURSOR typically created via CreateCursor or
> LoadCursor,
> nit: to minimize confusion, how about "An externally generated HCURSOR..."

Done.

Thanks
Ananta

[darin (slow to review), 2008-12-23 21:39:04]

> We cannot use DISALLOW_COPY_AND_ASSIGN as the renderer uses the default
> assignment operator. It cannot use Assign as it runs in a sandbox.

So, this means that we need to implement operator=, right?  Otherwise, we'll end
up with two WebCursor objects that think that they own the custom_cursor_ or
external_cursor_ reference.

Of course the style guide forbids implementing operator=, but in this case I
can't think of a good alternative.

Note: In addition to operator=, you also have to worry about the
copy-constructor.

[ananta, 2008-12-23 22:00:12]

On 2008/12/23 21:39:04, darin wrote:
> > We cannot use DISALLOW_COPY_AND_ASSIGN as the renderer uses the default
> > assignment operator. It cannot use Assign as it runs in a sandbox.
> 
> So, this means that we need to implement operator=, right?  Otherwise, we'll
end
> up with two WebCursor objects that think that they own the custom_cursor_ or
> external_cursor_ reference.

Done. Thanks for pointing this out.

> 
> Of course the style guide forbids implementing operator=, but in this case I
> can't think of a good alternative.
> 
> Note: In addition to operator=, you also have to worry about the
> copy-constructor.

Done.

-Ananta

[darin (slow to review), 2008-12-23 23:30:31]

things are getting complicated.  maybe a better solution would be to put
external_cursor_ and custom_cursor_ inside a reference counted inner class?

http://codereview.chromium.org/15088/diff/1112/1119
File webkit/glue/webcursor.cc (right):

http://codereview.chromium.org/15088/diff/1112/1119#newcode35
Line 35: Copy(other, false);
what happens if |other| gets destroyed?  won't this object hold references to
junk HCURSORs?

http://codereview.chromium.org/15088/diff/1112/1121
File webkit/glue/webcursor.h (right):

http://codereview.chromium.org/15088/diff/1112/1121#newcode46
Line 46: // Copy constructor/assigment operator combine. We need to ensure
nit: "assignment"

http://codereview.chromium.org/15088/diff/1112/1121#newcode73
Line 73: void Assign(const WebCursor& other);
i hate to keep feeding tweaks, but since you now have an assignment operator, it
probably would be nice to rename this Assign method to something else, like
InitFromWebCursor.

http://codereview.chromium.org/15088/diff/1112/1122
File webkit/glue/webcursor_win.cc (right):

http://codereview.chromium.org/15088/diff/1112/1122#newcode238
Line 238: return;
so in the case of a non-deepcopy, what we mean is that this WebCursor is
dependent on the lifetime of the original WebCursor, right?

[ananta, 2008-12-24 00:32:54]

Based on our discussion, the WebCursor instance now does not attempt to copy the
external HCURSOR. The copy constructor/assignment operator functions copy the
contents of the passed in WebCursor instance and set the custom_cursor_ member
to NULL. This is to ensure that it gets created if required in the next call to
GetCursor.

Thanks
Ananta


On 2008/12/23 23:30:31, darin wrote:
> things are getting complicated.  maybe a better solution would be to put
> external_cursor_ and custom_cursor_ inside a reference counted inner class?
> 
> http://codereview.chromium.org/15088/diff/1112/1119
> File webkit/glue/webcursor.cc (right):
> 
> http://codereview.chromium.org/15088/diff/1112/1119#newcode35
> Line 35: Copy(other, false);
> what happens if |other| gets destroyed?  won't this object hold references to
> junk HCURSORs?
> 
> http://codereview.chromium.org/15088/diff/1112/1121
> File webkit/glue/webcursor.h (right):
> 
> http://codereview.chromium.org/15088/diff/1112/1121#newcode46
> Line 46: // Copy constructor/assigment operator combine. We need to ensure
> nit: "assignment"
> 
> http://codereview.chromium.org/15088/diff/1112/1121#newcode73
> Line 73: void Assign(const WebCursor& other);
> i hate to keep feeding tweaks, but since you now have an assignment operator,
it
> probably would be nice to rename this Assign method to something else, like
> InitFromWebCursor.
> 
> http://codereview.chromium.org/15088/diff/1112/1122
> File webkit/glue/webcursor_win.cc (right):
> 
> http://codereview.chromium.org/15088/diff/1112/1122#newcode238
> Line 238: return;
> so in the case of a non-deepcopy, what we mean is that this WebCursor is
> dependent on the lifetime of the original WebCursor, right?

[darin (slow to review), 2008-12-24 00:45:04]

LGTM w/ these issues resolved.  Feel free to postpone the ToPlatformCursorType
removal to a future CL if you think that idea is good.

http://codereview.chromium.org/15088/diff/1192/1422
File webkit/glue/webcursor.cc (right):

http://codereview.chromium.org/15088/diff/1192/1422#newcode39
Line 39: Clear();
you need to add this to the top of the method:

  if (this == &other)
    return *this;

otherwise, the Clear() call will trash the "other" object as well.

http://codereview.chromium.org/15088/diff/1192/1424
File webkit/glue/webcursor.h (right):

http://codereview.chromium.org/15088/diff/1192/1424#newcode48
Line 48: const WebCursor& operator = (const WebCursor& other);
style nit: no whitespace around the "="

http://codereview.chromium.org/15088/diff/1192/1424#newcode69
Line 69: // Initialize this from the given Windows cursor.
nit: please add a note about how the caller must ensure that the given HCURSOR
remains valid for the lifetime of the WebCursor.

http://codereview.chromium.org/15088/diff/1192/1425
File webkit/glue/webcursor_win.cc (right):

http://codereview.chromium.org/15088/diff/1192/1425#newcode193
Line 193: WebCore::PlatformCursor::Type cursor_type =
ToPlatformCursorType(cursor);
nit: can we just ditch ToPlatformCursorType now?  it seems like for stock
cursors, we can still just get away with saving the HCURSOR to external_cursor_.

http://codereview.chromium.org/15088/diff/1192/1425#newcode227
Line 227: custom_cursor_ = NULL;
please add a note about how custom_cursor_ will just be re-created from
custom_data_ if needed.

[ananta, 2008-12-24 01:24:24]

On 2008/12/24 00:45:04, darin wrote:
> LGTM w/ these issues resolved.  Feel free to postpone the ToPlatformCursorType
> removal to a future CL if you think that idea is good.

I think it is a good idea and will do that in a future CL. We could
probably treat any HCURSOR as an external cursor and always set the
WebCursor type to custom. This needs a little more testing.

> 
> http://codereview.chromium.org/15088/diff/1192/1422
> File webkit/glue/webcursor.cc (right):
> 
> http://codereview.chromium.org/15088/diff/1192/1422#newcode39
> Line 39: Clear();
> you need to add this to the top of the method:
> 
>   if (this == &other)
>     return *this;
> 
> otherwise, the Clear() call will trash the "other" object as well.

Done.

> 
> http://codereview.chromium.org/15088/diff/1192/1424
> File webkit/glue/webcursor.h (right):
> 
> http://codereview.chromium.org/15088/diff/1192/1424#newcode48
> Line 48: const WebCursor& operator = (const WebCursor& other);
> style nit: no whitespace around the "="

Done.

> 
> http://codereview.chromium.org/15088/diff/1192/1424#newcode69
> Line 69: // Initialize this from the given Windows cursor.
> nit: please add a note about how the caller must ensure that the given HCURSOR
> remains valid for the lifetime of the WebCursor.

Done.

> 
> http://codereview.chromium.org/15088/diff/1192/1425
> File webkit/glue/webcursor_win.cc (right):
> 
> http://codereview.chromium.org/15088/diff/1192/1425#newcode193
> Line 193: WebCore::PlatformCursor::Type cursor_type =
> ToPlatformCursorType(cursor);
> nit: can we just ditch ToPlatformCursorType now?  it seems like for stock
> cursors, we can still just get away with saving the HCURSOR to
external_cursor_.

The comments above regarding this apply here.


> 
> http://codereview.chromium.org/15088/diff/1192/1425#newcode227
> Line 227: custom_cursor_ = NULL;
> please add a note about how custom_cursor_ will just be re-created from
> custom_data_ if needed.

Done.

[ananta, 2008-12-30 21:12:40]

Hi Darin

Based on our discussion last week about intercepting the SetCursor API
for windowless plugins and preventing the cursor from actually being set as the
browser UI thread would set it anyways. I have updated the CB with these
changes. This fixes the annoying cursor flicker when the mouse is moved on the
foreground tab with the windowless flash instance in the background tab
constantly invoking the SetCursor API.

Thanks
Ananta


On 2008/12/24 01:24:24, ananta wrote:
> On 2008/12/24 00:45:04, darin wrote:
> > LGTM w/ these issues resolved.  Feel free to postpone the
ToPlatformCursorType
> > removal to a future CL if you think that idea is good.
> 
> I think it is a good idea and will do that in a future CL. We could
> probably treat any HCURSOR as an external cursor and always set the
> WebCursor type to custom. This needs a little more testing.
> 
> > 
> > http://codereview.chromium.org/15088/diff/1192/1422
> > File webkit/glue/webcursor.cc (right):
> > 
> > http://codereview.chromium.org/15088/diff/1192/1422#newcode39
> > Line 39: Clear();
> > you need to add this to the top of the method:
> > 
> >   if (this == &other)
> >     return *this;
> > 
> > otherwise, the Clear() call will trash the "other" object as well.
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/1192/1424
> > File webkit/glue/webcursor.h (right):
> > 
> > http://codereview.chromium.org/15088/diff/1192/1424#newcode48
> > Line 48: const WebCursor& operator = (const WebCursor& other);
> > style nit: no whitespace around the "="
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/1192/1424#newcode69
> > Line 69: // Initialize this from the given Windows cursor.
> > nit: please add a note about how the caller must ensure that the given
HCURSOR
> > remains valid for the lifetime of the WebCursor.
> 
> Done.
> 
> > 
> > http://codereview.chromium.org/15088/diff/1192/1425
> > File webkit/glue/webcursor_win.cc (right):
> > 
> > http://codereview.chromium.org/15088/diff/1192/1425#newcode193
> > Line 193: WebCore::PlatformCursor::Type cursor_type =
> > ToPlatformCursorType(cursor);
> > nit: can we just ditch ToPlatformCursorType now?  it seems like for stock
> > cursors, we can still just get away with saving the HCURSOR to
> external_cursor_.
> 
> The comments above regarding this apply here.
> 
> 
> > 
> > http://codereview.chromium.org/15088/diff/1192/1425#newcode227
> > Line 227: custom_cursor_ = NULL;
> > please add a note about how custom_cursor_ will just be re-created from
> > custom_data_ if needed.
> 
> Done.

[darin (slow to review), 2009-01-09 00:55:02]

LGTM

http://codereview.chromium.org/15088/diff/1690/1694
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/15088/diff/1690/1694#newcode275
Line 275: // and remember the cursor being set. This is shipped over  to the
browser
nit: kill extra space between "over" and "to"

http://codereview.chromium.org/15088/diff/1690/1694#newcode1168
Line 1168: return GetCursor();
hmm... so we are effectively making it look to the plugin as though the
SetCursor call did nothing because we return GetCursor() instead of "cursor"
here.  Maybe this deserves a comment.  It might be reasonable to return "cursor"
instead to make it look like the cursor got set.

[ananta, 2009-01-09 01:22:28]

On 2009/01/09 00:55:02, darin wrote:
> LGTM
> 
> http://codereview.chromium.org/15088/diff/1690/1694
> File webkit/glue/plugins/webplugin_delegate_impl.cc (right):
> 
> http://codereview.chromium.org/15088/diff/1690/1694#newcode275
> Line 275: // and remember the cursor being set. This is shipped over  to the
> browser
> nit: kill extra space between "over" and "to"

Done

> 
> http://codereview.chromium.org/15088/diff/1690/1694#newcode1168
> Line 1168: return GetCursor();
> hmm... so we are effectively making it look to the plugin as though the
> SetCursor call did nothing because we return GetCursor() instead of "cursor"
> here.  Maybe this deserves a comment.  It might be reasonable to return
"cursor"
> instead to make it look like the cursor got set.
I changed it to return the previous cursor from the WebCursor member in the
plugin before updating it to the new cursor.

Submitted based on our discussion.