Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906273002:
Make LayoutSelection::SetSelection() local (Closed)
|
Created:
3 years, 6 months ago by yoichio Modified:
3 years, 6 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake LayoutSelection::SetSelection() local
Update member |paint_range| at Commit().
This CL also moves enum SelectionPaintInvalidationMode to local and
renames it appropriately.
BUG=708453
TEST=No change in behavior
Review-Url: https://codereview.chromium.org/2906273002
Cr-Commit-Position: refs/heads/master@{#475478}
Committed: https://chromium.googlesource.com/chromium/src/+/6ba7bee79232bdf31a7128df13ae4236d4e70659
Patch Set 1 #Patch Set 2 : update #
Total comments: 6
Patch Set 3 : nit #
Total comments: 4
Patch Set 4 : nit #
Messages
Total messages: 22 (12 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...
Description was changed from ========== Make SetSelection local BUG=708453 TEST=No change in behavior ========== to ========== Make LayoutSelection::SetSelection() local Update member |paint_range| at Commit(). This CL also moves enum SelectionPaintInvalidationMode to local and renames it appropriately. BUG=708453 TEST=No change in behavior ==========
yoichio@chromium.org changed reviewers: + yosin@chromium.org
https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:169: static SelectedMap CollectSelectedMap(const SelectionPaintRange& range, nit: Could you add a blank line between enum and function? https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:225: static void SetSelectionStateAndShouldInvalidateSelection( nit: s/SetSelectionStateAndShouldInvalidateSelection/SetSelectionStateAndInvalidate/ or better name, we don't need to have |Should| in function name.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:225: static void SetSelectionStateAndShouldInvalidateSelection( On 2017/05/29 07:59:37, yosin_UTC9 wrote: > nit: > s/SetSelectionStateAndShouldInvalidateSelection/SetSelectionStateAndInvalidate/ > or better name, we don't need to have |Should| in function name. Fmm, actually we don't invalidate LayoutObject yet. Just turn |ShouldInvalidateSelection| flag on.
https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:225: static void SetSelectionStateAndShouldInvalidateSelection( On 2017/05/29 at 08:36:26, yoichio wrote: > On 2017/05/29 07:59:37, yosin_UTC9 wrote: > > nit: > > s/SetSelectionStateAndShouldInvalidateSelection/SetSelectionStateAndInvalidate/ > > or better name, we don't need to have |Should| in function name. > > Fmm, actually we don't invalidate LayoutObject yet. > Just turn |ShouldInvalidateSelection| flag on. How about SetSelectionStateAndPaintInvalidationFlag? LayoutObject::ClearPaintInvalidationFlags() has bitfiels_.SetShouldInvalidateSelection(false);
https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:169: static SelectedMap CollectSelectedMap(const SelectionPaintRange& range, On 2017/05/29 07:59:37, yosin_UTC9 wrote: > nit: Could you add a blank line between enum and function? Done. https://codereview.chromium.org/2906273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:225: static void SetSelectionStateAndShouldInvalidateSelection( On 2017/05/29 09:38:15, yosin_UTC9 wrote: > On 2017/05/29 at 08:36:26, yoichio wrote: > > On 2017/05/29 07:59:37, yosin_UTC9 wrote: > > > nit: > > > > s/SetSelectionStateAndShouldInvalidateSelection/SetSelectionStateAndInvalidate/ > > > or better name, we don't need to have |Should| in function name. > > > > Fmm, actually we don't invalidate LayoutObject yet. > > Just turn |ShouldInvalidateSelection| flag on. > > How about SetSelectionStateAndPaintInvalidationFlag? > LayoutObject::ClearPaintInvalidationFlags() has > bitfiels_.SetShouldInvalidateSelection(false); > > Sine this is a local function and used only once, exact descriptive naming is not needed. Added some comment.
lgtm w/ nits https://codereview.chromium.org/2906273002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2906273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:170: static SelectedMap CollectSelectedMap(const SelectionPaintRange& range, Discuss offline. We'll split this function to remove flag. https://codereview.chromium.org/2906273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:226: // Set SetSelectionState and ShouldInvalidateSelection frag of LayoutObjects nit: s/frag/flag/ https://codereview.chromium.org/2906273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:228: static void UpdateLayoutObjectState(const SelectionPaintRange& new_range, Good (^_^)b https://codereview.chromium.org/2906273002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:365: SelectionPaintRange new_range( nit: s/SelectionPaintRange/const SelectionPaintRange/
The CQ bit was checked by yoichio@chromium.org
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/2906273002/#ps60001 (title: "nit")
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
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yoichio@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496128076362370,
"parent_rev": "5a1339aa58b7cb26f064fcb0f6d5d600390b50e8", "commit_rev":
"6ba7bee79232bdf31a7128df13ae4236d4e70659"}
Message was sent while issue was closed.
Description was changed from ========== Make LayoutSelection::SetSelection() local Update member |paint_range| at Commit(). This CL also moves enum SelectionPaintInvalidationMode to local and renames it appropriately. BUG=708453 TEST=No change in behavior ========== to ========== Make LayoutSelection::SetSelection() local Update member |paint_range| at Commit(). This CL also moves enum SelectionPaintInvalidationMode to local and renames it appropriately. BUG=708453 TEST=No change in behavior Review-Url: https://codereview.chromium.org/2906273002 Cr-Commit-Position: refs/heads/master@{#475478} Committed: https://chromium.googlesource.com/chromium/src/+/6ba7bee79232bdf31a7128df13ae... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6ba7bee79232bdf31a7128df13ae... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
