Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775663008:
LayoutObject::absoluteBoundingBoxRectForRange() should take EphemeralRange (Closed)
DescriptionLayoutObject::absoluteBoundingBoxRectForRange() should take EphemeralRange
Currently LayoutObject::absoluteBoundingBoxRectForRange
uses Range object to calculate textQuads. It should use EphemeralRange,
as Layoutobject are owned by DOM.
This Bug is divided into two parts.
1) This current CL will introduce textQuads taking EphemeralRange as input in VisibleUnit.
2) Other CL will make LayoutObject::absoluteBoundingBoxRectForRange() to take EphemeralRange
BUG=691198
Patch Set 1 #
Total comments: 7
Patch Set 2 : Y #Patch Set 3 : Y #
Total comments: 2
Patch Set 4 : Y #
Total comments: 21
Patch Set 5 : Y #Patch Set 6 : Y #Patch Set 7 : Review comments Addressed #
Total comments: 3
Patch Set 8 : Addressed Review comments #
Total comments: 7
Patch Set 9 : Split Patch:1(TextQuads at VisibleUnits) #
Total comments: 2
Messages
Total messages: 72 (40 generated)
Description was changed from ========== LayoutObject::absoluteBoundingBoxRectForRange() should take EphemeralRange Currently LayoutObject::absoluteBoundingBoxRectForRange uses Range object to calculate textQuads. It should use EphemeralRange, as Layoutobject are owned by DOM. This CL makes absoluteBoundingBoxRectForRange to use EphemeralRange. BUG=691198 ========== to ========== LayoutObject::absoluteBoundingBoxRectForRange() should take EphemeralRange Currently LayoutObject::absoluteBoundingBoxRectForRange uses Range object to calculate textQuads. It should use EphemeralRange, as Layoutobject are owned by DOM. This CL makes absoluteBoundingBoxRectForRange to use EphemeralRange. BUG=691198 R=SamsungPeerReview ==========
tanvir.rizvi@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com
Peer review looks good
Description was changed from ========== LayoutObject::absoluteBoundingBoxRectForRange() should take EphemeralRange Currently LayoutObject::absoluteBoundingBoxRectForRange uses Range object to calculate textQuads. It should use EphemeralRange, as Layoutobject are owned by DOM. This CL makes absoluteBoundingBoxRectForRange to use EphemeralRange. BUG=691198 R=SamsungPeerReview ========== to ========== LayoutObject::absoluteBoundingBoxRectForRange() should take EphemeralRange Currently LayoutObject::absoluteBoundingBoxRectForRange uses Range object to calculate textQuads. It should use EphemeralRange, as Layoutobject are owned by DOM. This CL makes absoluteBoundingBoxRectForRange to use EphemeralRange. BUG=691198 ==========
tanvir.rizvi@samsung.com changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
PTAL!!! Thanks
yosin@: As we proceed, we are having more and more identical functions in Range
and EphemeralRange.
How about making some member functions of Range wrappers of EphemeralRange? E.g.
void Range::textQuads(quads) const {
EphemeralRange(this).textQuads(quads);
}
Node* Range::commonAncestorContainer() const {
return EphemeralRange(this).commonAncestorContainer();
}
...
https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/EphemeralRange.cpp (right):
https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/EphemeralRange.cpp:152: void
EphemeralRangeTemplate<Strategy>::textQuads(
I'm shocked by that we currently don't have unit test for Range::textQuads..
Could you add unit tests for EphemeralRange::textQuads and
EphemeralRangeInFlatTree::textQuads, to help us make our code base healthier? :)
https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FindInPageCoordinates.cpp (right):
https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FindInPageCoordinates.cpp:147: if (range.isNull()
|| !range.startPosition().nodeAsRangeFirstNode())
Could you store the result of |range.startPosition().nodeAsRangeFirstNode()| to
eliminate repeated computation?
yoichio@chromium.org changed reviewers: + yoichio@chromium.org
https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EphemeralRange.cpp (right): https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EphemeralRange.cpp:152: void EphemeralRangeTemplate<Strategy>::textQuads( We want EphemeralRange to be independent from Layout. Could you define textQuads at VisibleUnit?
https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EphemeralRange.cpp (right): https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EphemeralRange.cpp:160: LayoutObject* r = node.layoutObject(); Please avoid to use one letter variable name. BTW, |r| comes from we called |LayoutObject| as |RenderedObject|. At this time |r| is meaningless.
https://codereview.chromium.org/2775663008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2775663008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2061: EXPECT_EQ(quadsStartToThree[0].toString(), quadsStartToTwo[0].toString()); toString() may not needed here https://codereview.chromium.org/2775663008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2095: EXPECT_EQ(quadsStartToThree[0].toString(), quadsStartToFive[0].toString()); ditto
PTAL!! Thanks. https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EphemeralRange.cpp (right): https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EphemeralRange.cpp:152: void EphemeralRangeTemplate<Strategy>::textQuads( On 2017/03/24 18:19:26, Xiaocheng wrote: > I'm shocked by that we currently don't have unit test for Range::textQuads.. > > Could you add unit tests for EphemeralRange::textQuads and > EphemeralRangeInFlatTree::textQuads, to help us make our code base healthier? :) Done. https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EphemeralRange.cpp:152: void EphemeralRangeTemplate<Strategy>::textQuads( On 2017/03/27 05:24:46, yoichio wrote: > We want EphemeralRange to be independent from Layout. > Could you define textQuads at VisibleUnit? Done. https://codereview.chromium.org/2775663008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EphemeralRange.cpp:160: LayoutObject* r = node.layoutObject(); On 2017/03/27 05:43:14, yosin_UTC9 wrote: > Please avoid to use one letter variable name. > BTW, |r| comes from we called |LayoutObject| as |RenderedObject|. At this time > |r| is meaningless. Done.
https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4008: void textQuads(Vector<FloatQuad>& quad, nit: Please make this function |static| and rename it to |textQuadAlgorithm| to align with other functions in the same file. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2059: EXPECT_EQ(2.0, quadsStartToTwo.size()); s/2.0/2u/ Ditto for all other vector size validations. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2061: EXPECT_EQ(quadsStartToThree[0], quadsStartToTwo[0]); Please also verify the actual values of these two quads. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2097: EXPECT_EQ("8,10; 12,10; 12,11; 8,11", quadsStartToFive[0].toString()); Please also verify the actual values of all other quads. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1004: textQuad(quads, range); Let's make textQuad return |Vector<FloatQuad>| directly, since WTF::Vector supports moving semantics. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FindInPageCoordinates.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FindInPageCoordinates.cpp:151: range.startPosition().nodeAsRangeFirstNode()->layoutObject(); Should have null ptr check for range.startPosition().nodeAsRangeFirstNode(). https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:535: EphemeralRange range(match.m_range.get()); nit: |const EphemeralRange| https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:571: EphemeralRange range(activeMatch()); nit: |const EphemeralRange|
I prefer splitting this patch more: Patch 1: Introducing VisibleUnit::textQuads and use it in LayoutObject::absoluteBoundingBoxRectForRange(Range). Patch 2: Change LayoutObject::absoluteBoundingBoxRectForRange to take EpehemeralRange
https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:33: #include "core/editing/EphemeralRange.h" Could you declare |class EphemeralRange;| instead of inclusion? https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FindInPageCoordinates.h (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FindInPageCoordinates.h:34: #include "core/editing/EphemeralRange.h" ditto
Could you run "git cl try" before requesting review? https://dev.chromium.org/developers/testing/try-server-usage
https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:33: #include "core/editing/EphemeralRange.h" On 2017/03/29 at 01:51:15, yoichio wrote: > Could you declare |class EphemeralRange;| instead of inclusion? |EphemeralRange| is just a alias, which cannot be forward declared. In fact, this bothers me a little because now a lot of headers have to include the full EphemeralRange.h instead of a forward declaration, which increases compiling time. Maybe we should split EphemeralRange.h into two headers: EphemeralRangeFwd.h: template <typename Strategy> class EphemeralRangeTemplate; using EphemeralRange = EphemeralRangeTemplate<EditingStrategy>; using EphemeralRangeInFlatTree = EphemeralRangeTemplate<EditingInFlatTreeStrategy>; EphemeralRange.h: #include "EphemeralRangeFwd.h" template <typename Strategy> class EphemeralRange { // Full class declaration }; Then we can include EphemeralRangeFwd.h in other header files, and EphemeralRange.h in .cpp files. Or maybe we can have an "EditingForward.h" collecting all such forward template class and alias declarations.
https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4008: void textQuads(Vector<FloatQuad>& quad, Could you change this function at "Range.cpp" with "TODO: we'll move blach..." in this CL? Then move this to "VisibleUnits.cpp" to see difference between Range and EphemeralRange version easier?
I prefer adding "template <typename Strategy> class EphemeralRangeTemplate; using EphemeralRange = EphemeralRangeTemplate<EditingStrategy>;" in header because it is not so longer than "class EphemeralRangeTemplate;".
On 2017/03/29 at 06:16:17, yoichio wrote: > I prefer adding > "template <typename Strategy> class EphemeralRangeTemplate; > using EphemeralRange = EphemeralRangeTemplate<EditingStrategy>;" in header > because it is not so longer than "class EphemeralRangeTemplate;". I filed crbug.com/706527 for the forward declaration issue. For this patch, I think it's OK to just include EphemeralRange.h. All other header files are doing that, so we shouldn't introduce an ad-hoc solution here.
The CQ bit was checked by tanvir.rizvi@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by tanvir.rizvi@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by shanmuga.m@samsung.com 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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by tanvir.rizvi@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
On 2017/03/29 01:52:33, yoichio wrote: > Could you run "git cl try" before requesting review? > https://dev.chromium.org/developers/testing/try-server-usage @yoichio, currenlty i don't have the permission for try bot. So unable to run.
The CQ bit was checked by tanvir.rizvi@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by shanmuga.m@samsung.com 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...
https://codereview.chromium.org/2775663008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2775663008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4077: Vector<FloatQuad> textQuad(const EphemeralRange& range) { nit: s/textQuad/textQuads/ https://codereview.chromium.org/2775663008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4081: Vector<FloatQuad> textQuad(const EphemeralRangeInFlatTree& range) { nit: nit: s/textQuad/textQuads/ https://codereview.chromium.org/2775663008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2775663008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1015: Vector<FloatQuad> quads = textQuad(range); Sorry, my mistake. I did an experiment and found that this doesn't invoke move assignment, but copy assignment instead. Please switch back to passing a |Vector<FloatQuad>*| to the function for output value. Note: Google C++ style guide prohibits using non-constant reference argument for output value: https://google.github.io/styleguide/cppguide.html#Reference_Arguments
https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4008: void textQuads(Vector<FloatQuad>& quad, On 2017/03/28 21:29:34, Xiaocheng wrote: > nit: Please make this function |static| and rename it to |textQuadAlgorithm| to > align with other functions in the same file. Done. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4008: void textQuads(Vector<FloatQuad>& quad, On 2017/03/29 02:31:40, yosin_UTC9 wrote: > Could you change this function at "Range.cpp" with "TODO: we'll move blach..." > in this CL? > Then move this to "VisibleUnits.cpp" to see difference between Range and > EphemeralRange version easier? I did not understand this comment from Range.cpp context. Could you please let me know on this ? https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2059: EXPECT_EQ(2.0, quadsStartToTwo.size()); On 2017/03/28 21:29:34, Xiaocheng wrote: > s/2.0/2u/ > > Ditto for all other vector size validations. Done. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2061: EXPECT_EQ(quadsStartToThree[0], quadsStartToTwo[0]); On 2017/03/28 21:29:34, Xiaocheng wrote: > Please also verify the actual values of these two quads. Done. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2097: EXPECT_EQ("8,10; 12,10; 12,11; 8,11", quadsStartToFive[0].toString()); On 2017/03/28 21:29:34, Xiaocheng wrote: > Please also verify the actual values of all other quads. Done. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1004: textQuad(quads, range); On 2017/03/28 21:29:34, Xiaocheng wrote: > Let's make textQuad return |Vector<FloatQuad>| directly, since WTF::Vector > supports moving semantics. Done. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FindInPageCoordinates.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FindInPageCoordinates.cpp:151: range.startPosition().nodeAsRangeFirstNode()->layoutObject(); On 2017/03/28 21:29:34, Xiaocheng wrote: > Should have null ptr check for range.startPosition().nodeAsRangeFirstNode(). Done. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:535: EphemeralRange range(match.m_range.get()); On 2017/03/28 21:29:34, Xiaocheng wrote: > nit: |const EphemeralRange| Done. https://codereview.chromium.org/2775663008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:571: EphemeralRange range(activeMatch()); On 2017/03/28 21:29:34, Xiaocheng wrote: > nit: |const EphemeralRange| Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tanvir.rizvi@samsung.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by srirama.m@samsung.com 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
In My Patch set 7, unit test for textQuads were failing after I rebased the code, Before Rebasing all Unit tests cases were passing. I checked and found out that the failures were because of the below patch https://codereview.chromium.org/2763013002, i confirmed by reverting this patch. This patch did changes for bounding box position. Looking at the bug and changes, the new test cases in VisibleUnits are done and looks fine. Please have a look at the above patch once. Thanks!!!
Latest patch, values for textQuads are coming different on different Mac windows and Linux and therefore unit test case failing. So we can't compare the values directly? I didn't verified on Mac and Windows the values as i don't have the setup with my patch and reverting the patch i mentioned in previous patch.
xiaochengh@chromium.org changed reviewers: + kojii@chromium.org
+kojii, +eae, +wanchang.ryu@lge.com https://codereview.chromium.org/2763013002 seems to be breaking the behavior. It makes LayoutText::absoluteQuadsForRange erase the output vector in some cases, which doesn't seem correct. Other code change looks fine. Please consider the split yoichio@ suggested. In general, for patches modifying multiple APIs, we prefer splitting it into multiple patches, each of which modifies only one API, so that each patch touches as few components as possible. https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4052: static void textQuadAlgorithm(Vector<FloatQuad>& quads, Same here, use |Vector<FloatQuad>*| https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.h (right): https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.h:343: CORE_EXPORT void textQuads(Vector<FloatQuad>&, const EphemeralRange&); Please use |Vector<FloatQuad>*| to pass output parameter because Google coding style doesn't allow passing non-const reference parameters: https://google.github.io/styleguide/cppguide.html#Reference_Arguments Some old code may violate it, but at least we shouldn't create new violations. https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2001: EXPECT_EQ("10,8; 15,8; 15,9; 10,9", quadsStartToTwo[0].toString()); Wait... Doesn't https://codereview.chromium.org/2763013002 break the behavior? "10,8; 15,8; 15,9; 10,9" is just the quad for "11111". The quad for "00" is missing. https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2002: EXPECT_EQ("15,8; 17,8; 17,9; 15,9", quadsStartToThree[0].toString()); Same here, "15,8; 17,8; 17,9; 15,9" is the quad for "22". The quads for "00" and "11111" are missing.
xiaochengh@chromium.org changed reviewers: + eae@chromium.org, wanchang.ryu@lge.com
xiaochengh@, thank you for catching this! Would it be possible to land a failing test with "Disabled_"? That'd help us to see what are passed to LayoutText::absoluteRectsForRange().
I think it's better to track in issue, filed http://crbug.com/707627
Description was changed from ========== LayoutObject::absoluteBoundingBoxRectForRange() should take EphemeralRange Currently LayoutObject::absoluteBoundingBoxRectForRange uses Range object to calculate textQuads. It should use EphemeralRange, as Layoutobject are owned by DOM. This CL makes absoluteBoundingBoxRectForRange to use EphemeralRange. BUG=691198 ========== to ========== LayoutObject::absoluteBoundingBoxRectForRange() should take EphemeralRange Currently LayoutObject::absoluteBoundingBoxRectForRange uses Range object to calculate textQuads. It should use EphemeralRange, as Layoutobject are owned by DOM. This Bug is divided into two parts. 1) This current CL will introduce textQuads taking EphemeralRange as input in VisibleUnit. 2) Other CL will make LayoutObject::absoluteBoundingBoxRectForRange() to take EphemeralRange BUG=691198 ==========
PTAL!!! I have divided this Bug into two parts 1) Introduce textQuads in VisibleUnits and LayoutObject::absoluteBoundingBoxRectForRange() to use that. 2) Make LayoutObject::absoluteBoundingBoxRectForRange() to take EphemeralRange as Input. This is the first Patch. Note: This patch correct behavior is dependent on https://codereview.chromium.org/2792953002. I have verified on my system locally merging the changes of this patch and all unit tests were passing with proper values . https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4052: static void textQuadAlgorithm(Vector<FloatQuad>& quads, On 2017/03/31 18:01:18, Xiaocheng wrote: > Same here, use |Vector<FloatQuad>*| Done. https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.h (right): https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.h:343: CORE_EXPORT void textQuads(Vector<FloatQuad>&, const EphemeralRange&); On 2017/03/31 18:01:19, Xiaocheng wrote: > Please use |Vector<FloatQuad>*| to pass output parameter because Google coding > style doesn't allow passing non-const reference parameters: > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > Some old code may violate it, but at least we shouldn't create new violations. Done. https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right): https://codereview.chromium.org/2775663008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2001: EXPECT_EQ("10,8; 15,8; 15,9; 10,9", quadsStartToTwo[0].toString()); On 2017/03/31 18:01:19, Xiaocheng wrote: > Wait... Doesn't https://codereview.chromium.org/2763013002 break the behavior? > > "10,8; 15,8; 15,9; 10,9" is just the quad for "11111". The quad for "00" is > missing. Based on the discussion in https://bugs.chromium.org/p/chromium/issues/detail?id=707627, a patch is submitted to take care of this. And so i have moved back to previous TextCase of #Patch7.
lgtm. Please wait for the fix to crbug.com/707627 lands. Or if you don't want to be blocked, you may mark the unit tests as DISABLED_ with the bug number 707627, and ask wanchang.ryu@ to unmark them in the fix.
RS LGTM for core/layout/
https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4052: static void textQuadAlgorithm(Vector<FloatQuad>* quads, Please change |Range::textQuads()| instead of copying this from "Range.cpp". We don't want to have duplicated code. https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/VisibleUnits.h (right): https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/VisibleUnits.h:343: CORE_EXPORT void textQuads(Vector<FloatQuad>*, const EphemeralRange&); Please name this function as |computeTextQuads()| to make function name starts with verb.
On 2017/04/04 01:25:22, yosin_UTC9 wrote: > https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): > > https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4052: static void > textQuadAlgorithm(Vector<FloatQuad>* quads, > Please change |Range::textQuads()| instead of copying this from "Range.cpp". > We don't want to have duplicated code. > > https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/VisibleUnits.h (right): > > https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/VisibleUnits.h:343: CORE_EXPORT void > textQuads(Vector<FloatQuad>*, const EphemeralRange&); > Please name this function as |computeTextQuads()| to make function name starts > with verb. Yosin@, Thanks for the review comments. It makes sense that we can avoid this definition at two places. Currently textQuads is only used in LayoutObject.cpp in absoluteBoundingBoxForRange, which we will modify to use ephemeralRange. Then having textQuads definition only in EphemeralRange.cpp wouldn't be the appropriate place ? If in future Range Needs it , then It can be used as EphemeralRange(range).
On 2017/04/04 04:28:18, tanvir wrote: > On 2017/04/04 01:25:22, yosin_UTC9 wrote: > > > https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): > > > > > https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/VisibleUnits.cpp:4052: static void > > textQuadAlgorithm(Vector<FloatQuad>* quads, > > Please change |Range::textQuads()| instead of copying this from "Range.cpp". > > We don't want to have duplicated code. > > > > > https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/VisibleUnits.h (right): > > > > > https://codereview.chromium.org/2775663008/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/VisibleUnits.h:343: CORE_EXPORT void > > textQuads(Vector<FloatQuad>*, const EphemeralRange&); > > Please name this function as |computeTextQuads()| to make function name starts > > with verb. > > Yosin@, Thanks for the review comments. > It makes sense that we can avoid this definition at two places. > Currently textQuads is only used in LayoutObject.cpp in > absoluteBoundingBoxForRange, > which we will modify to use ephemeralRange. > Then having textQuads definition only in EphemeralRange.cpp wouldn't be the > appropriate place ? > If in future Range Needs it , then It can be used as EphemeralRange(range). Please Ignore this comment. I forgot that we want EphemeralRange to be independent from Layout. Sorry for the Confusion
I would like to have two patches:
Patch 0: (optional) Get rid of useSelectionHeight from Range::textQuads() and
|LayoutText::absoluteQuadsForRange|.
It seems it is always false.
We can do later.
Patch 1: Make Range::textQuads() to use function computeTextQuads()
Note: we don't need to have useSelectionHeight since all call sites pass false.
Actually, there is
only one call site, though.
1-1 Introduce computeTextQuads(), with TODO to move "VisibleUnits.cpp" which
takes as file local function in Range.cpp at current implementation, having
function declaration in "VisibleUnits.h"
1-2 Make Range::textQuads() to call it.
void Range::textQuads(Vector<FloatQuqad&>& quads, bool useSelectionHeight) {
quads = std::move(computeTextQuads(EphemeralRange(this), useSelectionHeight);
}
Patch 2-A: Make LayoutObject::absoluteBoundingBoxRectForRange() to take
EphemeralRange
FloatRect LayoutObject::absoluteBoundingBoxRectForRange(const EphemralRange&
range) {
const Vector<FloatQuad> quads = computeTextQuads(range, false);
...
}
TextFinder::find() {
...
enclosingIntRect(LayoutObject::absoluteBoundingBoxRectForRange(
EphemeralRange(m_activeMatch.get())
...
}
int TextFinder::selectFindMatch(...) {
...
IntRect activeMatchBoundingBox = enclosingIntRect(
LayoutObject::absoluteBoundingBoxRectForRange(EphemeralRange(m_activeMatch.get())));
...
}
Patch 2-B: Move computeTextQuads() to "VisibleUnit.cpp"
2-A and 2-B can be parallel.
Please see patch[1], which makes |Range::boundingBox()| to work with
EphemeralRange.
[1] http://crrev.com/2776103002: updateMarkerRenderedRect() should use
EphemeralRange
On 2017/04/04 05:17:49, yosin_UTC9 wrote:
> I would like to have two patches:
>
> Patch 0: (optional) Get rid of useSelectionHeight from Range::textQuads() and
> |LayoutText::absoluteQuadsForRange|.
> It seems it is always false.
> We can do later.
>
> Patch 1: Make Range::textQuads() to use function computeTextQuads()
> Note: we don't need to have useSelectionHeight since all call sites pass
false.
> Actually, there is
> only one call site, though.
>
> 1-1 Introduce computeTextQuads(), with TODO to move "VisibleUnits.cpp" which
> takes as file local function in Range.cpp at current implementation, having
> function declaration in "VisibleUnits.h"
>
> 1-2 Make Range::textQuads() to call it.
>
> void Range::textQuads(Vector<FloatQuqad&>& quads, bool useSelectionHeight) {
> quads = std::move(computeTextQuads(EphemeralRange(this),
useSelectionHeight);
> }
>
> Patch 2-A: Make LayoutObject::absoluteBoundingBoxRectForRange() to take
> EphemeralRange
>
> FloatRect LayoutObject::absoluteBoundingBoxRectForRange(const EphemralRange&
> range) {
> const Vector<FloatQuad> quads = computeTextQuads(range, false);
> ...
> }
>
> TextFinder::find() {
> ...
> enclosingIntRect(LayoutObject::absoluteBoundingBoxRectForRange(
> EphemeralRange(m_activeMatch.get())
> ...
> }
>
> int TextFinder::selectFindMatch(...) {
> ...
> IntRect activeMatchBoundingBox = enclosingIntRect(
>
>
LayoutObject::absoluteBoundingBoxRectForRange(EphemeralRange(m_activeMatch.get())));
>
> ...
> }
>
> Patch 2-B: Move computeTextQuads() to "VisibleUnit.cpp"
>
>
> 2-A and 2-B can be parallel.
>
> Please see patch[1], which makes |Range::boundingBox()| to work with
> EphemeralRange.
>
>
> [1] http://crrev.com/2776103002: updateMarkerRenderedRect() should use
> EphemeralRange
Thanks. I will work in this way then.
I will be Out Of Office till 16th of April, and I will continue working on this
Bug after i come back.
Thank you.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
