Handle closing web contents in ExtensionOptionsGuest

Handle closing web contents in ExtensionOptionsGuest

Implement CloseContents for ExtensionOptionsGuest. CloseContents
propagates a 'close' event up to the <extensionoptions> node.
In WebUI, when the extension options overlay receives this event,
it will close the overlay.

Extension options pages that used window.close() to close their
tab in the old options page UI will now close their embedder
overlay.

BUG=386842, 386838
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291554

[ericzeng, 2014-08-20 00:53:33]

PTAL

[not at google - send to devlin, 2014-08-20 02:16:37]

Test that the event fires for the embedder?

https://codereview.chromium.org/490783002/diff/1/chrome/browser/guest_view/ex...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/490783002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:184:
extension_options_internal::OnClose::kEventName, args.Pass()));
Inline |args|? You can do

make_scoped_ptr(new base::DictionaryValue()).

[Fady Samuel, 2014-08-20 14:53:55]

https://codereview.chromium.org/490783002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/490783002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:50: var
extensionoptions = document.querySelector('extensionoptions');
This is weird to me. What if you have more than one extensionoptions on a page?

https://codereview.chromium.org/490783002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:68:
this.handleDismiss_();
The intent here is to remove the extensionoptions element when window.close is
called from the guest?

[not at google - send to devlin, 2014-08-20 14:56:48]

https://codereview.chromium.org/490783002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/490783002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:50: var
extensionoptions = document.querySelector('extensionoptions');
On 2014/08/20 14:53:55, Fady Samuel wrote:
> This is weird to me. What if you have more than one extensionoptions on a
page?

That's a good point. Should we be doing this querySelector-ing on the overlay,
not the document?

[ericzeng, 2014-08-20 17:26:01]

https://codereview.chromium.org/490783002/diff/1/chrome/browser/guest_view/ex...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/490783002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:184:
extension_options_internal::OnClose::kEventName, args.Pass()));
On 2014/08/20 02:16:37, kalman wrote:
> Inline |args|? You can do
> 
> make_scoped_ptr(new base::DictionaryValue()).

Done.

https://codereview.chromium.org/490783002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/490783002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:50: var
extensionoptions = document.querySelector('extensionoptions');
On 2014/08/20 14:56:48, kalman wrote:
> On 2014/08/20 14:53:55, Fady Samuel wrote:
> > This is weird to me. What if you have more than one extensionoptions on a
> page?
> 
> That's a good point. Should we be doing this querySelector-ing on the overlay,
> not the document?

I actually have a div that I should have nested the <extensionoptions> under.

https://codereview.chromium.org/490783002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:68:
this.handleDismiss_();
On 2014/08/20 14:53:55, Fady Samuel wrote:
> The intent here is to remove the extensionoptions element when window.close is
> called from the guest?

The higher level intent is to close the entire overlay when window.close is
called from the guest. Imagine if an extension options page in a tab had a
'save' button that saves the settings and then calls window.close() to close
itself. I want the same behavior for the overlay - when the extension options
page wants to close itself, its <extensionoptions> is removed and the overlay
closed.

[ericzeng, 2014-08-21 18:58:55]

ping, added a simple test to check if WebUI can see the onclose event from
<extensionoptions>.

[Fady Samuel, 2014-08-21 19:00:04]

guest_view lgtm

[not at google - send to devlin, 2014-08-21 19:05:18]

lgtm

https://codereview.chromium.org/490783002/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/webui/receives_extension_options_on_close.js
(right):

https://codereview.chromium.org/490783002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/webui/receives_extension_options_on_close.js:18: }
;

[ericzeng, 2014-08-21 19:07:53]

https://codereview.chromium.org/490783002/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/webui/receives_extension_options_on_close.js
(right):

https://codereview.chromium.org/490783002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/webui/receives_extension_options_on_close.js:18: }
On 2014/08/21 19:05:18, kalman wrote:
> ;

Done.

[ericzeng, 2014-08-21 19:10:32]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-21 19:12:29]

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

[commit-bot: I haz the power, 2014-08-21 23:45:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-21 23:50:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 23:50:59]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ericzeng, 2014-08-22 00:48:20]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-22 00:50:43]

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

[commit-bot: I haz the power, 2014-08-22 01:48:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 01:51:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 01:51:43]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[ericzeng, 2014-08-22 02:00:56]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-22 02:02:28]

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

[commit-bot: I haz the power, 2014-08-22 02:06:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 02:09:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 02:09:52]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[ericzeng, 2014-08-22 19:28:09]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-22 19:29:02]

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

[ericzeng, 2014-08-22 19:41:22]

The CQ bit was unchecked by ericzeng@chromium.org

[ericzeng, 2014-08-22 19:52:28]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-22 19:53:27]

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

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

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 21:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 21:12:00]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[ericzeng, 2014-08-22 21:56:53]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-22 21:57:57]

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

[commit-bot: I haz the power, 2014-08-22 23:32:42]

Committed patchset #10 (180001) as 291554