Bug 38488 – Spatial Navigation: create a getter for the "fudgeFactor"

• Component: Accessibility
• Status: CLOSED FIXED

[Antonio Gomes]

A couple of places in the Spatial Navigation code make use of a "fudge factor" to improve precision by working around outline focus metrics and such.

Currently each function using that has its own fudgeFactor variable declared locally. It makes sense to provide a getter for this so when it is to be changed it needs changing in one place only.

patch coming ...

[Antonio Gomes]

Created attachment 54961 [details]
patch v1

Adds a getter for the "fudgeFactor"

[Kenneth Rohde Christiansen]

Comment on attachment 54961 [details]
patch v1

WebCore/page/SpatialNavigation.cpp:481
 +      bounds.inflate(-fudgeFactor());
Maybe the minus here needs a comment, and you use it positively elsewhere.

[Antonio Gomes]

(In reply to comment #2)
> (From update of attachment 54961 [details])
> WebCore/page/SpatialNavigation.cpp:481
>  +      bounds.inflate(-fudgeFactor());
> Maybe the minus here needs a comment, and you use it positively elsewhere.

In fact, the fudgetFactor is a negative value. See 

+inline int fudgeFactor()
+{
+    return -2;
+}

so in this case, I am really inflating the rect by doing inflate(-1*(-2)) , while in the case of

static void deflateIfOverlapped(IntRect& a, IntRect& b)
{
(..)
-  static const int fudgeFactor = -2;
-
   // Avoid negative width or height values.
-  if ((a.width() + 2 * fudgeFactor > 0) && (a.height() + 2 * fudgeFactor > 0))
-    a.inflate(fudgeFactor);
+  if ((a.width() + 2 * fudgeFactor() > 0) && (a.height() + 2 * fudgeFactor() > 0))
+    a.inflate(fudgeFactor());

... I am deflating the rects. Since IntRect has no deflate() method, I do inflate(negative_value).

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 54961 [details] [details])
> > WebCore/page/SpatialNavigation.cpp:481
> >  +      bounds.inflate(-fudgeFactor());
> > Maybe the minus here needs a comment, and you use it positively elsewhere.
> 
> In fact, the fudgetFactor is a negative value. See 
> 
> +inline int fudgeFactor()
> +{
> +    return -2;
> +}
> 
> so in this case, I am really inflating the rect by doing inflate(-1*(-2)) ,
> while in the case of
> 
> static void deflateIfOverlapped(IntRect& a, IntRect& b)
> {
> (..)
> -  static const int fudgeFactor = -2;
> -
>    // Avoid negative width or height values.
> -  if ((a.width() + 2 * fudgeFactor > 0) && (a.height() + 2 * fudgeFactor > 0))
> -    a.inflate(fudgeFactor);
> +  if ((a.width() + 2 * fudgeFactor() > 0) && (a.height() + 2 * fudgeFactor() >
> 0))
> +    a.inflate(fudgeFactor());
> 
> ... I am deflating the rects. Since IntRect has no deflate() method, I do
> inflate(negative_value).

I would prefer you using a positive fudgeFactor (btw that is not really a factor, but a constant) and add the minuses in the code itself. You could even make fudgeFactor return a unsigned int.

[Antonio Gomes]

Created attachment 55409 [details]
patch v2


> I would prefer you using a positive fudgeFactor (btw that is not really a
> factor, but a constant) and add the minuses in the code itself. You could even
> make fudgeFactor return a unsigned int.

done

[Antonio Gomes]

Created attachment 55443 [details]
(committed with r59045, reviewed by Kenneth) patch v2.1 - same as v2, but fixing a minor

Patch 55409 (v2) was doing a assignment of a negative value to a unsigned int variable. patch 2.1 fixes that. Other than that it is the same.

[Kenneth Rohde Christiansen]

Comment on attachment 55443 [details]
(committed with r59045, reviewed by Kenneth) patch v2.1 - same as v2, but fixing a minor

r=me, though I would still prefer another name, it is not a factor as you are not really multiplying with it, but just doing inflate/deflate. Maybe fudgeThickness?

[Antonio Gomes]

(In reply to comment #7)
> (From update of attachment 55443 [details])
> r=me, though I would still prefer another name, it is not a factor as you are
> not really multiplying with it, but just doing inflate/deflate. Maybe
> fudgeThickness?

Hi Kenne, thank you for looking at the patch. 

fudgeThickness works for me here , but the expression/term "fudge factor" is being used in many other parts of WebCore with the same meaning. I'd rather like to keep it, unless you really think it needs change.

$ grep -nHRi fudge WebCore/ --exclude=*ChangeLog*
- WebCore/rendering/RenderLayer.cpp:3085:        result.inflate(view->maximalOutlineSize()); // Used to apply a fudge factor to dirty-rect checks on blocks/tables.
- WebCore/rendering/RenderView.h:190:    int m_maximalOutlineSize; // Used to apply a fudge factor to dirty-rect checks on blocks/tables.
- WebCore/rendering/RenderBlock.cpp:56:// Number of pixels to allow as a fudge factor when clicking above or below a line.
- WebCore/rendering/RenderBlock.cpp:57:// clicking up to verticalLineClickFudgeFactor pixels above a line will correspond to the closest point on the line.   
- WebCore/rendering/RenderBlock.cpp:58:static const int verticalLineClickFudgeFactor = 3;


$ grep -nHRi fudge WebKit --exclude=*ChangeLog*
- WebKit/mac/WebView/WebView.mm:5073:    // As a fudge factor, use 1000 instead of 1024, in case the reported byte

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 55443 [details] [details])
> > r=me, though I would still prefer another name, it is not a factor as you are
> > not really multiplying with it, but just doing inflate/deflate. Maybe
> > fudgeThickness?
> 
> Hi Kenne, thank you for looking at the patch. 
> 
> fudgeThickness works for me here , but the expression/term "fudge factor" is
> being used in many other parts of WebCore with the same meaning. I'd rather
> like to keep it, unless you really think it needs change.

OK fine, it is already r+ :-)

[Antonio Gomes]

Comment on attachment 55443 [details]
(committed with r59045, reviewed by Kenneth) patch v2.1 - same as v2, but fixing a minor

Clearing flags on attachment: 55443

Committed r59045: <http://trac.webkit.org/changeset/r59045>

[Simon Hausmann]

Revision r59045 cherry-picked into qtwebkit-2.0 with commit 836023e07365d07b7ab58a161939184e46375368