Avoid unused Computations in SelectionController::selectClosestWordFromHitTestResult

Avoid unused Computations in SelectionController::selectClosestWordFromHitTestResult

Currently if style has user-select none for a node,
selectClosestWordFromHitTestResult() does all the calculations for VisibleSelection
but then returns as false making all VisibleSelection calculation meaningless.

This CL avoids the unused computations by checking for
user-select state before calculating VisibleSelection.

BUG=701657
Review-Url: https://codereview.chromium.org/2757863003
Cr-Commit-Position: refs/heads/master@{#458317}
Committed: https://chromium.googlesource.com/chromium/src/+/4b59469154ec17254ae526c9327931ecf0f51b9b

[tanvir, 2017-03-17 12:19:59]

Description was changed from

==========
Avoid unused Computations in
SelectionController::selectClosestWordFromHitTestResult

Currently if style has user-select none on a node,
selectClosestWordFromHitTestResult does all the calculations for
VisibleSelection
and then returns as false making VisibleSelection calculation meaningless.

This CL avoids the unused computations by checking for
user-select state before calculating VisibleSelection.

BUG=701657
==========

to

==========
Avoid unused Computations in
SelectionController::selectClosestWordFromHitTestResult

Currently if style has user-select none on a node,
selectClosestWordFromHitTestResult does all the calculations for
VisibleSelection
and then returns as false making VisibleSelection calculation meaningless.

This CL avoids the unused computations by checking for
user-select state before calculating VisibleSelection.

BUG=701657

R=SamsungPeerReview
==========

[tanvir, 2017-03-17 12:21:24]

tanvir.rizvi@samsung.com changed reviewers:
+ shanmuga.m@samsung.com, srirama.m@samsung.com

[Srirama, 2017-03-17 12:36:46]

Peer review looks good to me

[tanvir, 2017-03-17 12:57:08]

Description was changed from

==========
Avoid unused Computations in
SelectionController::selectClosestWordFromHitTestResult

Currently if style has user-select none on a node,
selectClosestWordFromHitTestResult does all the calculations for
VisibleSelection
and then returns as false making VisibleSelection calculation meaningless.

This CL avoids the unused computations by checking for
user-select state before calculating VisibleSelection.

BUG=701657

R=SamsungPeerReview
==========

to

==========
Avoid unused Computations in
SelectionController::selectClosestWordFromHitTestResult

Currently if style has user-select none for a node,
selectClosestWordFromHitTestResult() does all the calculations for
VisibleSelection
but then returns as false making all VisibleSelection calculation meaningless.

This CL avoids the unused computations by checking for
user-select state before calculating VisibleSelection.

BUG=701657
==========

[tanvir, 2017-03-17 12:57:08]

tanvir.rizvi@samsung.com changed reviewers:
+ xiaochengh@chromium.org, yosin@chromium.org

[tanvir, 2017-03-17 13:16:51]

PTAL!!

With current CL, during Success case when user-select is NOT none,
layoutObject()->isSelectable() will be checked twice in function
selectClosestWordFromHitTestResult().

As updateSelectionForMouseDownDispatchingSelectStart() is getting called from
other places too, like  
1)handleMousePressEventSingleClick
2)selectClosestMisspellingFromHitTestResult
3)selectClosestWordOrLinkFromMouseEvent
4) setCaretAtHitTestResult

These functions do calculate VisibleSelection, but here also selection is not
getting used if user-select is none.
Do i need to investigate into other callers to
updateSelectionForMouseDownDispatchingSelectStart() also ?

Thanks!!!

[Xiaocheng, 2017-03-17 19:03:14]

lgtm

On 2017/03/17 at 13:16:51, tanvir.rizvi wrote:
> PTAL!!
> 
> With current CL, during Success case when user-select is NOT none,
layoutObject()->isSelectable() will be checked twice in function
selectClosestWordFromHitTestResult().

This doesn't seem to be a serious issue, as checking LayoutObject::isSelectable
is quite cheap compared to calculating a VisibleSelecion.

> 
> As updateSelectionForMouseDownDispatchingSelectStart() is getting called from
other places too, like  
> 1)handleMousePressEventSingleClick
> 2)selectClosestMisspellingFromHitTestResult
> 3)selectClosestWordOrLinkFromMouseEvent
> 4) setCaretAtHitTestResult
> 
> These functions do calculate VisibleSelection, but here also selection is not
getting used if user-select is none.
> Do i need to investigate into other callers to
updateSelectionForMouseDownDispatchingSelectStart() also ?

Yes, please, as long as you like to work on this.

> 
> Thanks!!!

[yosin_UTC9, 2017-03-21 03:28:10]

C++ code change seems to be good.

Could you add layout test to verify this change to avoid regression[1]?

[1] Importance and requirements of testing in Chromium
https://goo.gl/OpRI6x

[yosin_UTC9, 2017-03-21 03:36:39]

lgtm

Sorry for confusion.

user-select:none is checked in
updateSelectionForMouseDownDispatchingSelectStart() which is
called at the end of selectClosestWordFromHitTestResult() which this patch
changed.

[yosin_UTC9, 2017-03-21 03:36:43]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-03-21 03:37:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 05:14:00]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490067403510470, "parent_rev":
"2daa4d732346a9eb2e802bbab224c15df9b2adb2", "commit_rev":
"4b59469154ec17254ae526c9327931ecf0f51b9b"}

[commit-bot: I haz the power, 2017-03-21 05:14:49]

Description was changed from

==========
Avoid unused Computations in
SelectionController::selectClosestWordFromHitTestResult

Currently if style has user-select none for a node,
selectClosestWordFromHitTestResult() does all the calculations for
VisibleSelection
but then returns as false making all VisibleSelection calculation meaningless.

This CL avoids the unused computations by checking for
user-select state before calculating VisibleSelection.

BUG=701657
==========

to

==========
Avoid unused Computations in
SelectionController::selectClosestWordFromHitTestResult

Currently if style has user-select none for a node,
selectClosestWordFromHitTestResult() does all the calculations for
VisibleSelection
but then returns as false making all VisibleSelection calculation meaningless.

This CL avoids the unused computations by checking for
user-select state before calculating VisibleSelection.

BUG=701657

Review-Url: https://codereview.chromium.org/2757863003
Cr-Commit-Position: refs/heads/master@{#458317}
Committed:
https://chromium.googlesource.com/chromium/src/+/4b59469154ec17254ae526c93279...
==========

[commit-bot: I haz the power, 2017-03-21 05:14:50]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/4b59469154ec17254ae526c93279...