third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp - Issue 1827793003: Support currentScale for embedded SVG

Unified Diff: third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

Issue 1827793003: Support currentScale for embedded SVG (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« third_party/WebKit/LayoutTests/svg/custom/object-current-scale.html ('K') | « third_party/WebKit/LayoutTests/svg/custom/resources/green-rect.svg ('k') | third_party/WebKit/Source/core/svg/SVGSVGElement.cpp » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

diff --git a/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp b/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

index 675c646fe1e0f0087c862d0db4dc185920b0613e..3dc55ae7f2df7d011a246c4659b2e0560e467fb3 100644

--- a/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

+++ b/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

@@ -301,7 +301,20 @@ void LayoutSVGRoot::buildLocalToBorderBoxTransform()

float scale = style()->effectiveZoom();

FloatPoint translate = svg->currentTranslate();

LayoutSize borderAndPadding(borderLeft() + paddingLeft(), borderTop() + paddingTop());

- m_localToBorderBoxTransform = svg->viewBoxToViewTransform(contentWidth() / scale, contentHeight() / scale);

+ float viewWidth = contentWidth();

+ float viewHeight = contentHeight();

+ if (isEmbeddedThroughFrameContainingSVGDocument()) {

fs 2016/03/29 11:58:51 I think a better way to structure this code would

+ LayoutPart* ownerLayoutObject = svg->document().frame()->ownerLayoutObject();

+ float currentPageZoom = svg->document().frame()->pageZoomFactor();

fs 2016/03/29 11:58:51 Won't LocalFrame::setPageZoomFactor cause the effe

Shanmuga Pandi 2016/03/30 08:40:06 Yes. LocalFrame::setPageZoomFactor will cause the

fs 2016/03/30 09:04:12 I don't think you should be comparing anything at

Shanmuga Pandi 2016/03/30 09:17:38 Yes . I agree with that. Layout size of embedding

Yes . I agree with that. Layout size of embedding frame should not change.

Could you little more elaborate ? It will helpful to understand better!!

fs 2016/03/30 10:16:30 Well, something like: AffineTransform viewToBorde

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").

Shanmuga Pandi 2016/03/30 10:23:30 Thank you for your suggestion. But What will be t

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" ?

fs 2016/03/30 10:46:39 That would be an option I guess, there's nothing t

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.)

+ float ownerPageZoom = ownerLayoutObject->document().frame()->pageZoomFactor();

+ if (currentPageZoom != ownerPageZoom)

+ m_localToBorderBoxTransform = svg->viewBoxToViewTransform(viewWidth / ownerPageZoom, viewHeight / ownerPageZoom);

+ else

+ m_localToBorderBoxTransform = svg->viewBoxToViewTransform(viewWidth / scale, viewHeight / scale);

+ } else {

+ m_localToBorderBoxTransform = svg->viewBoxToViewTransform(viewWidth / scale, viewHeight / scale);

+ }

AffineTransform viewToBorderBoxTransform(scale, 0, 0, scale, borderAndPadding.width() + translate.x(), borderAndPadding.height() + translate.y());

m_localToBorderBoxTransform.preMultiply(viewToBorderBoxTransform);