New issue
Advanced search Search tips

Issue 721190 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Get rid of FrameSelection::SetSelectedRange()

Project Member Reported by yosin@chromium.org, May 11 2017

Issue description

There are 8, 3 are test, call sites of FrameSelection::SetSelectedRange().

- Call Non-Null Range: 1-4
- kNonDirectional: 1-5
- kCloseTyping: 2, 3, 4
- kDownstream: Expect 2 == keep current affinity, but we can replace it to kDownstream, because RangeSelection doesn't use affinity.


#1 InputMethodController::SetSelectionOffsets()
 if (range.IsNull())
    return false;
 GetFrame().Selection().SetSelectedRange(
      range, VP_DEFAULT_AFFINITY, SelectionDirectionalMode::kNonDirectional,
      options);

#2 ExpandSelectionToGranularity()
  if (new_range.IsCollapsed())
    return false;
  TextAffinity affinity = frame.Selection().GetSelectionInDOMTree().Affinity();
  frame.Selection().SetSelectedRange(new_range, affinity,
                                    SelectionDirectionalMode::kNonDirectional,
                                     FrameSelection::kCloseTyping);

#3 ExecuteDeleteToMark()
  if (mark.IsNotNull()) {
    bool selected = frame.Selection().SetSelectedRange(
        UnionEphemeralRanges(mark, frame.GetEditor().SelectedRange()),
        TextAffinity::kDownstream, SelectionDirectionalMode::kNonDirectional,
        FrameSelection::kCloseTyping);


#4 ExecuteSelectToMark()
  if (mark.IsNull() || selection.IsNull())
    return false;
  frame.Selection().SetSelectedRange(
      UnionEphemeralRanges(mark, selection), TextAffinity::kDownstream,
      SelectionDirectionalMode::kNonDirectional, FrameSelection::kCloseTyping);


#5 WebLocalFrameImpl::SelectRange()
  GetFrame()->Selection().SetSelectedRange(
      web_range.CreateEphemeralRange(GetFrame()), VP_DEFAULT_AFFINITY,
      SelectionDirectionalMode::kNonDirectional, kNotUserTriggered);


 
Hi Yosin, I want to work on this. Thanks.

Comment 2 by yosin@chromium.org, May 22 2017

Owner: yosin@chromium.org
Status: Started (was: Available)
I'm a virtual owner. tanvir.rizvi is working this.

- http://crrev.com/2897633002: Remove SetSelectedRange from ExpandSelectionToGranularity()
Project Member

Comment 3 by bugdroid1@chromium.org, May 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d8b32218f49b4a92fbf0e51f646bccd1d3d693a8

commit d8b32218f49b4a92fbf0e51f646bccd1d3d693a8
Author: tanvir.rizvi <tanvir.rizvi@samsung.com>
Date: Tue May 23 02:58:26 2017

Use SetSelection in InputMethodController::SetSelectionOffsets

InputMethodController::SetSelectionOffsets function is
changed to use SetSelection instead of SetSelectedRange.

BUG= 721190 

Review-Url: https://codereview.chromium.org/2893303002
Cr-Commit-Position: refs/heads/master@{#473793}

[modify] https://crrev.com/d8b32218f49b4a92fbf0e51f646bccd1d3d693a8/third_party/WebKit/Source/core/editing/InputMethodController.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, May 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86e3ba6c7d166370ed1c3516216a5e4075077d17

commit 86e3ba6c7d166370ed1c3516216a5e4075077d17
Author: tanvir.rizvi <tanvir.rizvi@samsung.com>
Date: Tue May 23 05:16:34 2017

Remove SetSelectedRange from ExpandSelectionToGranularity()

This CL removes SetSelectedRange from ExpandSelectionToGranularity
and instead uses FrameSelection::SetSelection.
This is done to get rid of FrameSelection::SetSelectedRange().

BUG= 721190 

Review-Url: https://codereview.chromium.org/2897633002
Cr-Commit-Position: refs/heads/master@{#473815}

[modify] https://crrev.com/86e3ba6c7d166370ed1c3516216a5e4075077d17/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, May 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5696d44bd68a8745a52e99d2dcac190c391eebbd

commit 5696d44bd68a8745a52e99d2dcac190c391eebbd
Author: tanvir.rizvi <tanvir.rizvi@samsung.com>
Date: Tue May 23 10:15:46 2017

Use SetSelection in ExecuteDeleteToMark

ExecuteDeleteToMark function is changed to use SetSelection instead of
SetSelectedRange.

BUG= 721190 

Review-Url: https://codereview.chromium.org/2900773002
Cr-Commit-Position: refs/heads/master@{#473847}

[modify] https://crrev.com/5696d44bd68a8745a52e99d2dcac190c391eebbd/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, May 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc768d48cd58d1b0c0690e9ba866991088196996

commit dc768d48cd58d1b0c0690e9ba866991088196996
Author: tanvir.rizvi <tanvir.rizvi@samsung.com>
Date: Tue May 23 12:27:48 2017

Use SetSelection in ExecuteSelectToMark()

ExecuteSelectToMark() is changed to use SetSelection()
instead of SetSelectedRange().

BUG= 721190 

Review-Url: https://codereview.chromium.org/2898893002
Cr-Commit-Position: refs/heads/master@{#473866}

[modify] https://crrev.com/dc768d48cd58d1b0c0690e9ba866991088196996/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp

Comment 7 by yosin@chromium.org, May 24 2017

There are no call sites of FrameSelection::SetSelectedRange().
We're ready to remove it!
Project Member

Comment 8 by bugdroid1@chromium.org, May 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dfe8a2dd435c4eed9b079ea408794da551497fc0

commit dfe8a2dd435c4eed9b079ea408794da551497fc0
Author: tanvir.rizvi <tanvir.rizvi@samsung.com>
Date: Thu May 25 09:22:06 2017

Get rid of FrameSelection::SetSelectedRange()

This CL removed FrameSelection::SetSelectedRange().
We use FrameSelection::SetSelection() now instead of SetSelectedRange().
This is done to avoid redundant code checks.

BUG= 721190 

Review-Url: https://codereview.chromium.org/2906493003
Cr-Commit-Position: refs/heads/master@{#474613}

[modify] https://crrev.com/dfe8a2dd435c4eed9b079ea408794da551497fc0/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/dfe8a2dd435c4eed9b079ea408794da551497fc0/third_party/WebKit/Source/core/editing/FrameSelection.h
[modify] https://crrev.com/dfe8a2dd435c4eed9b079ea408794da551497fc0/third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp

Comment 9 by yosin@chromium.org, May 26 2017

Status: Fixed (was: Started)

Sign in to add a comment