Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2393403002:
Introduce Selection class (Closed)
|
Created:
4 years, 2 months ago by yosin_UTC9 Modified:
4 years, 2 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce Selection class
This patch introduces |Selection| class which is exclude visible position
canonicalization from |VisibleSelection|. The relation between |Selection| and
|VisibleSelection| is similar to |Position| and |VisiblePosition|.
This patch is a preparation of implementing lazy visible position
canonicalization on |FrameSelection|, http://crrev.com/1958093002
BUG=139552, 603684, 606499, 625533, 644648
TEST=run_webkit_unittests --gtest_filler=SelectionTest.*
Committed: https://crrev.com/e7a92d5235ce35ad2f7127ebbefbcd357e569379
Cr-Commit-Position: refs/heads/master@{#424950}
Patch Set 1 : 2016-10-06T15:27:59 #
Total comments: 17
Patch Set 2 : 2016-10-07T17:20:15 #
Total comments: 2
Patch Set 3 : 2016-10-07T18:12:22 #Patch Set 4 : 2016-10-07T18:46:48 #
Total comments: 13
Patch Set 5 : 2016-10-12T12:30:28 #Patch Set 6 : 2016-10-12T13:41:52 #Patch Set 7 : 2016-10-12T13:54:28 #
Total comments: 6
Patch Set 8 : 2016-10-12T15:17:37 #Patch Set 9 : 2016-10-12T15:57:57 #
Messages
Total messages: 55 (25 generated)
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 2016-10-06T15:27:59 BUG= ========== to ========== Introduce Selection class This patch introduces |Selection| class which is exclude visible position canonicalization from |VisibleSelection|. The relation between |Selection| and |VisibleSelection| is similar to |Position| and |VisiblePosition|. This patch is a preparation of implementing lazy visible position canonicalization on |FrameSelection|, http://crrev.com/1958093002 BUG=139552, 603684, 606499, 625533, 644648 TEST=run_webkit_unittests --gtest_filler=SelectionTest.* ==========
yosin@chromium.org changed reviewers: + tkent@chromium.org, xiaochengh@chromium.org, yoichio@chromium.org
PTAL
We have web-exposed Selection interface, core/editing/Selectin.idl. Please use a different name.
I'm super excited about this class. It should eliminate tons of creations of VisibleSelections, and hence, synchronous layout calls. I'm fine with the implementation, with just some minor comments as below. https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Selection.cpp (right): https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.cpp:168: SelectionTemplate<Strategy>::Builder::extend( We should check m_selection's validity to ensure that it's built on the same version of DOM tree. https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Selection.h (right): https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.h:22: // pseudo immutable object, you can't change members. I can't understand the first sentence... Please rephrase. Besides, I think just saying it's immutable is good enough, as "pseudo-immutable" is confusing. https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.h:53: // Both |base| and |extent| can be null. I would prefer changing the comment to "|extent| can be non-null while |base| is null, which is undesired." to emphasize what is wrong with the deprecated version.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Selection.h (right): https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.h:30: // |Builder| is a helper class for constructing |SelectionTemplate| object. Who use this class and how does?
On 2016/10/06 at 07:09:28, tkent wrote: > We have web-exposed Selection interface, core/editing/Selectin.idl. > Please use a different name. I would like to keep class name Selection. An implementation of Selection.idl in DOMSelection.cpp. This is similar Window.idl to DOMWindow.cpp.
On 2016/10/06 at 09:31:12, yosin wrote:
> On 2016/10/06 at 07:09:28, tkent wrote:
> > We have web-exposed Selection interface, core/editing/Selectin.idl.
> > Please use a different name.
>
> I would like to keep class name Selection. An implementation of Selection.idl
in DOMSelection.cpp.
> This is similar Window.idl to DOMWindow.cpp.
I strongly object to the name Selection.{h,cpp}. It's very confusing.
Window.idl and DOMWindow.{h,cpp} aren't so confusing because we don't have
Window.{h,cpp}.
In WebKit, we added 'DOM' prefixes if we could not use unprefixed file names or
unprefixed C++ class names. We didn't add directory names to #include, and we
had file name conflicts frequently.
Now we have directory names in #include. We should try to unprefix.
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Selection.cpp (right): https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.cpp:20: #if ENABLE(ASSERT) ENABLE(ASSERT) -> DCHECK_IS_ON() https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.cpp:86: #if ENABLE(ASSERT) ENABLE(ASSERT) -> DCHECK_IS_ON() https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.cpp:108: #if ENABLE(ASSERT) ENABLE(ASSERT) -> DCHECK_IS_ON() https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.cpp:151: #if ENABLE(ASSERT) ENABLE(ASSERT) -> DCHECK_IS_ON() https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.cpp:190: #if ENABLE(ASSERT) ENABLE(ASSERT) -> DCHECK_IS_ON() https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Selection.h (right): https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.h:106: #if ENABLE(ASSERT) ENABLE(ASSERT) -> DCHECK_IS_ON()
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Selection.h (right): https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.h:40: // Move selection to |base|. |base| can't be null. How about naming |setBase| and |setExtent| so that Builder is a builder rather than Selection API.
The CQ bit was checked by yosin@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...
PTAL
- Renamed "Selection.{cpp,h}" to "SelectionTemplate.{cpp,h}"
- Renamed |Selection| to |SelectionInDOMTree|
- Update comments in |SelectionTemplate|
We may want to rename "Position.{cpp,h}", "EphemeralRange.{cpp,h}" to have
"Template" suffix
and |Position| and |EphemeralRange| to |PositonInDOMTree| and
|EphemeralRangeInDOMTree| respectively. But, it may not be happened in near
future...
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/Selection.h (right):
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Selection.h:22: // pseudo immutable
object, you can't change members.
On 2016/10/06 at 08:02:33, Xiaocheng wrote:
> I can't understand the first sentence... Please rephrase.
>
> Besides, I think just saying it's immutable is good enough, as
"pseudo-immutable" is confusing.
Done.
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Selection.h:30: // |Builder| is a helper
class for constructing |SelectionTemplate| object.
On 2016/10/06 at 08:52:17, yoichio wrote:
> Who use this class and how does?
We'll use |Builder| class everywhere we construct |Selection| object.
"SelectionTest.cpp" and http://crrev.com/1958093002 have usage samples.
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Selection.h:40: // Move selection to
|base|. |base| can't be null.
On 2016/10/07 at 04:30:40, yoichio wrote:
> How about naming |setBase| and |setExtent| so that
> Builder is a builder rather than Selection API.
No, we don't want to have base=null, extent=non-null case.
Clients of |Builder| class should use:
- collapse(base).extend(extent)
- setBaseAndExtent(base, extent)
- setBaseAndExtent(range)
- setBaseAndExtentDeprecated(base, extent); for execCommand
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Selection.h:53: // Both |base| and
|extent| can be null.
On 2016/10/06 at 08:02:33, Xiaocheng wrote:
> I would prefer changing the comment to "|extent| can be non-null while |base|
is null, which is undesired." to emphasize what is wrong with the deprecated
version.
Done.
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Selection.h:106: #if ENABLE(ASSERT)
On 2016/10/07 at 00:13:27, tkent wrote:
> ENABLE(ASSERT) -> DCHECK_IS_ON()
Done.
https://codereview.chromium.org/2393403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2393403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:169: SelectionTemplate<Strategy>::Builder::extend( We should call |m_selection.assertValid()| in this function to prevent such pattern: 1. builder.collapse() 2. do some change to DOM tree 3. builder.extend()
PTAL Updated for xiachengh@ comments. https://codereview.chromium.org/2393403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2393403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:169: SelectionTemplate<Strategy>::Builder::extend( On 2016/10/07 at 09:00:52, Xiaocheng wrote: > We should call |m_selection.assertValid()| in this function to prevent such pattern: > > 1. builder.collapse() > 2. do some change to DOM tree > 3. builder.extend() Good catch! I forgot to do so. Done.
https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Selection.h (right): https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Selection.h:40: // Move selection to |base|. |base| can't be null. On 2016/10/07 08:46:57, Yosi_UTC9 wrote: > On 2016/10/07 at 04:30:40, yoichio wrote: > > How about naming |setBase| and |setExtent| so that > > Builder is a builder rather than Selection API. > > No, we don't want to have base=null, extent=non-null case. > Clients of |Builder| class should use: > - collapse(base).extend(extent) > - setBaseAndExtent(base, extent) > - setBaseAndExtent(range) > - setBaseAndExtentDeprecated(base, extent); for execCommand setBase(base).setExtent(extent) doesn't work? We can assert when |base=null, extent=non-null| in setExtent() or build(). > // Move selection to |base|. |base| can't be null I don't think builder behaves as real Selection class. It should be just a container.
On 2016/10/07 at 09:32:13, yoichio wrote: > https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/Selection.h (right): > > https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/Selection.h:40: // Move selection to |base|. |base| can't be null. > On 2016/10/07 08:46:57, Yosi_UTC9 wrote: > > On 2016/10/07 at 04:30:40, yoichio wrote: > > > How about naming |setBase| and |setExtent| so that > > > Builder is a builder rather than Selection API. > > > > No, we don't want to have base=null, extent=non-null case. > > Clients of |Builder| class should use: > > - collapse(base).extend(extent) > > - setBaseAndExtent(base, extent) > > - setBaseAndExtent(range) > > - setBaseAndExtentDeprecated(base, extent); for execCommand > setBase(base).setExtent(extent) doesn't work? > We can assert when |base=null, extent=non-null| in setExtent() > or build(). > setBase/setExtent can allow us to write two patterns of code: - builder.setBase(base).setExtent(extent) - builder.setExtent(extent).setBase(base) collapse/extend force us to write builder.collpase(base).extend(extent) In this form, there are no ambiguity. > > // Move selection to |base|. |base| can't be null > I don't think builder behaves as real Selection class. > It should be just a container. collapse/extend is name of kind of setter with denoting order of them.
The CQ bit was checked by yosin@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 2016/10/07 09:45:31, Yosi_UTC9 wrote: > On 2016/10/07 at 09:32:13, yoichio wrote: > > > https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/editing/Selection.h (right): > > > > > https://codereview.chromium.org/2393403002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/editing/Selection.h:40: // Move selection to > |base|. |base| can't be null. > > On 2016/10/07 08:46:57, Yosi_UTC9 wrote: > > > On 2016/10/07 at 04:30:40, yoichio wrote: > > > > How about naming |setBase| and |setExtent| so that > > > > Builder is a builder rather than Selection API. > > > > > > No, we don't want to have base=null, extent=non-null case. > > > Clients of |Builder| class should use: > > > - collapse(base).extend(extent) > > > - setBaseAndExtent(base, extent) > > > - setBaseAndExtent(range) > > > - setBaseAndExtentDeprecated(base, extent); for execCommand > > setBase(base).setExtent(extent) doesn't work? > > We can assert when |base=null, extent=non-null| in setExtent() > > or build(). > > > setBase/setExtent can allow us to write two patterns of code: > - builder.setBase(base).setExtent(extent) > - builder.setExtent(extent).setBase(base) > > collapse/extend force us to write builder.collpase(base).extend(extent) > In this form, there are no ambiguity. > > > > // Move selection to |base|. |base| can't be null > > I don't think builder behaves as real Selection class. > > It should be just a container. > collapse/extend is name of kind of setter with denoting order of them. I see. Then, only having setBase and setBaseAndExtent doesn't work?
https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:142: return m_selection; DCHECK(m_selection.assertValid()) ? https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:189: if (range.isNull()) { Do we need this kind of resetting? https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:205: if (base.isNull()) { ditto https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right): https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.h:54: Builder& setBaseAndExtentDeprecated( I don't understand why you introduce "deprecated" function to the new class.
https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right): https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.h:89: visitor->trace(m_base); nit: I think we has no reason to define the function body in the header. https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...
The CQ bit was checked by yosin@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...
PTAL https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:142: return m_selection; On 2016/10/12 at 01:37:00, yoichio wrote: > DCHECK(m_selection.assertValid()) ? Good catch! Done. https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:189: if (range.isNull()) { On 2016/10/12 at 01:37:00, yoichio wrote: > Do we need this kind of resetting? Yes. There are usages in SelectionEditor::pdateCachedVisibleSelectionIfNeeded() in http://crrev.com/1958093002 If we don't allow to pass (null, null) to setBaseAndExtent() we should write: SelectionInFlatTree::Builder builder; const PositionInFlatTree& base = toPositionInFlatTree(m_selection.base()); const PositionInFlatTree& extent = toPositionInFlatTree(m_selection.extent()); if (base.isNotNull()) { DCHECK(extent.isNotNull()); builder.setBaseAndExtent(base, extent); } else { DCHECK(extent.isNull()); // optional: we can omit but I prefer to have. } This leads callers to do more things. Setting both base/extent at once is normal and common case if base/extent != null or base/extent == null. Again, base == null && extent != null case denotes something wrong in caller. https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:205: if (base.isNull()) { On 2016/10/12 at 01:37:00, yoichio wrote: > ditto See a reply in EphemeralRange version of setBaseAndExtent. https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:210: return collapse(base).extend(extent); Note: |base| can be following to |extent| https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right): https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.h:54: Builder& setBaseAndExtentDeprecated( On 2016/10/12 at 01:37:00, yoichio wrote: > I don't understand why you introduce "deprecated" function to > the new class. setBaseAndExtent() doesn't allow to pass setBaseAndExtent(null, non-null) but setBaseAndExtentDeprecate() allows. This function is used for rewriting following pattern in core/editing/commands/ VisibleSelection selection(base, extent); frame()->selection()->setSelection(selection); to SelectionInDOMTree::Builder builder; build.setBaseAndExtentDeprecate(base, extent); frame()->selection()->setSelection(selection.build()); https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.h:89: visitor->trace(m_base); On 2016/10/12 at 02:00:33, tkent wrote: > nit: I think we has no reason to define the function body in the header. > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... We can't use DEFINE_TRACE(T) macro since it doesn't support |template<T...>| directive. DEFINE_TRACE(T) is expanded into void T::trace(...) {...} template <typename V> void T::traceImpl(...) { ... } But, we would like to have template<typename S> void T<S>::trace() { ... } template<typename S> template <V> void T<S>::traceImpl(...) { ... } Others, e.g. EphemeralRangeTemplate, VisiblePositon, also have inline version of TRACE macro. :-<
https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right): https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.h:89: visitor->trace(m_base); On 2016/10/12 at 03:39:02, Yosi_UTC9 wrote: > On 2016/10/12 at 02:00:33, tkent wrote: > > nit: I think we has no reason to define the function body in the header. > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... > > We can't use DEFINE_TRACE(T) macro since it doesn't support |template<T...>| directive. > DEFINE_TRACE(T) is expanded into > void T::trace(...) {...} > template <typename V> void T::traceImpl(...) { ... } > > But, we would like to have > > template<typename S> void T<S>::trace() { ... } > template<typename S> template <V> void T<S>::traceImpl(...) { ... } > > Others, e.g. EphemeralRangeTemplate, VisiblePositon, also have inline version of TRACE macro. :-< Got it. Let's file a bug with component:Blink>MemoryAllocator>GarbageCollection.
On 2016/10/12 at 03:44:55, tkent wrote: > https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right): > > https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/SelectionTemplate.h:89: visitor->trace(m_base); > On 2016/10/12 at 03:39:02, Yosi_UTC9 wrote: > > On 2016/10/12 at 02:00:33, tkent wrote: > > > nit: I think we has no reason to define the function body in the header. > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... > > > > We can't use DEFINE_TRACE(T) macro since it doesn't support |template<T...>| directive. > > DEFINE_TRACE(T) is expanded into > > void T::trace(...) {...} > > template <typename V> void T::traceImpl(...) { ... } > > > > But, we would like to have > > > > template<typename S> void T<S>::trace() { ... } > > template<typename S> template <V> void T<S>::traceImpl(...) { ... } > > > > Others, e.g. EphemeralRangeTemplate, VisiblePositon, also have inline version of TRACE macro. :-< > > Got it. Let's file a bug with component:Blink>MemoryAllocator>GarbageCollection. Filed: http://crbug.com/654985
The CQ bit was checked by yosin@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by yosin@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...
PTAL Make assertValid() to return bool for writing DCHECK(assertValid())
Almost good to me, with a few comments. https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:90: bool SelectionTemplate<Strategy>::assertValid() const { I think we should check whether m_base and m_extent are in the same document. https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:175: SelectionTemplate<Strategy>::Builder::extend( I think we should DCHECK_EQ(m_selection.base().document(), position.document()). https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right): https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionTemplate.h:86: // Returns true if |this| selection holds valid values otherwise it causes, nit: weird position of the comma.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
PTAL - Update comments - Check extend() taken a position in same document of base. https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:90: bool SelectionTemplate<Strategy>::assertValid() const { On 2016/10/12 at 05:09:42, Xiaocheng wrote: > I think we should check whether m_base and m_extent are in the same document. Done. https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:175: SelectionTemplate<Strategy>::Builder::extend( On 2016/10/12 at 05:09:42, Xiaocheng wrote: > I think we should DCHECK_EQ(m_selection.base().document(), position.document()). Done. https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right): https://codereview.chromium.org/2393403002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionTemplate.h:86: // Returns true if |this| selection holds valid values otherwise it causes, On 2016/10/12 at 05:09:42, Xiaocheng wrote: > nit: weird position of the comma. Done.
lgtm
https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.h (right): https://codereview.chromium.org/2393403002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.h:54: Builder& setBaseAndExtentDeprecated( On 2016/10/12 03:39:02, Yosi_UTC9 wrote: > On 2016/10/12 at 01:37:00, yoichio wrote: > > I don't understand why you introduce "deprecated" function to > > the new class. > > setBaseAndExtent() doesn't allow to pass setBaseAndExtent(null, non-null) > but setBaseAndExtentDeprecate() allows. > > This function is used for rewriting following pattern in core/editing/commands/ > VisibleSelection selection(base, extent); > frame()->selection()->setSelection(selection); > to > SelectionInDOMTree::Builder builder; > build.setBaseAndExtentDeprecate(base, extent); > frame()->selection()->setSelection(selection.build()); Do you plan to deprecate this? Please comment "// TODO:*** ".
PTAL Add TODO
lgtm
lgtm
The CQ bit was checked by yosin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2393403002/#ps150001 (title: "2016-10-12T15:57:57")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Introduce Selection class This patch introduces |Selection| class which is exclude visible position canonicalization from |VisibleSelection|. The relation between |Selection| and |VisibleSelection| is similar to |Position| and |VisiblePosition|. This patch is a preparation of implementing lazy visible position canonicalization on |FrameSelection|, http://crrev.com/1958093002 BUG=139552, 603684, 606499, 625533, 644648 TEST=run_webkit_unittests --gtest_filler=SelectionTest.* ========== to ========== Introduce Selection class This patch introduces |Selection| class which is exclude visible position canonicalization from |VisibleSelection|. The relation between |Selection| and |VisibleSelection| is similar to |Position| and |VisiblePosition|. This patch is a preparation of implementing lazy visible position canonicalization on |FrameSelection|, http://crrev.com/1958093002 BUG=139552, 603684, 606499, 625533, 644648 TEST=run_webkit_unittests --gtest_filler=SelectionTest.* Committed: https://crrev.com/e7a92d5235ce35ad2f7127ebbefbcd357e569379 Cr-Commit-Position: refs/heads/master@{#424950} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e7a92d5235ce35ad2f7127ebbefbcd357e569379 Cr-Commit-Position: refs/heads/master@{#424950} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
