Avoid computing navigation data for nodes in the wrong navigation direction

Avoid computing navigation data for nodes in the wrong navigation direction

Spatial Navigation should consider only those nodes as candidate which are
exactly in the focus-direction. This implementation apply early stage
detection of probable candidate to avoid unwanted calculation.

BUG=467864
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192165

[Abhijeet Kandalkar Slow, 2015-03-17 09:59:36]

abhijeet.k@samsung.com changed reviewers:
+ c.shu@samsung.com, kolczyk@opera.com, vivekg@chromium.org

[Abhijeet Kandalkar Slow, 2015-03-17 10:21:02]

Please review patch

[c.shu, 2015-03-17 14:17:47]

On 2015/03/17 10:21:02, Abhijeet Kandalkar wrote:
> Please review patch

Please add layout tests to show the effect of this patch. Thanks.

[c.shu, 2015-03-17 14:20:14]

On 2015/03/17 14:17:47, c.shu wrote:
> On 2015/03/17 10:21:02, Abhijeet Kandalkar wrote:
> > Please review patch
> 
> Please add layout tests to show the effect of this patch. Thanks.

It seems the patch is for code improvement rather than behavior change. I take
it back. lgtm.

[Abhijeet Kandalkar Slow, 2015-03-18 10:27:51]

The CQ bit was checked by abhijeet.k@samsung.com

[commit-bot: I haz the power, 2015-03-18 10:28:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1010203002/1

[commit-bot: I haz the power, 2015-03-18 10:49:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-18 10:49:11]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[Abhijeet Kandalkar Slow, 2015-03-18 11:16:38]

abhijeet.k@samsung.com changed reviewers:
+ fs@opera.com, philipj@opera.com

[Abhijeet Kandalkar Slow, 2015-03-18 11:16:38]

Hi fs/philip
Could you please review patch.

[fs, 2015-03-18 14:02:11]

Please fix you description so that it's on the form:

My subject (short description)
<blank line>
Description...

https://codereview.chromium.org/1010203002/diff/1/Source/core/page/FocusContr...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1010203002/diff/1/Source/core/page/FocusContr...
Source/core/page/FocusController.cpp:889: if (!isRectInDirection(type,
current.rect, candidate.rect))
Could you perhaps make this the first thing in distanceDataForNode instead? (To
avoid exposing isRectInDirection.) There's not a whole lot of code being run
between here and the suggested placement, so I don't think it makes a big
different "optimization-wise".

[Abhijeet Kandalkar Slow, 2015-03-18 14:06:47]

On 2015/03/18 14:02:11, fs wrote:
> Please fix you description so that it's on the form:
> 
> My subject (short description)
> <blank line>
> Description...
> 
>
https://codereview.chromium.org/1010203002/diff/1/Source/core/page/FocusContr...
> File Source/core/page/FocusController.cpp (right):
> 
>
https://codereview.chromium.org/1010203002/diff/1/Source/core/page/FocusContr...
> Source/core/page/FocusController.cpp:889: if (!isRectInDirection(type,
> current.rect, candidate.rect))
> Could you perhaps make this the first thing in distanceDataForNode instead?
(To
> avoid exposing isRectInDirection.) There's not a whole lot of code being run
> between here and the suggested placement, so I don't think it makes a big
> different "optimization-wise".

Updated description. Please have look.

[fs, 2015-03-18 14:15:45]

On 2015/03/18 14:06:47, Abhijeet Kandalkar wrote:
> On 2015/03/18 14:02:11, fs wrote:
> > Please fix you description so that it's on the form:
> > 
> > My subject (short description)
> > <blank line>
> > Description...
> > 
> >
>
https://codereview.chromium.org/1010203002/diff/1/Source/core/page/FocusContr...
> > File Source/core/page/FocusController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1010203002/diff/1/Source/core/page/FocusContr...
> > Source/core/page/FocusController.cpp:889: if (!isRectInDirection(type,
> > current.rect, candidate.rect))
> > Could you perhaps make this the first thing in distanceDataForNode instead?
> (To
> > avoid exposing isRectInDirection.) There's not a whole lot of code being run
> > between here and the suggested placement, so I don't think it makes a big
> > different "optimization-wise".
> 
> Updated description. Please have look.

Yes, that's better. Try being a little more specific though. Maybe something
like: Avoid computing navigation data for nodes in the wrong direction ?

[Abhijeet Kandalkar Slow, 2015-03-18 14:26:42]

Updated as per review comments.

https://codereview.chromium.org/1010203002/diff/1/Source/core/page/FocusContr...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1010203002/diff/1/Source/core/page/FocusContr...
Source/core/page/FocusController.cpp:889: if (!isRectInDirection(type,
current.rect, candidate.rect))
On 2015/03/18 14:02:11, fs wrote:
> Could you perhaps make this the first thing in distanceDataForNode instead?
(To
> avoid exposing isRectInDirection.) There's not a whole lot of code being run
> between here and the suggested placement, so I don't think it makes a big
> different "optimization-wise".

Done.

[fs, 2015-03-18 14:36:15]

https://codereview.chromium.org/1010203002/diff/20001/Source/core/page/Spatia...
File Source/core/page/SpatialNavigation.cpp (right):

https://codereview.chromium.org/1010203002/diff/20001/Source/core/page/Spatia...
Source/core/page/SpatialNavigation.cpp:508: if (!isRectInDirection(type,
currentRect, nodeRect))
This won't compile.

https://codereview.chromium.org/1010203002/diff/20001/Source/core/page/Spatia...
Source/core/page/SpatialNavigation.cpp:520: deflateIfOverlapped(currentRect,
nodeRect);
Did you verify that this call never modify the rects such that isRectInDirection
would return false?

[Abhijeet Kandalkar Slow, 2015-03-19 06:09:42]

Updated patch for review.

https://codereview.chromium.org/1010203002/diff/20001/Source/core/page/Spatia...
File Source/core/page/SpatialNavigation.cpp (right):

https://codereview.chromium.org/1010203002/diff/20001/Source/core/page/Spatia...
Source/core/page/SpatialNavigation.cpp:508: if (!isRectInDirection(type,
currentRect, nodeRect))
On 2015/03/18 14:36:15, fs wrote:
> This won't compile.

Sorry for wrong code upload. Fixed it in patch set-3.

This check will be most useful in use case when will be at end of page and still
pressing DOWN navigation button, in this case we wont be calculating navigation
data for all node above.

I am in support of placing it in focuscontroller(Patch-1) will be good as we
will be discarding node in wrong navigation direction intermediately.

https://codereview.chromium.org/1010203002/diff/20001/Source/core/page/Spatia...
Source/core/page/SpatialNavigation.cpp:520: deflateIfOverlapped(currentRect,
nodeRect);
On 2015/03/18 14:36:15, fs wrote:
> Did you verify that this call never modify the rects such that
isRectInDirection
> would return false?

At early stage, if we are sure that actual node rects are unwanted then we don't
need to deflate them as correction.

Moreover if deflateIfOverlapped() modify both rects by same value then return
value will be same.

[fs, 2015-03-19 09:51:27]

lgtm

[Abhijeet Kandalkar Slow, 2015-03-19 09:54:01]

The CQ bit was checked by abhijeet.k@samsung.com

[Abhijeet Kandalkar Slow, 2015-03-19 09:54:01]

The patchset sent to the CQ was uploaded after l-g-t-m from c.shu@samsung.com
Link to the patchset: https://codereview.chromium.org/1010203002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 09:54:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1010203002/40001

[commit-bot: I haz the power, 2015-03-19 13:18:03]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192165