Bug 96151 - [chromium] Implement WebCompositorInputHandlerImpl on top of exposed API instead of CC internals
Summary: [chromium] Implement WebCompositorInputHandlerImpl on top of exposed API inst...
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: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-07 15:45 PDT by James Robinson
Modified: 2012-09-07 18:22 PDT (History)
6 users (show)

See Also:

Attachments
Patch (79.55 KB, patch)
2012-09-07 15:49 PDT, James Robinson 		no flags Details | Formatted Diff | Diff
checks bindToClient called once, asserts enums match, puts WCIHI in correct gyp section (81.32 KB, patch)
2012-09-07 16:45 PDT, James Robinson 		enne: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-09-07 15:45:39 PDT 
[chromium] Implement WebCompositorInputHandlerImpl on top of exposed API instead of CC internals
Comment 1 James Robinson 2012-09-07 15:49:21 PDT 
Created attachment 162890 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-07 15:53:29 PDT 
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adrienne Walker 2012-09-07 16:20:50 PDT 
Comment on attachment 162890 [details]
Patch

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

There's a lot of code change, but it seems pretty straightforward.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:237
> +    TRACE_EVENT_INSTANT0("cc", "WebCompositorInputHandlerImpl::bindToClient");
> +    if (!s_compositors)
> +        s_compositors = new HashSet<WebCompositorInputHandlerImpl*>;
> +    s_compositors->add(this);

Now that this doesn't happen on construction, do you want some check that bindToClient is only called once?

> Source/WebKit/chromium/src/WebToCCInputHandlerAdapter.cpp:60
> +        return static_cast<WebInputHandlerClient::ScrollStatus>(m_client->scrollBegin(point, static_cast<WebCore::CCInputHandlerClient::ScrollInputType>(type)));

Can you add compile-time asserts that these enums are the same?
Comment 4 James Robinson 2012-09-07 16:24:39 PDT 
(In reply to comment #3)
> (From update of attachment 162890 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162890&action=review
> 
> There's a lot of code change, but it seems pretty straightforward.
> 
> > Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:237
> > +    TRACE_EVENT_INSTANT0("cc", "WebCompositorInputHandlerImpl::bindToClient");
> > +    if (!s_compositors)
> > +        s_compositors = new HashSet<WebCompositorInputHandlerImpl*>;
> > +    s_compositors->add(this);
> 
> Now that this doesn't happen on construction, do you want some check that bindToClient is only called once?

Yeah, good idea.

> 
> > Source/WebKit/chromium/src/WebToCCInputHandlerAdapter.cpp:60
> > +        return static_cast<WebInputHandlerClient::ScrollStatus>(m_client->scrollBegin(point, static_cast<WebCore::CCInputHandlerClient::ScrollInputType>(type)));
> 
> Can you add compile-time asserts that these enums are the same?

Sure - just have to figure out how those macros work
Comment 5 James Robinson 2012-09-07 16:45:50 PDT 
Created attachment 162905 [details]
checks bindToClient called once, asserts enums match, puts WCIHI in correct gyp section
Comment 6 Adrienne Walker 2012-09-07 18:17:18 PDT 
Comment on attachment 162905 [details]
checks bindToClient called once, asserts enums match, puts WCIHI in correct gyp section

R=me.
Comment 7 James Robinson 2012-09-07 18:22:24 PDT 
Committed r127948: <http://trac.webkit.org/changeset/127948>