Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209513002:
Exclude start/end container in Range::getBorderAndTextQuads (Closed)
|
Created:
4 years, 4 months ago by eae Modified:
4 years, 4 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExclude start/end container in Range::getBorderAndTextQuads
Change Range::getBorderAndTextQuads to exclude the size of the container
elements for either end point. The porition of the element included in a
range is accounted for when computing the size for the start & end node.
TEST=fast/dom/Range/range-spanning-elements-bounding-client-rect.html
BUG=623301
R=yosin@chromium.org
Committed: https://crrev.com/7b05ec36906acc9e75c2e06ab68bbd0aa8ba691c
Cr-Commit-Position: refs/heads/master@{#412263}
Patch Set 1 #Patch Set 2 : Added test #
Total comments: 4
Patch Set 3 : Address test failure #
Total comments: 1
Patch Set 4 : Fix typo #
Messages
Total messages: 31 (17 generated)
The CQ bit was checked by eae@chromium.org to run a CQ dry run
Description was changed from ========== Exclude start/end container in Range::getBorderAndTextQuads ========== to ========== Exclude start/end container in Range::getBorderAndTextQuads Change Range::getBorderAndTextQuads to exclude the size of the container elements for either end point. The porition of the element included in a range is accounted for when computing the size for the start & end node. ==========
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 eae@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 ========== Exclude start/end container in Range::getBorderAndTextQuads Change Range::getBorderAndTextQuads to exclude the size of the container elements for either end point. The porition of the element included in a range is accounted for when computing the size for the start & end node. ========== to ========== Exclude start/end container in Range::getBorderAndTextQuads Change Range::getBorderAndTextQuads to exclude the size of the container elements for either end point. The porition of the element included in a range is accounted for when computing the size for the start & end node. TEST=fast/dom/Range/range-spanning-elements-bounding-client-rect.html BUG=623301 R=yosin@chromium.org ==========
eae@chromium.org changed reviewers: + yosin@chromium.org
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_...)
https://codereview.chromium.org/2209513002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Range/range-spanning-elements-bounding-client-rect.html (right): https://codereview.chromium.org/2209513002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Range/range-spanning-elements-bounding-client-rect.html:21: var range = document.createRange(); FYI: |new Range()| works magically. :-) https://codereview.chromium.org/2209513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2209513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1534: && !node->contains(startContainer) Since |nodeSet| is used only here, we have two options: 1. Populate |nodeSet| with inclusive descendants of |startContainer| and |endContainer|, or 2. Get rid of |nodeSet| From execution speed perspective, it seems it is better to use |nodeSet| rather than callin |Node::contains()|, which is O(depth).
https://codereview.chromium.org/2209513002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Range/range-spanning-elements-bounding-client-rect.html (right): https://codereview.chromium.org/2209513002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Range/range-spanning-elements-bounding-client-rect.html:21: var range = document.createRange(); On 2016/08/03 01:25:04, Yosi_UTC9 wrote: > FYI: |new Range()| works magically. :-) That's crazy talk! :) (old habits die hard) https://codereview.chromium.org/2209513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2209513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1534: && !node->contains(startContainer) On 2016/08/03 01:25:04, Yosi_UTC9 wrote: > Since |nodeSet| is used only here, we have two options: > 1. Populate |nodeSet| with inclusive descendants of |startContainer| and > |endContainer|, or > 2. Get rid of |nodeSet| > > From execution speed perspective, it seems it is better to use |nodeSet| rather > than callin |Node::contains()|, which is O(depth). > Oh, that's a good idea. Thanks for the suggestions. I'll rework it to do that and give it a try.
Turns out it doesn't quite work. It would have to be a different nodeSet given that it needs to check for the current node rather than the parent. If you still think it's worth it I'll gladly add a second nodeSet for that. Not sure if it is worth it though.
On 2016/08/03 at 17:09:44, eae wrote: > Turns out it doesn't quite work. It would have to be a different nodeSet given that it needs to check for the current node rather than the parent. If you still think it's worth it I'll gladly add a second nodeSet for that. Not sure if it is worth it though. At this time, I vote not to use |nodeSet| for simplicity of code. Once, we have a report about slowness of Range#getClientRects(), we consider about it.
On 2016/08/04 at 01:10:15, Yosi_UTC9 wrote: > On 2016/08/03 at 17:09:44, eae wrote: > > Turns out it doesn't quite work. It would have to be a different nodeSet given that it needs to check for the current node rather than the parent. If you still think it's worth it I'll gladly add a second nodeSet for that. Not sure if it is worth it though. > > At this time, I vote not to use |nodeSet| for simplicity of code. > Once, we have a report about slowness of Range#getClientRects(), we consider about it. I mean second node set.
Agreed. Are you fine with the current implemention then?
On 2016/08/04 at 18:04:47, eae wrote: > Agreed. Are you fine with the current implemention then? I'm fine with current implementation if it works. It seems this patch makes "fast/dom/Range/scale-page-client-rects.html" to fail... Could you check?
On 2016/08/05 01:24:06, Yosi_UTC9 wrote: > On 2016/08/04 at 18:04:47, eae wrote: > > Agreed. Are you fine with the current implemention then? > > I'm fine with current implementation if it works. > It seems this patch makes "fast/dom/Range/scale-page-client-rects.html" to > fail... > Could you check? Will do!
The CQ bit was checked by eae@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
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_...)
lgtm w/ nit; typo in a comment. https://codereview.chromium.org/2209513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Range.cpp (right): https://codereview.chromium.org/2209513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Range.cpp:1533: // Exclude start & end container if unless the entire corresponding s/if unless/unless/
The CQ bit was checked by eae@chromium.org
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/2209513002/#ps60001 (title: "Fix typo")
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.
Description was changed from ========== Exclude start/end container in Range::getBorderAndTextQuads Change Range::getBorderAndTextQuads to exclude the size of the container elements for either end point. The porition of the element included in a range is accounted for when computing the size for the start & end node. TEST=fast/dom/Range/range-spanning-elements-bounding-client-rect.html BUG=623301 R=yosin@chromium.org ========== to ========== Exclude start/end container in Range::getBorderAndTextQuads Change Range::getBorderAndTextQuads to exclude the size of the container elements for either end point. The porition of the element included in a range is accounted for when computing the size for the start & end node. TEST=fast/dom/Range/range-spanning-elements-bounding-client-rect.html BUG=623301 R=yosin@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Exclude start/end container in Range::getBorderAndTextQuads Change Range::getBorderAndTextQuads to exclude the size of the container elements for either end point. The porition of the element included in a range is accounted for when computing the size for the start & end node. TEST=fast/dom/Range/range-spanning-elements-bounding-client-rect.html BUG=623301 R=yosin@chromium.org ========== to ========== Exclude start/end container in Range::getBorderAndTextQuads Change Range::getBorderAndTextQuads to exclude the size of the container elements for either end point. The porition of the element included in a range is accounted for when computing the size for the start & end node. TEST=fast/dom/Range/range-spanning-elements-bounding-client-rect.html BUG=623301 R=yosin@chromium.org Committed: https://crrev.com/7b05ec36906acc9e75c2e06ab68bbd0aa8ba691c Cr-Commit-Position: refs/heads/master@{#412263} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7b05ec36906acc9e75c2e06ab68bbd0aa8ba691c Cr-Commit-Position: refs/heads/master@{#412263} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
