Bug 92997

Created attachment 156104 [details] Patch

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

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 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 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.

Created attachment 156109 [details] Patch

Comment on attachment 156109 [details] Patch Clearing flags on attachment: 156109 Committed r124478: <http://trac.webkit.org/changeset/124478>

All reviewed patches have been landed. Closing bug.