Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843463006:
Refactor LayoutSelection::SetSelection() (Closed)
DescriptionRefactor LayoutSelection::SetSelection()
Refactor the 1 big function to several functions:
CollectSelectedMap(), SetSelectionNone(), MarkSelection() and
InvalidateLayoutObjects().
TEST=No change in behavior.
BUG=708453
Patch Set 1 #
Total comments: 8
Patch Set 2 : update #
Total comments: 15
Patch Set 3 : update #
Total comments: 7
Patch Set 4 : update #
Total comments: 5
Messages
Total messages: 30 (20 generated)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Refactor LayoutSelection::SetSelection BUG=708453 ========== to ========== Refactor LayoutSelection::SetSelection() Refactor the 1 big function to several functions: CollectSelectedMap(), SetSelectionNone(), MarkSelection() and InvalidateLayoutObjects(). TEST=No change in behavior. BUG=708453 ==========
yoichio@chromium.org changed reviewers: + yosin@chromium.org
MarkSelection() creates HashMap and return using std::forward_as_tuple, does this work well to avoid deep copy the HashMap returning back to caller?
https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/c... 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/c... 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/c... 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/c... 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()
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/1/third_party/WebKit/Source/c... 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/c... 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/c... 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/c... 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/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Sour... 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/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... 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.
yosin@chromium.org changed reviewers: + xiaochengh@chromium.org
+xiaochengh@ for second eye. https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... 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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... 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/Sour... 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.
PTAL https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:180: return std::make_pair(selected_objects, selected_blocks); RVO can be applied when return expression type meets function return type but std::move is typed T&& not T. Clang warns such case of "std::move(std::make_pair(..))": http://releases.llvm.org/3.7.0/tools/clang/docs/ReleaseNotes.html#id4 https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:288: if (!frame_selection_->GetDocument().GetLayoutView()->GetFrameView()) On 2017/04/28 22:07:43, Xiaocheng wrote: > We shouldn't change the execution order in a refactoring patch. > > This check should be done after calculation of |new_selection_map|. Done. https://codereview.chromium.org/2843463006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:303: selection_start_ = start; On 2017/04/28 22:07:44, Xiaocheng wrote: > These four lines should be done right after SetSelectionStateNone(). > > Moving them here may change the behavior due to the |return| at L289. Done.
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2843463006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2843463006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:214: SelectedMap& new_selected_map) { s/SelectedMap&/SelectedMap*/ We use reference for output parameter. https://codereview.chromium.org/2843463006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:256: LayoutObject* start, Introducing |struct LayoutObjectWithOffset| before this patch is easier to read? https://codereview.chromium.org/2843463006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:301: if (!frame_selection_->GetDocument().GetLayoutView()->GetFrameView()) Can we move this if-statement at start of function? Since this function doesn't destroy FrameView. https://codereview.chromium.org/2843463006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:304: InvalidateLayoutObjects(selection_start_, is_start_pos_changed, It is better to pass old selection_{start,end}_pos_ instead of bool's. https://codereview.chromium.org/2843463006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:306: new_selected_map); Could you move |SelectedMap new_selected_map = CollectSelectedMap(start, end, end_pos);| into InvalidateLayoutObjects()? Since, we don't use |new_selected_map| after here. Also, we don't need to pass |new_selected_map|.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) |
