Bug 73058 – [chromium] Add WebKit API's to support the autosize algorithm in renderer process.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[David Levin]

See summary.

[David Levin]

Created attachment 116448 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 116448 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:168
> +    virtual WebSize visibleFrameSize() const = 0;

nit: you might want to put this above contentsSize since the next one in the list is 
also prefixed with content*.

nit: maybe this method should just be called size()?  it seems correct to refer to it
as the "size of the frame", which translates naturally to WebFrame::size().  we also
have WebWidget::size(), which is the size of the WebWidget (the size of the viewport).

> Source/WebKit/chromium/public/WebFrameClient.h:125
> +    virtual bool shouldAutoSize(WebFrame*, WebSize* /* minSize */, WebSize* /* maxSize */) { return false; }

nit: is it really necessary to comment out the parameter names?  we normally don't
compile with that warning enabled.  has something changed?

do you really have use cases for asking if an iframe should be autosized?  or do you
only care about this for the main frame?  if only for the main frame, then maybe this
should just be a method on WebViewClient?

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[David Levin]

Created attachment 116456 [details]
Patch

[David Levin]

All done as requested.

[David Levin]

Comment on attachment 116456 [details]
Patch

Investigating another approach per discussion with Darin.

[David Levin]

Created attachment 117452 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117452 [details]
Patch

Attachment 117452 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10696750

[David Levin]

(In reply to comment #8)
> (From update of attachment 117452 [details])
> Attachment 117452 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10696750

Due to 73420 not being landed yet.

[Darin Fisher (:fishd, Google)]

Comment on attachment 117452 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:2311
> +void WebViewImpl::queueBothResizeEventAndPaint()

why is this method called "queueBoth" and not just sendResizeEventAndPaint.  It seems to do exactly those functions without queuing anything.

I think you may want to use the term "repaint" instead of "paint" since I believe calling didInvalidateRect is really about requesting a paint instead of actually doing the painting.  Plus, I think ChromeClient (and WebCore) uses the term repaint for this function.

[Darin Fisher (:fishd, Google)]

Comment on attachment 117452 [details]
Patch

R=me assuming the renaming happens.

[David Levin]

Committed as http://trac.webkit.org/changeset/101759

Did renaming. I also went through the change made settled on the resize naming for everything in this patch. (I need to go back and change FrameView.  I left m_minAutoSize as is because m_minAutoResize didn't make sense to me.)

[Eric Seidel (no email)]

Comment on attachment 117452 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2311
>> +void WebViewImpl::queueBothResizeEventAndPaint()
> 
> why is this method called "queueBoth" and not just sendResizeEventAndPaint.  It seems to do exactly those functions without queuing anything.
> 
> I think you may want to use the term "repaint" instead of "paint" since I believe calling didInvalidateRect is really about requesting a paint instead of actually doing the painting.  Plus, I think ChromeClient (and WebCore) uses the term repaint for this function.

I ran across this code today.  I think it's wrong.

WebViewImpl should never be responsible for sending either repaint or resize events.  That's the FrameView's job.

I'm fixing FrameView to not delay initial layout.  That's causing tests for resizeEvents to fail, as we now (correctly) are sending 2 resize events.  One from the FrameView code (which was being suppressed because layout was suppressed for the first 250ms) and a second from this code. :(