Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781623010:
Add DocumentMarker::clone() (Closed)
DescriptionAdd DocumentMarker::clone()
Being split off from https://codereview.chromium.org/2784753002, where I'm
adding a subclass of DocumentMarker for TextMatchMarker, which necessitates
a virtual clone method so DocumentMarker subclasses can be copied properly. I'm
planning to add subclasses for the other types of DocumentMarker in
https://codereview.chromium.org/2780313002.
BUG=707867
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add missing file #Patch Set 3 : Take const reference instead of prematurely cloning marker #
Total comments: 1
Patch Set 4 : Rebase #
Total comments: 6
Patch Set 5 : Make clone() protected, add cloneWithNewOffsets() #Patch Set 6 : Remove dependency that's messing with trybots #
Messages
Total messages: 43 (26 generated)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2781623010/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2781623010/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:384: DocumentMarker* marker = (*i)->clone(); No need to change this part. |marker| should reference the existing marker. Note that the reference is still valid after calling |list->erase()|, due to Oilpan. https://codereview.chromium.org/2781623010/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h (right): https://codereview.chromium.org/2781623010/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:44: RenderedDocumentMarker* clone() const final; Implementation is still needed. It doesn't matter if we introduce another file. It will soon be removed anyway.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/30 at 02:20:28, xiaochengh wrote: > https://codereview.chromium.org/2781623010/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2781623010/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:384: DocumentMarker* marker = (*i)->clone(); > No need to change this part. |marker| should reference the existing marker. > > Note that the reference is still valid after calling |list->erase()|, due to Oilpan. > > https://codereview.chromium.org/2781623010/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h (right): > > https://codereview.chromium.org/2781623010/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:44: RenderedDocumentMarker* clone() const final; > Implementation is still needed. > > It doesn't matter if we introduce another file. It will soon be removed anyway. These are fixed in Patch #3
Almost good. https://codereview.chromium.org/2781623010/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.cpp (right): https://codereview.chromium.org/2781623010/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.cpp:1: // Copyright 2017 The Chromium Authors. All rights reserved. Please add this file to editing/BUILD.gn
On 2017/03/30 at 18:02:56, xiaochengh wrote: > Almost good. > > https://codereview.chromium.org/2781623010/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.cpp (right): > > https://codereview.chromium.org/2781623010/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.cpp:1: // Copyright 2017 The Chromium Authors. All rights reserved. > Please add this file to editing/BUILD.gn I think I did?
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Yeah, you did. I wasn't too focused...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rlanday@chromium.org changed reviewers: + yosin@chromium.org
rlanday@chromium.org changed required reviewers: + yosin@chromium.org
yosin, can you please take a look at this now that xiaochengh has lgtm'ed?
Description was changed from ========== Add DocumentMarker::clone() Being split off from https://codereview.chromium.org/2784753002, where I'm adding a subclass of DocumentMarker for TextMatchMarker, which necessitates a virtual clone method so DocumentMarker subclasses can be copied properly. I'm planning to add subclasses for the other types of DocumentMarker in https://codereview.chromium.org/2780313002. ========== to ========== Add DocumentMarker::clone() Being split off from https://codereview.chromium.org/2784753002, where I'm adding a subclass of DocumentMarker for TextMatchMarker, which necessitates a virtual clone method so DocumentMarker subclasses can be copied properly. I'm planning to add subclasses for the other types of DocumentMarker in https://codereview.chromium.org/2780313002. BUG=707867 ==========
https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:137: virtual DocumentMarker* clone() const; I don't understand why we need to have |clone()| member function, and copy ctor. We don't have any marker having same properties. The usage of copy-ctor in |DMC::removeMarkers()| is creating |RenderedDocumentMarker()|. So, we should change |DMC::removeMarkers()| not to use copy-ctor. https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:180: DocumentMarker(const DocumentMarker&); Could you add TODO to remove copy-ctor from DocumentMarker? Usage of ctor is questionable and error prone due by subclassing. Also, we want to make |m_type| to mark |const|.
Since RenderedDocumentMarker is used only for painting tick markers for find-in-page, we don't need to have it for other than TextMatch type. Do we really need to have |RenderedDocumentMarker|? Can we compute tick marks on demand?
On 2017/04/04 at 02:00:05, yosin wrote: > Since RenderedDocumentMarker is used only for painting tick markers for find-in-page, > we don't need to have it for other than TextMatch type. > > Do we really need to have |RenderedDocumentMarker|? Can we compute tick marks on demand? This patch only aims that introducing DocumentMarker::clone, so that we have a proper method to copy markers (to be used in DocumentMarkerList::copyMarkers in a follow-up patch crrev.com/2773343003). Adding RenderedDocumentMarker::clone is a side-product that seems unavoidable right now. The entire class will be removed and replace by a new class TextMatchMarker.
On 2017/04/04 at 02:00:05, yosin wrote: > Since RenderedDocumentMarker is used only for painting tick markers for find-in-page, > we don't need to have it for other than TextMatch type. > > Do we really need to have |RenderedDocumentMarker|? We're going to have a subclass for each type of marker after https://codereview.chromium.org/2780313002 anyway; we're moving the RenderedDocumentMarker functionality into TextMatchMarker. > Can we compute tick marks on demand? That's an interesting question; I assume precomputing the marks is supposed to be a performance optimization, but I don't know how much of a difference it makes.
https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:137: virtual DocumentMarker* clone() const; On 2017/04/04 at 01:37:51, yosin_UTC9 wrote: > I don't understand why we need to have |clone()| member function, and copy ctor. > We don't have any marker having same properties. > > The usage of copy-ctor in |DMC::removeMarkers()| is creating |RenderedDocumentMarker()|. > So, we should change |DMC::removeMarkers()| not to use copy-ctor. In https://codereview.chromium.org/2780313002 I'm converting DocumentMarker to a polymorphic class hierarchy. RenderedDocumentMarker is being removed in https://codereview.chromium.org/2723663002 so it's not super relevant. The clone method is so we can properly copy a DocumentMarker. The reason we need to copy markers in removeMarkers() is because if we pass the DoNotRemovePartiallyOverlappingMarker flag and remove the middle of a marker, we actually end up with two markers, which necessitates copying the marker. https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:137: virtual DocumentMarker* clone() const; On 2017/04/04 at 01:37:51, yosin_UTC9 wrote: > I don't understand why we need to have |clone()| member function, and copy ctor. > We don't have any marker having same properties. > > The usage of copy-ctor in |DMC::removeMarkers()| is creating |RenderedDocumentMarker()|. > So, we should change |DMC::removeMarkers()| not to use copy-ctor. In https://codereview.chromium.org/2780313002 I'm converting DocumentMarker to a polymorphic class hierarchy. RenderedDocumentMarker is being removed in https://codereview.chromium.org/2723663002 so it's not super relevant. The clone method is so we can properly copy a DocumentMarker. The reason we need to copy markers in removeMarkers() is because if we pass the DoNotRemovePartiallyOverlappingMarker flag and remove the middle of a marker, we actually end up with two markers, which necessitates copying the marker. https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:180: DocumentMarker(const DocumentMarker&); On 2017/04/04 at 01:37:51, yosin_UTC9 wrote: > Could you add TODO to remove copy-ctor from DocumentMarker? > Usage of ctor is questionable and error prone due by subclassing. I need this to implement clone().
https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2781623010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:137: virtual DocumentMarker* clone() const; On 2017/04/04 at 02:07:46, rlanday wrote: > On 2017/04/04 at 01:37:51, yosin_UTC9 wrote: > > I don't understand why we need to have |clone()| member function, and copy ctor. > > We don't have any marker having same properties. > > > > The usage of copy-ctor in |DMC::removeMarkers()| is creating |RenderedDocumentMarker()|. > > So, we should change |DMC::removeMarkers()| not to use copy-ctor. > > In https://codereview.chromium.org/2780313002 I'm converting DocumentMarker to a polymorphic class hierarchy. RenderedDocumentMarker is being removed in https://codereview.chromium.org/2723663002 so it's not super relevant. The clone method is so we can properly copy a DocumentMarker. The reason we need to copy markers in removeMarkers() is because if we pass the DoNotRemovePartiallyOverlappingMarker flag and remove the middle of a marker, we actually end up with two markers, which necessitates copying the marker. I'm not a fan of clone or copy since we should not have identical DocumentMarker objects. How about introducing createMarker(const DocumentMarker& marker, unsigned startOffset, unsigned endOffset);
On 2017/04/04 at 04:07:38, yosin wrote: > I'm not a fan of clone or copy since we should not have identical DocumentMarker objects. > How about introducing createMarker(const DocumentMarker& marker, unsigned startOffset, unsigned endOffset); I don’t think this would work as a static method because it needs to be overridden in the derived classes to work properly. We could have something like marker->cloneWithOffsets(unsigned startOffset, unsigned endOffset). DocumentMarkerList::copyMarkers() does copy the offsets though (possibly applying an offset)...we could either use the clone() method like I’m adding here and call shiftOffsets() afterward, or have a method cloneWithDeltaShift(int shiftAmount), or just re-use cloneWithOffsets() and manually compute the new offsets. What do you think?
On 2017/04/04 at 06:03:24, rlanday wrote: > On 2017/04/04 at 04:07:38, yosin wrote: > > I'm not a fan of clone or copy since we should not have identical DocumentMarker objects. > > How about introducing createMarker(const DocumentMarker& marker, unsigned startOffset, unsigned endOffset); > > I don’t think this would work as a static method because it needs to be overridden in the derived classes to work properly. > > We could have something like marker->cloneWithOffsets(unsigned startOffset, unsigned endOffset). DocumentMarkerList::copyMarkers() does copy the offsets though (possibly applying an offset)...we could either use the clone() method like I’m adding here and call shiftOffsets() afterward, or have a method cloneWithDeltaShift(int shiftAmount), or just re-use cloneWithOffsets() and manually compute the new offsets. What do you think? Regarding usage pattern of DMC::copyMarkers(), DocumentMarker::splitBefore(int offset) : DocumentMarker* is better. For removeMarkers(), we should change RenderedDocumetnMarker to take start, end and activeMatch, rather than DocumentMarker. BTW, we also want to |bool activeMatch| to enum class. Could you do this before other refactoring?
On 2017/04/04 at 08:27:58, yosin wrote: > On 2017/04/04 at 06:03:24, rlanday wrote: > > On 2017/04/04 at 04:07:38, yosin wrote: > > > I'm not a fan of clone or copy since we should not have identical DocumentMarker objects. > > > How about introducing createMarker(const DocumentMarker& marker, unsigned startOffset, unsigned endOffset); > > > > I don’t think this would work as a static method because it needs to be overridden in the derived classes to work properly. > > > > We could have something like marker->cloneWithOffsets(unsigned startOffset, unsigned endOffset). DocumentMarkerList::copyMarkers() does copy the offsets though (possibly applying an offset)...we could either use the clone() method like I’m adding here and call shiftOffsets() afterward, or have a method cloneWithDeltaShift(int shiftAmount), or just re-use cloneWithOffsets() and manually compute the new offsets. What do you think? > > Regarding usage pattern of DMC::copyMarkers(), DocumentMarker::splitBefore(int offset) : DocumentMarker* is better. > For removeMarkers(), we should change RenderedDocumetnMarker to take start, end and activeMatch, rather than DocumentMarker. > BTW, we also want to |bool activeMatch| to enum class. Could you do this before other refactoring? For copyMarkers(), I think we should just compute the new offsets inside copyMarkers() and have a cloneWithOffsets() method (since it can both pin the new offsets to a range, and apply a shift). I think we should do the same for removeMarkers(). RenderedDocumentMarker is being removed so I think whatever we do with it is kind of a moot point. I'll put up a separate CL for adding the enum.
@yosin, I added the cloneWithNewOffsets() method and made clone() protected, what do you think of this?
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
