Browser Plugin: Implement AutoSize

Browser Plugin: Implement AutoSize

How it works:

There are five new properties on BrowserPlugin:

autoSize
minHeight
minWidth
maxHeight
maxWidth

If autoSize is enabled, then we ask the guest for autosize.

A lot of the complexity comes from the fact that the guest's size may change, while the backing store is fixed in size to avoid excessive back and forth allocation of a shared memory buffer. 

The damage buffer is created to fit a guest of size (maxWidth, maxHeight).

ViewHostMsg_UpdateRect_Params has a view_size parameter that indicates the size of guest. We carry that value into the embedder.

Some code was added to the embedder's backing store to clear the backing store. This is called when view_size changes to clear out any pixels outside the current view_size's bounds.

The basic plumbing was landed already. This patch completes this work.

BUG=157620
Test=BrowserPluginHostTest.AutoSizeBeforeNavigation, BrowserPluginHostTest.AutoSizeAfterNavigation, BrowserPluginTest.AutoSizeAttributes

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

[lazyboy, 2012-11-06 22:22:35]

Some comments/questions, I'm still trying to understand the flow completely.

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right):

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:56:
IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_SetAutoSize,
nit: can fit in one line?

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:207:
gfx::Size(min_width_, min_height_), gfx::Size(max_width_, max_height_));
probably easier to keep these as gfx::Size min_auto_size_ and gfx::Size
max_auto_size_

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:330: if ((auto_size_ &&
InAutoSizeBounds(params.view_size)) ||
Do we need to handle the case where auto_size is set and scale_factor changes?

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:178: //backing_store_.reset();
rm

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:196: 
nit: rm

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:227: void
BrowserPlugin::SizeChanged(const gfx::Size& old_view_size) {
This is size changed due to auto sizing only, maybe rename the func to make it
obvious: SizeChangedDueToAutoSize or sth similar.

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:237: // Construct the
loadStart event object.
nit: fix comment.

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:513: // the backing store has
been updated before the developer attempts to
I'm not sure I totally understand this one, if you post_task SizeChanged instead
of calling directly, it would get called after this function finishes execution?

[Fady Samuel, 2012-11-06 23:52:34]

Addressed nits + added tests: PTAL.

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right):

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:56:
IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_SetAutoSize,
On 2012/11/06 22:22:35, lazyboy wrote:
> nit: can fit in one line?

Done.

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:207:
gfx::Size(min_width_, min_height_), gfx::Size(max_width_, max_height_));
On 2012/11/06 22:22:35, lazyboy wrote:
> probably easier to keep these as gfx::Size min_auto_size_ and gfx::Size
> max_auto_size_

Done.

https://codereview.chromium.org/11360106/diff/5005/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:330: if ((auto_size_ &&
InAutoSizeBounds(params.view_size)) ||
On 2012/11/06 22:22:35, lazyboy wrote:
> Do we need to handle the case where auto_size is set and scale_factor changes?

Good catch. In this case, we still want to drop the change.

Speaking of which, we probably need some high-DPI testing. I'll file a bug for
that.

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:178: //backing_store_.reset();
On 2012/11/06 22:22:35, lazyboy wrote:
> rm

Done.

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:196: 
On 2012/11/06 22:22:35, lazyboy wrote:
> nit: rm

Done.

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:227: void
BrowserPlugin::SizeChanged(const gfx::Size& old_view_size) {
On 2012/11/06 22:22:35, lazyboy wrote:
> This is size changed due to auto sizing only, maybe rename the func to make it
> obvious: SizeChangedDueToAutoSize or sth similar.

Done.

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:237: // Construct the
loadStart event object.
On 2012/11/06 22:22:35, lazyboy wrote:
> nit: fix comment.

Done.

https://codereview.chromium.org/11360106/diff/5005/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:513: // the backing store has
been updated before the developer attempts to
On 2012/11/06 22:22:35, lazyboy wrote:
> I'm not sure I totally understand this one, if you post_task SizeChanged
instead
> of calling directly, it would get called after this function finishes
execution?

Yes. Also, if there are multiple updateRects queued up before the SizeChanged,
then we'll only see the latest changed size. This reduces the spew of fired
events to some degree (although I do still get multiple fires in google.com, for
example).

[lazyboy, 2012-11-07 07:39:08]

lgtm

*/browser_plugin/* looks good, thanks.

http://chromiumcodereview.appspot.com/11360106/diff/5005/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://chromiumcodereview.appspot.com/11360106/diff/5005/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:513: // the backing store has
been updated before the developer attempts to
On 2012/11/06 23:52:34, Fady Samuel wrote:
> On 2012/11/06 22:22:35, lazyboy wrote:
> > I'm not sure I totally understand this one, if you post_task SizeChanged
> instead
> > of calling directly, it would get called after this function finishes
> execution?
> 
> Yes. Also, if there are multiple updateRects queued up before the SizeChanged,
> then we'll only see the latest changed size. This reduces the spew of fired
> events to some degree (although I do still get multiple fires in
http://google.com, for
> example).
Ah, I was about to suggest you write a test for that so we are sure this is
doing what we are expecting it to do.

http://chromiumcodereview.appspot.com/11360106/diff/7005/content/browser/brow...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://chromiumcodereview.appspot.com/11360106/diff/7005/content/browser/brow...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:1149:
"document.getElementById('plugin').autoSize = true;"));
Also how about adding tests that doesn't specify autoSize=true but tries to
specify some of {minW,maxW,minH,maxH}, should be easy and useful to add.

Also set .autoSize = false explicitly.

http://chromiumcodereview.appspot.com/11360106/diff/7005/content/browser/brow...
File content/browser/browser_plugin/test_browser_plugin_guest.cc (right):

http://chromiumcodereview.appspot.com/11360106/diff/7005/content/browser/brow...
content/browser/browser_plugin/test_browser_plugin_guest.cc:80:
auto_view_size_message_loop_runner_->Quit();
Running this line implies send_message_loop_runner_ == NULL, right? Is it
possible to DCHECK this? (I don't remember what happens when DCHECK fails in
tests).
I am suggesting this b/c more than one WaitForXXX is handled in this function
and this isn't like other simpler WaitXXXs in this class, doesn't hurt to be a
bit verbose.

http://chromiumcodereview.appspot.com/11360106/diff/7005/content/browser/brow...
File content/browser/browser_plugin/test_browser_plugin_guest.h (right):

http://chromiumcodereview.appspot.com/11360106/diff/7005/content/browser/brow...
content/browser/browser_plugin/test_browser_plugin_guest.h:99: gfx::Size
auto_view_size_;
rename to expected_auto_view_size_, otherwise a bit confusing.

[Fady Samuel, 2012-11-07 20:21:10]

+jam@ for content OWNER review.

http://codereview.chromium.org/11360106/diff/7005/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11360106/diff/7005/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:1149:
"document.getElementById('plugin').autoSize = true;"));
On 2012/11/07 07:39:09, lazyboy wrote:
> Also how about adding tests that doesn't specify autoSize=true but tries to
> specify some of {minW,maxW,minH,maxH}, should be easy and useful to add.
> 
> Also set .autoSize = false explicitly.
> 

Added a couple of subtests.

http://codereview.chromium.org/11360106/diff/7005/content/browser/browser_plu...
File content/browser/browser_plugin/test_browser_plugin_guest.cc (right):

http://codereview.chromium.org/11360106/diff/7005/content/browser/browser_plu...
content/browser/browser_plugin/test_browser_plugin_guest.cc:80:
auto_view_size_message_loop_runner_->Quit();
On 2012/11/07 07:39:09, lazyboy wrote:
> Running this line implies send_message_loop_runner_ == NULL, right? Is it
> possible to DCHECK this? (I don't remember what happens when DCHECK fails in
> tests).
> I am suggesting this b/c more than one WaitForXXX is handled in this function
> and this isn't like other simpler WaitXXXs in this class, doesn't hurt to be a
> bit verbose.

I don't think that's quite true, we don't clear MessageLoopRunners when they
quit so if "WaitForUpdateRect" is called before "WaitForViewSize" this DCHECK
would fail. I suppose I could modify all the WaitFor* method to clear the
scoped_refptr after the MessageLoopRunner returns, but I'd prefer to do that in
a separate CL.

http://codereview.chromium.org/11360106/diff/7005/content/browser/browser_plu...
File content/browser/browser_plugin/test_browser_plugin_guest.h (right):

http://codereview.chromium.org/11360106/diff/7005/content/browser/browser_plu...
content/browser/browser_plugin/test_browser_plugin_guest.h:99: gfx::Size
auto_view_size_;
On 2012/11/07 07:39:09, lazyboy wrote:
> rename to expected_auto_view_size_, otherwise a bit confusing.

Done.

[jam, 2012-11-09 17:11:09]

lgtm, sorry for the delay I just saw this. in the future feel free to IM me if I
don't respond within a few hours.

can you avoid the sync IPC? it seems unfortunate to send it. why does the
renderer need to block for it?

https://codereview.chromium.org/11360106/diff/6020/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/11360106/diff/6020/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:931: msg.type() !=
BrowserPluginHostMsg_SetAutoSize::ID &&
anyway to avoid this sync IPC?

[Fady Samuel, 2012-11-09 17:41:09]

https://codereview.chromium.org/11360106/diff/6020/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/11360106/diff/6020/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:931: msg.type() !=
BrowserPluginHostMsg_SetAutoSize::ID &&
On 2012/11/09 17:11:10, John Abd-El-Malek wrote:
> anyway to avoid this sync IPC?

Yes, but not easily. We have a bug open related to this:

http://code.google.com/p/chromium/issues/detail?id=155915

The major issue is we need to share a buffer between two processes and we need
to make sure that both processes are aware of the new buffer prior to using it.
Using a SYNC IPC is the easy way to do this.

Coordinating this via async messages is certainly possible, but quite a bit
harder to get right.

[Fady Samuel, 2012-11-09 18:02:53]

https://codereview.chromium.org/11360106/diff/6020/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/11360106/diff/6020/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:931: msg.type() !=
BrowserPluginHostMsg_SetAutoSize::ID &&
On 2012/11/09 17:41:10, Fady Samuel wrote:
> On 2012/11/09 17:11:10, John Abd-El-Malek wrote:
> > anyway to avoid this sync IPC?
> 
> Yes, but not easily. We have a bug open related to this:
> 
> http://code.google.com/p/chromium/issues/detail?id=155915
> 
> The major issue is we need to share a buffer between two processes and we need
> to make sure that both processes are aware of the new buffer prior to using
it.
> Using a SYNC IPC is the easy way to do this.
> 
> Coordinating this via async messages is certainly possible, but quite a bit
> harder to get right.

As per offline discussion, this is a hard, general problem that needs to be
fixed for resize (auto or otherwise). We will address this for M25, in a
separate patch.

[commit-bot: I haz the power, 2012-11-09 18:03:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11360106/6020

[commit-bot: I haz the power, 2012-11-09 19:17:20]

Change committed as 166948