[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





--- Comment #8 from Tien-Ren Chen <trchen at chromium.org>  2012-08-20 15:02:18 PST ---
(In reply to comment #5)
> (From update of attachment 158702 [details])
> 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.

After all I think we don't have to add another flag. I think we can reuse WebGestureEvent.boundingBox which stands for the touch point radius for the single tap events. For an artificial tap we can set that to a zero-sized rect.

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

I think maybe handleLinkDisambiguation would be better.

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

It is the bounding box of the touch target candidates, in window coordinate.

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

I think moving all these to WebCore/page/chromium/ may be a good idea.

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

This is a non-exhaustive list of elements we don't want to zoom to. We added those tags because we've seen a few dumb pages that adds event handlers on the page body and disambiguation popups become annoying. So far we haven't seen many dumb SVG documents but we might have to tune it if we get reports from QA.

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

Done

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