[webkit-reviews] review granted: [Bug 61309] Need a callback for when the preferred rendered size changed. : [Attachment 94879] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted David Levin
<levin at chromium.org>'s request for review:
Bug 61309: Need a callback for when the preferred rendered size changed.
https://bugs.webkit.org/show_bug.cgi?id=61309

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94879&action=review

I think the implementation is fine, but the interface could use a bit of
polish.  Can you get Darin F's input before committing as well?  He likes to
look at public WebKit API changes.

> Source/WebCore/page/ChromeClient.h:164
> +	   // Indicates that the preferred dimensions for the rendered HTML
contents may have changed.

this comment+name aren't super helpful, in particular it's really unclear how
this is supposed to related to contentsSizeChanged().  maybe this should be
contentsPreferredSizeMayHaveChanged()?

why not pass the size out to the caller now instead of making them call back in
to ask for the new preferred contents size?

> Source/WebCore/page/ChromeClient.h:165
> +	   virtual void renderedSizeMayHaveChanged(Frame*) const = 0;

did you consider just giving this an empty impl here instead of declaring it
pure virtual?

> Source/WebKit/efl/ChangeLog:9
> +	   * WebCoreSupport/ChromeClientEfl.h:
> +	   (WebCore::ChromeClientEfl::renderedSizeMayHaveChanged): Stubbed out
renderedSizeMayHaveChanged.

if the call wasn't declared pure virtual then you wouldn't need to stub it out
here (and everywhere else that implements ChromeClient)