Bug 58013

Summary: [Chromium] Needs eventSender.scalePageBy()
Product: WebKit Reporter: Kent Tamura <tkent>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, dglazkov, fishd, fsamuel, hamaji, haraken, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Kent Tamura 2011-04-06 22:44:31 PDT
eventSender.scalePageBy() is needed to pass fast/repaint/scale-page-shrink.html
Comment 1 Adam Klein 2011-05-24 13:39:00 PDT
fast/events/scroll-in-scaled-page-with-overflow-hidden.html also needs this method (added in http://trac.webkit.org/changeset/87187)
Comment 2 Kentaro Hara 2011-07-29 06:48:15 PDT
Created attachment 102356 [details]
Patch
Comment 3 WebKit Review Bot 2011-07-29 07:29:18 PDT
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
Comment 4 Kentaro Hara 2011-07-29 09:19:17 PDT
Created attachment 102370 [details]
Patch
Comment 5 Kent Tamura 2011-07-31 19:24:48 PDT
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() ?
Comment 6 Kentaro Hara 2011-08-01 01:32:26 PDT
Created attachment 102490 [details]
Patch
Comment 7 Kentaro Hara 2011-08-01 01:33:06 PDT
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.
Comment 8 Kentaro Hara 2011-08-01 01:35:14 PDT
Darin: We would like to add a public API, WebView::scalePageBy(). Would you please take a look?
Comment 9 Darin Fisher (:fishd, Google) 2011-08-01 16:23:06 PDT
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.
Comment 10 Kentaro Hara 2011-08-02 00:29:07 PDT
Created attachment 102621 [details]
Patch
Comment 11 Kentaro Hara 2011-08-02 00:29:20 PDT
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.
Comment 12 Fady Samuel 2011-08-03 12:48:47 PDT
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.
Comment 13 Kentaro Hara 2011-08-03 16:27:12 PDT
Created attachment 102853 [details]
Patch
Comment 14 Kentaro Hara 2011-08-03 16:28:13 PDT
> 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.
Comment 15 WebKit Review Bot 2011-08-03 19:01:44 PDT
Comment on attachment 102853 [details]
Patch

Clearing flags on attachment: 102853

Committed r92341: <http://trac.webkit.org/changeset/92341>
Comment 16 WebKit Review Bot 2011-08-03 19:01:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Shinichiro Hamaji 2011-08-04 01:39:45 PDT
Committed r92361: <http://trac.webkit.org/changeset/92361>
Comment 18 Shinichiro Hamaji 2011-08-04 04:24:50 PDT
Committed r92367: <http://trac.webkit.org/changeset/92367>