Bug 72176 - [Chromium] setPageScaleFactor and associated methods should take scaling limits into account
Summary: [Chromium] setPageScaleFactor and associated methods should take scaling limi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
: 71622 (view as bug list)
Depends on:
Blocks: 70559
  Show dependency treegraph
 
Reported: 2011-11-11 13:44 PST by Fady Samuel
Modified: 2011-11-14 15:02 PST (History)
6 users (show)

See Also:


Attachments
Patch (9.73 KB, patch)
2011-11-11 13:46 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2011-11-12 08:59 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (16.79 KB, patch)
2011-11-13 22:27 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (16.75 KB, patch)
2011-11-14 08:48 PST, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2011-11-11 13:44:26 PST
[Chromium] setPageScaleFactor and associated methods should take scaling limits into account
Comment 1 Fady Samuel 2011-11-11 13:46:54 PST
Created attachment 114765 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-11 13:49:45 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 James Robinson 2011-11-11 17:57:31 PST
Comment on attachment 114765 [details]
Patch

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

This code is all really inconsistent about whether it is talking about 'pageScale', 'scaleFactor', or 'pageScaleFactor'. Can you pick one and stick to it?

> Source/WebKit/chromium/public/WebView.h:218
> +    virtual void setPageAndScrollOffsetScaleFactor(float) = 0;

did you mean PageScaleFactorAndScrollOffset? We talk about the 'PageScaleFactor' elsewhere and it's weird to break it up like this

> Source/WebKit/chromium/src/WebViewImpl.cpp:1873
> +    IntPoint pt = offset;

don't use abbreviations for variable names in WebKit, - use full words

> Source/WebKit/chromium/src/WebViewImpl.cpp:1876
> +    pt.setX(std::max(0, pt.x()));

don't use std::foo() in WebKit, put a using namespace std; declaration at the top of the file and then just call foo(). pretty sure we already have the using declaration in this file.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1878
> +    return pt;

are you sure there aren't any IntPoint/IntSize/etc utilities to do this instead of doing it all component by component?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1958
> +    m_minimumPageScaleFactor = min(max(minPageScale, minPageScaleFactor), maxPageScaleFactor);
> +    m_maximumPageScaleFactor = max(min(maxPageScale, maxPageScaleFactor), minPageScaleFactor);

you're clamping the passed-in values to within [0.25,4] ? that doesn't seem right - those values should be defaults but if a caller wants some other values we should honor them, imo. the caller has access to these values and can do whatever they like with them
Comment 4 Alexandre Elias 2011-11-11 20:58:55 PST
I don't have proper Bugzilla permissions to do an inline review, but here's my comments:

> Source/WebKit/chromium/src/WebViewImpl.cpp:1857
> +    float minimumScale = m_minimumPageScaleFactor * deviceScaleFactor();

Hmm, using deviceScaleFactor here will result in inconsistent clamping compared to the Impl-thread clamping, which doesn't have access to deviceScaleFactor (and probably doesn't need to except for this).  How about preapplying deviceScaleFactor to m_minimumPageScaleFactor/m_maximumPageScaleFactor?

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1958
>> +    m_maximumPageScaleFactor = max(min(maxPageScale, maxPageScaleFactor), minPageScaleFactor);
> 
> you're clamping the passed-in values to within [0.25,4] ? that doesn't seem right - those values should be defaults but if a caller wants some other values we should honor them, imo. the caller has access to these values and can do whatever they like with them

We want to prevent the viewport tag from setting crazy limits, so the clamping here is fine by me.  I just think we should clamp by m_minimumPageScaleFactor * deviceScaleFactor here instead, as mentioned above. 

We also need to clamp m_minimumPageScaleFactor so that we can't zoom out further than the document width.  The code we wrote earlier to do this looks like:

min_scale = std::max(min_scale, (float) viewport_size.width() / (content_size.width() / currentPageScaleFactor));
Comment 5 Fady Samuel 2011-11-12 08:59:12 PST
Created attachment 114836 [details]
Patch
Comment 6 WebKit Review Bot 2011-11-12 09:27:50 PST
Comment on attachment 114836 [details]
Patch

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

New failing tests:
fast/repaint/background-scaling.html
fast/repaint/scale-page-shrink.html
Comment 7 Darin Fisher (:fishd, Google) 2011-11-13 20:43:44 PST
Comment on attachment 114836 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:216
> +    // Scales the page and the scroll offset by a factor of scaleFactor, while

nit: comment mentions "scaleFactor" but you left the parameter unnamed, which makes this
comment a tad confusing.  Also as James said, we should probably use only one term for this.
seems like we are converging on pageScaleFactor.

> Source/WebKit/chromium/public/WebView.h:218
> +    virtual void setPageScaleFactorAndPreserveScrollOffset(float) = 0;

nit: since you are not setting "PreserveScrollOffset", I think a better name for this method
would be:  setPageScaleFactorPreservingScrollOffset(float pageScaleFactor)
Comment 8 Fady Samuel 2011-11-13 22:22:07 PST
Comment on attachment 114836 [details]
Patch

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

>> Source/WebKit/chromium/public/WebView.h:216
>> +    // Scales the page and the scroll offset by a factor of scaleFactor, while
> 
> nit: comment mentions "scaleFactor" but you left the parameter unnamed, which makes this
> comment a tad confusing.  Also as James said, we should probably use only one term for this.
> seems like we are converging on pageScaleFactor.

There's only so much I can do here. Recall that pageScaleFactor() is a method. C++ doesn't seem to allow the use of both a formal parameter variable called pageScaleFactor and a method called pageScaleFactor().

>> Source/WebKit/chromium/public/WebView.h:218
>> +    virtual void setPageScaleFactorAndPreserveScrollOffset(float) = 0;
> 
> nit: since you are not setting "PreserveScrollOffset", I think a better name for this method
> would be:  setPageScaleFactorPreservingScrollOffset(float pageScaleFactor)

Done.
Comment 9 Fady Samuel 2011-11-13 22:23:12 PST
Comment on attachment 114765 [details]
Patch

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

>> Source/WebKit/chromium/public/WebView.h:218
>> +    virtual void setPageAndScrollOffsetScaleFactor(float) = 0;
> 
> did you mean PageScaleFactorAndScrollOffset? We talk about the 'PageScaleFactor' elsewhere and it's weird to break it up like this

I had a hard time coming up with a name for this one

Changing it to setPageScaleFactorAndPreserveScrollOffset, which is logically what it does. It does not preserve the scroll offset in pixels but it preserves where  you are logically in the page by scaling the scroll offset. I'm still not sure if this is the best name. I'm open to suggestions.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1873
>> +    IntPoint pt = offset;
> 
> don't use abbreviations for variable names in WebKit, - use full words

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1876
>> +    pt.setX(std::max(0, pt.x()));
> 
> don't use std::foo() in WebKit, put a using namespace std; declaration at the top of the file and then just call foo(). pretty sure we already have the using declaration in this file.

Sorry, I copied and pasted this from Chromium code I had locally and missed this change.  Fixed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1878
>> +    return pt;
> 
> are you sure there aren't any IntPoint/IntSize/etc utilities to do this instead of doing it all component by component?

I can use clampNegativeToZero, and shrunkTo. Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1958
>> +    m_maximumPageScaleFactor = max(min(maxPageScale, maxPageScaleFactor), minPageScaleFactor);
> 
> you're clamping the passed-in values to within [0.25,4] ? that doesn't seem right - those values should be defaults but if a caller wants some other values we should honor them, imo. the caller has access to these values and can do whatever they like with them

Please see Alexandre's comments below.
Comment 10 Fady Samuel 2011-11-13 22:27:33 PST
Created attachment 114879 [details]
Patch
Comment 11 Fady Samuel 2011-11-13 22:28:43 PST
fishd, jamesr: Ideally, I'd like to rename applyScrollAndScale to setPageScaleFactor* but the issue is that scrolling happens before scaling in this method and setPageScaleFactor* might not capture that. Perhaps setPageScaleFactorAfterScroll? Does that sound reasonable or confusing?
Comment 12 WebKit Review Bot 2011-11-13 22:30:36 PST
Attachment 114879 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit/chromium/src/WebViewImpl.h:161:  The parameter name "scaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/public/WebView.h:218:  The parameter name "scaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Fady Samuel 2011-11-14 08:42:33 PST
(In reply to comment #12)
> Attachment 114879 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> 
> Source/WebKit/chromium/src/WebViewImpl.h:161:  The parameter name "scaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WebKit/chromium/public/WebView.h:218:  The parameter name "scaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 2 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Sigh, getting rid of these.
Comment 14 Fady Samuel 2011-11-14 08:48:24 PST
Created attachment 114955 [details]
Patch
Comment 15 Fady Samuel 2011-11-14 14:47:14 PST
Comment on attachment 114955 [details]
Patch

Clearing flags on attachment: 114955

Committed r100196: <http://trac.webkit.org/changeset/100196>
Comment 16 Fady Samuel 2011-11-14 14:47:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Fady Samuel 2011-11-14 15:02:34 PST
*** Bug 71622 has been marked as a duplicate of this bug. ***