Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1965543002:
[InputEvent] Support |sequence<Range> getRanges()| in 'beforeinput' (Closed)
Description[InputEvent] Support |sequence<Range> getRanges()| in 'beforeinput'
This is the first step to support |getRanges()| in 'beforeinput', the
next step will be introducing |StaticRange| and change the method to
|sequence<StaticRange> getRanges()|.
This CL adds supports to all delete related actions, such as
delete-character/word/line/paragraph (forward and backward).
Will add support to other inputTypes when the spec is ready.
SPEC=http://w3c.github.io/editing/input-events.html
BUG=609142
Committed: https://crrev.com/5d7c0b13d1cd8e480c7a1b4d9bdd53f9383f2aef
Cr-Commit-Position: refs/heads/master@{#397131}
Patch Set 1 #
Total comments: 4
Patch Set 2 : dtapuska's review #
Total comments: 11
Patch Set 3 : Use SelectionModifier, rename RangeVector #
Total comments: 8
Patch Set 4 : Yosin's review #
Total comments: 2
Patch Set 5 : Rebase & add TODOs for |base::MakeUnique<T>| #
Total comments: 4
Patch Set 6 : Don't use |std::unique_ptr<>| with |HeapVector| #
Total comments: 10
Patch Set 7 : Ojan's review #
Total comments: 2
Patch Set 8 : Update comments about store and clear |Range| #Messages
Total messages: 43 (11 generated)
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
dtapuska@ can you take a look at this CL please? Thanks! (I will fix the layout tests on Mac)
https://codereview.chromium.org/1965543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-ranges.html (right): https://codereview.chromium.org/1965543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-ranges.html:44: eventSender.keyDown('backspace', [isMacOSX ? 'altKey' : 'ctrlKey']); copyKey can be used and avoid the platform specific checks. https://codereview.chromium.org/1965543002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1965543002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1745: DEFINE_STATIC_LOCAL(HeapVector<Member<Range>>, emptyVector, (new HeapVector<Member<Range>>)); I don't think it is worth worrying about a static local here; it just is messy. Alternatively you can take a pointer in and pass nullptr in.
Hi dtapuska@, can you take a look again please? Thanks! https://codereview.chromium.org/1965543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-ranges.html (right): https://codereview.chromium.org/1965543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-ranges.html:44: eventSender.keyDown('backspace', [isMacOSX ? 'altKey' : 'ctrlKey']); On 2016/05/12 02:00:09, dtapuska wrote: > copyKey can be used and avoid the platform specific checks. It seems that 'copyKey' is for drag&drop (although it does the same thing but might be confusing), and 'Accel' key won't work here since it's for 'ctrl' and 'command'. One option is to introduce an alias for 'copyKey', but it might not be worth it since delete-word is the only use case. https://codereview.chromium.org/1965543002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1965543002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1745: DEFINE_STATIC_LOCAL(HeapVector<Member<Range>>, emptyVector, (new HeapVector<Member<Range>>)); On 2016/05/12 02:00:09, dtapuska wrote: > I don't think it is worth worrying about a static local here; it just is messy. > Alternatively you can take a pointer in and pass nullptr in. Removed static local and instead passing pointers along the path. https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.h (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.h:174: using HeapRanges = HeapVector<Member<Range>>; Is this the right place? https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1868: return nullptr; Have to change them all to pointers to get ride of static local here...
chongz@chromium.org changed reviewers: + yosin@chromium.org
yosin@ can you take a look at this CL please? Thanks!
https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:139: selection->modify(FrameSelection::AlterationExtend, direction, granularity); Please use functions in VisbileUnits.h, rather than using |FrameSelection|. We think |FrameSelection| should be a singleton per |LocalFrame| and will deprecate |FrameSelection| with null-Frame.
https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:139: selection->modify(FrameSelection::AlterationExtend, direction, granularity); On 2016/05/13 at 05:26:12, Yosi_UTC9 wrote: > Please use functions in VisbileUnits.h, rather than using |FrameSelection|. > We think |FrameSelection| should be a singleton per |LocalFrame| and will deprecate |FrameSelection| with null-Frame. Correction. Could you move |SelectionEdtior::modify()| to |VisibleSelectionTemplate<T>| and use it here w/ |VisibleSelection|? This task is in my TODO list but I don't have time to do it now. Sorry... If you have another task for a week and wait for me, I'll do it for next week.
https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.h (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.h:174: using HeapRanges = HeapVector<Member<Range>>; On 2016/05/12 20:53:49, chongz wrote: > Is this the right place? I'd actually put where it is used; probably in the Editor. Depends on complication I guess; but it is ok here if the compilation units don't depend on something common. I see EditingUtilities.h doesn't depend on Editor.h so it might be best here but with a better name. I'd also probably call it what it is RangeVector as opposed to HeapRanges; heap doesn't really imply a whole lot. All Ranges are generated from the oilpan heap. https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:149: HeapRanges* getRanges() const; Comment? https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.h (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.h:58: HeapVector<Member<Range>> getRanges() const { return m_ranges; }; This will likely deserve a comment. Probably in the class definition how the ranges works. In that the underlying range is a live range that is "active" during event dispatch but gets cleared after dispatch.
On 2016/05/13 05:42:34, Yosi_UTC9 wrote: > https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): > > https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:139: > selection->modify(FrameSelection::AlterationExtend, direction, granularity); > On 2016/05/13 at 05:26:12, Yosi_UTC9 wrote: > > Please use functions in VisbileUnits.h, rather than using |FrameSelection|. > > We think |FrameSelection| should be a singleton per |LocalFrame| and will > deprecate |FrameSelection| with null-Frame. > > Correction. > Could you move |SelectionEdtior::modify()| to |VisibleSelectionTemplate<T>| and > use it here w/ |VisibleSelection|? > This task is in my TODO list but I don't have time to do it now. Sorry... > If you have another task for a week and wait for me, I'll do it for next week. Sure I can have a try if you don't have time. But I'm not sure how it works, do I simply move all 'modify' related functions like 'modifyExtendingRight()' and 'modifyExtendingForward'? Thanks!
On 2016/05/17 at 03:14:04, chongz wrote: > On 2016/05/13 05:42:34, Yosi_UTC9 wrote: > > https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): > > > > https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:139: > > selection->modify(FrameSelection::AlterationExtend, direction, granularity); > > On 2016/05/13 at 05:26:12, Yosi_UTC9 wrote: > > > Please use functions in VisbileUnits.h, rather than using |FrameSelection|. > > > We think |FrameSelection| should be a singleton per |LocalFrame| and will > > deprecate |FrameSelection| with null-Frame. > > > > Correction. > > Could you move |SelectionEdtior::modify()| to |VisibleSelectionTemplate<T>| and > > use it here w/ |VisibleSelection|? > > This task is in my TODO list but I don't have time to do it now. Sorry... > > If you have another task for a week and wait for me, I'll do it for next week. > > Sure I can have a try if you don't have time. But I'm not sure how it works, do I simply move all 'modify' related functions like 'modifyExtendingRight()' and 'modifyExtendingForward'? > Thanks! I'll do it now since it blocks me. Please wait a while we'll have SelectionModify class instead of using frame less FrameSeleciton, it is a very bad actor. We don't expose modifyXXX. They should be a local of SelectionModify class.
On 2016/05/17 at 05:06:08, Yosi_UTC9 wrote: > On 2016/05/17 at 03:14:04, chongz wrote: > > On 2016/05/13 05:42:34, Yosi_UTC9 wrote: > > > https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): > > > > > > https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:139: > > > selection->modify(FrameSelection::AlterationExtend, direction, granularity); > > > On 2016/05/13 at 05:26:12, Yosi_UTC9 wrote: > > > > Please use functions in VisbileUnits.h, rather than using |FrameSelection|. > > > > We think |FrameSelection| should be a singleton per |LocalFrame| and will > > > deprecate |FrameSelection| with null-Frame. > > > > > > Correction. > > > Could you move |SelectionEdtior::modify()| to |VisibleSelectionTemplate<T>| and > > > use it here w/ |VisibleSelection|? > > > This task is in my TODO list but I don't have time to do it now. Sorry... > > > If you have another task for a week and wait for me, I'll do it for next week. > > > > Sure I can have a try if you don't have time. But I'm not sure how it works, do I simply move all 'modify' related functions like 'modifyExtendingRight()' and 'modifyExtendingForward'? > > Thanks! > > I'll do it now since it blocks me. Please wait a while we'll have SelectionModify class instead of using frame less FrameSeleciton, it is a very bad actor. > We don't expose modifyXXX. They should be a local of SelectionModify class. SelectionModifier class, renamed, is ready to use. Here is sample usage: http://crrev.com/394652 Enjoy!
yosin@ PTAL, thanks! https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.h (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.h:174: using HeapRanges = HeapVector<Member<Range>>; On 2016/05/13 13:27:17, dtapuska wrote: > On 2016/05/12 20:53:49, chongz wrote: > > Is this the right place? > > I'd actually put where it is used; probably in the Editor. Depends on > complication I guess; but it is ok here if the compilation units don't depend on > something common. I see EditingUtilities.h doesn't depend on Editor.h so it > might be best here but with a better name. > > I'd also probably call it what it is RangeVector as opposed to HeapRanges; heap > doesn't really imply a whole lot. All Ranges are generated from the oilpan heap. > Done. https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:149: HeapRanges* getRanges() const; On 2016/05/13 13:27:17, dtapuska wrote: > Comment? Done. https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:139: selection->modify(FrameSelection::AlterationExtend, direction, granularity); On 2016/05/13 05:42:33, Yosi_UTC9 wrote: > On 2016/05/13 at 05:26:12, Yosi_UTC9 wrote: > > Please use functions in VisbileUnits.h, rather than using |FrameSelection|. > > We think |FrameSelection| should be a singleton per |LocalFrame| and will > deprecate |FrameSelection| with null-Frame. > > Correction. > Could you move |SelectionEdtior::modify()| to |VisibleSelectionTemplate<T>| and > use it here w/ |VisibleSelection|? > This task is in my TODO list but I don't have time to do it now. Sorry... > If you have another task for a week and wait for me, I'll do it for next week. Switched to |SelectionModifier|. https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.h (right): https://codereview.chromium.org/1965543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.h:58: HeapVector<Member<Range>> getRanges() const { return m_ranges; }; On 2016/05/13 13:27:17, dtapuska wrote: > This will likely deserve a comment. Probably in the class definition how the > ranges works. In that the underlying range is a live range that is "active" > during event dispatch but gets cleared after dispatch. Done.
https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:151: RangeVector* getRanges() const; Let's use std::unique_ptr<RangeVector> https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:135: RangeVector* RangesFromCurrentSelectionOrExtendCaret(const LocalFrame& frame, SelectionDirection direction, TextGranularity granularity) Let's use std::unique_ptr<RangeVector> to specify ownership. https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:142: if (!selectionModifier.selection().isNone()) nit: early return is better. if (selecitonModifier.selection().isNone()) return ranges; ranges->append(firstRangeOf(selectionModifier.selection())); return ranges; https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1873: return RangesFromCurrentSelectionOrExtendCaret(frame(), DirectionBackward, CharacterGranularity); nit: %s/frame()/m_frame/ Since L1867 uses |m_frame|. IMHO, using |m_frame| is better than |frame()|, on stepping in debugger, |frame()| requires additional step.
yosin@ I've updated as per your comments, PTAL, thanks! https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:151: RangeVector* getRanges() const; On 2016/05/19 05:01:02, Yosi_UTC9 wrote: > Let's use std::unique_ptr<RangeVector> Done. https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:135: RangeVector* RangesFromCurrentSelectionOrExtendCaret(const LocalFrame& frame, SelectionDirection direction, TextGranularity granularity) On 2016/05/19 05:01:02, Yosi_UTC9 wrote: > Let's use std::unique_ptr<RangeVector> to specify ownership. Done. https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:142: if (!selectionModifier.selection().isNone()) On 2016/05/19 05:01:02, Yosi_UTC9 wrote: > nit: early return is better. > > if (selecitonModifier.selection().isNone()) > return ranges; > ranges->append(firstRangeOf(selectionModifier.selection())); > return ranges; Done. https://codereview.chromium.org/1965543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1873: return RangesFromCurrentSelectionOrExtendCaret(frame(), DirectionBackward, CharacterGranularity); On 2016/05/19 05:01:02, Yosi_UTC9 wrote: > nit: %s/frame()/m_frame/ > Since L1867 uses |m_frame|. > IMHO, using |m_frame| is better than |frame()|, on stepping in debugger, > |frame()| requires additional step. Done.
lgtm https://codereview.chromium.org/1965543002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:140: std::unique_ptr<RangeVector> ranges(new RangeVector); We should use |base::MakeUnique<T>| https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ptr_ut...
lgtm
chongz@chromium.org changed reviewers: + ojan@chromium.org
ojan@ PTAL, thanks!
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:120001) has been deleted
ojan@ can you take a quick look at this CL please? Thanks! https://codereview.chromium.org/1965543002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:140: std::unique_ptr<RangeVector> ranges(new RangeVector); On 2016/05/25 07:15:08, Yosi_UTC9 wrote: > We should use |base::MakeUnique<T>| > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ptr_ut... Not able to use |base::MakeUnique<T>| due to pre-submit errors "Do not use Chromium class from namespace base inside Blink core". Modifying DEPS won't work... Pre-submit file: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
chongz@chromium.org changed reviewers: + oilpan-reviews@chromium.org
oilpan-reviews@ can I have a review on the usage of |std::unique_ptr<>| and |HeapVector<>|? Basically I'm not sure if I should use them together. Details are inside the comments, PTAL, thanks! https://codereview.chromium.org/1965543002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:141: std::unique_ptr<RangeVector> ranges(new RangeVector); oilpan-reviews@ I'm not sure if I should use |std::unique_ptr<T>| on |RangeVector| (which is |HeapVector<Member<Range>>| from "Range.h"). It seems to me that |HeapVector<T>| is |IS_GARBAGE_COLLECTED_TYPE()| so I'd expect it to crash with |std::unique_ptr<T>|, but actually it's not... Also I noticed that the new method from "wtf/PtrUtil.h" |WTF::wrapUnique()| won't accept |HeapVector<>|, but it seems that |IS_GARBAGE_COLLECTED_TYPE()| does not have to come from |GarbageCollected<T>|?
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1965543002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:141: std::unique_ptr<RangeVector> ranges(new RangeVector); On 2016/05/30 17:39:14, chongz wrote: > oilpan-reviews@ I'm not sure if I should use |std::unique_ptr<T>| on > |RangeVector| (which is |HeapVector<Member<Range>>| from "Range.h"). > > It seems to me that |HeapVector<T>| is |IS_GARBAGE_COLLECTED_TYPE()| so I'd > expect it to crash with |std::unique_ptr<T>|, but actually it's not... > > Also I noticed that the new method from "wtf/PtrUtil.h" |WTF::wrapUnique()| > won't accept |HeapVector<>|, but it seems that |IS_GARBAGE_COLLECTED_TYPE()| > does not have to come from |GarbageCollected<T>|? Combining std::unique_ptr<> with an Oilpan managed object doesn't make sense, as you've guessed. std::unique_ptr<T> doesn't restrict what Ts (non-garbage collected types) it can be used with, hence it will compile just fine when used with a HeapVector. The reason why it doesn't fail at run-time is that HeapAllocator::free(), which 'delete' delegates to, is a no-op over heap collection objects. With Oilpan now always enabled, I don't actually know if there's a real reason for not using NOTREACHED() in its body instead.. https://codereview.chromium.org/2021103002/ tries to impose that constraint.
https://codereview.chromium.org/1965543002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:141: std::unique_ptr<RangeVector> ranges(new RangeVector); On 2016/05/30 18:30:41, sof wrote: > On 2016/05/30 17:39:14, chongz wrote: > > oilpan-reviews@ I'm not sure if I should use |std::unique_ptr<T>| on > > |RangeVector| (which is |HeapVector<Member<Range>>| from "Range.h"). > > > > It seems to me that |HeapVector<T>| is |IS_GARBAGE_COLLECTED_TYPE()| so I'd > > expect it to crash with |std::unique_ptr<T>|, but actually it's not... > > > > Also I noticed that the new method from "wtf/PtrUtil.h" |WTF::wrapUnique()| > > won't accept |HeapVector<>|, but it seems that |IS_GARBAGE_COLLECTED_TYPE()| > > does not have to come from |GarbageCollected<T>|? > > Combining std::unique_ptr<> with an Oilpan managed object doesn't make sense, as > you've guessed. > > std::unique_ptr<T> doesn't restrict what Ts (non-garbage collected types) it can > be used with, hence it will compile just fine when used with a HeapVector. The > reason why it doesn't fail at run-time is that HeapAllocator::free(), which > 'delete' delegates to, is a no-op over heap collection objects. > > With Oilpan now always enabled, I don't actually know if there's a real reason > for not using NOTREACHED() in its body instead.. > https://codereview.chromium.org/2021103002/ tries to impose that constraint. Thanks for the review! This makes sense to me, I've removed the use of |std::unique_ptr<>|.
https://codereview.chromium.org/1965543002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1965543002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:141: std::unique_ptr<RangeVector> ranges(new RangeVector); On 2016/05/30 19:08:37, chongz wrote: > On 2016/05/30 18:30:41, sof wrote: > > On 2016/05/30 17:39:14, chongz wrote: > > > oilpan-reviews@ I'm not sure if I should use |std::unique_ptr<T>| on > > > |RangeVector| (which is |HeapVector<Member<Range>>| from "Range.h"). > > > > > > It seems to me that |HeapVector<T>| is |IS_GARBAGE_COLLECTED_TYPE()| so I'd > > > expect it to crash with |std::unique_ptr<T>|, but actually it's not... > > > > > > Also I noticed that the new method from "wtf/PtrUtil.h" |WTF::wrapUnique()| > > > won't accept |HeapVector<>|, but it seems that |IS_GARBAGE_COLLECTED_TYPE()| > > > does not have to come from |GarbageCollected<T>|? > > > > Combining std::unique_ptr<> with an Oilpan managed object doesn't make sense, > as > > you've guessed. > > > > std::unique_ptr<T> doesn't restrict what Ts (non-garbage collected types) it > can > > be used with, hence it will compile just fine when used with a HeapVector. The > > reason why it doesn't fail at run-time is that HeapAllocator::free(), which > > 'delete' delegates to, is a no-op over heap collection objects. > > > > With Oilpan now always enabled, I don't actually know if there's a real reason > > for not using NOTREACHED() in its body instead.. > > https://codereview.chromium.org/2021103002/ tries to impose that constraint. > > Thanks for the review! This makes sense to me, I've removed the use of > |std::unique_ptr<>|. Thanks for bringing up the problem; we can actually go one better and catch this at compile-time, see https://codereview.chromium.org/2021103002/
https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1745: InputEvent* beforeInputEvent = InputEvent::createBeforeInput(InputEvent::InputType::InsertText, data, InputEvent::EventCancelable::IsCancelable, InputEvent::EventIsComposing::NotComposing, nullptr); Should this have a TODO to pass in the range? https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1755: InputEvent* beforeInputEvent = InputEvent::createBeforeInput(inputType, data, cancelable, InputEvent::EventIsComposing::IsComposing, nullptr); Should this have a TODO to pass in the range? https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.cpp:127: // Clear |m_ranges| manually in case event object was held by JavaScript. Can you explain this in more detail? I thought we could mostly avoid manually clearing in an Oilpan world. Disclaimer: My knowledge of oilpan is super limited.
ojan@ Thanks for the review! I've added TODOs and detailed descriptions as per your comments, PTAL. https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1745: InputEvent* beforeInputEvent = InputEvent::createBeforeInput(InputEvent::InputType::InsertText, data, InputEvent::EventCancelable::IsCancelable, InputEvent::EventIsComposing::NotComposing, nullptr); On 2016/05/31 17:47:29, ojan wrote: > Should this have a TODO to pass in the range? Added TODOs, current spec has not defined |getRanges()| for all |inputType| yet. https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1755: InputEvent* beforeInputEvent = InputEvent::createBeforeInput(inputType, data, cancelable, InputEvent::EventIsComposing::IsComposing, nullptr); On 2016/05/31 17:47:29, ojan wrote: > Should this have a TODO to pass in the range? Done. https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.cpp:127: // Clear |m_ranges| manually in case event object was held by JavaScript. On 2016/05/31 17:47:29, ojan wrote: > Can you explain this in more detail? I thought we could mostly avoid manually > clearing in an Oilpan world. Disclaimer: My knowledge of oilpan is super > limited. Added more detailed comments. We want to dereference |Range| in case developers want to hold |InputEvent| objects but not aware that it will reference |Range| objects. Actually this operation should be safe since it's only clearing the vector but not freeing the memory. So the |Range| object inside shouldn't be freed if (by any chance) it's still referenced by someone else.
drive-by-comment https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.idl (right): https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.idl:15: sequence<Range> getRanges(); Web platform API-atypical for this to be "getRanges()" and not a readonly |ranges| attribute?
https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.idl (right): https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.idl:15: sequence<Range> getRanges(); On 2016/05/31 18:34:24, sof wrote: > Web platform API-atypical for this to be "getRanges()" and not a readonly > |ranges| attribute? This is done on purpose. This is intended to return a StaticRange which will be a snapshot at a given time. If we were to use a variable Ranges then it couldn't be a StaticRange and would have to be a live Range; which has detrimental side-effects to performance and ease of object cloning.
https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.idl (right): https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.idl:15: sequence<Range> getRanges(); On 2016/05/31 18:34:24, sof wrote: > Web platform API-atypical for this to be "getRanges()" and not a readonly > |ranges| attribute? Thanks for the question! Please see dtapuska@'s comments above, and for more details please see my next CL (still working): https://codereview.chromium.org/2022863002/diff/1/third_party/WebKit/Source/c...
https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.cpp:127: // Clear |m_ranges| manually in case event object was held by JavaScript. On 2016/05/31 at 18:32:26, chongz wrote: > On 2016/05/31 17:47:29, ojan wrote: > > Can you explain this in more detail? I thought we could mostly avoid manually > > clearing in an Oilpan world. Disclaimer: My knowledge of oilpan is super > > limited. > Added more detailed comments. We want to dereference |Range| in case developers want to hold |InputEvent| objects but not aware that it will reference |Range| objects. > > Actually this operation should be safe since it's only clearing the vector but not freeing the memory. So the |Range| object inside shouldn't be freed if (by any chance) it's still referenced by someone else. I thought the idea was that we would expose StaticRange here, in which case it's OK for them to hold on to it. Am I misremembering? Also, is that web exposed? e.g. if I call getRanges on an input event after we've cleared m_ranges, what happens?
On 2016/05/31 21:35:58, ojan wrote: > https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/events/InputEvent.cpp (right): > > https://codereview.chromium.org/1965543002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/events/InputEvent.cpp:127: // Clear |m_ranges| > manually in case event object was held by JavaScript. > On 2016/05/31 at 18:32:26, chongz wrote: > > On 2016/05/31 17:47:29, ojan wrote: > > > Can you explain this in more detail? I thought we could mostly avoid > manually > > > clearing in an Oilpan world. Disclaimer: My knowledge of oilpan is super > > > limited. > > Added more detailed comments. We want to dereference |Range| in case > developers want to hold |InputEvent| objects but not aware that it will > reference |Range| objects. > > > > Actually this operation should be safe since it's only clearing the vector but > not freeing the memory. So the |Range| object inside shouldn't be freed if (by > any chance) it's still referenced by someone else. > > I thought the idea was that we would expose StaticRange here, in which case it's > OK for them to hold on to it. Am I misremembering? According to https://github.com/garykac/staticrange/blob/master/staticrange.md#note-on-ran... |InputEvent| has to keep an internal copy of |Range| in case previous event handlers in chain modified DOM, and |getRanges()| should return a snapshot of |StaticRange|. Also according to https://github.com/garykac/staticrange/blob/master/staticrange.md#motivation > > "This problem is exacerbated when an application caches an object that happens to contain a |Range| along with other data that the application needs..." Clearing |m_ranges| after dispatch seems to be the only solution... > > Also, is that web exposed? e.g. if I call getRanges on an input event after > we've cleared m_ranges, what happens? Yes, it will return an empty list. See LayoutTest: https://codereview.chromium.org/1965543002/diff/180001/third_party/WebKit/Lay... Alternative solutions would be: a. Don't clear |m_ranges|, if we think the extra cost for holding |InputEvent| is OK. b. Keep an internal copy of |StaticRanges| after dispatch, but it might be confusing. I'm OK with either...
lgtm once you make the comment a bit more clear. https://codereview.chromium.org/1965543002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1965543002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.cpp:98: EventDispatchMediator* InputEvent::createMediator() At some point, we should get rid of this EventDispatchMediator pattern and do something a bit simpler. For example, the InputEvent could have beforeDispatchEvent and afterDispatchEvent virtual methods that dispatchEvent calls instead. Can you put a TODO for now? https://codereview.chromium.org/1965543002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.cpp:131: // explicitly call |getRanges()|->|toRange()| if they want to keep a copy of |Range|. I see. I reread the meeting notes for this. https://docs.google.com/document/d/1hCj6QX77NYIVY0RWrMHT1Yra6t8_Qu8PopaWLG0AM... It's not about memory or performance measurement exactly. There's a few things that went into this: 1. We don't want to expose live Range objects for the author to hold onto because they are live, so they slow down all DOM operations. So, beforeInput doesn't have a way of getting Ranges, just StaticRanges. 2. Naive code that calls getRanges will do the wrong thing if one beforeInput callback modifies the range that's being acted on before the next one that gets called. So the solution was to have a getRanges method that computes the range based off the set of internally stored Ranges. But, we don't want to hold on to those Ranges indefinitely to avoid #1, so we clear them. This will be a weird part of this API, but I don't have a better suggestion at the moment. Please update the comment. Otherwise, looks good.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, dtapuska@chromium.org, ojan@chromium.org Link to the patchset: https://codereview.chromium.org/1965543002/#ps200001 (title: "Update comments about store and clear |Range|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965543002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965543002/200001
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [InputEvent] Support |sequence<Range> getRanges()| in 'beforeinput' This is the first step to support |getRanges()| in 'beforeinput', the next step will be introducing |StaticRange| and change the method to |sequence<StaticRange> getRanges()|. This CL adds supports to all delete related actions, such as delete-character/word/line/paragraph (forward and backward). Will add support to other inputTypes when the spec is ready. SPEC=http://w3c.github.io/editing/input-events.html BUG=609142 ========== to ========== [InputEvent] Support |sequence<Range> getRanges()| in 'beforeinput' This is the first step to support |getRanges()| in 'beforeinput', the next step will be introducing |StaticRange| and change the method to |sequence<StaticRange> getRanges()|. This CL adds supports to all delete related actions, such as delete-character/word/line/paragraph (forward and backward). Will add support to other inputTypes when the spec is ready. SPEC=http://w3c.github.io/editing/input-events.html BUG=609142 Committed: https://crrev.com/5d7c0b13d1cd8e480c7a1b4d9bdd53f9383f2aef Cr-Commit-Position: refs/heads/master@{#397131} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5d7c0b13d1cd8e480c7a1b4d9bdd53f9383f2aef Cr-Commit-Position: refs/heads/master@{#397131} |
