Add autoresize capability for Chromium.

Add autoresize capability for Chromium.

[darin (slow to review), 2011-11-28 16:50:40]

As you can see below, I'm curious about pushing the auto-size guts down into
WebCore.  It seems like the more natural place for that code to live.  The
WebKit layer is just supposed to be dumb API glue.

http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebV...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebV...
Source/WebKit/chromium/src/WebViewImpl.cpp:1063: void
WebViewImpl::layoutBeforeScrollbarUpdate()
It feels like this should be a function on FrameView or ScrollView, perhaps
guarded by #if PLATFORM(CHROMIUM) or #if ENABLE(AUTO_SIZE)

http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebV...
Source/WebKit/chromium/src/WebViewImpl.cpp:1307: if (m_autoSize &&
!m_size.isEmpty() && !m_lastAutoSize.isEmpty()) {
what about the case where the page is rendered through the compositor (via GL)
instead?

i'm a little confused about why changes to painting are required.  i would have
expected the resizing to happen synchronously.  i don't think i have the big
picture in mind yet.

[levin, 2011-11-28 18:34:01]

http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebV...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebV...
Source/WebKit/chromium/src/WebViewImpl.cpp:1063: void
WebViewImpl::layoutBeforeScrollbarUpdate()
On 2011/11/28 16:50:40, darin wrote:
> It feels like this should be a function on FrameView or ScrollView, perhaps
> guarded by #if PLATFORM(CHROMIUM) or #if ENABLE(AUTO_SIZE)

I was mildly considering that. I'm not sure it we want to add a bool and two
rects to FrameView for Chromium.

Alternately we could leave the bool and two rects in WebViewImpl and just expose
a method FrameView::autoSize(minSize, maxSize). (We would still need to add the
bool m_inLayoutBeforeScrollbarUpdate to FrameView but could ifdef that out --
likely ifdef chromium to avoid more defines/configurations until it is proven
that we need them.)

http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebV...
Source/WebKit/chromium/src/WebViewImpl.cpp:1307: if (m_autoSize &&
!m_size.isEmpty() && !m_lastAutoSize.isEmpty()) {
On 2011/11/28 16:50:40, darin wrote:
> what about the case where the page is rendered through the compositor (via GL)
> instead?
> 
That case is right below.  It writes into the same canvas, see
"doPixelReadbackToCanvas(canvas, resizeRect);" so the same scaling should be
applied.



> i'm a little confused about why changes to painting are required.  i would
have
> expected the resizing to happen synchronously.  i don't think i have the big
> picture in mind yet.

"This is only to compensate for a very short term intermediate state while the
widget is being resized (animated) to match the desired size."

My explanation in the comment must not be clear. :) The rendered size is
synchronously resized but the ui window isn't. Even after the UI window gets a
new size, it typically animates from its current size to the new size.

So if the current size is 1000X1000 and the rendered size is 800x800, then as
the animation takes place, the region between 800 and 1000 is left unfilled in
the canvas which leads to it being filled with garbage. Alternately, I suppose I
could do calculations to determine the region in unscaledRect which isn't in
m_lastAutoSize and fill that in with white so get rid of the garbage. The
scaling seemed more elegant last night but either is fine. (At the moment I
think the fill in with white might be nice because that is likely faster and
avoids any issues if the ui decides to never get to the renderer size.)

[levin, 2011-11-28 18:39:06]

http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebV...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebV...
Source/WebKit/chromium/src/WebViewImpl.cpp:1063: void
WebViewImpl::layoutBeforeScrollbarUpdate()
On 2011/11/28 18:34:01, levin wrote:
> On 2011/11/28 16:50:40, darin wrote:
> > It feels like this should be a function on FrameView or ScrollView, perhaps
> > guarded by #if PLATFORM(CHROMIUM) or #if ENABLE(AUTO_SIZE)
> 
> I was mildly considering that. I'm not sure it we want to add a bool and two
> rects to FrameView for Chromium.
> 
> Alternately we could leave the bool and two rects in WebViewImpl and just
expose
> a method FrameView::autoSize(minSize, maxSize). (We would still need to add
the
> bool m_inLayoutBeforeScrollbarUpdate to FrameView but could ifdef that out --
> likely ifdef chromium to avoid more defines/configurations until it is proven
> that we need them.)

I guess as long as it is ifdef'ed we could add all this to FrameView and
FrameView isn't a memory critical structure. On the plus side that avoid a
virtual call and hopefully will make it more obvious rather than the obtuse name
"layoutBeforeScrollbarUpdate".

We'll still need to store m_autoSize at this layer as well.

[levin, 2011-11-30 12:03:47]

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
File Source/WebKit/chromium/public/WebViewClient.h (right):

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
Source/WebKit/chromium/public/WebViewClient.h:251: // Call when the size of the
view automatically changed.
Should be "called".

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/src/W...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/src/W...
Source/WebKit/chromium/src/WebViewImpl.cpp:2571: // TODO: Before checking in
make this async (to avoid unexpected recursion in js code)
This needs to be fixed up still and possibly consolidated with code from
WebViewImpl::resize.

[darin (slow to review), 2011-11-30 23:29:05]

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
File Source/WebKit/chromium/public/WebViewClient.h (right):

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
Source/WebKit/chromium/public/WebViewClient.h:252: virtual void
autoSizeChanged(const WebSize&) { }
Why do we need this?  Isn't WebWidgetClient::setSize sufficient to convey the
same information?

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
File Source/WebKit/chromium/public/WebWidgetClient.h (right):

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
Source/WebKit/chromium/public/WebWidgetClient.h:110: virtual void setSize(const
WebSize&) { }
didResize or didChangeSize seems better to me.

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/src/W...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/src/W...
Source/WebKit/chromium/src/WebViewImpl.cpp:2570: // TODO: move to higher level??
this is sync in after the layout code??
making this async could also introduce problems as you could have other tasks in
the queue that will care about the current value of m_size.

maybe the solution should simply defer this work until the last possible moment.
 i'm not clear on what the callstack looks like leading to here, but maybe there
is a spot further up the stack where we can notify the embedder about the size
change?

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/src/W...
Source/WebKit/chromium/src/WebViewImpl.cpp:2575: // Should this just call void
WebViewImpl::resize(const WebSize& newSize)?
probably should factor out some common code into a helper function

[levin, 2011-12-01 20:23:45]

Replies below.

I'm not updating the code here anymore. I've attached the patch to
https://bugs.webkit.org/show_bug.cgi?id=73058 since we are pretty close now.

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
File Source/WebKit/chromium/public/WebViewClient.h (right):

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
Source/WebKit/chromium/public/WebViewClient.h:252: virtual void
autoSizeChanged(const WebSize&) { }
On 2011/11/30 23:29:05, darin wrote:
> Why do we need this?  Isn't WebWidgetClient::setSize sufficient to convey the
> same information?

You're right. It was old cruft laying around (part of what I meant by
unpolished). Sorry about that.

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
File Source/WebKit/chromium/public/WebWidgetClient.h (right):

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/publi...
Source/WebKit/chromium/public/WebWidgetClient.h:110: virtual void setSize(const
WebSize&) { }
On 2011/11/30 23:29:05, darin wrote:
> didResize or didChangeSize seems better to me.

Made it didAutoResize to reflect the fact that it is only called during when
auto sizing is enabled.

Also I moved it to be near the other did* methods.

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/src/W...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/src/W...
Source/WebKit/chromium/src/WebViewImpl.cpp:2570: // TODO: move to higher level??
this is sync in after the layout code??
On 2011/11/30 23:29:05, darin wrote:
> making this async could also introduce problems as you could have other tasks
in
> the queue that will care about the current value of m_size.
> 
> maybe the solution should simply defer this work until the last possible
moment.
>  i'm not clear on what the callstack looks like leading to here, but maybe
there
> is a spot further up the stack where we can notify the embedder about the size
> change?

After looking at this code further, I realized that it basically is async (the
resize event gets queued up and painting is async).

I didn't to anything to make sure that a resize event doesn't trigger another
queue up another resize event. I don't have a way to know that a resize event it
being handled (and it wasn't critical to handle this issue -- just a nice to
have).

http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/src/W...
Source/WebKit/chromium/src/WebViewImpl.cpp:2575: // Should this just call void
WebViewImpl::resize(const WebSize& newSize)?
On 2011/11/30 23:29:05, darin wrote:
> probably should factor out some common code into a helper function

Done.