Bug 92997

Summary: [Chromium] Add a stub for WebView::getTouchHighlightQuads()
Product: WebKit Reporter: Peter Beverloo <peter>
Component: WebKit APIAssignee: Peter Beverloo <peter>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, jamesr, tkent+wkapi, trchen, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687, 79150, 91648    
Attachments:
Description Flags
Patch
none
Patch none

Description Peter Beverloo 2012-08-02 08:59:57 PDT
Patch coming up.
Comment 1 Peter Beverloo 2012-08-02 09:09:22 PDT
Created attachment 156104 [details]
Patch
Comment 2 Peter Beverloo 2012-08-02 09:10:16 PDT
I'm not sure about the #if-guards as no others are used in WebView.h. Should I remove them?
Comment 3 WebKit Review Bot 2012-08-02 09:11:38 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 4 Adam Barth 2012-08-02 09:22:21 PDT
Comment on attachment 156104 [details]
Patch

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

> Source/Platform/chromium/public/WebTouchCandidatesInfo.h:48
> +    WebTouchCandidatesInfo()
> +        : numberOfCandidates(0),
> +        : unitedBounds(),
> +        : smallestDimension(0)

This isn't quite right.  Try:

WebTouchCandidatesInfo()
    : numberOfCandidates(0)
    , smallestDimension(0)

> Source/WebKit/chromium/public/WebView.h:63
> +#if defined(OS_ANDROID)

These ifdefs aren't needed.

> Source/WebKit/chromium/public/WebView.h:474
> +    // Touch ----------------------------------------------------------------

Two blank lines above section headings.

> Source/WebKit/chromium/public/WebView.h:476
> +#if defined(OS_ANDROID)

This #ifdef isn't needed.

> Source/WebKit/chromium/public/WebView.h:483
> +    virtual WebVector<WebFloatQuad> getTouchHighlightQuads(const WebPoint&,
> +                                                           int padding,

Should we use a WebRect instead?

> Source/WebKit/chromium/public/WebView.h:485
> +                                                           WebTouchCandidatesInfo* outTouchInfo,
> +                                                           WebColor& outTapHighlightColor) = 0;

WebTouchCandidatesInfo* -> WebTouchCandidatesInfo& (we use non-const reference types for out parameters.
Comment 5 Peter Beverloo 2012-08-02 09:37:02 PDT
Comment on attachment 156104 [details]
Patch

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

Thank you for the review! Could you take a look at the updated patch too, please?

>> Source/Platform/chromium/public/WebTouchCandidatesInfo.h:48
>> +        : smallestDimension(0)
> 
> This isn't quite right.  Try:
> 
> WebTouchCandidatesInfo()
>     : numberOfCandidates(0)
>     , smallestDimension(0)

Uch, bad one. Thanks. I've added an include to this file in WebViewImpl.h to make sure that it's being compiled.

>> Source/WebKit/chromium/public/WebView.h:63
>> +#if defined(OS_ANDROID)
> 
> These ifdefs aren't needed.

Done.

>> Source/WebKit/chromium/public/WebView.h:474
>> +    // Touch ----------------------------------------------------------------
> 
> Two blank lines above section headings.

Done.

>> Source/WebKit/chromium/public/WebView.h:476
>> +#if defined(OS_ANDROID)
> 
> This #ifdef isn't needed.

Done.

>> Source/WebKit/chromium/public/WebView.h:483
>> +                                                           int padding,
> 
> Should we use a WebRect instead?

I'd prefer not to change the API too much if it's likely to change as it is. Tien-Ren will have a much better idea than I do about what is necessary here.

>> Source/WebKit/chromium/public/WebView.h:485
>> +                                                           WebColor& outTapHighlightColor) = 0;
> 
> WebTouchCandidatesInfo* -> WebTouchCandidatesInfo& (we use non-const reference types for out parameters.

Done.
Comment 6 Peter Beverloo 2012-08-02 09:37:43 PDT
Created attachment 156109 [details]
Patch
Comment 7 WebKit Review Bot 2012-08-02 10:40:43 PDT
Comment on attachment 156109 [details]
Patch

Clearing flags on attachment: 156109

Committed r124478: <http://trac.webkit.org/changeset/124478>
Comment 8 WebKit Review Bot 2012-08-02 10:40:48 PDT
All reviewed patches have been landed.  Closing bug.