Bug 66568 – Expose Fixed Layout Size mode to Chromium's WebKit API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Fady Samuel]

Expose Fixed Layout Size mode to Chromium's WebKit API

[Fady Samuel]

Created attachment 104523 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 104523 [details]
Patch

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?

[Fady Samuel]

(In reply to comment #2)
> (From update of attachment 104523 [details])
> 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?

I will make all the changes you suggested. I don't believe it's meaningful to set the fixed layout size on a subframe at this point in time (I can't think of  reasonable use case at the moment). So should I leave it in WebView?

[Fady Samuel]

Created attachment 104534 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 104534 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:214
>  

nit: add one more new line here to preserve the rule that there should be
two new lines between sections.

[Fady Samuel]

Created attachment 104537 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104537 [details]
Patch

Clearing flags on attachment: 104537

Committed r93434: <http://trac.webkit.org/changeset/93434>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.