Change panels to be able to turn off autoresize.

Change panels to be able to turn off autoresize.

BUG=109343
TEST=PanelBrowserTest.ResizePanel


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

[jennb, 2012-02-28 23:45:15]

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel.cc:130: WebContents* web_contents =
browser()->GetSelectedWebContents();
This is repeated in the if-section too. Move before if-clause?

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_browsertest.cc:855: // Verify resizing an
auto-resizable panel is a no-op for now.
update comment to say it also turns off auto-resizability.

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_browsertest.cc:858: gfx::Size new_size(150, 200);
Could do:

gfx::Size new_size  original_bounds.size();
new_size.width() + some_delta;  // create some different size
new_size.height() + some_delta;

Can delete the two EXPECT_NE lines below as we've explicitly made new_size
different from original_bounds.

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_browsertest.cc:866: 
extra blank line?

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_manager.cc:186: panel->SetAutoResizable(false);
When this calls DisableAutoResize, it passes the restored_size to render view
host. The very next line changes the size. Will this cause redundant render view
resizing?

[levin, 2012-02-29 00:04:42]

All addressed.

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel.cc:130: WebContents* web_contents =
browser()->GetSelectedWebContents();
On 2012/02/28 23:45:15, jennb wrote:
> This is repeated in the if-section too. Move before if-clause?

Done.

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_browsertest.cc:855: // Verify resizing an
auto-resizable panel is a no-op for now.
On 2012/02/28 23:45:15, jennb wrote:
> update comment to say it also turns off auto-resizability.

Done.

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_browsertest.cc:858: gfx::Size new_size(150, 200);
On 2012/02/28 23:45:15, jennb wrote:
> Could do:
> 
> gfx::Size new_size  original_bounds.size();
> new_size.width() + some_delta;  // create some different size
> new_size.height() + some_delta;
> 
> Can delete the two EXPECT_NE lines below as we've explicitly made new_size
> different from original_bounds.

Done.

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_browsertest.cc:866: 
On 2012/02/28 23:45:15, jennb wrote:
> extra blank line?

Done.

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_manager.cc:186: panel->SetAutoResizable(false);
On 2012/02/28 23:45:15, jennb wrote:
> When this calls DisableAutoResize, it passes the restored_size to render view
> host. The very next line changes the size. Will this cause redundant render
view
> resizing?

I moved this to be inside of ResizePanelWindow. I think that is correct.

[levin, 2012-02-29 00:06:17]

Darin would you review the file not in the panel directory?

(You're welcome to look at the panels ones too if you wish.)

This is the chromium side of https://bugs.webkit.org/show_bug.cgi?id=77452

[jennb, 2012-02-29 00:11:50]

https://chromiumcodereview.appspot.com/9517010/diff/4001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/4001/chrome/browser/ui/pa...
chrome/browser/ui/panels/docked_panel_strip.cc:475:
panel->SetAutoResizable(false, new_size);
This works for now. We'll have to figure out how to refactor this entire method
to make it work for detached panels. (FYI)

https://chromiumcodereview.appspot.com/9517010/diff/4001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/4001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_browsertest.cc:877: panel->SetAutoResizable();
takes a param

https://chromiumcodereview.appspot.com/9517010/diff/4001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_browsertest.cc:884: L"changeSize(50);"));
This only applies to the update-preferred-size.html used in the
DISABLED_AutoResize test.

[jennb, 2012-02-29 19:46:00]

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_browsertest.cc:803:
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, AutoResize) {
Remember to leave this DISABLED before commit as it will probably still be
flaky.

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:177: DCHECK(auto_sizing_enabled_);
Could one of these update-preferred-size IPCs been in flight while we change the
panel to not-auto-resizable? That would cause this dcheck to fail. The previous
if-check was a guard against this scenario.

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:183:
docked_strip_->ResizePanelWindow(panel, new_size,
Is it safe to separate this like this:

docked_strip_->ResizePanelWindow(panel, new_size);
panel->SetAutoResizable(false);

? That would make the ResizePanelWindow() method clearer and avoid the need
touch auto-resizable state of a panel for OnPreferredWindowSizeChanged().

[levin, 2012-02-29 23:48:28]

All fixed up.

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_browsertest.cc:803:
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, AutoResize) {
On 2012/02/29 19:46:00, jennb wrote:
> Remember to leave this DISABLED before commit as it will probably still be
> flaky.

Done. Thanks!

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:177: DCHECK(auto_sizing_enabled_);
On 2012/02/29 19:46:00, jennb wrote:
> Could one of these update-preferred-size IPCs been in flight while we change
the
> panel to not-auto-resizable? That would cause this dcheck to fail. The
previous
> if-check was a guard against this scenario.

This is an important concern. It is handled here:
http://code.google.com/codesearch#OAMlx_jo-ck/src/content/browser/renderer_ho...

It would be unfortunate if every consumer of auto resize needed to be aware of
the ipc subtleties :).


However, one problem was that preferred size notifications were always enabled
on OSX so this callback was always done. I've fixed that by separating out the
callbacks for preferred size changes vs autoresize.

https://chromiumcodereview.appspot.com/9517010/diff/11001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:183:
docked_strip_->ResizePanelWindow(panel, new_size,
Good call. Done.

[jennb, 2012-03-01 00:14:40]

Panel changes LGTM.

I'm not sure it will be obvious to others the difference between preferred
content size changed vs content size changed due to auto-resize. Will need Darin
to approve the non-panels changes as I don't feel qualified to make that call.

https://chromiumcodereview.appspot.com/9517010/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:175: void
PanelManager::OnPreferredWindowSizeChanged(
Match name to new DueToAutoResize?

[levin, 2012-03-01 00:19:11]

Ben would you look at the changes for
chrome/browser/ui/browser.h
chrome/browser/ui/browser.cc
chrome/browser/ui/browser_window.h

Jenn has covered the ui/panels code and I'm counting on Darin for the others.

[darin (slow to review), 2012-03-01 00:30:49]

https://chromiumcodereview.appspot.com/9517010/diff/13002/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/13002/content/renderer/re...
content/renderer/render_view_impl.cc:4170: webview()->resize(new_size);
do you need to assign to size_?  should some code be shared between this
function and OnResize?

[levin, 2012-03-01 01:56:19]

https://chromiumcodereview.appspot.com/9517010/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:175: void
PanelManager::OnPreferredWindowSizeChanged(
On 2012/03/01 00:14:40, jennb wrote:
> Match name to new DueToAutoResize?

Done as discussed.

https://chromiumcodereview.appspot.com/9517010/diff/13002/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9517010/diff/13002/content/renderer/re...
content/renderer/render_view_impl.cc:4170: webview()->resize(new_size);
On 2012/03/01 00:30:49, darin wrote:
> do you need to assign to size_?  should some code be shared between this
> function and OnResize?

Good idea. Done.

(There was a problem with OnResize setting up the send ack bit but I addressed
that and added some asserts to verify that it is set when appropriate.)

[Ben Goodger (Google), 2012-03-01 02:35:33]

What is this for?

[levin, 2012-03-01 02:50:25]

On 2012/03/01 02:35:33, Ben Goodger (Google) wrote:
> What is this for?

To allow panels to turn off auto-resize. Panels can auto fit their html content
but the user may also may to resize them manually in which case the
auto-resizing needs to be turned off.

[levin, 2012-03-01 04:01:27]

On 2012/03/01 02:50:25, levin wrote:
> On 2012/03/01 02:35:33, Ben Goodger (Google) wrote:
> > What is this for?
> 
> To allow panels to turn off auto-resize. Panels can auto fit their html
content
> but the user may also may to resize them manually in which case the
> auto-resizing needs to be turned off.

This may be too much info but I'm not sure how much you know or wnat to know
about this area....

Specifically, the changes done to browser/ui/browser.h are related to
autoreszing.

Some places in Chromium may want to autoresize to fit the html content. These
include Panels (in some cases) and the pop-up from browser buttons (the
extension buttons), and notification.

The autoresize allows the render layer to handle this and let the browser know
about resize changes that have been done.

These callbacks about auto-resizing were re-using "UpdatePreferrredSize" before
but that call is specifically about preferred size changes (which let the
browser know the desired size for the html - this is helpful for cases like the
"green" button on OS X windows that try to right size the window). It is nice to
keep these two callbacks separate for a number of reasons (including the fact
that the auto-resize call shouldn't be done when auto-resizing is off but the
UpdatePreferredSize call may be done).

[Ben Goodger (Google), 2012-03-01 19:15:47]

Is autoresize a panel-specific feature?

[levin, 2012-03-01 19:19:44]

On 2012/03/01 19:15:47, Ben Goodger (Google) wrote:
> Is autoresize a panel-specific feature?

Short answer: Yes.

Long answer:
Notifications and the window that pops up when you click a browser button also
use autoresize but they use the old mechanism (which has issues). Panels is the
only consumer of the new way (though more will be moved to using it).

[levin, 2012-03-01 19:20:25]

On 2012/03/01 19:19:44, levin wrote:
> On 2012/03/01 19:15:47, Ben Goodger (Google) wrote:
> > Is autoresize a panel-specific feature?
> 
> Short answer: Yes.
> 
> Long answer:
> Notifications and the window that pops up when you click a browser button also
> use autoresize but they use the old mechanism (which has issues). Panels is
the
> only consumer of the new way (though more will be moved to using it).

notifications = html notifications or balloons or toast.

[Ben Goodger (Google), 2012-03-01 19:24:09]

So typically we don't have feature-specific IPC stuff run through all the render
view delegates/web contents delegates etc. We have a feature-specific stub in
the renderer and a feature-specific IPC message filter that lives in chrome.
This allows you to sidestep the whole content stack completely and be more self
contained.

[levin, 2012-03-01 19:32:47]

On 2012/03/01 19:24:09, Ben Goodger (Google) wrote:
> So typically we don't have feature-specific IPC stuff run through all the
render
> view delegates/web contents delegates etc. We have a feature-specific stub in
> the renderer and a feature-specific IPC message filter that lives in chrome.
> This allows you to sidestep the whole content stack completely and be more
self
> contained.

This is a generic RenderView/RenderViewHost feature which is turned on by using 
RenderViewHost::SetShouldAutoResize

At the moment it is only used by panels, but it needs to be used by html
notifications balloon and by extension bubbles (or whatever they are called) in
order to avoid windows that fluctuate between two or more different sizes in a
rapid fashion (which is a fundamental flaw with the old way of doing it).

Are you suggesting that all three features should use this feature specific
stub? (It sounds like that involves a much bigger rewrite of all of this.)

[levin, 2012-03-01 19:45:25]

On 2012/03/01 19:24:09, Ben Goodger (Google) wrote:
> So typically we don't have feature-specific IPC stuff run through all the
render
> view delegates/web contents delegates etc. We have a feature-specific stub in
> the renderer and a feature-specific IPC message filter that lives in chrome.
> This allows you to sidestep the whole content stack completely and be more
self
> contained.


Another thing to note is that the ipc that happens is ViewHostMsg_UpdateRect
which results in RenderWidgetHost::OnMsgUpdateRect. When auto-resize is on, this
results in a call to RenderWidgetHost::OnRenderAutoResized which is handled by
RenderViewHost::OnRenderAutoResized.

[Ben Goodger (Google), 2012-03-01 20:29:34]

Talked with John about this a bit more, and we agree that your changes to
content seem like a reasonable way to implement the content side of this.

I don't like that browser is used as a conduit for panel messages. Panels share
almost nothing in common with the browser, other than that they embed a
WebContents. I had a discussion with some on the panels team what seems like a
couple of months ago now and indicated that I wanted to see this code converted
to use TabContents directly, via a DOMView. What's the status on that? The
panels code is excessively convoluted and is causing us trouble in other areas.

[jennb, 2012-03-01 20:58:01]

On 2012/03/01 20:29:34, Ben Goodger (Google) wrote:
> Talked with John about this a bit more, and we agree that your changes to
> content seem like a reasonable way to implement the content side of this.
> 
> I don't like that browser is used as a conduit for panel messages. Panels
share
> almost nothing in common with the browser, other than that they embed a
> WebContents. I had a discussion with some on the panels team what seems like a
> couple of months ago now and indicated that I wanted to see this code
converted
> to use TabContents directly, via a DOMView. What's the status on that? The
> panels code is excessively convoluted and is causing us trouble in other
areas.

We haven't tackled that major refactor of Panels yet. It's still on our radar.
We'll probably leverage the work stevenjb has done to factor BaseWindow out of
BrowserWindow.

[Ben Goodger (Google), 2012-03-01 20:59:08]

OK. We can tackle that in M20. I will follow up then.

For now, this LGTM.

[darin (slow to review), 2012-03-01 22:46:50]

LGTM for content/renderer/

[commit-bot: I haz the power, 2012-03-02 02:10:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/levin@chromium.org/9517010/28002

[commit-bot: I haz the power, 2012-03-02 02:10:33]

Presubmit check for 9517010-28002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/public/browser/render_view_host_delegate.h,content/public/browser/web_contents_delegate.h

Presubmit checks took 3.2s to calculate.

[jam, 2012-03-02 02:17:14]

lgtm

[commit-bot: I haz the power, 2012-03-02 02:20:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/levin@chromium.org/9517010/28002

[commit-bot: I haz the power, 2012-03-02 05:39:38]

Try job failure for 9517010-28002 (retry) on win_rel for steps "check_deps,
ui_tests".
It's a second try, previously, steps "check_deps, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-02 05:47:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/levin@chromium.org/9517010/28002

[commit-bot: I haz the power, 2012-03-02 16:13:36]

Change committed as 124664