Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854043002:
Make Range::TextQuads() to use ComputeTextBounds() template function. (Closed)
DescriptionMake Range::TextQuads() to use ComputeTextBounds() template function.
This is CL#3 to achieve the template behaviour for Range::textQuads
and Range::textRects.
CL#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(),
template and was used by Range::computeTextRects() only.
This CL set will make Range::textQuads() to use ComputeTextBounds() template.
BUG=691198
Review-Url: https://codereview.chromium.org/2854043002
Cr-Commit-Position: refs/heads/master@{#470345}
Committed: https://chromium.googlesource.com/chromium/src/+/f37555b2599b65017770d88c79ed9b4aae097e45
Patch Set 1 #
Total comments: 1
Patch Set 2 : Addressed Review Comments #
Total comments: 6
Patch Set 3 : Addressed Review Comments #
Total comments: 2
Patch Set 4 : updated with nit's #Messages
Total messages: 29 (17 generated)
Description was changed from ========== Make Range::TextQuads() to use ComputeTextBounds() template function. This is PatchSet#3 to achieve the template behaviour for Range::textQuads and Range::textRects. PatchSet#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(), template and was used by Range::computeTextRects() only. This patch set will make Range::textQuads() to use ComputeTextBounds() template. BUG=691198 ========== to ========== Make Range::TextQuads() to use ComputeTextBounds() template function. This is PatchSet#3 to achieve the template behaviour for Range::textQuads and Range::textRects. PatchSet#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(), template and was used by Range::computeTextRects() only. This patch set will make Range::textQuads() to use ComputeTextBounds() template. BUG=691198 ==========
tanvir.rizvi@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com
Description was changed from ========== Make Range::TextQuads() to use ComputeTextBounds() template function. This is PatchSet#3 to achieve the template behaviour for Range::textQuads and Range::textRects. PatchSet#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(), template and was used by Range::computeTextRects() only. This patch set will make Range::textQuads() to use ComputeTextBounds() template. BUG=691198 ========== to ========== Make Range::TextQuads() to use ComputeTextBounds() template function. This is PatchSet#3 to achieve the template behaviour for Range::textQuads and Range::textRects. PatchSet#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(), template and was used by Range::computeTextRects() only. This patch set will make Range::textQuads() to use ComputeTextBounds() template. BUG=691198 ==========
tanvir.rizvi@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com
PTAL!!!
peer review is fine
tanvir.rizvi@samsung.com changed reviewers: + yosin@chromium.org
tanvir.rizvi@samsung.com changed reviewers: + xiaochengh@chromium.org
Thanks for the patch. https://codereview.chromium.org/2854043002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Range.cpp (left): https://codereview.chromium.org/2854043002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Range.cpp:1507: use_selection_height); Could you first remove |use_selection_height| from this function and LayoutText::AbsoluteQuadsForRange()?
On 2017/05/02 21:15:03, Xiaocheng wrote: > Thanks for the patch. > > https://codereview.chromium.org/2854043002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/dom/Range.cpp (left): > > https://codereview.chromium.org/2854043002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/Range.cpp:1507: use_selection_height); > Could you first remove |use_selection_height| from this function and > LayoutText::AbsoluteQuadsForRange()? https://codereview.chromium.org/2853343003/ was added to fix the review comments. PTAL!!!
Description was changed from ========== Make Range::TextQuads() to use ComputeTextBounds() template function. This is PatchSet#3 to achieve the template behaviour for Range::textQuads and Range::textRects. PatchSet#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(), template and was used by Range::computeTextRects() only. This patch set will make Range::textQuads() to use ComputeTextBounds() template. BUG=691198 ========== to ========== Make Range::TextQuads() to use ComputeTextBounds() template function. This is CL#3 to achieve the template behaviour for Range::textQuads and Range::textRects. CL#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(), template and was used by Range::computeTextRects() only. This CL set will make Range::textQuads() to use ComputeTextBounds() template. BUG=691198 ==========
+tkent lgtm with a nit https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1483: static Vector<IntRect> ComputeTextRects(const EphemeralRange& range) { Good catch! https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1501: quads.insert(quads.size(), result); nit: Please use Vector::appendVector: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/wtf/V...
xiaochengh@chromium.org changed reviewers: + tkent@chromium.org
The CQ bit was checked by tkent@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.
https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1487: static Vector<FloatQuad> ComputeTextQuads(const EphemeralRange& range) { Why do we need this function? Range::TextQuads can call ComputeTextBounds<FloatQuad>() directly, right?
Patchset #3 (id:40001) has been deleted
PTAL!! Thanks https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1487: static Vector<FloatQuad> ComputeTextQuads(const EphemeralRange& range) { On 2017/05/07 22:55:11, tkent wrote: > Why do we need this function? Range::TextQuads can call > ComputeTextBounds<FloatQuad>() directly, right? Our plan is like below. 1) Next Patch will move ComputeTextRects,ComputeTextQuads and ComputeTextBounds to VisibleUnits. 2) Change CallSites to use ComputeTextQuads. 3) Get Rid of Range::textQuads. So we are not calling ComputeTextBounds directly from Range::TextQuads. Thanks. https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1501: quads.insert(quads.size(), result); On 2017/05/05 17:14:28, Xiaocheng wrote: > nit: Please use Vector::appendVector: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/wtf/V... Done.
lgtm w/ nits https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2854043002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1487: static Vector<FloatQuad> ComputeTextQuads(const EphemeralRange& range) { On 2017/05/07 at 22:55:11, tkent wrote: > Why do we need this function? Range::TextQuads can call ComputeTextBounds<FloatQuad>() directly, right? We'll replace Range::textQuads() with ComputeTextQuads() in following CL and get rid of Range::textQuads(). https://codereview.chromium.org/2854043002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2854043002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1487: static Vector<FloatQuad> ComputeTextQuads(const EphemeralRange& range) { Let's add TODO for ComputeTextQuads() will replace Range::textQuads() https://codereview.chromium.org/2854043002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1500: const Vector<FloatQuad>& result = ComputeTextQuads(EphemeralRange(this)); nit: We don't need to use variable |result|.
lgtm
The CQ bit was checked by tanvir.rizvi@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2854043002/#ps80001 (title: "updated with nit's")
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": 80001, "attempt_start_ts": 1494331901073130,
"parent_rev": "edb20606028c861740ec4135481db9e214de5be6", "commit_rev":
"f37555b2599b65017770d88c79ed9b4aae097e45"}
Message was sent while issue was closed.
Description was changed from ========== Make Range::TextQuads() to use ComputeTextBounds() template function. This is CL#3 to achieve the template behaviour for Range::textQuads and Range::textRects. CL#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(), template and was used by Range::computeTextRects() only. This CL set will make Range::textQuads() to use ComputeTextBounds() template. BUG=691198 ========== to ========== Make Range::TextQuads() to use ComputeTextBounds() template function. This is CL#3 to achieve the template behaviour for Range::textQuads and Range::textRects. CL#2(https://codereview.chromium.org/2839633002/) ComputeTextBounds(), template and was used by Range::computeTextRects() only. This CL set will make Range::textQuads() to use ComputeTextBounds() template. BUG=691198 Review-Url: https://codereview.chromium.org/2854043002 Cr-Commit-Position: refs/heads/master@{#470345} Committed: https://chromium.googlesource.com/chromium/src/+/f37555b2599b65017770d88c79ed... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f37555b2599b65017770d88c79ed... |
