Bug 73058 - [chromium] Add WebKit API's to support the autosize algorithm in renderer process.
Summary: [chromium] Add WebKit API's to support the autosize algorithm in renderer pro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 73420
Blocks: 73035 73631
  Show dependency treegraph
 
Reported: 2011-11-23 16:18 PST by David Levin
Modified: 2013-07-24 15:47 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.89 KB, patch)
2011-11-23 16:20 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (3.98 KB, patch)
2011-11-23 16:56 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (16.70 KB, patch)
2011-12-01 11:53 PST, David Levin
fishd: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-11-23 16:18:09 PST
See summary.
Comment 1 David Levin 2011-11-23 16:20:02 PST
Created attachment 116448 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-11-23 16:29:02 PST
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?
Comment 3 WebKit Review Bot 2011-11-23 16:30:54 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 David Levin 2011-11-23 16:56:41 PST
Created attachment 116456 [details]
Patch
Comment 5 David Levin 2011-11-23 16:57:25 PST
All done as requested.
Comment 6 David Levin 2011-11-23 17:30:30 PST
Comment on attachment 116456 [details]
Patch

Investigating another approach per discussion with Darin.
Comment 7 David Levin 2011-12-01 11:53:30 PST
Created attachment 117452 [details]
Patch
Comment 8 WebKit Review Bot 2011-12-01 12:41:27 PST
Comment on attachment 117452 [details]
Patch

Attachment 117452 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10696750
Comment 9 David Levin 2011-12-01 17:31:46 PST
(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.
Comment 10 Darin Fisher (:fishd, Google) 2011-12-01 21:41:31 PST
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.
Comment 11 Darin Fisher (:fishd, Google) 2011-12-01 21:42:12 PST
Comment on attachment 117452 [details]
Patch

R=me assuming the renaming happens.
Comment 12 David Levin 2011-12-02 01:01:00 PST
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.)
Comment 13 Eric Seidel (no email) 2013-07-24 15:47:13 PDT
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. :(