[Webkit-unassigned] [Bug 94182] [chromium] Implement Disambiguation Popup (a.k.a. link preview, on-demand zoom)

Tue Aug 28 14:58:27 PDT 2012

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

--- Comment #14 from Tien-Ren Chen <trchen at chromium.org>  2012-08-28 14:58:29 PST ---
(In reply to comment #11)
> (From update of attachment 160927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160927&action=review
> 
> > Source/WebCore/page/TouchDisambiguation.cpp:46
> > +namespace {
> 
> Generally, WebKit prefers to use file-level static functions rather than anonymous namespaces.

Okay, but I'll need an anonymous namespace for struct TouchTargetData anyway. Do we still prefer static function in this case?

> > Source/WebCore/page/TouchDisambiguation.cpp:54
> > +bool isEventNode(Node *node)
> > +{
> > +    return node && (node->supportsFocus()
> > +        || node->hasEventListeners(eventNames().clickEvent)
> > +        || node->hasEventListeners(eventNames().mousedownEvent)
> > +        || node->hasEventListeners(eventNames().mouseupEvent));
> > +}
> 
> This looks like it should be a member function on Node.  See also the discussion in https://bugs.webkit.org/show_bug.cgi?id=94727#c6
> 
> It sounds like there's already a willRespondToMouseClickEvents.  Should we use that instead?  If not, we should move this code near that code.

Looks interesting. There is some subtle difference but generally it catches everything we want to catch. I think we can try that first.

> > Source/WebCore/page/TouchDisambiguation.cpp:56
> > +// Calculate the union of the bounding boxes of all descendant nodes that propagates events up.
> 
> This comment just explains what the code does, not why it does it.  We should remove it.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:57
> > +IntRect calculateEventNodeBoundingBox(Node* eventNode)
> 
> The word "calculate" doesn't really add anything to the name of this function.  Perhaps "boundingBoxForDescendantsThatRespondToMouseEvents" ?  That might be a bit long...  boundingBoxForEventNodes ?

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:60
> > +    if (!eventNode->document() || !eventNode->document()->view())
> > +        return IntRect();
> 
> eventNode->document() can only be 0 for DocumentType nodes.  It seems unlikely eventNode can ever be a DocumentType node (i.e. <!DOCTYPE html>).  We should remove the 0 check.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:65
> > +        // skip the whole sub-tree if the node doesn't propagate events
> 
> Please use proper sentence punctuation, including initial capital letters and trailing periods.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:78
> > +    float divPadding = (float)1 / padding;
> 
> What does "divPadding" mean?

Cached reciprocal for DIVision ... Yea div can be confused with <div>, let me change this.

> > Source/WebCore/page/TouchDisambiguation.cpp:82
> > +    if (boundingBox.isEmpty())
> > +        return 0;
> 
> Perhaps this should be the first statement in the function.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:98
> > +void listGoodTouchTargets(Vector<IntRect>& goodTargets, const IntRect& touchBox, Frame* mainFrame, float pageScaleFactor)
> 
> Typically we place the out parameters at the end of the parameter list.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:102
> > +    int touchPointPadding = (max(touchBox.width(), touchBox.height()) + 1) / 2;
> 
> Why do we add 1?

Rounds up

> > Source/WebCore/page/TouchDisambiguation.cpp:104
> > +    //         We have to pre-apply page scale factor here.
> 
> You've got an extra space before the "we".

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:105
> > +    int padding = touchPointPadding / pageScaleFactor;
> 
> Should touchPointPadding be a float so we don't lose precision?

I think it's ok. The semantics is more like padding=pixelSnapped(touchPointPadding.scale(1/pageScaleFactor)) as in transformation. We do need to make it round up though.

> > Source/WebCore/page/TouchDisambiguation.cpp:113
> > +    // Find event handler node in the ancestor chain for each hit test result
> 
> This comment just says what the code does, not why.  We should remove it.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:130
> > +    // Only keep good touch targets with score >= max(score)/2, and calculate the zoom rect
> 
> ditto

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:132
> > +        if (it->second.score < bestScore * 0.5)
> 
> I might add a comment explaining why this is a good heuristic.

Done

> > Source/WebCore/page/TouchDisambiguation.h:41
> > +void listGoodTouchTargets(Vector<IntRect>& goodTargets, const IntRect& touchBox, Frame* mainFrame, float pageScaleFactor);
> 
> list -> find

Done

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