Callbacks through ChromeClient->RenderView->RenderViewHost for ContentsDidCha...

Callbacks through ChromeClient->RenderView->RenderViewHost for ContentsDidChangeSize so that extensions can change their toolbar size when the contained contents changes size.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=13130

[rafaelw, 2009-04-01 03:52:55]

Hey guys, please review.

[Aaron Boodman, 2009-04-01 18:28:35]

http://codereview.chromium.org/56122/diff/1045/46
File chrome/browser/extensions/extension_view.cc (right):

http://codereview.chromium.org/56122/diff/1045/46#newcode22
Line 22: set_preferred_size(gfx::Size(0, 100));
Add a comment here explaining why we are setting it small?

http://codereview.chromium.org/56122/diff/1045/48
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/56122/diff/1045/48#newcode251
Line 251: virtual void DidContentsChangeSize(const int new_width, const int
new_height)
See RequestOpenURL(), below, for how this line should be wrapped.

http://codereview.chromium.org/56122/diff/1045/53
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/56122/diff/1045/53#newcode1916
Line 1916: int new_height) {
int new_height should be aligned with WebWidget*

http://codereview.chromium.org/56122/diff/1045/53#newcode1919
Line 1919: if (BindingsPolicy::is_extension_enabled(enabled_bindings_)) {
If we are going to special case this code, we should at least add a comment in
RenderViewHostDelegate that OnSizeChange only works for extensions.

I think it would be much cleaner to modify the current bindings bit mask to be a
more general configuration bit mask, then modify RenderViewHost to have a
setting for receiving or not receiving size change events.

http://codereview.chromium.org/56122/diff/1045/53#newcode1920
Line 1920: int width = webview()->GetMainFrame()->GetContentsPreferredWidth();
width appears not to be used here?

http://codereview.chromium.org/56122/diff/1045/53#newcode1921
Line 1921: Send(new ViewHostMsg_DidContentsChangeSize(routing_id_, new_width,
new_height));
80 cols.

http://codereview.chromium.org/56122/diff/1045/52
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/56122/diff/1045/52#newcode211
Line 211: int new_height);
new_height should be indented to align with WebWidget*.

http://codereview.chromium.org/56122/diff/1045/40
File webkit/glue/chrome_client_impl.h (right):

http://codereview.chromium.org/56122/diff/1045/40#newcode105
Line 105: virtual void contentsSizeChanged(WebCore::Frame*, const
WebCore::IntSize&)
Not sure what the rule is, but I'd put the second param on the next line to
avoid having the const dangling on its own.

http://codereview.chromium.org/56122/diff/1045/45
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/56122/diff/1045/45#newcode497
Line 497: int new_height) {}
Indent.

[Matt Perry, 2009-04-01 18:30:09]

http://codereview.chromium.org/56122/diff/1045/46
File chrome/browser/extensions/extension_view.cc (right):

http://codereview.chromium.org/56122/diff/1045/46#newcode23
Line 23: this->SetVisible(false);
nit: don't use "this->" to call methods.

http://codereview.chromium.org/56122/diff/1045/46#newcode29
Line 29: this->GetParent()->GetParent()->Layout();
This is fragile.  It only works for the case where ExtensionView is a child of
ExtensionToolstrip which is a child of the BookmarksBarView.  Eventually we'll
have other views like sidebars and popouts and dialogs.

For now, it's probably OK to just NULL-check the GetParent calls and put a TODO
comment here saying to clean it up.  I think we'll eventually want an
ExtensionViewDelegate that toolstrips and sidebars can implement differently.

http://codereview.chromium.org/56122/diff/1045/46#newcode29
Line 29: this->GetParent()->GetParent()->Layout();
It also seems to me that there should be a better way to do this.  We should ask
someone more familiar with views (sky, beng, erikkay) whether we can schedule a
relayout+paint from a child view without crawling up the chain.

http://codereview.chromium.org/56122/diff/1045/48
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/56122/diff/1045/48#newcode252
Line 252: {}
nit: braces like this: Foo(..) {
}

or like this: Foo(..) {}

http://codereview.chromium.org/56122/diff/1045/53
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/56122/diff/1045/53#newcode1918
Line 1918: // Only notifiy the RenderViewHost if it's client is an
ExtensionView.
grammar nit: "its"

http://codereview.chromium.org/56122/diff/1045/53#newcode1920
Line 1920: int width = webview()->GetMainFrame()->GetContentsPreferredWidth();
What is this call for?  You don't use width anywhere.

http://codereview.chromium.org/56122/diff/1045/53#newcode1921
Line 1921: Send(new ViewHostMsg_DidContentsChangeSize(routing_id_, new_width,
new_height));
line length

http://codereview.chromium.org/56122/diff/1045/42
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/56122/diff/1045/42#newcode458
Line 458: int width = webFrameImpl->GetContentsPreferredWidth();
This feels wrong to me.  DidContentsSizeChange should report the size given to
contentsSizeChanged.  RenderView can then send up a message with the preferred
width instead, but we should make clear that the width given is the preferred
width, not the actual content width.

http://codereview.chromium.org/56122/diff/1045/41
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/56122/diff/1045/41#newcode542
Line 542: return frame_->document()->renderView()->minPrefWidth();
You need to NULL-check the document and the renderView.

[rafaelw, 2009-04-02 21:33:17]

Made the suggested changes. Added function implementation in scaffolding files
so that linux & mac compile. Cleaned up a bit ExtensionView resizing logic.

http://codereview.chromium.org/56122/diff/1045/46
File chrome/browser/extensions/extension_view.cc (right):

http://codereview.chromium.org/56122/diff/1045/46#newcode22
Line 22: set_preferred_size(gfx::Size(0, 100));
On 2009/04/01 18:28:35, Aaron Boodman wrote:
> Add a comment here explaining why we are setting it small?

Done.

http://codereview.chromium.org/56122/diff/1045/46#newcode23
Line 23: this->SetVisible(false);
On 2009/04/01 18:30:09, Matt Perry wrote:
> nit: don't use "this->" to call methods.

Done.

http://codereview.chromium.org/56122/diff/1045/46#newcode29
Line 29: this->GetParent()->GetParent()->Layout();
On 2009/04/01 18:30:09, Matt Perry wrote:
> This is fragile.  It only works for the case where ExtensionView is a child of
> ExtensionToolstrip which is a child of the BookmarksBarView.  Eventually we'll
> have other views like sidebars and popouts and dialogs.
> 
> For now, it's probably OK to just NULL-check the GetParent calls and put a
TODO
> comment here saying to clean it up.  I think we'll eventually want an
> ExtensionViewDelegate that toolstrips and sidebars can implement differently.

Done.

http://codereview.chromium.org/56122/diff/1045/46#newcode29
Line 29: this->GetParent()->GetParent()->Layout();
On 2009/04/01 18:30:09, Matt Perry wrote:
> It also seems to me that there should be a better way to do this.  We should
ask
> someone more familiar with views (sky, beng, erikkay) whether we can schedule
a
> relayout+paint from a child view without crawling up the chain.

I talked with beng and looked deeply through View & RootView. It appears that
the view hierarchy isn't really setup at the moment to have size changes
propagate from children up to parents. I think we should revisit this issue when
we are closer to implementing the final UI and then consider making changes to
the view system

http://codereview.chromium.org/56122/diff/1045/48
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/56122/diff/1045/48#newcode251
Line 251: virtual void DidContentsChangeSize(const int new_width, const int
new_height)
On 2009/04/01 18:28:35, Aaron Boodman wrote:
> See RequestOpenURL(), below, for how this line should be wrapped.

Done.

http://codereview.chromium.org/56122/diff/1045/48#newcode252
Line 252: {}
On 2009/04/01 18:30:09, Matt Perry wrote:
> nit: braces like this: Foo(..) {
> }
> 
> or like this: Foo(..) {}

Done.

http://codereview.chromium.org/56122/diff/1045/53
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/56122/diff/1045/53#newcode1916
Line 1916: int new_height) {
On 2009/04/01 18:28:35, Aaron Boodman wrote:
> int new_height should be aligned with WebWidget*

Done.

http://codereview.chromium.org/56122/diff/1045/53#newcode1918
Line 1918: // Only notifiy the RenderViewHost if it's client is an
ExtensionView.
On 2009/04/01 18:30:09, Matt Perry wrote:
> grammar nit: "its"

Done.

http://codereview.chromium.org/56122/diff/1045/53#newcode1919
Line 1919: if (BindingsPolicy::is_extension_enabled(enabled_bindings_)) {
There is a comment above the function declaration in render_view_host.h. Do you
want the same comment in the .cc file?

On 2009/04/01 18:28:35, Aaron Boodman wrote:
> If we are going to special case this code, we should at least add a comment in
> RenderViewHostDelegate that OnSizeChange only works for extensions.
> 
> I think it would be much cleaner to modify the current bindings bit mask to be
a
> more general configuration bit mask, then modify RenderViewHost to have a
> setting for receiving or not receiving size change events.

http://codereview.chromium.org/56122/diff/1045/53#newcode1920
Line 1920: int width = webview()->GetMainFrame()->GetContentsPreferredWidth();
This was a slip. I was retrieving it in two places (ChromeClientImpl and here).
This is the appropriate place to use it and I have made the fix. ChromeClient
passes the width & height it received unchanged.

On 2009/04/01 18:28:35, Aaron Boodman wrote:
> width appears not to be used here?

http://codereview.chromium.org/56122/diff/1045/53#newcode1921
Line 1921: Send(new ViewHostMsg_DidContentsChangeSize(routing_id_, new_width,
new_height));
On 2009/04/01 18:28:35, Aaron Boodman wrote:
> 80 cols.

Done.

http://codereview.chromium.org/56122/diff/1045/53#newcode1921
Line 1921: Send(new ViewHostMsg_DidContentsChangeSize(routing_id_, new_width,
new_height));
On 2009/04/01 18:30:09, Matt Perry wrote:
> line length

Done.

http://codereview.chromium.org/56122/diff/1045/42
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/56122/diff/1045/42#newcode458
Line 458: int width = webFrameImpl->GetContentsPreferredWidth();
On 2009/04/01 18:30:09, Matt Perry wrote:
> This feels wrong to me.  DidContentsSizeChange should report the size given to
> contentsSizeChanged.  RenderView can then send up a message with the preferred
> width instead, but we should make clear that the width given is the preferred
> width, not the actual content width.

Done.

http://codereview.chromium.org/56122/diff/1045/40
File webkit/glue/chrome_client_impl.h (right):

http://codereview.chromium.org/56122/diff/1045/40#newcode105
Line 105: virtual void contentsSizeChanged(WebCore::Frame*, const
WebCore::IntSize&)
On 2009/04/01 18:28:35, Aaron Boodman wrote:
> Not sure what the rule is, but I'd put the second param on the next line to
> avoid having the const dangling on its own.

Done.

http://codereview.chromium.org/56122/diff/1045/41
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/56122/diff/1045/41#newcode542
Line 542: return frame_->document()->renderView()->minPrefWidth();
On 2009/04/01 18:30:09, Matt Perry wrote:
> You need to NULL-check the document and the renderView.

Done. Not sure what to return if either is null. I guess 0. Let me know if this
is wrong.

http://codereview.chromium.org/56122/diff/1045/45
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/56122/diff/1045/45#newcode497
Line 497: int new_height) {}
On 2009/04/01 18:28:35, Aaron Boodman wrote:
> Indent.

Done.

[Aaron Boodman, 2009-04-02 21:37:55]

lgtm, but wait for matt.

[Matt Perry, 2009-04-02 21:41:21]

looks good! 1 last comment

http://codereview.chromium.org/56122/diff/82/96
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/56122/diff/82/96#newcode1928
Line 1928: // TODO(rafaelw): Because we want to dynamically size down & up, the
host gets
this isn't a TODO, is it?

[rafaelw, 2009-04-04 01:08:46]

Ok. Check it out.

[Matt Perry, 2009-04-04 01:21:16]

LGTM

http://codereview.chromium.org/56122/diff/2056/5008
File chrome/browser/extensions/extension_view.cc (right):

http://codereview.chromium.org/56122/diff/2056/5008#newcode35
Line 35: void ExtensionView::DidContentsPreferredWidthChange(const int
pre_width) {
nit: I like pref_width better :)