Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349653002:
Added support of getClientRects() for SVG Elements. (Closed)
|
Created:
4 years, 3 months ago by Shanmuga Pandi Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded support of getClientRects() for SVG Elements.
As per spec, if the element has an associated SVG layout box return
a sequence containing a single DOMRect object that describes
the bounding box of the element as defined by the SVG specification,
applying the transforms that apply to the element and its ancestors.
Spec:
https://drafts.csswg.org/cssom-view/#dom-element-getclientrects
BUG=643044
Committed: https://crrev.com/f5b0ce2c5f861ddc285bf79fec1a4281a1d3db15
Cr-Commit-Position: refs/heads/master@{#421850}
Patch Set 1 #Patch Set 2 : nits #
Total comments: 8
Patch Set 3 : Align with review comments #
Total comments: 7
Patch Set 4 : Align with review comments #
Messages
Total messages: 27 (13 generated)
shanmuga.m@samsung.com changed reviewers: + rob.buis@samsung.com
@rob, PTAL. Thanks
On 2016/09/16 07:29:05, Shanmuga Pandi wrote: > @rob, > PTAL. > Thanks Gentle Ping!!
https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1043: // FIXME: Handle table/inline-table with a caption. Don't we want to keep these FIXMEs?
https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1043: // FIXME: Handle table/inline-table with a caption. On 2016/09/19 17:24:59, rwlbuis wrote: > Don't we want to keep these FIXMEs? I kept the FIXME for table/inline-table... And removed only Fixme related to SVG elements.
Peer looks good to me.
shanmuga.m@samsung.com changed reviewers: + foolip@chromium.org, fs@opera.com, pdr@chromium.org
PTAL!! Thanks
Please expand the description a bit (IIRC, there's a specref that might make sense to mention. Etc.) https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/getClientRects.html (right): https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/getClientRects.html:2: <title>SVGElement::getClientRects()</title> Element.getClientRects() on an SVGElement https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/getClientRects.html:11: var r1 = document.querySelector("rect").getClientRects()[0]; assert_equals(rectlist.length, 1); too maybe? https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/getClientRects.html:18: }); Also add a subtest for the <g> with multiple shapes/descendants (as specifically mentioned in the bug.)
Description was changed from ========== Added support of getClientRects() for SVG Elements. BUG=643044 ========== to ========== Added support of getClientRects() for SVG Elements. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 ==========
Description was changed from ========== Added support of getClientRects() for SVG Elements. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 ========== to ========== Added support of getClientRects() for SVG Elements. As per spec, if the element has an associated SVG layout box return a sequence containing a single DOMRect object that describes the bounding box of the element as defined by the SVG specification, applying the transforms that apply to the element and its ancestors. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 ==========
Description was changed from ========== Added support of getClientRects() for SVG Elements. As per spec, if the element has an associated SVG layout box return a sequence containing a single DOMRect object that describes the bounding box of the element as defined by the SVG specification, applying the transforms that apply to the element and its ancestors. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 ========== to ========== Added support of getClientRects() for SVG Elements. As per spec, if the element has an associated SVG layout box return a sequence containing a single DOMRect object that describes the bounding box of the element as defined by the SVG specification, applying the transforms that apply to the element and its ancestors. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 ==========
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/getClientRects.html (right): https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/getClientRects.html:2: <title>SVGElement::getClientRects()</title> On 2016/09/22 10:39:44, fs wrote: > Element.getClientRects() on an SVGElement Done. https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/getClientRects.html:11: var r1 = document.querySelector("rect").getClientRects()[0]; On 2016/09/22 10:39:44, fs wrote: > assert_equals(rectlist.length, 1); too maybe? Done. https://codereview.chromium.org/2349653002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/getClientRects.html:18: }); On 2016/09/22 10:39:44, fs wrote: > Also add a subtest for the <g> with multiple shapes/descendants (as specifically > mentioned in the bug.) Done.
LGTM w/ nits https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1049: } else if (elementLayoutObject->isBoxModelObject() || elementLayoutObject->isBR()) { Nit: While we're shuffling code, we could consider doing something like: if (<condition>) { // Do stuff return; } if (<next condition>) { ... } i.e get rid of the 'else' branch. https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1061: // FIXME: Handle table/inline-table with a caption. Nit: I guess this comment might be better suited for clientQuads now. https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:718: void clientQuads(Vector<FloatQuad>& quads); Nit: Could this be const?
https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1049: } else if (elementLayoutObject->isBoxModelObject() || elementLayoutObject->isBR()) { On 2016/09/29 12:04:21, fs wrote: > Nit: While we're shuffling code, we could consider doing something like: > > if (<condition>) { > // Do stuff > return; > } > if (<next condition>) { > ... > } > > i.e get rid of the 'else' branch. Done. https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1061: // FIXME: Handle table/inline-table with a caption. On 2016/09/29 12:04:21, fs wrote: > Nit: I guess this comment might be better suited for clientQuads now. Done. https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:718: void clientQuads(Vector<FloatQuad>& quads); On 2016/09/29 12:04:21, fs wrote: > Nit: Could this be const? Adding const will give build break due to document().updateStyleAndLayoutIgnorePendingStylesheetsForNode(this).. it expects node*..
https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2349653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:718: void clientQuads(Vector<FloatQuad>& quads); On 2016/09/29 at 13:14:13, Shanmuga Pandi wrote: > On 2016/09/29 12:04:21, fs wrote: > > Nit: Could this be const? > > Adding const will give build break due to > document().updateStyleAndLayoutIgnorePendingStylesheetsForNode(this).. > > it expects node*.. I almost expected that would be the case. Thanks.
The CQ bit was checked by shanmuga.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/2349653002/#ps60001 (title: "Align with review comments")
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 ========== Added support of getClientRects() for SVG Elements. As per spec, if the element has an associated SVG layout box return a sequence containing a single DOMRect object that describes the bounding box of the element as defined by the SVG specification, applying the transforms that apply to the element and its ancestors. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 ========== to ========== Added support of getClientRects() for SVG Elements. As per spec, if the element has an associated SVG layout box return a sequence containing a single DOMRect object that describes the bounding box of the element as defined by the SVG specification, applying the transforms that apply to the element and its ancestors. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Added support of getClientRects() for SVG Elements. As per spec, if the element has an associated SVG layout box return a sequence containing a single DOMRect object that describes the bounding box of the element as defined by the SVG specification, applying the transforms that apply to the element and its ancestors. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 ========== to ========== Added support of getClientRects() for SVG Elements. As per spec, if the element has an associated SVG layout box return a sequence containing a single DOMRect object that describes the bounding box of the element as defined by the SVG specification, applying the transforms that apply to the element and its ancestors. Spec: https://drafts.csswg.org/cssom-view/#dom-element-getclientrects BUG=643044 Committed: https://crrev.com/f5b0ce2c5f861ddc285bf79fec1a4281a1d3db15 Cr-Commit-Position: refs/heads/master@{#421850} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f5b0ce2c5f861ddc285bf79fec1a4281a1d3db15 Cr-Commit-Position: refs/heads/master@{#421850} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
