[Webkit-unassigned] [Bug 79150] [chromium] Add Link Preview behavior to WebViewImpl

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79150


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #128058|review?                     |review-
               Flag|                            |




--- Comment #7 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2012-02-23 22:47:51 PST ---
(From update of attachment 128058)
View in context: https://bugs.webkit.org/attachment.cgi?id=128058&action=review

>> Source/WebKit/chromium/public/WebView.h:35
>> +#if defined(ANDROID)
> 
> This functionality probably shouldn't be dependent on ANDROID.  It should be available all the time.

Agreed.

> Source/WebKit/chromium/public/WebView.h:44
> +struct WebTouchCandidatesInfo {

style nits:
1- each top-level type gets its own header file.
2- this struct should be declared within the WebKit namespace.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3183
> +struct TouchNodeData {

This is way too much code to be adding to WebView.  You need to refactor this into some self-contained piece.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3184
> +    Node* mNode;

style nit: public member variables do not have a "m" prefix.  they just use normal variable naming style.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3318
> +    outTouchInfo->numberOfCandidates = 0;

nit: this function is massive.  it needs to be decomposed into smaller units.

I'm not sure how to best advise you on improving the design here.  It might help
to step back and have you educate a WebKit reviewer about what you are trying to
accomplish.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.