Refactor LayoutSelection::SetSelection() (issue 2843463006 by yoichio@chromium.org)

yoi...@chromium.org

unread,

Apr 27, 2017, 3:36:49 AM4/27/17

to yo...@chromium.org, chromium...@chromium.org, blink-...@chromium.org

Reviewers: yosin_UTC9
CL: https://codereview.chromium.org/2843463006/

Message:
MarkSelection() creates HashMap and return using std::forward_as_tuple,
does this work well to avoid deep copy the HashMap returning back to caller?

Description:
Refactor LayoutSelection::SetSelection()

Refactor the 1 big function to several functions:
CollectSelectedMap(), SetSelectionNone(), MarkSelection() and
InvalidateLayoutObjects().

TEST=No change in behavior.
BUG=708453

Affected files (+130, -124 lines):
M third_party/WebKit/Source/core/editing/LayoutSelection.cpp

yoi...@chromium.org

unread,

Apr 27, 2017, 3:36:55 AM4/27/17

to yo...@chromium.org, chromium...@chromium.org, blink-...@chromium.org

https://codereview.chromium.org/2843463006/

yosin@chromium.org via codereview.chromium.org

unread,

Apr 27, 2017, 5:50:30 AM4/27/17

to yoi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode183
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:183: for
(auto iterator : map)
nit: s/auto/const auto&/
nit: s/iterator/key_value/ or another good name. "iterator" is usually
abbrev to "it"
and used as:
for (auto it = old_selected_blocks.begin(); it !=
old_selected_blocks.end(); ++it)

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode232
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:232: for
(auto iterator : new_selected_objects)
nit: s/auto/const auto&/
nit: s/iterator/key_value/ or another good name. "iterator" is usually
abbrev to "it"
and used as:
for (auto it = old_selected_blocks.begin(); it !=
old_selected_blocks.end(); ++it)

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode236
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:236: for
(auto iterator : old_selected_blocks) {
nit: s/auto/const auto&/
nit: s/iterator/key_value/ or another good name. "iterator" is usually
abbrev to "it"
and used as:
for (auto it = old_selected_blocks.begin(); it !=
old_selected_blocks.end(); ++it)

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode297
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:297: if
(!frame_selection_->GetDocument().GetLayoutView()->GetFrameView())
We should move this if-statement to beginning of this function.
Or at least after L281 MarkSelection()

https://codereview.chromium.org/2843463006/

yoi...@chromium.org

unread,

Apr 27, 2017, 10:16:26 PM4/27/17

to yo...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode183
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:183: for
(auto iterator : map)

On 2017/04/27 09:50:29, yosin_OOO_til_May_7 wrote:
> nit: s/auto/const auto&/
> nit: s/iterator/key_value/ or another good name. "iterator" is usually
abbrev to
> "it"
> and used as:
> for (auto it = old_selected_blocks.begin(); it !=
old_selected_blocks.end();
> ++it)

Done.

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode232
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:232: for
(auto iterator : new_selected_objects)

On 2017/04/27 09:50:29, yosin_OOO_til_May_7 wrote:
> nit: s/auto/const auto&/
> nit: s/iterator/key_value/ or another good name. "iterator" is usually
abbrev to
> "it"
> and used as:
> for (auto it = old_selected_blocks.begin(); it !=
old_selected_blocks.end();
> ++it)

Done.

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode236
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:236: for
(auto iterator : old_selected_blocks) {

On 2017/04/27 09:50:29, yosin_OOO_til_May_7 wrote:
> nit: s/auto/const auto&/
> nit: s/iterator/key_value/ or another good name. "iterator" is usually
abbrev to
> "it"
> and used as:
> for (auto it = old_selected_blocks.begin(); it !=
old_selected_blocks.end();
> ++it)

Done.

https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode297
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:297: if
(!frame_selection_->GetDocument().GetLayoutView()->GetFrameView())

On 2017/04/27 09:50:29, yosin_OOO_til_May_7 wrote:
> We should move this if-statement to beginning of this function.
> Or at least after L281 MarkSelection()

Done.

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode179
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:179: return
std::forward_as_tuple(selected_objects, selected_blocks);
Does this correctly move |selected_objects| and |selected_blocks|
to caller w/o deep copy?

https://codereview.chromium.org/2843463006/

yosin@chromium.org via codereview.chromium.org

unread,

Apr 27, 2017, 11:21:51 PM4/27/17

to yoi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode134
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:134: typedef
HashMap<LayoutObject*, SelectionState> SelectedObjectMap;
nit: better to use |using|

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode140
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:140: typedef
HashMap<LayoutBlock*, SelectionState> SelectedBlockMap;
nit: better to use |using|

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode142
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:142: static
std::tuple<SelectedObjectMap, SelectedBlockMap> CollectSelectedMap(
nit: it is better to use |std::pair<>| since it has only two members.

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode179
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:179: return
std::forward_as_tuple(selected_objects, selected_blocks);

On 2017/04/28 at 02:16:26, yoichio wrote:
> Does this correctly move |selected_objects| and |selected_blocks|
> to caller w/o deep copy?

|std::forward_as_tuple()| is used for function argument[1].

We should write:
return std::move(std::make_pair(selection_objects, selected_blocks));

[1] http://en.cppreference.com/w/cpp/utility/tuple/forward_as_tuple

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode240
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:240:
LayoutBlock* block = it.key;
nit: s/LayoutBlock*/LayoutBlock* const/
Is it better to use |LayoutBlock&|?

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode242
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:242:
SelectionState old_selection_state = it.value;
nit: s/SelectionState/const SelectionState/

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode279
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:279:
std::tie(old_selected_objects, old_selected_blocks) =
I think it is better to introduce |struct SelectedMap {
SelectedObjectMap objects; SelectedBlockMap blocks; }|
Rather than destructuing tuple.

https://codereview.chromium.org/2843463006/

yoi...@chromium.org

unread,

Apr 28, 2017, 4:00:34 AM4/28/17

to yo...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode134
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:134: typedef
HashMap<LayoutObject*, SelectionState> SelectedObjectMap;

On 2017/04/28 03:21:51, yosin_OOO_til_May_7 wrote:
> nit: better to use |using|

Done.

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode140
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:140: typedef
HashMap<LayoutBlock*, SelectionState> SelectedBlockMap;

On 2017/04/28 03:21:51, yosin_OOO_til_May_7 wrote:
> nit: better to use |using|

Done.

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode142
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:142: static
std::tuple<SelectedObjectMap, SelectedBlockMap> CollectSelectedMap(

On 2017/04/28 03:21:51, yosin_OOO_til_May_7 wrote:
> nit: it is better to use |std::pair<>| since it has only two members.

Done.

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode179
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:179: return
std::forward_as_tuple(selected_objects, selected_blocks);

On 2017/04/28 03:21:51, yosin_OOO_til_May_7 wrote:
> On 2017/04/28 at 02:16:26, yoichio wrote:
> > Does this correctly move |selected_objects| and |selected_blocks|
> > to caller w/o deep copy?
>
> |std::forward_as_tuple()| is used for function argument[1].
>
> We should write:
> return std::move(std::make_pair(selection_objects, selected_blocks));
>
> [1] http://en.cppreference.com/w/cpp/utility/tuple/forward_as_tuple

Acknowledged.

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode240
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:240:
LayoutBlock* block = it.key;

On 2017/04/28 03:21:51, yosin_OOO_til_May_7 wrote:
> nit: s/LayoutBlock*/LayoutBlock* const/
> Is it better to use |LayoutBlock&|?

Done.

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode242
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:242:
SelectionState old_selection_state = it.value;

On 2017/04/28 03:21:51, yosin_OOO_til_May_7 wrote:
> nit: s/SelectionState/const SelectionState/

Done.

https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode279
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:279:
std::tie(old_selected_objects, old_selected_blocks) =

On 2017/04/28 03:21:51, yosin_OOO_til_May_7 wrote:
> I think it is better to introduce |struct SelectedMap {
SelectedObjectMap
> objects; SelectedBlockMap blocks; }|
> Rather than destructuing tuple.

Done.

https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode180
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:180: return
std::make_pair(selected_objects, selected_blocks);
I'm not sure if this is optimized by copy elision.

https://codereview.chromium.org/2843463006/

yosin@chromium.org via codereview.chromium.org

unread,

Apr 28, 2017, 5:05:08 AM4/28/17

to yoi...@chromium.org, xiaoc...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

+xiaochengh@ for second eye.

https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode180
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:180: return
std::make_pair(selected_objects, selected_blocks);

On 2017/04/28 at 08:00:34, yoichio wrote:
> I'm not sure if this is optimized by copy elision.

RVO can use move semantics[1], to make sure RVO, we can use
|std::move()|

return std::move(std::make_pair(selected_objects, selected_blocks));

[1] https://en.wikipedia.org/wiki/Return_value_optimization

https://codereview.chromium.org/2843463006/

xiaoc...@chromium.org

unread,

Apr 28, 2017, 6:07:44 PM4/28/17

to yoi...@chromium.org, yo...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right):

https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode288
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:288: if
(!frame_selection_->GetDocument().GetLayoutView()->GetFrameView())
We shouldn't change the execution order in a refactoring patch.

This check should be done after calculation of |new_selection_map|.

https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Source/core/editing/LayoutSelection.cpp#newcode303
third_party/WebKit/Source/core/editing/LayoutSelection.cpp:303:
selection_start_ = start;
These four lines should be done right after SetSelectionStateNone().

Moving them here may change the behavior due to the |return| at L289.

https://codereview.chromium.org/2843463006/

Reply all

Reply to author

Forward