Summary: | [Chromium] Needs eventSender.scalePageBy() | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||||
Component: | Tools / Tests | Assignee: | 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
Kent Tamura
2011-04-06 22:44:31 PDT
fast/events/scroll-in-scaled-page-with-overflow-hidden.html also needs this method (added in http://trac.webkit.org/changeset/87187) Created attachment 102356 [details]
Patch
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 Created attachment 102370 [details]
Patch
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() ? Created attachment 102490 [details]
Patch
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. Darin: We would like to add a public API, WebView::scalePageBy(). Would you please take a look? 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. Created attachment 102621 [details]
Patch
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. 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. Created attachment 102853 [details]
Patch
> 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 on attachment 102853 [details] Patch Clearing flags on attachment: 102853 Committed r92341: <http://trac.webkit.org/changeset/92341> All reviewed patches have been landed. Closing bug. Committed r92361: <http://trac.webkit.org/changeset/92361> Committed r92367: <http://trac.webkit.org/changeset/92367> |