Making VisibleSelection operator== take base and extent into account.

Making VisibleSelection operator== take base and extent into account.

If the user long presses on a word to select, a selection is created with base and extent at the long press point (and start/end at the start/end of the word). If the user then starts dragging the handle, in the beginning of the drag the selection is updated to have base/extent at start/end positions. However unless == takes base and extent into account, the update doesn't take place, which causes problems down the line.

BUG=NONE

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

[mfomitchev, 2015-04-16 03:20:40]

mfomitchev@chromium.org changed reviewers:
+ rbyers@chromium.org, yoichio@chromium.org, yosin@chromium.org

[yosin_UTC9, 2015-04-16 03:34:08]

LGTM

I feel this is natural for operator==(). If we really need to compare start/end,
we should have specific function for it.

[yosin_UTC9, 2015-04-16 03:36:33]

On 2015/04/16 03:34:08, Yosi_UTC9 wrote:
> LGTM
> 
> I feel this is natural for operator==(). If we really need to compare
start/end,
> we should have specific function for it.

Background, in ideal situation, since start and end are calculated from
base/extent,
comparing one set is enough, either start/end or base/extent, but as description
said this isn't true.

[yosin_UTC9, 2015-04-16 04:59:04]

https://codereview.chromium.org/1089283002/diff/1/Source/core/editing/Visible...
File Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/1089283002/diff/1/Source/core/editing/Visible...
Source/core/editing/VisibleSelection.h:185: return a.start() == b.start() &&
a.end() == b.end() && a.affinity() == b.affinity() && a.isBaseFirst() ==
b.isBaseFirst()
Since we compare base/extent equality, we don't need to check equality of
|isBaseFirst()|.

https://codereview.chromium.org/1089283002/diff/1/Source/core/editing/Visible...
Source/core/editing/VisibleSelection.h:185: return a.start() == b.start() &&
a.end() == b.end() && a.affinity() == b.affinity() && a.isBaseFirst() ==
b.isBaseFirst()
if one of selections is null, we don't need to check start/end and base/extent.
So, we could write:

if (a.affinity() != b.affinity() || a.isDirectional() != b.isDirectional())
  return false;

if (a.isNull())
  return b.isNull();

return a.start() == b.start() && a.end() == b.end() && a.base() == b.base() &&
a.extent() == b.extent();

[mfomitchev, 2015-04-16 18:11:32]

rbyers: can you PTAL:?

https://codereview.chromium.org/1089283002/diff/1/Source/core/editing/Visible...
File Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/1089283002/diff/1/Source/core/editing/Visible...
Source/core/editing/VisibleSelection.h:185: return a.start() == b.start() &&
a.end() == b.end() && a.affinity() == b.affinity() && a.isBaseFirst() ==
b.isBaseFirst()
On 2015/04/16 04:59:04, Yosi_UTC9 wrote:
> if one of selections is null, we don't need to check start/end and
base/extent.
> So, we could write:
> 
> if (a.affinity() != b.affinity() || a.isDirectional() != b.isDirectional())
>   return false;
> 
> if (a.isNull())
>   return b.isNull();
> 
> return a.start() == b.start() && a.end() == b.end() && a.base() == b.base() &&
> a.extent() == b.extent();
> 

Added the None check and removed isBaseFirst() check. Other than that, I am not
sure we need to explicitly short-circuit: if any of the == evaluate to false, it
won't evaluate any other == anyway. And it seems that checking start/end
positions first is pretty effective in eliminating unequal results right away.
Even if this makes sense, I don't think it needs to be a part of this CL.

[Rick Byers, 2015-04-16 18:48:57]

Rubber-stamp LGTM (Yosi is the expert).

[mfomitchev1, 2015-04-16 19:02:33]

The CQ bit was checked by mfomitchev@google.com

[mfomitchev1, 2015-04-16 19:02:33]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/1089283002/#ps40001
(title: "Adding None check.")

[commit-bot: I haz the power, 2015-04-16 19:03:42]

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

[mfomitchev, 2015-04-17 00:17:34]

mfomitchev@chromium.org changed reviewers:
+ sgurun@chromium.org
- mfomitchev@google.com

[commit-bot: I haz the power, 2015-04-17 03:13:20]

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

[commit-bot: I haz the power, 2015-04-17 03:13:20]

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

[yosin_UTC9, 2015-04-17 04:15:59]

https://codereview.chromium.org/1089283002/diff/40001/Source/core/editing/Vis...
File Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/1089283002/diff/40001/Source/core/editing/Vis...
Source/core/editing/VisibleSelection.h:186: return b.isNone();
We need to check affinity() and isDirectional() even if selection is none.
This is the reason why editing/deleting/delete-cell-contents-win.html failed on
Mac.

The root cause is found in FrameSelection::setSelection()

void FrameSelection::setSelection() {
...
  VisibleSelection s = validateSelection(newSelection);
    if (shouldAlwaysUseDirectionalSelection(m_frame))
        s.setIsDirectional(true);  /// Set isDirectional regardless selection
type, e.g. including selection is empty
...
   if (m_selection == s) {  // Check new selection is different from current
selection
     ...
     return;
   }
...
  // Log selection change event in test_runner.
  m_frame->editor().respondToChangedSelection(oldSelection, options);
...

[mfomitchev, 2015-04-17 14:07:17]

On 2015/04/17 04:15:59, Yosi_UTC9 wrote:
>
https://codereview.chromium.org/1089283002/diff/40001/Source/core/editing/Vis...
> File Source/core/editing/VisibleSelection.h (right):
> 
>
https://codereview.chromium.org/1089283002/diff/40001/Source/core/editing/Vis...
> Source/core/editing/VisibleSelection.h:186: return b.isNone();
> We need to check affinity() and isDirectional() even if selection is none.
> This is the reason why editing/deleting/delete-cell-contents-win.html failed
on
> Mac.
> 
> The root cause is found in FrameSelection::setSelection()
> 
> void FrameSelection::setSelection() {
> ...
>   VisibleSelection s = validateSelection(newSelection);
>     if (shouldAlwaysUseDirectionalSelection(m_frame))
>         s.setIsDirectional(true);  /// Set isDirectional regardless selection
> type, e.g. including selection is empty
> ...
>    if (m_selection == s) {  // Check new selection is different from current
> selection
>      ...
>      return;
>    }
> ...
>   // Log selection change event in test_runner.
>   m_frame->editor().respondToChangedSelection(oldSelection, options);
> ...

Ah, I see. Updated, thanks!

[mfomitchev, 2015-04-17 14:07:21]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2015-04-17 14:07:22]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1089283002/#ps60001
(title: "Need to check affinity() and isDirectional() even if selection is
none.")

[commit-bot: I haz the power, 2015-04-17 14:07:44]

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

[commit-bot: I haz the power, 2015-04-17 15:48:13]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193950