[webkit-reviews] review denied: [Bug 66568] Expose Fixed Layout Size mode to Chromium's WebKit API : [Attachment 104523] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Fady Samuel
<fsamuel at chromium.org>'s request for review:
Bug 66568: Expose Fixed Layout Size mode to Chromium's WebKit API
https://bugs.webkit.org/show_bug.cgi?id=66568

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104523&action=review


> Source/WebKit/chromium/public/WebView.h:220
> +    // Gets the fixed layout size.

this comment and the one for setFixedLayoutSize seem redundant with the name
of the methods.  i'd just leave out the comments.

same goes for the functions that control whether or not this mode is enabled.

the comment could use a little bit of help too, and here's a stab at how i
might revise this part:

  // Fixed Layout --------------------------------------------------------

  // In fixed layout mode, the layout of the page is independent of the
  // view port size, given by WebWidget::size().

  virtual bool isFixedLayoutModeEnabled() const = 0;
  virtual void enableFixedLayoutMode(bool enable) = 0;

  virtual WebSize fixedLayoutSize() const = 0;
  virtual void setFixedLayoutSize(const WebSize&) = 0;

> Source/WebKit/chromium/public/WebView.h:227
> +    virtual bool useFixedLayout() const = 0;

nit: there is a convention for enable methods like these:

  virtual bool isFixedLayoutModeEnabled() const = 0;
  virtual void enableFixedLayoutMode(bool enable) = 0;

> Source/WebKit/chromium/src/WebViewImpl.cpp:1868
> +	   return WebSize(0, 0);

nit: just use the default constructor for WebSize.  "return WebSize();"

> Source/WebKit/chromium/src/WebViewImpl.cpp:1872
> +	   return WebSize(0, 0);

ditto

> Source/WebKit/chromium/src/WebViewImpl.cpp:1874
> +    return frame->view()->fixedLayoutSize();

given that this is a control on the FrameView, perhaps these methods
should really be on WebFrame instead of WebView?  is it meaningful to
only set fixed layout size on a subframe?