Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(332)

Issue 8714006: Add autoresize capability for Chromium. (Closed)

Created:
9 years ago by levin
Modified:
9 years ago
CC:
chromium-reviews, Dmitry Titov
Base URL:
http://git.chromium.org/external/WebKit_trimmed.git@master
Visibility:
Public.

Description

Add autoresize capability for Chromium.

Patch Set 1 #

Total comments: 5

Patch Set 2 : Rearrange code. #

Patch Set 3 : Current patch #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -0 lines) Patch
M Source/WebKit/chromium/public/WebView.h View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
M Source/WebKit/chromium/public/WebViewClient.h View 1 2 1 chunk +3 lines, -0 lines 3 comments Download
M Source/WebKit/chromium/public/WebWidgetClient.h View 1 2 2 chunks +5 lines, -0 lines 2 comments Download
M Source/WebKit/chromium/src/WebFrameImpl.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/WebKit/chromium/src/WebViewImpl.h View 1 2 3 chunks +25 lines, -0 lines 0 comments Download
M Source/WebKit/chromium/src/WebViewImpl.cpp View 1 2 5 chunks +36 lines, -0 lines 5 comments Download

Messages

Total messages: 6 (0 generated)
darin (slow to review)
As you can see below, I'm curious about pushing the auto-size guts down into WebCore. ...
9 years ago (2011-11-28 16:50:40 UTC) #1
levin
http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebViewImpl.cpp File Source/WebKit/chromium/src/WebViewImpl.cpp (right): http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebViewImpl.cpp#newcode1063 Source/WebKit/chromium/src/WebViewImpl.cpp:1063: void WebViewImpl::layoutBeforeScrollbarUpdate() On 2011/11/28 16:50:40, darin wrote: > It ...
9 years ago (2011-11-28 18:34:01 UTC) #2
levin
http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebViewImpl.cpp File Source/WebKit/chromium/src/WebViewImpl.cpp (right): http://codereview.chromium.org/8714006/diff/1/Source/WebKit/chromium/src/WebViewImpl.cpp#newcode1063 Source/WebKit/chromium/src/WebViewImpl.cpp:1063: void WebViewImpl::layoutBeforeScrollbarUpdate() On 2011/11/28 18:34:01, levin wrote: > On ...
9 years ago (2011-11-28 18:39:06 UTC) #3
levin
http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/public/WebViewClient.h File Source/WebKit/chromium/public/WebViewClient.h (right): http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/public/WebViewClient.h#newcode251 Source/WebKit/chromium/public/WebViewClient.h:251: // Call when the size of the view automatically ...
9 years ago (2011-11-30 12:03:47 UTC) #4
darin (slow to review)
http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/public/WebViewClient.h File Source/WebKit/chromium/public/WebViewClient.h (right): http://codereview.chromium.org/8714006/diff/9001/Source/WebKit/chromium/public/WebViewClient.h#newcode252 Source/WebKit/chromium/public/WebViewClient.h:252: virtual void autoSizeChanged(const WebSize&) { } Why do we ...
9 years ago (2011-11-30 23:29:05 UTC) #5
levin
9 years ago (2011-12-01 20:23:45 UTC) #6
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.

Powered by Google App Engine
This is Rietveld 408576698