Bug 92997 – [Chromium] Add a stub for WebView::getTouchHighlightQuads()

• Component: WebKit API
• Status: RESOLVED FIXED

[Peter Beverloo]

Patch coming up.

[Peter Beverloo]

Created attachment 156104 [details]
Patch

[Peter Beverloo]

I'm not sure about the #if-guards as no others are used in WebView.h. Should I remove them?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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.

[Peter Beverloo]

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.

[Peter Beverloo]

Created attachment 156109 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156109 [details]
Patch

Clearing flags on attachment: 156109

Committed r124478: <http://trac.webkit.org/changeset/124478>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.