New issue
Advanced search Search tips

Issue 803278 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Rename FrameSelection::SetSelection(const SelectionInDOMTree&) to SetSelectionAndEndTyping()

Project Member Reported by rlanday@chromium.org, Jan 18 2018

Issue description

 https://crbug.com/801388  was caused by changing this:

GetFrame().Selection().SetSelection(
    SelectionInDOMTree::Builder().SetBaseAndExtent(range).Build(), 0);

to this:

GetFrame().Selection().SetSelection(
    SelectionInDOMTree::Builder().SetBaseAndExtent(range).Build());

The problem is that the second code snippet calls FrameSelection::SetSelection() with no options parameter, which does the following:

void FrameSelection::SetSelection(const SelectionInDOMTree& selection) {
  SetSelection(selection, SetSelectionOptions::Builder()
                              .SetShouldCloseTyping(true)
                              .SetShouldClearTypingStyle(true)
                              .Build());
}

We should probably make this method not do SetShouldCloseTyping(true) and SetShouldClearTypingStyle(true) since it's unexpected and has caused at least one regression. This will require modifying callers to set these options themselves, if necessary.
 

Comment 1 by yosin@chromium.org, Jan 18 2018

Summary: Rename FrameSelection::SetSelection(const SelectionInDOMTree&) to SetSelectionAndEndTyping() (was: Make FrameSelection::SetSelection() not apply options by default)
According to TODO in FrameSeleciton.h, we should rename to one parameter version
to SetSelectionAndEndTyping()

  // TODO(editing-dev): We should rename this function to
  // SetSelectionAndEndTyping()
  // Set selection with end of typing processing == close typing and clear
  // typing style.
  void SetSelection(const SelectionInDOMTree&);

There are 172 occurrences of SetSeleciton/1.
There are 27 occurrences of SetSelection/2.

Comment 2 by yosin@chromium.org, Jan 18 2018

Owner: yosin@chromium.org
Status: Started (was: Available)
I take this to finish before branch M65.
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 18 2018

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

commit 1336836c162c3c9734e5b72a3471412e19577ac5
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Thu Jan 18 09:46:36 2018

Rename one parameter version of FrameSelection::SetSelection() to SetSelectionAndEndTyping()

This patch renames one parameter version of |FrameSelection::SetSelection()| to
|SetSelectionAndEndTyping()| to avoid miss usage[1] for improving code health.

[1] http://crrev.com/c/871184 Fix bug applying text formatting with open IME
composition


TBR=dmazzoni@chromium.org

Bug:  803278 
Change-Id: I1cc105a27d9127d59b136eb48b97f1ce3bb8bd4e
Reviewed-on: https://chromium-review.googlesource.com/872550
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530101}
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/dom/RangeTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/CaretDisplayItemClientTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/Editor.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/FrameSelection.h
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/GranularityStrategyTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/LayoutSelectionTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/SelectionController.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/SelectionEditor.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/SelectionEditor.h
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/SetSelectionOptions.h
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/ApplyBlockElementCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/ApplyStyleCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/InsertListCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/InsertParagraphSeparatorCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/InsertTextCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/commands/TypingCommandTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/finder/TextFinder.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/ime/InputMethodControllerTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/editing/suggestion/TextSuggestionControllerTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/frame/WebFrameWidgetImpl.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/input/EventHandlerTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/page/DragController.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp
[modify] https://crrev.com/1336836c162c3c9734e5b72a3471412e19577ac5/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp

Comment 4 by yosin@chromium.org, Jan 22 2018

Owner: ----
Status: Fixed (was: Started)

Sign in to add a comment