[webkit-reviews] review denied: [Bug 77452] [chromium] Add the ability to turn off autoresize. : [Attachment 124775] Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied David Levin
<levin at chromium.org>'s request for review:
Bug 77452: [chromium] Add the ability to turn off autoresize.
https://bugs.webkit.org/show_bug.cgi?id=77452

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124775&action=review


Couple of questions (perhaps just needs clarification):

> Source/WebKit/chromium/src/WebViewImpl.cpp:2120
> +    ASSERT(!m_shouldAutoResize || minSize == maxSize);

I would understand:
ASSERT(enable || minSize == maxSize);
.. because it would check incoming parameters for consistency.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2131
> +	   resize(m_minAutoSize);

It would be good to add a comment describing the mechanism of turning off
auto-resize, to clarify why the 'new size' parameter and a resize to it is
needed.
Otherwise, first look at API brings a question, why not have:

enableAutoSize(minSize, maxSize)
disableAutoSize() // w/o prameters

since when auto-resize if off, the layout should be seeded with the current
window size...