[Webkit-unassigned] [Bug 94182] [chromium] Implement Link Preview (a.k.a. on-demand zoom)

[bugzilla-daemon at webkit.org]

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


Adam Barth <abarth at webkit.org> changed:

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




--- Comment #5 from Adam Barth <abarth at webkit.org>  2012-08-19 21:04:06 PST ---
(From update of attachment 158702)
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?

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