Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839633002:
Introduce ComputeTextBounds template in Range.cpp (Closed)
DescriptionIntroduce ComputeTextBounds template in Range.cpp
Currently textQuads and ComputeTextRects algorithm is
mostly same. The difference of behaviour of these two functions
can be handled efficiently by using templates.
This CL is the Second patch set to achieve the above behaviour.
This CL introduces ComputeTextBounds as template function, which
will be used by computeTextRects only in this patch.
BUG=691198
Review-Url: https://codereview.chromium.org/2839633002
Cr-Commit-Position: refs/heads/master@{#467707}
Committed: https://chromium.googlesource.com/chromium/src/+/deb0172c664a689bff370a530764f975480443f5
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated PS #
Total comments: 4
Patch Set 3 : AddressedReviewComments #Messages
Total messages: 24 (15 generated)
Description was changed from ========== Introduce ComputeTextBounds template in Range.cpp Currently textQuads and ComputeTextRects algorithm is mostly same. The difference of behaviour of these two functions can be handled efficiently by using templates. This CL is the Second patch set to achieve the above behaviour. This CL introduces ComputeTextBounds as template function, which will be used by computeTextRects only in this patch. BUG=691198 ========== to ========== Introduce ComputeTextBounds template in Range.cpp Currently textQuads and ComputeTextRects algorithm is mostly same. The difference of behaviour of these two functions can be handled efficiently by using templates. This CL is the Second patch set to achieve the above behaviour. This CL introduces ComputeTextBounds as template function, which will be used by computeTextRects only in this patch. BUG=691198 ==========
tanvir.rizvi@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com, yosin@chromium.org
PS#2. PTAL. Thanks!!!
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.
Code changes are good. Please wait patch[1] for using |const LayoutText&| for |CollectAbsoluteBoundsForRange()|. [1] http://crrev.com/2837273002: Mark LayoutText::AbsoluteRectsForRange() and AbsoluteQuadsForRange() as const member function https://codereview.chromium.org/2839633002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2839633002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Range.cpp:1446: LayoutText* layout_text) { nit: s/LayoutText*/const LayoutText&/ Please wait for patch[1] [1] http://crrev.com/2837273002: Mark LayoutText::AbsoluteRectsForRange() and AbsoluteQuadsForRange() as const member function
PTAL!!! Thanks https://codereview.chromium.org/2839633002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2839633002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Range.cpp:1446: LayoutText* layout_text) { On 2017/04/25 06:54:34, yosin_UTC9 wrote: > nit: s/LayoutText*/const LayoutText&/ > > Please wait for patch[1] > > [1] http://crrev.com/2837273002: Mark LayoutText::AbsoluteRectsForRange() and > AbsoluteQuadsForRange() as const member function Done.
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.
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm +tkent@ for OWNERS review.
https://codereview.chromium.org/2839633002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2839633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1443: static void CollectAbsoluteBoundsForRange(Vector<IntRect>& rects, Please make |rects| the last argument. https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > When defining a function, parameter order is: inputs, then outputs. https://codereview.chromium.org/2839633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1450: template <typename ElementType> |ElementType| is confusing with DOM Element. Can you rename it to RectType?
Addressed Review Comments. PTAL https://codereview.chromium.org/2839633002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2839633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1443: static void CollectAbsoluteBoundsForRange(Vector<IntRect>& rects, On 2017/04/27 06:40:37, tkent wrote: > Please make |rects| the last argument. > > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > > When defining a function, parameter order is: inputs, then outputs. Done. https://codereview.chromium.org/2839633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1450: template <typename ElementType> On 2017/04/27 06:40:37, tkent wrote: > |ElementType| is confusing with DOM Element. Can you rename it to RectType? Done.
The CQ bit was checked by tkent@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2839633002/#ps40001 (title: "AddressedReviewComments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493304197762180,
"parent_rev": "6a56addf7e3a7f7a70558980a216b2d7081251ba", "commit_rev":
"deb0172c664a689bff370a530764f975480443f5"}
Message was sent while issue was closed.
Description was changed from ========== Introduce ComputeTextBounds template in Range.cpp Currently textQuads and ComputeTextRects algorithm is mostly same. The difference of behaviour of these two functions can be handled efficiently by using templates. This CL is the Second patch set to achieve the above behaviour. This CL introduces ComputeTextBounds as template function, which will be used by computeTextRects only in this patch. BUG=691198 ========== to ========== Introduce ComputeTextBounds template in Range.cpp Currently textQuads and ComputeTextRects algorithm is mostly same. The difference of behaviour of these two functions can be handled efficiently by using templates. This CL is the Second patch set to achieve the above behaviour. This CL introduces ComputeTextBounds as template function, which will be used by computeTextRects only in this patch. BUG=691198 Review-Url: https://codereview.chromium.org/2839633002 Cr-Commit-Position: refs/heads/master@{#467707} Committed: https://chromium.googlesource.com/chromium/src/+/deb0172c664a689bff370a530764... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/deb0172c664a689bff370a530764... |
