Chromium Code Reviews
|
|
|
Created:
5 years ago by Miyoung Shin(g) Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-events_chromium.org, blink-reviews-html_chromium.org, dglazkov+blink, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[Reland]Refactor the selection code in EventHandler
The code related to handling the selection
moves out of EventHandler to SelectionHandler.
This patch makes EventHandler simpler as well.
BUG=483376, 491501
R=yosin@chromium.org, rbyers@chromium.org, tkent@chromium.org, esprehn@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197046
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 50
Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 28
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 1
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 22
Patch Set 10 : #
Total comments: 2
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #
Total comments: 2
Patch Set 16 : #
Total comments: 4
Patch Set 17 : #
Total comments: 8
Patch Set 18 : rebase #Patch Set 19 : rebase #
Messages
Total messages: 67 (17 generated)
yosin@, I've named the |SelectionController| even though we don't decide the class name yet. If you prefer another name, please let me know. PTAL when you get back. Happy vacation!
rbyers@chromium.org changed reviewers: + mfomitchev@chromium.org
rbyers@chromium.org changed reviewers: + yoichio@chromium.org
I took a quick skim through this - it seems much cleaner than I expected, definitely seems like a good clean-up (EventHandler is too huge to reason about today). Good work! I didn't dive into many of the details but I left a few comments. I think we need to wait for Yosin to get back (I think he was planning on going to BlinkOn so hopefully we'll see him there). But /cc yoichio and mfomitchev in case they have any feedback. https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:954: selectionController().initializeSelectionState(); This is a behavior change, right? https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:2026: selectionController().initializeSelectionState(); is this a a behavior change? https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... File Source/core/page/EventHandler.h (right): https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.h:326: OwnPtrWillBeMember<SelectionController> m_selectionController; There's a 1:1 correspondance and identical lifetimes between EventHandler and SelectionController, right? If so then perhaps this can be simpler as just SelectionController instead of OwnPtrWillBeMember<SelectionController>.
https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:954: selectionController().initializeSelectionState(); On 2015/05/01 13:22:09, Rick Byers wrote: > This is a behavior change, right? Done. I'd missed this. I should change the behavior at http://crrev.com/1049233003. https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:2026: selectionController().initializeSelectionState(); On 2015/05/01 13:22:09, Rick Byers wrote: > is this a a behavior change? Done. https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... File Source/core/page/EventHandler.h (right): https://codereview.chromium.org/1113323002/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.h:326: OwnPtrWillBeMember<SelectionController> m_selectionController; On 2015/05/01 13:22:09, Rick Byers wrote: > There's a 1:1 correspondance and identical lifetimes between EventHandler and > SelectionController, right? If so then perhaps this can be simpler as just > SelectionController instead of OwnPtrWillBeMember<SelectionController>. Oh. right. Thanks for making it simpler.
Thanks for quick work! And sorry for late response. It seems we missed to f2f on Sydney. I was/am feeling not good due by I caught cold on my vacation. (T_T) https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: Please use copyright comments in "EventHandler.cpp". Since, this file holds contents from "EventHandler.cpp" https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:105: m_frame->selection().setNonDirectionalSelectionIfNeeded(selection, granularity); Can we have private getter |selection()|? There are lots of |m_frame->selection()| in this file. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:469: void SelectionController::selectionForContextMenu(const MouseEventWithHitTestResults& mev, const LayoutPoint& position) prepareForContextMenu? Please name functions to start with verb. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:471: if (!m_frame->selection().contains(position) nit: early return is better. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:486: void SelectionController::releaseSelection(MouseEventWithHitTestResults& mev) How about |preparePassMousePressEventToSubframe|? I thought |collapseSelectionIfNeeded| or |collapseSelectionIfMousePressInSelection|, but they aren't unclear. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:489: if (m_frame->selection().contains(p)) { nit: early return is better. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: Please use copyright comments in "EventHandler.h". Since, this file holds contents from "EventHandler.h" https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:7: #include "core/CoreExport.h" nit: Please have a blank line between guard macro and the first #include. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:14: class LocalFrame; Please in alphabetical order. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:20: class CORE_EXPORT SelectionController { nit: Please put |final|. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:21: DISALLOW_ALLOCATION(); nit: Please add |WTF_MAKE_NONCOPYABLE(SelectionController)| https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:21: DISALLOW_ALLOCATION(); Let's make |SelectionControler| allocatable to avoid including "SelectionController.h" in "EventHandler.h". See "Page.h", there are number of controllers holding in |OwnPtr<T>|, e.g. |const OwnPtr<AutoscrollController> m_autoscrollController|. This is Blink idiom of "pimpl" pattern. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:48: void initializeSelectionState() { m_selectionState = HaveNotStartedSelection; } Let's do this in CPP file. See: http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:51: bool singleClickInSelection() { return m_singleClickInSelection; } nit: |const|. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:58: enum SelectionState { HaveNotStartedSelection, PlacedCaret, ExtendedSelection }; Let't use |enum class| for better static check. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (left): https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:666: To avoid introducing unrelated change, please keep this blank line. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:222: , m_selectionController(SelectionController(frame)) Just |m_selectionController(frame)| https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:222: , m_selectionController(SelectionController(frame)) Let's use |OwnPtr<SelectionController>|. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:329: selectionController().clear(); ctor of |SelectionController| should call |clear()| or ctor should imply cleared state. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:884: selectionController().setAllowSelection(false); Could you keep original variable name here? So, we'll have - SelectionController::mouseDownMayStartSelect() - SelectionController::setMouseDownMayStartSelect(newValue) I feel "allow selection" and "may start selection" are different concept. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3313: void EventHandler::updateSelectionForMouseDrag() Please move |EventHandler::updateSelectionForMouseDrag()| to original location for ease of comparing of original implementation. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3658: // If we're clicking into a frame that is selected, the frame will appear Please move this comment to |SelectionController|. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... File Source/core/page/EventHandler.h (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.h:30: #include "core/editing/SelectionController.h" Let's use |OwnPtr<ScrollController| to avoid including "SelectionController.h" here. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.h:196: void updateSelectionForMouseDrag(); nit: Please move this to original position, e.g. L97. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.h:322: SelectionController m_selectionController; Let's use |const OwnPtr<SelectionController>|.
Oh. I'm sorry, How do you feel better now? As I met Rick in Sydney, he showed me your picture through his phone, but I couldn't find you. I hope to see you on the next blinkOn if I go there. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > nit: Please use copyright comments in "EventHandler.cpp". Since, this file holds > contents from "EventHandler.cpp" Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:105: m_frame->selection().setNonDirectionalSelectionIfNeeded(selection, granularity); On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > Can we have private getter |selection()|? There are lots of > |m_frame->selection()| in this file. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:469: void SelectionController::selectionForContextMenu(const MouseEventWithHitTestResults& mev, const LayoutPoint& position) On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > prepareForContextMenu? > > Please name functions to start with verb. Done. Thanks, I will try to name functions in future as you said. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:471: if (!m_frame->selection().contains(position) On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > nit: early return is better. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:486: void SelectionController::releaseSelection(MouseEventWithHitTestResults& mev) On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > How about |preparePassMousePressEventToSubframe|? I thought > |collapseSelectionIfNeeded| or |collapseSelectionIfMousePressInSelection|, but > they aren't unclear. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:489: if (m_frame->selection().contains(p)) { On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > nit: early return is better. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > nit: Please use copyright comments in "EventHandler.h". Since, this file holds > contents from "EventHandler.h" Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:7: #include "core/CoreExport.h" On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > nit: Please have a blank line between guard macro and the first #include. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:14: class LocalFrame; On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > Please in alphabetical order. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:20: class CORE_EXPORT SelectionController { On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > nit: Please put |final|. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:21: DISALLOW_ALLOCATION(); On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > Let's make |SelectionControler| allocatable to avoid including > "SelectionController.h" in "EventHandler.h". > > See "Page.h", there are number of controllers holding in |OwnPtr<T>|, e.g. > |const OwnPtr<AutoscrollController> m_autoscrollController|. This is Blink idiom > of "pimpl" pattern. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:21: DISALLOW_ALLOCATION(); On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > nit: Please add |WTF_MAKE_NONCOPYABLE(SelectionController)| Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:48: void initializeSelectionState() { m_selectionState = HaveNotStartedSelection; } On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > Let's do this in CPP file. > > See: > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... Oh, I thought that it's ok to refer to the legacy-coding style of the WebKit, because we have still many codes in header file like this. Thanks for the good information. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:51: bool singleClickInSelection() { return m_singleClickInSelection; } On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > nit: |const|. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:58: enum SelectionState { HaveNotStartedSelection, PlacedCaret, ExtendedSelection }; On 2015/05/18 08:53:46, Yosi_UTC9 wrote: > Let't use |enum class| for better static check. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (left): https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:666: On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > To avoid introducing unrelated change, please keep this blank line. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:222: , m_selectionController(SelectionController(frame)) On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > Let's use |OwnPtr<SelectionController>|. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:329: selectionController().clear(); On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > ctor of |SelectionController| should call |clear()| or ctor should imply cleared > state. Sorry but I couldn't catch your comment. This is a part of eventhandler::clear(). When calling |EventHandler::clear|, we should clear explicitly states of |SelectionController|. aren't we? If I'm wrong, could you correct me? https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:884: selectionController().setAllowSelection(false); On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > Could you keep original variable name here? So, we'll have > - SelectionController::mouseDownMayStartSelect() > - SelectionController::setMouseDownMayStartSelect(newValue) > > I feel "allow selection" and "may start selection" are different concept. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3313: void EventHandler::updateSelectionForMouseDrag() On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > Please move |EventHandler::updateSelectionForMouseDrag()| to original location > for ease of comparing of original implementation. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:3658: // If we're clicking into a frame that is selected, the frame will appear On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > Please move this comment to |SelectionController|. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... File Source/core/page/EventHandler.h (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.h:30: #include "core/editing/SelectionController.h" On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > Let's use |OwnPtr<ScrollController| to avoid including "SelectionController.h" > here. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.h:196: void updateSelectionForMouseDrag(); On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > nit: Please move this to original position, e.g. L97. Done. https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.h:322: SelectionController m_selectionController; On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > Let's use |const OwnPtr<SelectionController>|. Done.
Almost good shape! We are very closed to land this patch. >Oh. I'm sorry, How do you feel better now? Still not good. I may need this week to get better. >As I met Rick in Sydney, he showed me your picture through his phone, but I >couldn't find you. >I hope to see you on the next blinkOn if I go there. I hope to see. BTW, what time zone are you in? https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/20001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:329: selectionController().clear(); On 2015/05/19 09:01:42, Miyoung Shin(g) wrote: > On 2015/05/18 08:53:47, Yosi_UTC9 wrote: > > ctor of |SelectionController| should call |clear()| or ctor should imply > cleared > > state. > > Sorry but I couldn't catch your comment. > This is a part of eventhandler::clear(). When calling |EventHandler::clear|, we > should clear explicitly states of |SelectionController|. aren't we? > If I'm wrong, could you correct me? I mean we don't need to call |SelectionController::clear()| here. ctor of |SelectionController| should call |clear()| or initialize member variables as same as |clear()|. It seems |SelectionController| ctor initializes what |clear()| does. https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:510: if (mev.hitTestResult().isMisspelled()) nit: Let's use early return pattern. https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:41: enum AppendTrailingWhitespace { ShouldAppendTrailingWhitespace, DontAppendTrailingWhitespace }; It seems |AppendTrailingWhitespace| used only in |SelectionController|. Can we move |AppendTrailingWhitespace| inside |SelectionController| and make it to |enum class|? https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:51: void selectClosestWordFromHitTestResult(const HitTestResult&, AppendTrailingWhitespace); Do we need to expose |selectClosestWordXXX| as public? It seems they are used in |SelectionController| only. So, we can mark them |private|. https://codereview.chromium.org/1113323002/diff/40001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/40001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:222: , m_selectionController(SelectionController::create(frame, &frame->selection())) nit: We don't need to pass |FrameSelection| in second parameter. https://codereview.chromium.org/1113323002/diff/40001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:329: selectionController().clear(); nit: We don't need to call |clear()| here. |SelectionController| does what |clear()| does. Basically, after ctor, object should be clear state.
>BTW, what time zone are you in? I'm in KST, Korea. :) https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:510: if (mev.hitTestResult().isMisspelled()) On 2015/05/19 09:41:19, Yosi_UTC9 wrote: > nit: Let's use early return pattern. Done. https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:41: enum AppendTrailingWhitespace { ShouldAppendTrailingWhitespace, DontAppendTrailingWhitespace }; On 2015/05/19 09:41:19, Yosi_UTC9 wrote: > It seems |AppendTrailingWhitespace| used only in |SelectionController|. Can we > move |AppendTrailingWhitespace| inside |SelectionController| and make it to > |enum class|? Done. https://codereview.chromium.org/1113323002/diff/40001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:51: void selectClosestWordFromHitTestResult(const HitTestResult&, AppendTrailingWhitespace); On 2015/05/19 09:41:19, Yosi_UTC9 wrote: > Do we need to expose |selectClosestWordXXX| as public? It seems they are used in > |SelectionController| only. So, we can mark them |private|. Done. https://codereview.chromium.org/1113323002/diff/40001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/40001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:222: , m_selectionController(SelectionController::create(frame, &frame->selection())) On 2015/05/19 09:41:19, Yosi_UTC9 wrote: > nit: We don't need to pass |FrameSelection| in second parameter. Done. https://codereview.chromium.org/1113323002/diff/40001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:329: selectionController().clear(); On 2015/05/19 09:41:19, Yosi_UTC9 wrote: > nit: We don't need to call |clear()| here. |SelectionController| does what > |clear()| does. Basically, after ctor, object should be clear state. Done.
yosin@chromium.org changed reviewers: + tkent@chromium.org
LGTM w/nits +tkent@, since he is review in same time zone. ;-) https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:49: static void setSelectionIfNeeded(FrameSelection& selection, const VisibleSelection& newSelection) Could you move static functions into anonymous namespace to use modern C++? https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:51: if (selection.selection() != newSelection) nit: Could you use early return pattern? https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:55: static inline bool dispatchSelectStart(Node* node) Could you remove |inline| modifiers? Let's compiler decide inline or notinline. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:578: return *m_selection; return m_frame->selection(); https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:66: FrameSelection& selection() const; nit: |selection()| getter should be private. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:79: FrameSelection* const m_selection; nit: We don't need to cache |m_frame->selection()|. |selection()| can be implemented as |FrameSelection& SelectionController::selection() const { return m_frame->selection(); }| https://codereview.chromium.org/1113323002/diff/60001/Source/core/html/HTMLLa... File Source/core/html/HTMLLabelElement.cpp (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/html/HTMLLa... Source/core/html/HTMLLabelElement.cpp:164: if (!Position::nodeIsUserSelectNone(this) && frame->selection().isRange() && !frame->eventHandler().selectionController().singleClickInSelection()) We may want to re-factor this fragment in another patch. Since, here is the only call site of |singleClickInSelection()|. Note: This the place setting |true| to |isLabelTextSelect|, it seems we don't need to check |isLabelTextSelected|.
We've not yet have good name for |SelectionController|... In these days, I feel this is not so bad name, though.
https://codereview.chromium.org/1113323002/diff/60001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/core.gypi#n... Source/core/core.gypi:1381: 'editing/SelectionType.h', Add SelectionController.h too. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:41: class CORE_EXPORT SelectionController final { should inherit from |public NoBaseWillBeGarbageCollected<SelectionController>|. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:44: static PassOwnPtr<SelectionController> create(LocalFrame*); The return type should be PassOwnPtrWillBeRawPtr. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:67: Need to add DECLARE_TRACE(). Also, add DEFINE_TRACE() to SelectionController.cpp. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:78: LocalFrame* const m_frame; should be |RawPtrWillBeMember<LocalFrame>| https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:79: FrameSelection* const m_selection; should be |RawPtrWillBeMember<FrameSelection>|. https://codereview.chromium.org/1113323002/diff/60001/Source/core/page/EventH... File Source/core/page/EventHandler.h (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/page/EventH... Source/core/page/EventHandler.h:325: const OwnPtr<SelectionController> m_selectionController; should be |OwnPtrWIllBeMember|, and needs to update DEFINE_TRACE in EventHandler.cpp.
https://codereview.chromium.org/1113323002/diff/60001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/core.gypi#n... Source/core/core.gypi:1381: 'editing/SelectionType.h', On 2015/05/20 03:31:01, tkent wrote: > Add SelectionController.h too. Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:49: static void setSelectionIfNeeded(FrameSelection& selection, const VisibleSelection& newSelection) On 2015/05/20 02:27:37, Yosi_UTC9 wrote: > Could you move static functions into anonymous namespace to use modern C++? Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:51: if (selection.selection() != newSelection) On 2015/05/20 02:27:37, Yosi_UTC9 wrote: > nit: Could you use early return pattern? Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:55: static inline bool dispatchSelectStart(Node* node) On 2015/05/20 02:27:37, Yosi_UTC9 wrote: > Could you remove |inline| modifiers? Let's compiler decide inline or notinline. Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:578: return *m_selection; On 2015/05/20 02:27:37, Yosi_UTC9 wrote: > return m_frame->selection(); Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:41: class CORE_EXPORT SelectionController final { On 2015/05/20 03:31:01, tkent wrote: > should inherit from |public NoBaseWillBeGarbageCollected<SelectionController>|. Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:44: static PassOwnPtr<SelectionController> create(LocalFrame*); On 2015/05/20 03:31:01, tkent wrote: > The return type should be PassOwnPtrWillBeRawPtr. Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:66: FrameSelection& selection() const; On 2015/05/20 02:27:37, Yosi_UTC9 wrote: > nit: |selection()| getter should be private. Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:67: On 2015/05/20 03:31:01, tkent wrote: > Need to add DECLARE_TRACE(). > Also, add DEFINE_TRACE() to SelectionController.cpp. Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:78: LocalFrame* const m_frame; On 2015/05/20 03:31:01, tkent wrote: > should be |RawPtrWillBeMember<LocalFrame>| Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:79: FrameSelection* const m_selection; On 2015/05/20 02:27:37, Yosi_UTC9 wrote: > nit: We don't need to cache |m_frame->selection()|. |selection()| can be > implemented as |FrameSelection& SelectionController::selection() const { return > m_frame->selection(); }| Done. https://codereview.chromium.org/1113323002/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:79: FrameSelection* const m_selection; On 2015/05/20 03:31:01, tkent wrote: > should be |RawPtrWillBeMember<FrameSelection>|. I removed it. https://codereview.chromium.org/1113323002/diff/60001/Source/core/html/HTMLLa... File Source/core/html/HTMLLabelElement.cpp (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/html/HTMLLa... Source/core/html/HTMLLabelElement.cpp:164: if (!Position::nodeIsUserSelectNone(this) && frame->selection().isRange() && !frame->eventHandler().selectionController().singleClickInSelection()) On 2015/05/20 02:27:37, Yosi_UTC9 wrote: > We may want to re-factor this fragment in another patch. Since, here is the only > call site of |singleClickInSelection()|. > > Note: This the place setting |true| to |isLabelTextSelect|, it seems we don't > need to check |isLabelTextSelected|. Ok. I will check this code. https://codereview.chromium.org/1113323002/diff/60001/Source/core/page/EventH... File Source/core/page/EventHandler.h (right): https://codereview.chromium.org/1113323002/diff/60001/Source/core/page/EventH... Source/core/page/EventHandler.h:325: const OwnPtr<SelectionController> m_selectionController; On 2015/05/20 03:31:01, tkent wrote: > should be |OwnPtrWIllBeMember|, and needs to update DEFINE_TRACE in > EventHandler.cpp. Done.
https://codereview.chromium.org/1113323002/diff/80001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/80001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:45: ~SelectionController(); Please remove ~SelectionController(). https://codereview.chromium.org/1113323002/diff/80001/Source/core/page/EventH... File Source/core/page/EventHandler.h (right): https://codereview.chromium.org/1113323002/diff/80001/Source/core/page/EventH... Source/core/page/EventHandler.h:325: const OwnPtr<SelectionController> m_selectionController; OwnPtr -> OwnPtrWillBeMember
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/1113323002/diff/80001/Source/core/editing/Sel... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/80001/Source/core/editing/Sel... Source/core/editing/SelectionController.h:45: ~SelectionController(); On 2015/05/20 23:57:40, tkent wrote: > Please remove ~SelectionController(). > Done. https://codereview.chromium.org/1113323002/diff/80001/Source/core/page/EventH... File Source/core/page/EventHandler.h (right): https://codereview.chromium.org/1113323002/diff/80001/Source/core/page/EventH... Source/core/page/EventHandler.h:325: const OwnPtr<SelectionController> m_selectionController; On 2015/05/20 23:57:40, tkent wrote: > OwnPtr -> OwnPtrWillBeMember Done.
The CQ bit was checked by tkent@chromium.org
lgtm
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/1113323002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1113323002/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195735
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:120001) has been created in https://codereview.chromium.org/1157833002/ by wfh@chromium.org. The reason for reverting is: crbug.com/491501 text selection broken.
I'm sorry. I made a mistake to set the wrong variable in EventHandler.cpp. I will upload the new patch soon. https://codereview.chromium.org/1113323002/diff/120001/Source/core/page/Event... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/120001/Source/core/page/Event... Source/core/page/EventHandler.cpp:456: selectionController().updateSelectionForMouseDrag(m_mousePressNode.get(), m_dragStartPos, m_lastKnownMouseGlobalPosition); I figured out that I set the wrong |m_lastKnownMouseGlobalPosition|. Instead, I should use |m_lastKnownMousePosition|.
On 2015/05/24 14:05:55, Miyoung Shin(g) wrote: > I'm sorry. I made a mistake to set the wrong variable in EventHandler.cpp. > I will upload the new patch soon. > > https://codereview.chromium.org/1113323002/diff/120001/Source/core/page/Event... > File Source/core/page/EventHandler.cpp (right): > > https://codereview.chromium.org/1113323002/diff/120001/Source/core/page/Event... > Source/core/page/EventHandler.cpp:456: > selectionController().updateSelectionForMouseDrag(m_mousePressNode.get(), > m_dragStartPos, m_lastKnownMouseGlobalPosition); > I figured out that I set the wrong |m_lastKnownMouseGlobalPosition|. Instead, I > should use |m_lastKnownMousePosition|. Give me patch for replacing this variable. I must recompile my Chromium with APNG and fixed text selection.
Please add a test for crbug.com/491501.
> Give me patch for replacing this variable. I must recompile my Chromium with > APNG and fixed text selection. How about "git cl patch 1113323002" ? if impossible, could you let me know your Email account and your baseline? On 2015/05/25 00:39:10, tkent wrote: > Please add a test for crbug.com/491501. I'm trying to add TC using eventSender.mouseXXXX. But it seems that the client points treats to the global points equally. I think we need to add API in eventSender to separate the global points to the client points, or to add TC in chrome side. WDYT?
On 2015/05/25 01:36:26, Miyoung Shin(g) wrote: > I'm trying to add TC using eventSender.mouseXXXX. But it seems that the client > points treats to the global points equally. > I think we need to add API in eventSender to separate the global points to the > client points, or to add TC in chrome side. > WDYT? How about using GTest, webkit_unit_tests, by introducing EventHandlerTest.cpp?
On 2015/05/25 01:58:18, Yosi_UTC9 wrote: > On 2015/05/25 01:36:26, Miyoung Shin(g) wrote: > > I'm trying to add TC using eventSender.mouseXXXX. But it seems that the client > > points treats to the global points equally. > > I think we need to add API in eventSender to separate the global points to the > > client points, or to add TC in chrome side. > > WDYT? > > How about using GTest, webkit_unit_tests, by introducing EventHandlerTest.cpp? Thanks, I've added TC in WebFrameTest because it has already similar tests. As for EventHandlerTest, I think we need to add unit_test step by step for future. If I have time, I will try to do it.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
It'd be better if you first moved code but didn't change things and then made refactors of it. https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:49: namespace { remove anonymous namespace, just use static. https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:51: void setSelectionIfNeeded(FrameSelection& selection, const VisibleSelection& newSelection) static https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:58: bool dispatchSelectStart(Node* node) static https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:79: bool canMouseDownStartSelect(Node* node) static https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:90: int textDistance(const Position& start, const Position& end) static https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:2: * Copyright (C) 2006, 2007, 2009, 2010, 2011 Apple Inc. All rights reserved. Missing Google here https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:41: class CORE_EXPORT SelectionController final : public NoBaseWillBeGarbageCollected<SelectionController> { Does this actually need to be exported? I don't think it does. https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:68: explicit SelectionController(LocalFrame*); reference https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:70: enum class AppendTrailingWhitespace { ShouldAppend, DontAppend }; In the future it's preferred if you don't both move code and change stuff at the same time. So you'd just move code, then switch to enum class in a follow up patch. https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:82: enum class SelectionState { HaveNotStartedSelection, PlacedCaret, ExtendedSelection }; ditto https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/data/... File Source/web/tests/data/drag_selection.html (right): https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/data/... Source/web/tests/data/drag_selection.html:3: <head> Leave out html, head and body. https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/data/... Source/web/tests/data/drag_selection.html:6: <style type="text/css"> remove type
https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/WebFr... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/WebFr... Source/web/tests/WebFrameTest.cpp:4238: TEST_F(WebFrameTest, dragSelectionAfterScroll) Can you make EventHandlerTest.cpp or SelectionControllerTest.cpp instead of adding a test to WebFrameTest?
https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:49: namespace { On 2015/05/25 21:28:12, esprehn wrote: > remove anonymous namespace, just use static. It seems both anonymous namespace and static function are same preferred way in Google C++ coding style guide: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nonmember,_S... Quote: If you must define a nonmember function and it is only needed in its .cc file, use an unnamed namespace or static linkage (eg static int Foo() {...}) to limit its scope.
esprehn@, tkent@, yosin@, I'm on vacation this week. when I get back, I will take a look at your comment. Thanks.
https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:49: namespace { Also anonymous namespace and static function co-exist in Blink. @esprehn, @yosin, Which of one should I use ? https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... File Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:2: * Copyright (C) 2006, 2007, 2009, 2010, 2011 Apple Inc. All rights reserved. On 2015/05/25 21:28:12, esprehn wrote: > Missing Google here Done. https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:41: class CORE_EXPORT SelectionController final : public NoBaseWillBeGarbageCollected<SelectionController> { On 2015/05/25 21:28:12, esprehn wrote: > Does this actually need to be exported? I don't think it does. Done. https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:68: explicit SelectionController(LocalFrame*); On 2015/05/25 21:28:12, esprehn wrote: > reference Done. https://codereview.chromium.org/1113323002/diff/180001/Source/core/editing/Se... Source/core/editing/SelectionController.h:70: enum class AppendTrailingWhitespace { ShouldAppend, DontAppend }; On 2015/05/25 21:28:12, esprehn wrote: > In the future it's preferred if you don't both move code and change stuff at the > same time. So you'd just move code, then switch to enum class in a follow up > patch. It conflicts with the comment suggested by @yosin at #6. @yosin, Can I invite your opinion for it? https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/WebFr... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/WebFr... Source/web/tests/WebFrameTest.cpp:4238: TEST_F(WebFrameTest, dragSelectionAfterScroll) On 2015/05/25 23:57:25, tkent wrote: > Can you make EventHandlerTest.cpp or SelectionControllerTest.cpp instead of > adding a test to WebFrameTest? I made EventHandlerTest.cpp for the validation of the problem because the problem happened after landing the patch was caused by using the wrong variable in EventHandler.cpp. https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/data/... File Source/web/tests/data/drag_selection.html (right): https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/data/... Source/web/tests/data/drag_selection.html:3: <head> On 2015/05/25 21:28:13, esprehn wrote: > Leave out html, head and body. Done. And this file has merged to EventHandlerTest.cpp. https://codereview.chromium.org/1113323002/diff/180001/Source/web/tests/data/... Source/web/tests/data/drag_selection.html:6: <style type="text/css"> On 2015/05/25 21:28:13, esprehn wrote: > remove type Done.
esprehn@, tkent@, yosin@, Could you take a look ?
Can you re-post the patch with "git cl upload --similarity=5" to show a diff between EventHandler and SelectionHandler? https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:54: if (selection.selection() == newSelection) Please don't change code structure when you move the code. The original code was if (selection.selection() != newSelection) selection.setSelection(newSelection); You changed it, and it made code review harder. Please do such code change later in another patch. There are a lot of similar changes in this file.
> Can you re-post the patch with "git cl upload --similarity=5" to show a diff > between EventHandler and SelectionHandler? OK, I will. hi, yosin@, Other reviewers have a concern to change the origin code. They want to move the code to selectionController from EventController first, then arrange the code like enum class, 'early return'. If you agree with it, I will separate the patches. WDYT? https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:54: if (selection.selection() == newSelection) On 2015/06/04 03:34:18, tkent wrote: > Please don't change code structure when you move the code. > The original code was > if (selection.selection() != newSelection) > selection.setSelection(newSelection); > > You changed it, and it made code review harder. Please do such code change > later in another patch. > > There are a lot of similar changes in this file. 'early return' is required by yosin@. I think I need to make a consensus with yours.
On 2015/06/04 08:53:04, Miyoung Shin(g) wrote: > > Can you re-post the patch with "git cl upload --similarity=5" to show a diff > > between EventHandler and SelectionHandler? > OK, I will. > > hi, yosin@, > Other reviewers have a concern to change the origin code. > They want to move the code to selectionController from EventController first, > then arrange the code like enum class, 'early return'. > If you agree with it, I will separate the patches. WDYT? > > https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... > File Source/core/editing/SelectionController.cpp (right): > > https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... > Source/core/editing/SelectionController.cpp:54: if (selection.selection() == > newSelection) > On 2015/06/04 03:34:18, tkent wrote: > > Please don't change code structure when you move the code. > > The original code was > > if (selection.selection() != newSelection) > > selection.setSelection(newSelection); > > > > You changed it, and it made code review harder. Please do such code change > > later in another patch. > > > > There are a lot of similar changes in this file. > > 'early return' is required by yosin@. > I think I need to make a consensus with yours. Let's do clean up in another patch. Sorry for confusion. Could you try to use "--similarity" option with "git cl upload" to help "git cl upload" identify SelectionController.cpp is copied from EventHandler.cpp? Since, default value of --similar is 50, we may want to try 25, 37, 44, ..., e.g. git cl upload --similarity=25
This patch is a conflict with the patch that moved EventHandler from core/page/ to core/input at http://crrev.com/1148813007. So I'm asking a few of question to owner. If it is clear, I could upload the new patch.
Patchset #11 (id:220001) has been deleted
Patchset #11 (id:240001) has been deleted
Patchset #11 (id:260001) has been deleted
On 2015/06/05 01:27:59, Yosi_UTC9 wrote: > On 2015/06/04 08:53:04, Miyoung Shin(g) wrote: > > > Can you re-post the patch with "git cl upload --similarity=5" to show a diff > > > between EventHandler and SelectionHandler? > > OK, I will. > > > > hi, yosin@, > > Other reviewers have a concern to change the origin code. > > They want to move the code to selectionController from EventController first, > > then arrange the code like enum class, 'early return'. > > If you agree with it, I will separate the patches. WDYT? > > > > > https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... > > File Source/core/editing/SelectionController.cpp (right): > > > > > https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... > > Source/core/editing/SelectionController.cpp:54: if (selection.selection() == > > newSelection) > > On 2015/06/04 03:34:18, tkent wrote: > > > Please don't change code structure when you move the code. > > > The original code was > > > if (selection.selection() != newSelection) > > > selection.setSelection(newSelection); > > > > > > You changed it, and it made code review harder. Please do such code change > > > later in another patch. > > > > > > There are a lot of similar changes in this file. > > > > 'early return' is required by yosin@. > > I think I need to make a consensus with yours. > > Let's do clean up in another patch. Sorry for confusion. > I tried to keep the origin code as much as possible. I will do the below items at the next patch. - member name - enum class - early return - namespace instead of static - selection() instead of frame->selection() > Could you try to use "--similarity" option with "git cl upload" to help "git cl > upload" identify SelectionController.cpp is copied from EventHandler.cpp? Since, > default value of --similar is 50, we may want to try 25, 37, 44, ..., e.g. git > cl upload --similarity=25 I'm not sure it's correct way to upload "git cl patch --similarity=xx" PatchSet 11 : --similarity=25 PatchSet 12 : --similarity=37 PatchSet 13 : --similarity=44 PatchSet 14 : --similarity=50 If I'm wrong, could you correct me ?
On 2015/06/08 09:23:48, Miyoung Shin(g) wrote: > On 2015/06/05 01:27:59, Yosi_UTC9 wrote: > > On 2015/06/04 08:53:04, Miyoung Shin(g) wrote: > > > > Can you re-post the patch with "git cl upload --similarity=5" to show a > diff > > > > between EventHandler and SelectionHandler? > > > OK, I will. > > > > > > hi, yosin@, > > > Other reviewers have a concern to change the origin code. > > > They want to move the code to selectionController from EventController > first, > > > then arrange the code like enum class, 'early return'. > > > If you agree with it, I will separate the patches. WDYT? > > > > > > > > > https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... > > > File Source/core/editing/SelectionController.cpp (right): > > > > > > > > > https://codereview.chromium.org/1113323002/diff/200001/Source/core/editing/Se... > > > Source/core/editing/SelectionController.cpp:54: if (selection.selection() == > > > newSelection) > > > On 2015/06/04 03:34:18, tkent wrote: > > > > Please don't change code structure when you move the code. > > > > The original code was > > > > if (selection.selection() != newSelection) > > > > selection.setSelection(newSelection); > > > > > > > > You changed it, and it made code review harder. Please do such code > change > > > > later in another patch. > > > > > > > > There are a lot of similar changes in this file. > > > > > > 'early return' is required by yosin@. > > > I think I need to make a consensus with yours. > > > > Let's do clean up in another patch. Sorry for confusion. > > > > I tried to keep the origin code as much as possible. > I will do the below items at the next patch. > - member name > - enum class > - early return > - namespace instead of static > - selection() instead of frame->selection() > > > Could you try to use "--similarity" option with "git cl upload" to help "git > cl > > upload" identify SelectionController.cpp is copied from EventHandler.cpp? > Since, > > default value of --similar is 50, we may want to try 25, 37, 44, ..., e.g. git > > cl upload --similarity=25 > > I'm not sure it's correct way to upload "git cl patch --similarity=xx" > PatchSet 11 : --similarity=25 > PatchSet 12 : --similarity=37 > PatchSet 13 : --similarity=44 > PatchSet 14 : --similarity=50 > If I'm wrong, could you correct me ? It's correct. I would have A+ mark for SelectionController.cpp with diff from EventHandler.cpp, however these attempts don't work. :-<
> > > Could you try to use "--similarity" option with "git cl upload" to help "git > > cl > > > upload" identify SelectionController.cpp is copied from EventHandler.cpp? > > Since, > > > default value of --similar is 50, we may want to try 25, 37, 44, ..., e.g. > git > > > cl upload --similarity=25 > > > > I'm not sure it's correct way to upload "git cl patch --similarity=xx" > > PatchSet 11 : --similarity=25 > > PatchSet 12 : --similarity=37 > > PatchSet 13 : --similarity=44 > > PatchSet 14 : --similarity=50 > > If I'm wrong, could you correct me ? Have you tried --similarity=5 suggested in my previous comment?
On 2015/06/08 14:39:48, tkent wrote: > Have you tried --similarity=5 suggested in my previous comment? Sorry, I missed --similarity=5. Now, PatchSet 15 : --similarity=5
https://codereview.chromium.org/1113323002/diff/360001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/360001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:77: static inline bool canMouseDownStartSelect(Node* node) Please move this to the next to handleMousePressEventSingleClick() to minimize the diff. In general please don't change the order of functions. There are other instances of order changes.
https://codereview.chromium.org/1113323002/diff/360001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/360001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:77: static inline bool canMouseDownStartSelect(Node* node) On 2015/06/09 06:03:18, tkent wrote: > Please move this to the next to handleMousePressEventSingleClick() to minimize > the diff. > > In general please don't change the order of functions. There are other > instances of order changes. > Done.
lgtm. Thank you for updating patches. It's easy to review this CL. https://codereview.chromium.org/1113323002/diff/380001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/380001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:509: Page* page = m_frame->page(); nit: I prefer removing these three lines, and calling focusDocumentView() in EventHandler::handleGestureLongPress(). https://codereview.chromium.org/1113323002/diff/380001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:537: void SelectionController::passMousePressEventToSubframe(MouseEventWithHitTestResults& mev) Can you make the argument |const MouseEventWithHitTestResults&|?
https://codereview.chromium.org/1113323002/diff/380001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/380001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:509: Page* page = m_frame->page(); On 2015/06/10 00:14:12, tkent wrote: > nit: I prefer removing these three lines, and calling focusDocumentView() in > EventHandler::handleGestureLongPress(). Done. https://codereview.chromium.org/1113323002/diff/380001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:537: void SelectionController::passMousePressEventToSubframe(MouseEventWithHitTestResults& mev) On 2015/06/10 00:14:12, tkent wrote: > Can you make the argument |const MouseEventWithHitTestResults&|? Done.
https://codereview.chromium.org/1113323002/diff/400001/Source/core/input/Even... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/400001/Source/core/input/Even... Source/core/input/EventHandler.cpp:2085: return true; Add focusDocumentView(). https://codereview.chromium.org/1113323002/diff/400001/Source/core/input/Even... File Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/1113323002/diff/400001/Source/core/input/Even... Source/core/input/EventHandlerTest.cpp:25: virtual void SetUp() override; Remove |virtual|.
https://codereview.chromium.org/1113323002/diff/400001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/400001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:43: #include "core/input/EventHandler.h" Remove this line. https://codereview.chromium.org/1113323002/diff/400001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:509: m_frame->eventHandler().focusDocumentView(); Remove this line.
https://codereview.chromium.org/1113323002/diff/400001/Source/core/editing/Se... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1113323002/diff/400001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:43: #include "core/input/EventHandler.h" On 2015/06/10 01:23:43, tkent wrote: > Remove this line. Done. https://codereview.chromium.org/1113323002/diff/400001/Source/core/editing/Se... Source/core/editing/SelectionController.cpp:509: m_frame->eventHandler().focusDocumentView(); On 2015/06/10 01:23:43, tkent wrote: > Remove this line. Done. https://codereview.chromium.org/1113323002/diff/400001/Source/core/input/Even... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1113323002/diff/400001/Source/core/input/Even... Source/core/input/EventHandler.cpp:2085: return true; On 2015/06/10 01:21:34, tkent wrote: > Add focusDocumentView(). Ah. I caught your point now. https://codereview.chromium.org/1113323002/diff/400001/Source/core/input/Even... File Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/1113323002/diff/400001/Source/core/input/Even... Source/core/input/EventHandlerTest.cpp:25: virtual void SetUp() override; On 2015/06/10 01:21:34, tkent wrote: > Remove |virtual|. Done.
tkent@, yosin@, esprehn@, I'm sorry to ask the review for a long time. Could you take a look one more?
On 2015/06/12 00:56:02, Miyoung Shin(g) wrote: > tkent@, yosin@, esprehn@, > I'm sorry to ask the review for a long time. Could you take a look one more? lgtm Thanks for working in long iteration.
The CQ bit was checked by tkent@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org Link to the patchset: https://codereview.chromium.org/1113323002/#ps420001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1113323002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/58647)
Patchset #18 (id:420001) has been deleted
Patchset #18 (id:440001) has been deleted
The CQ bit was checked by myid.o.shin@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1113323002/#ps480001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1113323002/480001
Message was sent while issue was closed.
Committed patchset #19 (id:480001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197046 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
