Bug 66568 - Expose Fixed Layout Size mode to Chromium's WebKit API
Summary: Expose Fixed Layout Size mode to Chromium's WebKit API
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:
Depends on:
Blocks:
 
Reported: 2011-08-19 10:31 PDT by Fady Samuel
Modified: 2011-08-19 13:37 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.30 KB, patch)
2011-08-19 10:32 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (4.22 KB, patch)
2011-08-19 11:33 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (4.23 KB, patch)
2011-08-19 11:41 PDT, 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-08-19 10:31:41 PDT
Expose Fixed Layout Size mode to Chromium's WebKit API
Comment 1 Fady Samuel 2011-08-19 10:32:36 PDT
Created attachment 104523 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-08-19 10:42:53 PDT
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?
Comment 3 Fady Samuel 2011-08-19 10:46:38 PDT
(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?
Comment 4 Fady Samuel 2011-08-19 11:33:45 PDT
Created attachment 104534 [details]
Patch
Comment 5 Darin Fisher (:fishd, Google) 2011-08-19 11:35:47 PDT
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.
Comment 6 Fady Samuel 2011-08-19 11:41:31 PDT
Created attachment 104537 [details]
Patch
Comment 7 WebKit Review Bot 2011-08-19 13:37:31 PDT
Comment on attachment 104537 [details]
Patch

Clearing flags on attachment: 104537

Committed r93434: <http://trac.webkit.org/changeset/93434>
Comment 8 WebKit Review Bot 2011-08-19 13:37:35 PDT
All reviewed patches have been landed.  Closing bug.