556016 - Fennec should use NodesFromRect

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip

This is a wip (with patch v5 from bug 489127 applied on trunk) for the front end part of fennec.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-2

This one descend into iframes but still fail when it is times to dispatch the click into the iframe.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch

This patch probably need some code cleanup (i guess mostly function rename), but it works on the (future) version of nodesFromRect.

Mark, if you want to test you can apply the patch for bug 489127 and just change the line:
(!ptContent->IsNodeOfType(nsINode::eELEMENT | nsINode::eTEXT) ||

to 
(!(ptContent->IsNodeOfType(nsINode::eELEMENT) || ptContent->IsNodeOfType(nsINode::eTEXT)) ||

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

I have update the function name and use a utility provide by the Browser object to retrieve the position of an element nested into an iframe

Comment 4

[:Felipe Gomes (needinfo for replies!)]

I'm not quite updated on the logic of panning code, but if elementFromPoint is in the path of perfomance sensitive code, I think you shouldn't change the current elementFromPoint, but rather create a special one and only use it when dispatching single clicks.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

hmm well, actually, I think what I just said is not very important.
Internally elementFromPoint will go to the same GetFramesForArea path, so the savings probably won't be so relevant (it would just avoid the isClickableElement check and distance calc if it's clickable)

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch updated on trunk

updated patch on trunk

Felipe, about the performance problem, don't worry about it, it looks like i was using a rotten build yesterday because with the platform nodesFromRect's patch applied everything works fine without the perfs problem I was seeing yesterday. Sorry for that.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 436669 [details] [diff] [review]
Patch updated on trunk

>diff -r 1dfc6b80394e chrome/content/browser.js

>+  getClosestElement: function getClosestElement(aWindowUtils, aX, aY) {
>+    let touchRadius = gPrefService.getIntPref("browser.ui.touch.radius");
>+    let tapRadius = Math.min(64.0, touchRadius / Math.pow(this._browserView.getZoomLevel(), 2));
>+
>+    let nodes = aWindowUtils.nodesFromRect(aX, aY, 0.75 * tapRadius,
>+                                                   0.50 * tapRadius,
>+                                                   0.25 * tapRadius,
>+                                                   0.50 * tapRadius, true, false);

What do the 0.75, 0.5, 0.25 and 0.5 multipliers do? Perhaps a comment would be helpful. Maybe even consts.

>+    let target = null, treshold = Number.POSITIVE_INFINITY;

"treshold" -> "threshold"

>+        distance *= 1.2;
>+
>+      if (distance < treshold) {

and here

>+  _computeDistanceFromRect: function _getDistanceFromRect(aX, aY, aRect) {

sync up the function name

>+    let x = 0, y =0;

y = 0

>+    let xmost = aRect.left + aRect.width;
>+    let ymost = aRect.top + aRect.height;

xmax and ymax ?

>+    if (aRect.left < aX && aX < xmost)
>+      x = Math.min(xmost - aX, aX - aRect.left);
>+    else if (aX < aRect.left)
>+      x = aRect.left - aX;
>+    else if (aX > xmost)
>+      x = aX - xmost;
>+
>+    if (aRect.top < aY && aY < ymost)
>+      y = Math.min(ymost - aY, aY - aRect.top);
>+    else if (aY < aRect.top)
>+      y = aRect.top - aY;
>+    if (aY > ymost)
>+      y = aY - ymost;

You could use a comment to help clarify what you are doing here

I almost want the core code you added in a separate class, just to keep Browser from getting to big.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Address review comments.

>>+    let xmost = aRect.left + aRect.width;
>>+    let ymost = aRect.top + aRect.height;

>xmax and ymax ?

I could change them if you think it's easier to understand but I've take these names from the code of nsRect.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 437869 [details] [diff] [review]
Patch v0.2

I'll probably change LinkTappingHelper to ElementTapHelper, but looks good. Thanks for the changes.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

I think we want to re-examine the "shift" multipliers, but this looks good and works fine on 192 (without nodesFromRect support)

pushed:
http://hg.mozilla.org/mobile-browser/rev/4c037361e6f9

Comment 11

[Dão Gottwald [:dao]]

bugspam