[BrowserTag] Send dib info with NavigateGuest message,

[BrowserTag] Send dib info with NavigateGuest message,
This would fix guest painting in: set no src + resize + navigate to src case.

Without this change, for the scenario above, we don't get any dib at all from
embedder upon setting the non-empty src. Now we also ship dib on NavigateGuest
if necessary. This dib is the last accumulated dib from updateGeometry().

Note that our unit test exercises this case, but doesn't look for correct
painting, it only expects UpdateRect to be called with correct size. I've updated the test now to look for correct sized damage buffer instead.


BUG=151948
TEST= Create an empty src guest and keep changing its size in
SetInterval from the embedder, then in the middle set a non-empty src. Observed
that guest paints correctly with final size. Updated unit test to look for damage buffer with correct size instead of looking for UpdateRect with correct size.

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

[lazyboy, 2012-09-21 22:52:15]

Hey Fady,
Couple things:
1. It is possible I've missed taking scale_factor into account in
browser_plugin.cc for my change.
2. The description probably needs to be reworded. Sending it to you since you
already know what's happening here, I'll update the description before sending
it to creis@.

Also, not sure how write tests for this.
Ty

[Fady Samuel, 2012-09-21 23:01:19]

On 2012/09/21 22:52:15, lazyboy wrote:
> Hey Fady,
> Couple things:
> 1. It is possible I've missed taking scale_factor into account in
> browser_plugin.cc for my change.
> 2. The description probably needs to be reworded. Sending it to you since you
> already know what's happening here, I'll update the description before sending
> it to creis@.
> 
> Also, not sure how write tests for this.
> Ty

I haven't looked carefully at this code yet but a couple of points:

--force-device-scale-factor=2 to get high-DPI

and

I bet you can add a "WaitForDeadBeef" method to TestBrowserPluginGuest :P

[lazyboy, 2012-09-24 21:32:44]

I have modified unit test to check for damage buffer with correct size. Verified
the test fails when I revert the NavigateGuest change.

Tested with --force-device-scale-factor=2 in chromeos, nothing surprising there.

PTAL.

On 2012/09/21 23:01:19, Fady Samuel wrote:
> On 2012/09/21 22:52:15, lazyboy wrote:
> > Hey Fady,
> > Couple things:
> > 1. It is possible I've missed taking scale_factor into account in
> > browser_plugin.cc for my change.
> > 2. The description probably needs to be reworded. Sending it to you since
you
> > already know what's happening here, I'll update the description before
sending
> > it to creis@.
> > 
> > Also, not sure how write tests for this.
> > Ty
> 
> I haven't looked carefully at this code yet but a couple of points:
> 
> --force-device-scale-factor=2 to get high-DPI
> 
> and
> 
> I bet you can add a "WaitForDeadBeef" method to TestBrowserPluginGuest :P

[Fady Samuel, 2012-09-24 22:16:58]

http://codereview.chromium.org/10965048/diff/6008/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/10965048/diff/6008/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:155: TransportDIB*
damage_buffer = NULL;
This makes this method a lot harder to read. Could you please factor out
TransportDIB mapping into a separate method.

http://codereview.chromium.org/10965048/diff/6008/content/common/browser_plug...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/10965048/diff/6008/content/common/browser_plug...
content/common/browser_plugin_messages.h:91: // the BrowserPlugin's browser-side
BrowserPluginHost as a guest, and navigate
This comment is out of date. Please update to mention BrowserPluginEmbedder and
BrowserPluginGuest.

http://codereview.chromium.org/10965048/diff/6008/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10965048/diff/6008/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:105:
*GetPendingResizeParams()));
Shouldn't this get freed after we send it?

[lazyboy, 2012-09-25 17:57:22]

https://chromiumcodereview.appspot.com/10965048/diff/6008/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/6008/content/browser/bro...
content/browser/browser_plugin/browser_plugin_embedder.cc:155: TransportDIB*
damage_buffer = NULL;
On 2012/09/24 22:16:58, Fady Samuel wrote:
> This makes this method a lot harder to read. Could you please factor out
> TransportDIB mapping into a separate method.

Done.

https://chromiumcodereview.appspot.com/10965048/diff/6008/content/common/brow...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10965048/diff/6008/content/common/brow...
content/common/browser_plugin_messages.h:91: // the BrowserPlugin's browser-side
BrowserPluginHost as a guest, and navigate
On 2012/09/24 22:16:58, Fady Samuel wrote:
> This comment is out of date. Please update to mention BrowserPluginEmbedder
and
> BrowserPluginGuest.

Done.

https://chromiumcodereview.appspot.com/10965048/diff/6008/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/6008/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:105:
*GetPendingResizeParams()));
On 2012/09/24 22:16:58, Fady Samuel wrote:
> Shouldn't this get freed after we send it?

Done.

[Fady Samuel, 2012-09-25 18:02:01]

LGTM with a minor nit.

Please have Charlie OWNER review this.

https://chromiumcodereview.appspot.com/10965048/diff/12001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/10965048/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:31: class TransportDIB;
We probably don't need this forward declaration if we are including
transport_dib.h

[lazyboy, 2012-09-25 18:47:03]

Added Charlie for reviewing.

On 2012/09/25 18:02:01, Fady Samuel wrote:
> LGTM with a minor nit.
> 
> Please have Charlie OWNER review this.
> 
>
https://chromiumcodereview.appspot.com/10965048/diff/12001/content/browser/br...
> File content/browser/browser_plugin/browser_plugin_embedder.h (right):
> 
>
https://chromiumcodereview.appspot.com/10965048/diff/12001/content/browser/br...
> content/browser/browser_plugin/browser_plugin_embedder.h:31: class
TransportDIB;
> We probably don't need this forward declaration if we are including
> transport_dib.h

[lazyboy, 2012-09-25 18:47:13]

https://chromiumcodereview.appspot.com/10965048/diff/12001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/10965048/diff/12001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:31: class TransportDIB;
On 2012/09/25 18:02:01, Fady Samuel wrote:
> We probably don't need this forward declaration if we are including
> transport_dib.h

Done.

[Charlie Reis, 2012-09-25 22:44:24]

Seems good overall, though I admit that I don't know much about things like
damage buffers and transport DIBs.  One question about the IPC message and some
nits below.

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:139: // too long.
How would you delay the ResizeGuest?  Would that result in a flicker?  (Sorry, I
don't know this part of the code very well.  Just wondering what the TODO is
suggesting.)

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:147: // Invalid
transport dib, just resize the WebContents.
nit: so just

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/common/bro...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/common/bro...
content/common/browser_plugin_messages.h:93:
IPC_MESSAGE_ROUTED4(BrowserPluginHostMsg_NavigateGuest,
Isn't this sent from the embedder's renderer process to the browser, instructing
it to navigate the guest?  I don't think we want to move it down to this section
("These messages are from the guest renderer to the browser process").

[Fady Samuel, 2012-09-25 22:49:38]

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:139: // too long.
On 2012/09/25 22:44:24, creis wrote:
> How would you delay the ResizeGuest?  Would that result in a flicker?  (Sorry,
I
> don't know this part of the code very well.  Just wondering what the TODO is
> suggesting.)

This TODO is outdated actually. SetDamageBuffer needs to get called before we
unblock the embedder (we're thinking of a way to make resize async between
embedder and browser, but we don't yet have concrete code).

About the only thing we can do later is SizeContents right at the end and I
don't think that's worth schedule as a task, so I'd probably remove the TODO.

[lazyboy, 2012-09-25 23:15:29]

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:139: // too long.
Removed the TODO.



On 2012/09/25 22:49:38, Fady Samuel wrote:
> On 2012/09/25 22:44:24, creis wrote:
> > How would you delay the ResizeGuest?  Would that result in a flicker? 
(Sorry,
> I
> > don't know this part of the code very well.  Just wondering what the TODO is
> > suggesting.)
> 
> This TODO is outdated actually. SetDamageBuffer needs to get called before we
> unblock the embedder (we're thinking of a way to make resize async between
> embedder and browser, but we don't yet have concrete code).
> 
> About the only thing we can do later is SizeContents right at the end and I
> don't think that's worth schedule as a task, so I'd probably remove the TODO.

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:147: // Invalid
transport dib, just resize the WebContents.
On 2012/09/25 22:44:24, creis wrote:
> nit: so just

Done.

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/common/bro...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10965048/diff/17002/content/common/bro...
content/common/browser_plugin_messages.h:93:
IPC_MESSAGE_ROUTED4(BrowserPluginHostMsg_NavigateGuest,
On 2012/09/25 22:44:24, creis wrote:
> Isn't this sent from the embedder's renderer process to the browser,
instructing
> it to navigate the guest?  I don't think we want to move it down to this
section
> ("These messages are from the guest renderer to the browser process").

Reverted moving. I needed this after the ResizeGuest_Params struct definition,
moved the struct up instead.
Done.

[Charlie Reis, 2012-09-25 23:24:22]

Thanks, LGTM.

[commit-bot: I haz the power, 2012-09-26 01:45:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10965048/16003

[commit-bot: I haz the power, 2012-09-26 01:58:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10965048/10005

[commit-bot: I haz the power, 2012-09-26 03:37:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10965048/14009

[commit-bot: I haz the power, 2012-09-26 05:16:44]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-09-28 04:14:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10965048/8032

[commit-bot: I haz the power, 2012-09-28 05:47:07]

Retried try job too often for step(s) content_browsertests

[lazyboy, 2012-09-28 18:21:09]

Fady, can you take a look at the recent changes to fix OSX transport dib issue?
Thanks.

[Fady Samuel, 2012-09-28 18:31:46]

LGTM with one nit: Please add a comment. Thanks for fixing all these nasty
platform-specific issues :-)

https://chromiumcodereview.appspot.com/10965048/diff/20006/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/20006/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:487: delete damage_buffer_;
Please add a comment explaining why we don't need to send
ViewHostMsg_FreeTransportDIB to get the browser to free the TransportDIB for us.

[lazyboy, 2012-09-28 19:05:01]

https://chromiumcodereview.appspot.com/10965048/diff/20006/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/20006/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:487: delete damage_buffer_;
On 2012/09/28 18:31:46, Fady Samuel wrote:
> Please add a comment explaining why we don't need to send
> ViewHostMsg_FreeTransportDIB to get the browser to free the TransportDIB for
us.

Done.

[commit-bot: I haz the power, 2012-09-28 20:02:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10965048/15029

[commit-bot: I haz the power, 2012-09-28 21:25:14]

Retried try job too often for step(s) content_browsertests

[lazyboy, 2012-09-28 23:35:46]

On 2012/09/28 21:25:14, I haz the power (commit-bot) wrote:
> Retried try job too often for step(s) content_browsertests

Fady, Hopefully this is the last round of OS specific fixes:
Fixed mac renderer browsertests: Use mock transport dibs in tests.
Will be checking other platforms before I send it to the queue again.
Thanks.

[Fady Samuel, 2012-09-28 23:46:48]

LGTM with nits.

http://codereview.chromium.org/10965048/diff/9023/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10965048/diff/9023/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:465: if (damage_buffer_) {
No need for this guard if you already have the check.

http://codereview.chromium.org/10965048/diff/9023/content/renderer/browser_pl...
File content/renderer/browser_plugin/mock_browser_plugin.cc (right):

http://codereview.chromium.org/10965048/diff/9023/content/renderer/browser_pl...
content/renderer/browser_plugin/mock_browser_plugin.cc:26: if (damage_buffer_) {
Please replace this guard with a DCHECK

[lazyboy, 2012-09-29 00:50:06]

Thanks for checking during late evening!

https://chromiumcodereview.appspot.com/10965048/diff/9023/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/9023/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:465: if (damage_buffer_) {
On 2012/09/28 23:46:48, Fady Samuel wrote:
> No need for this guard if you already have the check. 

Done. Thanks.

https://chromiumcodereview.appspot.com/10965048/diff/9023/content/renderer/br...
File content/renderer/browser_plugin/mock_browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10965048/diff/9023/content/renderer/br...
content/renderer/browser_plugin/mock_browser_plugin.cc:26: if (damage_buffer_) {
On 2012/09/28 23:46:48, Fady Samuel wrote:
> Please replace this guard with a DCHECK

Done.

[commit-bot: I haz the power, 2012-09-29 00:54:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10965048/26003

[commit-bot: I haz the power, 2012-09-29 14:14:12]

Change committed as 159404