Bug 42236 - [Chromium] Replace ChromiumBridge::widgetSetCursor with ChromeClient::setCursor
Summary: [Chromium] Replace ChromiumBridge::widgetSetCursor with ChromeClient::setCursor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on: 42232
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-13 23:51 PDT by Darin Fisher (:fishd, Google)
Modified: 2011-01-18 13:02 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.97 KB, patch)
2011-01-14 16:42 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (5.63 KB, patch)
2011-01-15 13:37 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2010-07-13 23:51:56 PDT
Replace ChromiumBridge::widgetSetCursor with ChromeClient::setCursor
Comment 1 Adam Klein 2011-01-14 14:32:19 PST
Request for some clarification: the only call to ChromiumBridge::widgetSetCursor() is in Widget::setCursor() (WidgetChromium.cpp). But Widget itself doesn't have easy access to a ChromeClient.  Is the intention just to move ChromiumBridge's logic up into WidgetChromium (it involves using pseudo-RTTI and static_cast<FrameView*>(this)), or did you have something else in mind?
Comment 2 Adam Klein 2011-01-14 14:39:40 PST
Oh, I see that Widget actually has a nice root() method which does this for me.  Using that seems to be the way to go.
Comment 3 Darin Fisher (:fishd, Google) 2011-01-14 15:33:15 PST
Yes, you can see in ChromiumBridge.cpp how we get to the ChromeClientImpl class given only a Widget.  It used to be the case that ChromeClient did not have the setCursor method; however, it was added for WebKit2 (in r63339).  We can use ChromeClient from our WidgetChromium implementation to reach the new setCursor method.  We could not previously cast to ChromeClientImpl from WidgetChromium because WebCore cannot depend directly on code in WebKit.
Comment 4 Adam Klein 2011-01-14 16:42:45 PST
Created attachment 79024 [details]
Patch
Comment 5 Darin Fisher (:fishd, Google) 2011-01-14 16:57:26 PST
Comment on attachment 79024 [details]
Patch

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

> Source/WebCore/platform/chromium/WidgetChromium.cpp:63
> +    FrameView* view;

Sorry, I just realized there is a much simpler way to achieve this!

HostWindow also gained a setCursor method, and you can get the HostWindow
from a Widget's parent ScrollView.  That will dramatically simplify this
code.
Comment 6 Darin Fisher (:fishd, Google) 2011-01-14 16:58:15 PST
The HostWindow approach makes more sense too because there is a "rule" that WebCore/platform/ should not depend back onto code outside of WebCore/platform/ (with the exception of wtf/).
Comment 7 Adam Klein 2011-01-15 13:37:19 PST
Created attachment 79075 [details]
Patch
Comment 8 Adam Klein 2011-01-15 13:38:42 PST
Indeed, much simpler.  And it matches with the other platforms do.
Comment 9 WebKit Commit Bot 2011-01-18 13:02:50 PST
Comment on attachment 79075 [details]
Patch

Clearing flags on attachment: 79075

Committed r76048: <http://trac.webkit.org/changeset/76048>
Comment 10 WebKit Commit Bot 2011-01-18 13:02:56 PST
All reviewed patches have been landed.  Closing bug.