Bug 58013 – [Chromium] Needs eventSender.scalePageBy()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kent Tamura]

eventSender.scalePageBy() is needed to pass fast/repaint/scale-page-shrink.html

[Adam Klein]

fast/events/scroll-in-scaled-page-with-overflow-hidden.html also needs this method (added in http://trac.webkit.org/changeset/87187)

[Kentaro Hara]

Created attachment 102356 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102356 [details]
Patch

Attachment 102356 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9264522

New failing tests:
fast/dom/Element/scale-page-bounding-client-rect.html
fast/dom/Range/scale-page-bounding-client-rect.html

[Kentaro Hara]

Created attachment 102370 [details]
Patch

[Kent Tamura]

Comment on attachment 102370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102370&action=review

> LayoutTests/ChangeLog:21
> +        * platform/chromium-linux/compositing/scaling/tiled-layer-recursion-expected.png: Added.
> +        * platform/chromium-linux/fast/repaint/scale-page-shrink-expected.png: Added.
> +        * platform/chromium-linux/fast/repaint/scale-page-shrink-expected.txt: Added.
> +        * platform/chromium-mac/fast/dom/Element/scale-page-bounding-client-rect-expected.txt: Removed.
> +        * platform/chromium-mac/fast/dom/Range/scale-page-bounding-client-rect-expected.txt: Removed.
> +        * platform/chromium-win/fast/dom/Element/scale-page-bounding-client-rect-expected.txt: Removed.
> +        * platform/chromium-win/fast/dom/Element/scale-page-client-rects-expected.txt: Removed.
> +        * platform/chromium-win/fast/dom/Range/scale-page-bounding-client-rect-expected.txt: Removed.
> +        * platform/chromium-win/fast/dom/Range/scale-page-client-rects-expected.txt: Removed.
> +        * platform/chromium/test_expectations.txt: Enabled three tests.

It seems we need new expectation files for the followings on chromium-mac and chromium-win.

compositing/scaling/tiled-layer-recursion.txt
repaint/scale-page-shrink.html
fast/dom/Element/scale-page-bounding-client-rect.html
fast/dom/Range/scale-page-client-rect.html

Removing wrong expectations is ok, but removing entries in test_expectations.txt wil break the tests because of lack of correct expectations.

> Source/WebKit/chromium/public/WebView.h:209
> +    // Page scaling --------------------------------------------------------
> +
> +    // Scales a page by a factor of scaleFactor and then sets a scroll position to (x, y).
> +    virtual void scalePageBy(float scaleFactor, float x, float y) = 0;

Changing public interface only for DumpRenderTree should be avoided.
Can you move EventSender.scalePageBy() to window.internals.scalePageBy() ?

[Kentaro Hara]

Created attachment 102490 [details]
Patch

[Kentaro Hara]

Comment on attachment 102370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102370&action=review

>> LayoutTests/ChangeLog:21
>> +        * platform/chromium/test_expectations.txt: Enabled three tests.
> 
> It seems we need new expectation files for the followings on chromium-mac and chromium-win.
> 
> compositing/scaling/tiled-layer-recursion.txt
> repaint/scale-page-shrink.html
> fast/dom/Element/scale-page-bounding-client-rect.html
> fast/dom/Range/scale-page-client-rect.html
> 
> Removing wrong expectations is ok, but removing entries in test_expectations.txt wil break the tests because of lack of correct expectations.

I added the lines for tiled-layer-recursion.html and scale-page-shrink.html to test_expectations.txt. 

There is no need for scale-page-bouding-client-rect.html and scale-page-client-rect.html, since the results for these two tests are now equivalent to the results in LayoutTests/fast/dom/.

>> Source/WebKit/chromium/public/WebView.h:209
>> +    virtual void scalePageBy(float scaleFactor, float x, float y) = 0;
> 
> Changing public interface only for DumpRenderTree should be avoided.
> Can you move EventSender.scalePageBy() to window.internals.scalePageBy() ?

We need to reset the page scale at the head of each DumpRenderTree test. Specifically, we need to reset the page scale in TestShell::resetTestController(). Since DumpRenderTree should not access WebCore::Frame::scalePage() directly, I think that the public API for scalePageBy() is required.

[Kentaro Hara]

Darin: We would like to add a public API, WebView::scalePageBy(). Would you please take a look?

[Darin Fisher (:fishd, Google)]

Comment on attachment 102490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102490&action=review

> Source/WebKit/chromium/public/WebView.h:206
> +    // Page scaling --------------------------------------------------------

nit: preserve two new lines above comment.

> Source/WebKit/chromium/public/WebView.h:209
> +    virtual void scalePageBy(float scaleFactor, float x, float y) = 0;

nit: use WebFloatPoint instead of raw "x" and "y"

this method sounds like it is related to setZoomLevel.  how does it differ?
how is it related?  if related, then shouldn't it be in the "Zoom" section?

it is not obvious how zooming differs from scaling.  they sound like synonyms.

> Source/WebKit/chromium/public/WebView.h:211
>      // Media ---------------------------------------------------------------

ditto

> Source/WebKit/chromium/src/WebViewImpl.cpp:1819
> +void WebViewImpl::scalePageBy(float scaleFactor, float x, float y)

nit: please insert new method definitions in the same order as they appear in the header, WebViewImpl.h.

> Source/WebKit/chromium/src/WebViewImpl.h:388
> +    void scalePageBy(float scaleFactor, float x, float y);

nit: please insert new method declarations in the same order as they appear in the interface, WebView.

[Kentaro Hara]

Created attachment 102621 [details]
Patch

[Kentaro Hara]

Comment on attachment 102490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102490&action=review

>> Source/WebKit/chromium/public/WebView.h:206
>> +    // Page scaling --------------------------------------------------------
> 
> nit: preserve two new lines above comment.

Done.

>> Source/WebKit/chromium/public/WebView.h:209
>> +    virtual void scalePageBy(float scaleFactor, float x, float y) = 0;
> 
> nit: use WebFloatPoint instead of raw "x" and "y"
> 
> this method sounds like it is related to setZoomLevel.  how does it differ?
> how is it related?  if related, then shouldn't it be in the "Zoom" section?
> 
> it is not obvious how zooming differs from scaling.  they sound like synonyms.

- I changed the signature to "void scalePage(float scaleFactor, WebPoint origin)". 

- I agreed that scalePage() is highly related to Zoom operations. Thus, I included scalePage() in the Zoom section, as you indicated.

- I am sorry but I cannot yet describe the difference in browser behavior between scalePage() and setZoomLevel(). However, scalePage() is not a synonym for setZoomLevel() since the implementations of these two methods are different. For example, at the level of WebView implementation, we can set any factor for scalePage() but can set the factor within a predefined range for setZoomLevel(). At the level of WebCore implementation, scalePage() manages |m_pageScaleFactor|, but setZoomLevel() manages |m_pageZoomFactor| and |m_textZoomFactor|.  I know this is not a good explanation for validating the existence of scalePage(), but scalePage() is a different thing from setZoomLevel().

>> Source/WebKit/chromium/public/WebView.h:211
>>      // Media ---------------------------------------------------------------
> 
> ditto

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1819
>> +void WebViewImpl::scalePageBy(float scaleFactor, float x, float y)
> 
> nit: please insert new method definitions in the same order as they appear in the header, WebViewImpl.h.

Done.

>> Source/WebKit/chromium/src/WebViewImpl.h:388
>> +    void scalePageBy(float scaleFactor, float x, float y);
> 
> nit: please insert new method declarations in the same order as they appear in the interface, WebView.

Done.

[Fady Samuel]

It seems I wrote a patch very close to this one, not realizing there were Apple layout tests that used scalePageBy that already existed. https://bugs.webkit.org/show_bug.cgi?id=65631

fishd, in a nutshell, scaling magnifies or shrinks the page without affecting layout. Zooming affects layout of the page. 

(In reply to comment #11)
> (From update of attachment 102490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102490&action=review
> 
> >> Source/WebKit/chromium/public/WebView.h:206
> >> +    // Page scaling --------------------------------------------------------
> > 
> > nit: preserve two new lines above comment.
> 
> Done.
> 
> >> Source/WebKit/chromium/public/WebView.h:209
> >> +    virtual void scalePageBy(float scaleFactor, float x, float y) = 0;
> > 
> > nit: use WebFloatPoint instead of raw "x" and "y"
> > 
> > this method sounds like it is related to setZoomLevel.  how does it differ?
> > how is it related?  if related, then shouldn't it be in the "Zoom" section?
> > 
> > it is not obvious how zooming differs from scaling.  they sound like synonyms.
> 
> - I changed the signature to "void scalePage(float scaleFactor, WebPoint origin)". 
> 
> - I agreed that scalePage() is highly related to Zoom operations. Thus, I included scalePage() in the Zoom section, as you indicated.
> 
> - I am sorry but I cannot yet describe the difference in browser behavior between scalePage() and setZoomLevel(). However, scalePage() is not a synonym for setZoomLevel() since the implementations of these two methods are different. For example, at the level of WebView implementation, we can set any factor for scalePage() but can set the factor within a predefined range for setZoomLevel(). At the level of WebCore implementation, scalePage() manages |m_pageScaleFactor|, but setZoomLevel() manages |m_pageZoomFactor| and |m_textZoomFactor|.  I know this is not a good explanation for validating the existence of scalePage(), but scalePage() is a different thing from setZoomLevel().
> 
> >> Source/WebKit/chromium/public/WebView.h:211
> >>      // Media ---------------------------------------------------------------
> > 
> > ditto
> 
> Done.
> 
> >> Source/WebKit/chromium/src/WebViewImpl.cpp:1819
> >> +void WebViewImpl::scalePageBy(float scaleFactor, float x, float y)
> > 
> > nit: please insert new method definitions in the same order as they appear in the header, WebViewImpl.h.
> 
> Done.
> 
> >> Source/WebKit/chromium/src/WebViewImpl.h:388
> >> +    void scalePageBy(float scaleFactor, float x, float y);
> > 
> > nit: please insert new method declarations in the same order as they appear in the interface, WebView.
> 
> Done.

[Kentaro Hara]

Created attachment 102853 [details]
Patch

[Kentaro Hara]

> It seems I wrote a patch very close to this one, not realizing there were Apple layout tests that used scalePageBy that already existed. https://bugs.webkit.org/show_bug.cgi?id=65631
> 
> fishd, in a nutshell, scaling magnifies or shrinks the page without affecting layout. Zooming affects layout of the page. 

Thank you very much. I added the comment to WebView.h.

[WebKit Review Bot]

Comment on attachment 102853 [details]
Patch

Clearing flags on attachment: 102853

Committed r92341: <http://trac.webkit.org/changeset/92341>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Shinichiro Hamaji]

Committed r92361: <http://trac.webkit.org/changeset/92361>

[Shinichiro Hamaji]

Committed r92367: <http://trac.webkit.org/changeset/92367>