Implement <extensionoptions> deferred attach logic

Implement <extensionoptions> deferred attach logic

Due to how the browser plugin is created, it is possible for the
<extensionoptions> element to attempt to attach to the embedder before
the browser plugin has bindings. This occurs when an ExtensionOptions object
gets an extension id and creates a guest, but has not yet been added
to the DOM. Use the deferred attach logic from <webview> so that if this
case occurs, wait until the browser plugin gets its bindings before
trying to attach the guest view.

BUG=386838
Committed: https://crrev.com/617e4fc4f9be2bb575d1a5161272f13abe625788
Cr-Commit-Position: refs/heads/master@{#292215}

[ericzeng, 2014-08-12 19:39:04]

PTAL

[not at google - send to devlin, 2014-08-12 19:58:29]

lgtm

https://codereview.chromium.org/457653003/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/457653003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:140: if (name ==
'internalbindings' && !oldValue && newValue) {
I guess that 'internalbindings' is some magical thing?

https://codereview.chromium.org/457653003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:151: }
Empty else block?

https://codereview.chromium.org/457653003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:161: return 'function'
== typeof this.browserPluginNode['-internal-attach'];
No yoda condition preferably.

Though I find this hasBindings() method kind of weird. It's only used in places
where -internal-attach is referenced right afterwards. Either check for
-internal-attach explicitly at the call sites, or have a
"getBrowserPluginBindings" method which returns null if they don't exist (or...
if it's not a function, I suppose).

And *really* this should be a method like initializeBindingsIfReady() which does
all this stuff by itself.

I guess webview is already like this, though, and I don't really want to touch
that.

https://codereview.chromium.org/457653003/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js
(right):

https://codereview.chromium.org/457653003/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:63: },
1000);
Why 1000? Just make it 0, no need to make this test run 1s longer than it needs
to.

[Fady Samuel, 2014-08-12 21:28:00]

Could you hold off a day or so for this? I'm in the process of cleaning up the
attachment code: https://codereview.chromium.org/444813002/

[Fady Samuel, 2014-08-12 21:36:39]

https://codereview.chromium.org/457653003/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/457653003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:140: if (name ==
'internalbindings' && !oldValue && newValue) {
On 2014/08/12 19:58:29, kalman wrote:
> I guess that 'internalbindings' is some magical thing?

internalbindings is a browserplugin attribute that gets added when BrowserPlugin
is created.

[ericzeng, 2014-08-12 21:53:33]

https://codereview.chromium.org/457653003/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/457653003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:161: return 'function'
== typeof this.browserPluginNode['-internal-attach'];
On 2014/08/12 19:58:29, kalman wrote:
> No yoda condition preferably.
> 
> Though I find this hasBindings() method kind of weird. It's only used in
places
> where -internal-attach is referenced right afterwards. Either check for
> -internal-attach explicitly at the call sites, or have a
> "getBrowserPluginBindings" method which returns null if they don't exist
(or...
> if it's not a function, I suppose).
> 
> And *really* this should be a method like initializeBindingsIfReady() which
does
> all this stuff by itself.
> 
> I guess webview is already like this, though, and I don't really want to touch
> that.

I'm not very happy with the whole flow of the script at this point, here's a
refactor with intializeBindingsIfReady(). Although it looks like this will be
moot because of Fady's browser plugin refactor.

https://codereview.chromium.org/457653003/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/extension_options/embed_self/test.js
(right):

https://codereview.chromium.org/457653003/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_self/test.js:63: },
1000);
On 2014/08/12 19:58:29, kalman wrote:
> Why 1000? Just make it 0, no need to make this test run 1s longer than it
needs
> to.

Unfortunately I need a small delay in order to reproduce the bug. If appendChild
is run immediately after the extension id is set it looks like the browser
plugin can be initialized in time for extensionoptions to call attach. I can
shorten it but the delay needs to be there to prevent the race from occurring.

[ericzeng, 2014-08-12 21:56:16]

On 2014/08/12 21:28:00, Fady Samuel wrote:
> Could you hold off a day or so for this? I'm in the process of cleaning up the
> attachment code: https://codereview.chromium.org/444813002/

Oops, didn't know you were working on this right now. Will your CL fix the race
between when the guest view tries to attach and when the browser plugin is
ready?

[Fady Samuel, 2014-08-25 19:09:49]

The internal-attach change has landed. Please model your change here after this:

https://codereview.chromium.org/440463002/

[ericzeng, 2014-08-26 22:36:06]

PTAL

Fady - this patch doesn't specifically address the "display:none" issue, it
mainly addresses the problem where it tries to create a guest before the browser
plugin was ready. I'm not sure if reattachment is worth addressing if it loses
all of the autosize dimensions from before, because that is currently my only
use case for reattachment.

[Fady Samuel, 2014-08-26 22:39:17]

lgtm

[ericzeng, 2014-08-27 19:31:34]

Ben, is it ok if I remove the previous tests? This CL is more of a refactor of
how the element is created, since the original bug doesn't really exist after
one of Fady's refactors.

[not at google - send to devlin, 2014-08-27 19:33:48]

Sure.

and still lgtm, this is more Fady's domain than mine anyway.

[ericzeng, 2014-08-27 19:36:54]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-27 19:37:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ericzeng@chromium.org/457653003/120001

[commit-bot: I haz the power, 2014-08-27 20:53:07]

Committed patchset #7 (id:120001) as e05b2063247d32fa7cb4a02fbd3f2cad26370e58

[commit-bot: I haz the power, 2014-09-10 02:53:47]

Patchset 7 (id:??) landed as
https://crrev.com/617e4fc4f9be2bb575d1a5161272f13abe625788
Cr-Commit-Position: refs/heads/master@{#292215}