Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1827793003:
Support currentScale for embedded SVG (Closed)
|
Created:
4 years, 9 months ago by Shanmuga Pandi Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport currentScale for embedded SVG
Added Support of SVGSVGElement.currentScale for embedded
SVG Objects.
Also this patch removing pageZoomFactor modification such that by
applying currentScale will not affect the pageZoomFactor of document.
BUG=174568
Committed: https://crrev.com/db673092a93cfc8f1bc706f47302394519e6d208
Cr-Commit-Position: refs/heads/master@{#384465}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Ignore pageZoomFactor modification #
Total comments: 15
Patch Set 3 : Align with review comments #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== Support currentScale for embedded SVG BUG=174568 ========== to ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. BUG=174568 R=SamsungPeerReview ==========
shanmuga.m@samsung.com changed reviewers: + rob.buis@samsung.com
PTAL!! Thanks
On 2016/03/24 11:56:15, Shanmuga Pandi wrote: > PTAL!! > > Thanks Looks good to me.
shanmuga.m@samsung.com changed reviewers: + ed@chromium.org, fs@opera.com, pdr@chromium.org
PTAL!!
Description was changed from ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. BUG=174568 R=SamsungPeerReview ========== to ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. BUG=174568 ==========
I'm not sure to what degree we should be "nurturing" this hack (of using the page zoom), but... https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:7: var obj = document.getElementById("object"); Could use querySelector and avoid the id=... https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:12: setTimeout(function() { testRunner.notifyDone(); }, 0); Could you use runAfterLayoutAndPaint here instead? (It would replace most of the scaffolding.) https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:17: <object id="object" onload="setScale()" data="resources/green-rect.svg" type="image/svg+xml"/> I'd suggest that you add a "reference" (another green rect or so) in the local document, so that it's possible to see that changing the scale in the embedded SVG did not affect the parent document. https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:307: if (isEmbeddedThroughFrameContainingSVGDocument()) { I think a better way to structure this code would be to have a function/method that computes the scale to use, and then compute the view width/height and call viewBoxToViewTransform() with those. https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:309: float currentPageZoom = svg->document().frame()->pageZoomFactor(); Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, meaning that the effective zoom alone will be enough here? (Also, the "if parent frame zoom and current frame zoom differs - then use owner zoom" feels kind of dodgy...)
https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:309: float currentPageZoom = svg->document().frame()->pageZoomFactor(); On 2016/03/29 11:58:51, fs wrote: > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, meaning > that the effective zoom alone will be enough here? (Also, the "if parent frame > zoom and current frame zoom differs - then use owner zoom" feels kind of > dodgy...) Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to update. But consider the below case. <style> body { zoom: 50%;} </style> <body> <object type="image/svg+xml" data="rect.svg"> </object> </body> With that content, I could see value of currentPageZoom and ownerPageZoom is 1. And style()->effectiveZoom() is 1 and ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose comparing the parent frame zoom and current frame zoom rather than effectiveZoom. consider the case, mainFrame pageZoom as 0.5 childFrame pageZoom as 0.25 svg rect on childFrame with size 100X100, should be displayed as 25x25. And by applying the currentScale to only to current frame, will not change the FrameView::setFrameRect and layout size of layoutEmbeddedObject will be based on parentFrame. To compensate that I have divided with parent scale.
https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:309: float currentPageZoom = svg->document().frame()->pageZoomFactor(); On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > On 2016/03/29 11:58:51, fs wrote: > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, meaning > > that the effective zoom alone will be enough here? (Also, the "if parent frame > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > dodgy...) > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to update. > But consider the below case. > > <style> > body { zoom: 50%;} > </style> > > <body> > <object type="image/svg+xml" data="rect.svg"> > </object> > </body> > > > With that content, > I could see value of currentPageZoom and ownerPageZoom is 1. > And style()->effectiveZoom() is 1 and ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose comparing the parent frame zoom and current frame zoom rather than effectiveZoom. I don't think you should be comparing anything at all. > consider the case, > mainFrame pageZoom as 0.5 > childFrame pageZoom as 0.25 > svg rect on childFrame with size 100X100, should be displayed as 25x25. More like 0.5 * 0.25 * 100 = 12.5 I think. > And by applying the currentScale to only to current frame, will not change the FrameView::setFrameRect and layout size of layoutEmbeddedObject will be based on parentFrame. To compensate that I have divided with parent scale. It would be expected that the layout size of the embedding "frame" would not change. It's also quite possible that this is where using the "page zoom" to implement currentScale starts to crumble (and may explain why this wasn't done in the first place.) The more obvious implementation of currentScale would be to keep a separate currentScale and apply that before the viewBox transform but after the "border box transform".
https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:309: float currentPageZoom = svg->document().frame()->pageZoomFactor(); On 2016/03/30 09:04:12, fs wrote: > On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > > On 2016/03/29 11:58:51, fs wrote: > > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, > meaning > > > that the effective zoom alone will be enough here? (Also, the "if parent > frame > > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > > dodgy...) > > > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to > update. > > But consider the below case. > > > > <style> > > body { zoom: 50%;} > > </style> > > > > <body> > > <object type="image/svg+xml" data="rect.svg"> > > </object> > > </body> > > > > > > With that content, > > I could see value of currentPageZoom and ownerPageZoom is 1. > > And style()->effectiveZoom() is 1 and > ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose > comparing the parent frame zoom and current frame zoom rather than > effectiveZoom. > > I don't think you should be comparing anything at all. > > > consider the case, > > mainFrame pageZoom as 0.5 > > childFrame pageZoom as 0.25 > > svg rect on childFrame with size 100X100, should be displayed as 25x25. > > More like 0.5 * 0.25 * 100 = 12.5 I think. > > > And by applying the currentScale to only to current frame, will not change the > FrameView::setFrameRect and layout size of layoutEmbeddedObject will be based on > parentFrame. To compensate that I have divided with parent scale. > > It would be expected that the layout size of the embedding "frame" would not > change. It's also quite possible that this is where using the "page zoom" to > implement currentScale starts to crumble (and may explain why this wasn't done > in the first place.) Yes . I agree with that. Layout size of embedding frame should not change. > The more obvious implementation of currentScale would be to > keep a separate currentScale and apply that before the viewBox transform but > after the "border box transform". Could you little more elaborate ? It will helpful to understand better!!
https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:309: float currentPageZoom = svg->document().frame()->pageZoomFactor(); On 2016/03/30 at 09:17:38, Shanmuga Pandi wrote: > On 2016/03/30 09:04:12, fs wrote: > > On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > > > On 2016/03/29 11:58:51, fs wrote: > > > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, > > meaning > > > > that the effective zoom alone will be enough here? (Also, the "if parent > > frame > > > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > > > dodgy...) > > > > > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to > > update. > > > But consider the below case. > > > > > > <style> > > > body { zoom: 50%;} > > > </style> > > > > > > <body> > > > <object type="image/svg+xml" data="rect.svg"> > > > </object> > > > </body> > > > > > > > > > With that content, > > > I could see value of currentPageZoom and ownerPageZoom is 1. > > > And style()->effectiveZoom() is 1 and > > ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose > > comparing the parent frame zoom and current frame zoom rather than > > effectiveZoom. > > > > I don't think you should be comparing anything at all. > > > > > consider the case, > > > mainFrame pageZoom as 0.5 > > > childFrame pageZoom as 0.25 > > > svg rect on childFrame with size 100X100, should be displayed as 25x25. > > > > More like 0.5 * 0.25 * 100 = 12.5 I think. > > > > > And by applying the currentScale to only to current frame, will not change the > > FrameView::setFrameRect and layout size of layoutEmbeddedObject will be based on > > parentFrame. To compensate that I have divided with parent scale. > > > > > > It would be expected that the layout size of the embedding "frame" would not > > change. It's also quite possible that this is where using the "page zoom" to > > implement currentScale starts to crumble (and may explain why this wasn't done > > in the first place.) > > Yes . I agree with that. Layout size of embedding frame should not change. > > > The more obvious implementation of currentScale would be to > > keep a separate currentScale and apply that before the viewBox transform but > > after the "border box transform". > > Could you little more elaborate ? It will helpful to understand better!! Well, something like: AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, borderAndPadding.width() + translate.x(), borderAndPadding.height() + translate.y()); viewToBorderBoxTransform.scale(svg->currentScale()); ... I.e have SVGSVGElement::m_currentScale and then have the accessors read/update that (rather than modifying "page zoom").
https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:309: float currentPageZoom = svg->document().frame()->pageZoomFactor(); On 2016/03/30 10:16:30, fs wrote: > On 2016/03/30 at 09:17:38, Shanmuga Pandi wrote: > > On 2016/03/30 09:04:12, fs wrote: > > > On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > > > > On 2016/03/29 11:58:51, fs wrote: > > > > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, > > > meaning > > > > > that the effective zoom alone will be enough here? (Also, the "if parent > > > frame > > > > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > > > > dodgy...) > > > > > > > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to > > > update. > > > > But consider the below case. > > > > > > > > <style> > > > > body { zoom: 50%;} > > > > </style> > > > > > > > > <body> > > > > <object type="image/svg+xml" data="rect.svg"> > > > > </object> > > > > </body> > > > > > > > > > > > > With that content, > > > > I could see value of currentPageZoom and ownerPageZoom is 1. > > > > And style()->effectiveZoom() is 1 and > > > ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose > > > comparing the parent frame zoom and current frame zoom rather than > > > effectiveZoom. > > > > > > I don't think you should be comparing anything at all. > > > > > > > consider the case, > > > > mainFrame pageZoom as 0.5 > > > > childFrame pageZoom as 0.25 > > > > svg rect on childFrame with size 100X100, should be displayed as 25x25. > > > > > > More like 0.5 * 0.25 * 100 = 12.5 I think. > > > > > > > And by applying the currentScale to only to current frame, will not change > the > > > FrameView::setFrameRect and layout size of layoutEmbeddedObject will be > based on > > > parentFrame. To compensate that I have divided with parent scale. > > > > > > > > > > It would be expected that the layout size of the embedding "frame" would not > > > change. It's also quite possible that this is where using the "page zoom" to > > > implement currentScale starts to crumble (and may explain why this wasn't > done > > > in the first place.) > > > > Yes . I agree with that. Layout size of embedding frame should not change. > > > > > The more obvious implementation of currentScale would be to > > > keep a separate currentScale and apply that before the viewBox transform but > > > after the "border box transform". > > > > Could you little more elaborate ? It will helpful to understand better!! > > Well, something like: > > AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, > borderAndPadding.width() + translate.x(), borderAndPadding.height() + > translate.y()); > viewToBorderBoxTransform.scale(svg->currentScale()); > ... > > I.e have SVGSVGElement::m_currentScale and then have the accessors read/update > that (rather than modifying "page zoom"). Thank you for your suggestion. But What will be the behavior with currentScale with svg which is in mainFrame ? We still need to modify "page zoom" ?
https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:309: float currentPageZoom = svg->document().frame()->pageZoomFactor(); On 2016/03/30 at 10:23:30, Shanmuga Pandi wrote: > On 2016/03/30 10:16:30, fs wrote: > > On 2016/03/30 at 09:17:38, Shanmuga Pandi wrote: > > > On 2016/03/30 09:04:12, fs wrote: > > > > On 2016/03/30 at 08:40:06, Shanmuga Pandi wrote: > > > > > On 2016/03/29 11:58:51, fs wrote: > > > > > > Won't LocalFrame::setPageZoomFactor cause the effective zoom to update, > > > > meaning > > > > > > that the effective zoom alone will be enough here? (Also, the "if parent > > > > frame > > > > > > zoom and current frame zoom differs - then use owner zoom" feels kind of > > > > > > dodgy...) > > > > > > > > > > Yes. LocalFrame::setPageZoomFactor will cause the the effective zoom to > > > > update. > > > > > But consider the below case. > > > > > > > > > > <style> > > > > > body { zoom: 50%;} > > > > > </style> > > > > > > > > > > <body> > > > > > <object type="image/svg+xml" data="rect.svg"> > > > > > </object> > > > > > </body> > > > > > > > > > > > > > > > With that content, > > > > > I could see value of currentPageZoom and ownerPageZoom is 1. > > > > > And style()->effectiveZoom() is 1 and > > > > ownerLayoutObject->style()->effectiveZoom() is 0.5. So I felt to choose > > > > comparing the parent frame zoom and current frame zoom rather than > > > > effectiveZoom. > > > > > > > > I don't think you should be comparing anything at all. > > > > > > > > > consider the case, > > > > > mainFrame pageZoom as 0.5 > > > > > childFrame pageZoom as 0.25 > > > > > svg rect on childFrame with size 100X100, should be displayed as 25x25. > > > > > > > > More like 0.5 * 0.25 * 100 = 12.5 I think. > > > > > > > > > And by applying the currentScale to only to current frame, will not change > > the > > > > FrameView::setFrameRect and layout size of layoutEmbeddedObject will be > > based on > > > > parentFrame. To compensate that I have divided with parent scale. > > > > > > > > > > > > > > It would be expected that the layout size of the embedding "frame" would not > > > > change. It's also quite possible that this is where using the "page zoom" to > > > > implement currentScale starts to crumble (and may explain why this wasn't > > done > > > > in the first place.) > > > > > > Yes . I agree with that. Layout size of embedding frame should not change. > > > > > > > The more obvious implementation of currentScale would be to > > > > keep a separate currentScale and apply that before the viewBox transform but > > > > after the "border box transform". > > > > > > Could you little more elaborate ? It will helpful to understand better!! > > > > Well, something like: > > > > AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, > > borderAndPadding.width() + translate.x(), borderAndPadding.height() + > > translate.y()); > > viewToBorderBoxTransform.scale(svg->currentScale()); > > ... > > > > I.e have SVGSVGElement::m_currentScale and then have the accessors read/update > > that (rather than modifying "page zoom"). > > Thank you for your suggestion. > But What will be the behavior with currentScale with svg which is in mainFrame ? We still need to modify "page zoom" ? That would be an option I guess, there's nothing that prevents doing the same everywhere though I think. (This would also work for case where "outermost svg" != document root.)
Description was changed from ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. BUG=174568 ========== to ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 ==========
Description was changed from ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 ========== to ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 ==========
Description was changed from ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 ========== to ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 ==========
PTAL! https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html (right): https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:7: var obj = document.getElementById("object"); On 2016/03/29 11:58:51, fs wrote: > Could use querySelector and avoid the id=... Done. https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:12: setTimeout(function() { testRunner.notifyDone(); }, 0); On 2016/03/29 11:58:51, fs wrote: > Could you use runAfterLayoutAndPaint here instead? (It would replace most of the > scaffolding.) Done. https://codereview.chromium.org/1827793003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:17: <object id="object" onload="setScale()" data="resources/green-rect.svg" type="image/svg+xml"/> On 2016/03/29 11:58:51, fs wrote: > I'd suggest that you add a "reference" (another green rect or so) in the local > document, so that it's possible to see that changing the scale in the embedded > SVG did not affect the parent document. Done.
https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/object-current-scale-expected.html (right): https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale-expected.html:2: <svg xmlns='http://www.w3.org/2000/svg' width='400' height='400' viewBox='0 0 200 200'> The namespace declaration isn't needed. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale-expected.html:5: <svg xmlns='http://www.w3.org/2000/svg' width='400' height='400' viewBox='0 0 200 200'> Ditto. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html (right): https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:13: if (window.testRunner) You can pass 'true' as the second argument to runAfterLayoutAndPaint to have it do this for you. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:19: <body> You don't need this <body>. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:22: <svg width='400' height='400' viewBox='0 0 200 200'> This is a pretty roundabout way to get a 100x100 rectangle =). (Same in the expected.html) https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right): https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:72: , m_currentScale(1.0) Nit: Just 1 should do (and is what coding style prescribes.) https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:124: updateCurrentTranslate(); I'd suggest you rename this to make this look slightly less confusing. Maybe updateUserTransform or something like that. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGSVGElement.h (right): https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGSVGElement.h:68: float currentScale() const { return m_currentScale; } Here we rely on the setter gating access and hence not allowing modifications if this is not an outermost root. If the element is reparented though that will no longer hold. I.e see: https://svgwg.org/svg2-draft/struct.html#__svg__SVGSVGElement__currentScale for the semantics of this property.
https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/object-current-scale-expected.html (right): https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale-expected.html:2: <svg xmlns='http://www.w3.org/2000/svg' width='400' height='400' viewBox='0 0 200 200'> On 2016/03/31 10:53:04, fs wrote: > The namespace declaration isn't needed. Done. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html (right): https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:13: if (window.testRunner) On 2016/03/31 10:53:04, fs wrote: > You can pass 'true' as the second argument to runAfterLayoutAndPaint to have it > do this for you. Done. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:19: <body> On 2016/03/31 10:53:04, fs wrote: > You don't need this <body>. Done. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html:22: <svg width='400' height='400' viewBox='0 0 200 200'> On 2016/03/31 10:53:04, fs wrote: > This is a pretty roundabout way to get a 100x100 rectangle =). (Same in the > expected.html) Acknowledged. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right): https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:72: , m_currentScale(1.0) On 2016/03/31 10:53:04, fs wrote: > Nit: Just 1 should do (and is what coding style prescribes.) Done. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:124: updateCurrentTranslate(); On 2016/03/31 10:53:04, fs wrote: > I'd suggest you rename this to make this look slightly less confusing. Maybe > updateUserTransform or something like that. Done. https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGSVGElement.h (right): https://codereview.chromium.org/1827793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGSVGElement.h:68: float currentScale() const { return m_currentScale; } On 2016/03/31 10:53:04, fs wrote: > Here we rely on the setter gating access and hence not allowing modifications if > this is not an outermost root. If the element is reparented though that will no > longer hold. I.e see: > > https://svgwg.org/svg2-draft/struct.html#__svg__SVGSVGElement__currentScale > > for the semantics of this property. Right!! Thanks for pointing out. Done
This LGTM, although I wouldn't mind seeing a few more test (like for instance for the case where an <svg> moves from being an outermost root to being a nested root.) Please allow pdr time to comment though.
On 2016/03/31 at 11:58:25, fs wrote: > This LGTM, although I wouldn't mind seeing a few more test (like for instance for the case where an <svg> moves from being an outermost root to being a nested root.) > > Please allow pdr time to comment though. LGTM, this seems reasonable.
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827793003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827793003/40001
Message was sent while issue was closed.
Description was changed from ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 ========== to ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 ========== to ========== Support currentScale for embedded SVG Added Support of SVGSVGElement.currentScale for embedded SVG Objects. Also this patch removing pageZoomFactor modification such that by applying currentScale will not affect the pageZoomFactor of document. BUG=174568 Committed: https://crrev.com/db673092a93cfc8f1bc706f47302394519e6d208 Cr-Commit-Position: refs/heads/master@{#384465} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/db673092a93cfc8f1bc706f47302394519e6d208 Cr-Commit-Position: refs/heads/master@{#384465} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
