Bug 38488

Created attachment 54961 [details] patch v1 Adds a getter for the "fudgeFactor"

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.

(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).

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

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

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.

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?

(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

(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+ :-)

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>

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