[webkit-reviews] review denied: [Bug 72176] [Chromium] setPageScaleFactor and associated methods should take scaling limits into account : [Attachment 114765] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Fady Samuel
<fsamuel at chromium.org>'s request for review:
Bug 72176: [Chromium] setPageScaleFactor and associated methods should take
scaling limits into account
https://bugs.webkit.org/show_bug.cgi?id=72176

Attachment 114765: Patch
https://bugs.webkit.org/attachment.cgi?id=114765&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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