<webview>: resizing with display:none set should resize on attachment

<webview>: resizing with display:none set should resize on attachment

BUG=330264, 419020
TEST=WebViewTest.Shim_TestResizeWebviewWithDisplayNoneResizesContent
TBR=creis@chromium.org for minor browser_plugin_delegate.h change.

Committed: https://crrev.com/e7c9f7781cbc6db099dacbefdc19ac2fd9bfc19a
Cr-Commit-Position: refs/heads/master@{#298133}

[Fady Samuel, 2014-10-03 19:14:10]

fsamuel@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lazyboy, 2014-10-03 20:30:05]

https://codereview.chromium.org/624063002/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/624063002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1588:
window.console.log('The resize test has been injected into webview.');
if (!results || !results.length) embedder.test.fail();

https://codereview.chromium.org/624063002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1594:
window.console.log('The guest script for a two-way comm channel has ' +
Same here.

https://codereview.chromium.org/624063002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1609:
webview.style.width = '400px';
From what i've seen this probably is race-y, you're not guaranteed to be in a
display:none state here.
Or this might not be an issue.

One way to check would be to set some flag where you set it to block and make
sure you see a 'resize' response after that fact.

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.cc (right):

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:77:
pending_requests_.push_back(request);
Can you add a TODO here as we discussed: about queues with > 1 element wouldn't
be necessary to maintain.

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:160: return;
Couldn't there be more stuff in the queue to be processed?

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:189:
linked_ptr<AttachRequest> pending_request = pending_requests_.front();
Can this picking from queue and putting it to pending_response_ be in one place?

e.g.
PutTask(request) { pending_requests_.insert(request); }
PickTask() {
  if (!ready || pending_response_ || pending_requests.empty()) return;
  AttachGuestInternal(pending_requests_.pop_front()); // ya that's void, but you
get the point
}

So ::Ready() would just call PickTask();
OnGuestAttached() would call PickTask() after it cleared pending_response_
AttachGuest() would call PutTask(); PickTask();

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.h (right):

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:22: class AttachRequest {
Add a description on this since this is public.

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:39: bool has_callback()
const { return !callback_.IsEmpty(); }
HasCallback() and define it in .cc file.

[Fady Samuel, 2014-10-03 21:52:31]

PTAL

https://codereview.chromium.org/624063002/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/624063002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1588:
window.console.log('The resize test has been injected into webview.');
On 2014/10/03 20:30:05, lazyboy wrote:
> if (!results || !results.length) embedder.test.fail();

Done.

https://codereview.chromium.org/624063002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1594:
window.console.log('The guest script for a two-way comm channel has ' +
On 2014/10/03 20:30:05, lazyboy wrote:
> Same here.

Done.

https://codereview.chromium.org/624063002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1609:
webview.style.width = '400px';
On 2014/10/03 20:30:05, lazyboy wrote:
> From what i've seen this probably is race-y, you're not guaranteed to be in a
> display:none state here.
> Or this might not be an issue.
> 
> One way to check would be to set some flag where you set it to block and make
> sure you see a 'resize' response after that fact.

I don't know the full flow of code so I'm not 100% sure it's not racy but this
is all happening in one thread. I have verified that the AttachGuest gets added
to the queue.

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.cc (right):

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:77:
pending_requests_.push_back(request);
On 2014/10/03 20:30:05, lazyboy wrote:
> Can you add a TODO here as we discussed: about queues with > 1 element
wouldn't
> be necessary to maintain.

Done.

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:160: return;
On 2014/10/03 20:30:05, lazyboy wrote:
> Couldn't there be more stuff in the queue to be processed?

Good catch!

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:189:
linked_ptr<AttachRequest> pending_request = pending_requests_.front();
On 2014/10/03 20:30:05, lazyboy wrote:
> Can this picking from queue and putting it to pending_response_ be in one
place?
> 
> e.g.
> PutTask(request) { pending_requests_.insert(request); }
> PickTask() {
>   if (!ready || pending_response_ || pending_requests.empty()) return;
>   AttachGuestInternal(pending_requests_.pop_front()); // ya that's void, but
you
> get the point
> }
> 
> So ::Ready() would just call PickTask();
> OnGuestAttached() would call PickTask() after it cleared pending_response_
> AttachGuest() would call PutTask(); PickTask();

Done. I gave them slightly more descriptive names.

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.h (right):

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:22: class AttachRequest {
On 2014/10/03 20:30:05, lazyboy wrote:
> Add a description on this since this is public.

Done.

https://codereview.chromium.org/624063002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:39: bool has_callback()
const { return !callback_.IsEmpty(); }
On 2014/10/03 20:30:05, lazyboy wrote:
> HasCallback() and define it in .cc file.

Done.

[lazyboy, 2014-10-03 22:10:19]

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.cc (right):

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:42: 
nit: remove empty line.

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:193:
pending_response_.reset();
The repetitive pattern (4 times) suggests we would want to add a
OnGuestAttachedInternal that does the v8 stuff and OnGuestAttached would call
that and then pending_reponse_.reset() with PerformPendingAttachRequest();

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.h (right):

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:67: virtual void
SetElementInstanceID(int element_instance_id) OVERRIDE;
I'm not sure why OVERRIDEs didn't change to override here but on the block
below.

[Fady Samuel, 2014-10-03 22:12:30]

A bit more cleanup. PTAL.

[Fady Samuel, 2014-10-03 22:23:59]

PTAL

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.cc (right):

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:42: 
On 2014/10/03 22:10:19, lazyboy wrote:
> nit: remove empty line.

Done.

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:193:
pending_response_.reset();
On 2014/10/03 22:10:19, lazyboy wrote:
> The repetitive pattern (4 times) suggests we would want to add a
> OnGuestAttachedInternal that does the v8 stuff and OnGuestAttached would call
> that and then pending_reponse_.reset() with PerformPendingAttachRequest();

Done.

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.h (right):

https://codereview.chromium.org/624063002/diff/40001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:67: virtual void
SetElementInstanceID(int element_instance_id) OVERRIDE;
On 2014/10/03 22:10:19, lazyboy wrote:
> I'm not sure why OVERRIDEs didn't change to override here but on the block
> below.

Done.

[lazyboy, 2014-10-03 22:32:35]

lgtm

[Fady Samuel, 2014-10-03 22:38:44]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-10-03 22:39:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/624063002/70001

[commit-bot: I haz the power, 2014-10-04 00:31:13]

Committed patchset #5 (id:70001) as c76e68a9e7948b73f1f7bedf77ff49ff8c781189

[commit-bot: I haz the power, 2014-10-04 00:32:24]

Patchset 5 (id:??) landed as
https://crrev.com/e7c9f7781cbc6db099dacbefdc19ac2fd9bfc19a
Cr-Commit-Position: refs/heads/master@{#298133}