[webkit-reviews] review denied: [Bug 94182] [chromium] Implement Link Preview (a.k.a. on-demand zoom) : [Attachment 158702] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tien-Ren Chen <trchen at chromium.org>'s
request for review:
Bug 94182: [chromium] Implement Link Preview (a.k.a. on-demand zoom)
https://bugs.webkit.org/show_bug.cgi?id=94182

Attachment 158702: Patch
https://bugs.webkit.org/attachment.cgi?id=158702&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=158702&action=review


This review is mostly questions.  The the main thing I'd like to change is to
remove generatedByLinkPreview.	Is there a way around adding that state to
WebKit?

>> Source/WebKit/chromium/public/WebInputEvent.h:382
>> +	bool generatedByLinkPreview;
> 
> Here's a thought.  This value could be replaced by "float errorDistance;" 
The browser could send down a value corresponding to basically the size of a
finger and it could feed into the scoring function.  When the tap already has
been disambiguated, the error can be set to zero.

Is there a way to keep this state in Chromium?	It's not really a concern of
WebKit...  All that might be required is for the RenderView to remember whether
it is currently showing a link previous popup.

> Source/WebKit/chromium/public/WebViewClient.h:290
> +    // Return true if the embedder will start a link preview so the input
event will be swallowed
> +    virtual bool triggersLinkPreview(const WebRect& windowZoomRect) { return
false; }

triggersLinkPreview  ->  shouldCauseLinkDisambiguation ?

What is windowZoomRect?  How can a rect cause link disambiguation?  This is
probably fine, it's just a bit confusing.

> Source/WebKit/chromium/src/WebViewImpl.cpp:710
> +bool isEventNode(Node *node)
> +{
> +    return node && (node->supportsFocus()
> +	   || node->hasEventListeners(eventNames().clickEvent)
> +	   || node->hasEventListeners(eventNames().mousedownEvent)
> +	   || node->hasEventListeners(eventNames().mouseupEvent));
> +}

Should this be in WebCore?  It's not really specific to Chromium and it doesn't
really seem like a concern of the entire WebView.

It's tempting to implement every feature in WebViewImpl.cpp, but if we did
that, WebViewImpl.cpp would grow to be even more huge than it is today.

> Source/WebKit/chromium/src/WebViewImpl.cpp:713
> +IntRect calculateEventNodeBoundingBox(Node* eventNode)

Again, this doesn't seem specific to Chromium and seems like something that
could live in WebCore somewhere.

> Source/WebKit/chromium/src/WebViewImpl.cpp:732
> +float scoreTouchTarget(IntPoint touchPoint, int padding, IntRect
boundingBox)

Should this be in a separate file in WebKit/chromium/src ?  We try to avoid
putting everything in WebViewImpl.cpp.

> Source/WebKit/chromium/src/WebViewImpl.cpp:772
> +    // Find event handler node in the ancestor chain for each hit test
result

I wonder if a bunch of this should be moved into the same file as
scoreTouchTarget

> Source/WebKit/chromium/src/WebViewImpl.cpp:778
> +	       if (node->isDocumentNode() ||
node->hasTagName(HTMLNames::htmlTag) || node->hasTagName(HTMLNames::bodyTag))
> +		   break;

How does this work for SVG documents?

> Source/WebKit/chromium/src/WebViewImpl.cpp:798
> +    // Only keep good touch targets with score >= max(score)/2, and
calculate the zoom rect
> +    int numberOfGoodTargets = 0;
> +    IntRect windowZoomRect = IntRect(touchPoint, IntSize(1, 1));
> +    windowZoomRect.inflate(touchPointPadding);
> +    for (HashMap<Node*, TouchTargetData>::iterator it =
touchTargets.begin(); it != touchTargets.end(); ++it) {
> +	   if (it->second.score < bestScore * 0.5)
> +	       continue;
> +	   numberOfGoodTargets++;
> +	   windowZoomRect.unite(it->second.windowBoundingBox);
> +    }

This too.

> Source/WebKit/chromium/src/WebViewImpl.cpp:800
> +    // TODO: replace touch adjustment code when numberOfGoodTargets == 1?

TODO -> FIXME

> Source/WebKit/chromium/src/WebViewImpl.cpp:804
> +    return m_client && m_client->triggersLinkPreview(windowZoomRect);

I see.	So windowZoomRect is something like a bounding box of the touch
targets.

> Source/WebKit/chromium/src/WebViewImpl.cpp:833
> +	   if (!event.generatedByLinkPreview) {

Maybe the Chromium side can use its own version of this state to answer
triggersLinkPreview or another client call made here?