Implement autosizing for <extensionoptions>

Implement autosizing for <extensionoptions>

This CL enables autosizing for extension options guests using
the built-in autosizing functionality in GuestViewBase.

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

[ericzeng, 2014-07-30 03:30:01]

PTAL, also I'm open to suggestions for how to properly test this feature. I'm
thinking that I can create an element with certain min/max sizes and check to
make sure that the actual size is within those bounds. However, I'm not sure how
running them inside a browser test might affect the layout and sizes.

[not at google - send to devlin, 2014-07-30 17:44:57]

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

https://codereview.chromium.org/431503002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:143:
const gfx::Size& new_size) {
indentation

https://codereview.chromium.org/431503002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:146:
args->SetInteger(extensionoptions::kHeight, new_size.height());
better:

OnSizeChanged event;
event.width = new_size.width();
event.height = new_size.height();
DispatchEventToEmbedder(new GuestViewBase::Event(
    OnSizeChanged::kEventName, event.ToValue()));

or thereabouts.

https://codereview.chromium.org/431503002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:148:
extensions::api::extension_options_internal::OnSizeChanged::kEventName,
you might want to "using extensions::api::extension_options_internal" at the top
to pull all of these in.

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

https://codereview.chromium.org/431503002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.h:44:
virtual void SizeChanged(const gfx::Size& old_size,
indentation

https://codereview.chromium.org/431503002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/431503002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:368:
"extensionOptionsInternal.onSizeChanged": {
you shouldn't need to specify this? it should be covered by the
extensionOptionsInternal rule.

https://codereview.chromium.org/431503002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_options_internal.idl (right):

https://codereview.chromium.org/431503002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/extension_options_internal.idl:9: static void
onSizeChanged();
should this have a width and height argument?

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:12: var
EXTENSION_OPTIONS_ATTRIBUTE_MAXHEIGHT = 'maxheight';
just inline each of these?

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:18:
EXTENSION_OPTIONS_ATTRIBUTE_MAXHEIGHT,
can this be a map of name --> default value (potentially null)? then you could
use that below rather than the special logic you apply to
autosize/minwidth/minheight.

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:59: attributeName,
this.extensionoptionsNode.getAttribute(attributeName));
indentation += 2

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:172: } else {
no else after return.

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:196:
self.extensionoptionsNode.setAttribute('extension', value);
why this second case? seems like if 'extension' were in
EXTENSION_OPTIONS_ATTRIBUTES this would be covered.

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/extension_options_events.js (right):

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options_events.js:12: var
SizeChangedEvent = CreateEvent('extensionOptionsInternal.onSizeChanged');
why can't this be in the EXTENSION_OPTIONS_EVENTS list?

[ericzeng, 2014-07-30 23:29:26]

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

https://codereview.chromium.org/431503002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:143:
const gfx::Size& new_size) {
On 2014/07/30 17:44:56, kalman wrote:
> indentation

Done.

https://codereview.chromium.org/431503002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:146:
args->SetInteger(extensionoptions::kHeight, new_size.height());
On 2014/07/30 17:44:57, kalman wrote:
> better:
> 
> OnSizeChanged event;
> event.width = new_size.width();
> event.height = new_size.height();
> DispatchEventToEmbedder(new GuestViewBase::Event(
>     OnSizeChanged::kEventName, event.ToValue()));
> 
> or thereabouts.

GuestViewBase would need some refactoring in order to make this change.
OnSizeChanged::Create returns a base::ListValue, but GuestViewBase::Event
requires a base::DictionaryValue in the constructor.

https://codereview.chromium.org/431503002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:148:
extensions::api::extension_options_internal::OnSizeChanged::kEventName,
On 2014/07/30 17:44:56, kalman wrote:
> you might want to "using extensions::api::extension_options_internal" at the
top
> to pull all of these in.

Done.

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

https://codereview.chromium.org/431503002/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.h:44:
virtual void SizeChanged(const gfx::Size& old_size,
On 2014/07/30 17:44:57, kalman wrote:
> indentation

Done.

https://codereview.chromium.org/431503002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/431503002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:368:
"extensionOptionsInternal.onSizeChanged": {
On 2014/07/30 17:44:57, kalman wrote:
> you shouldn't need to specify this? it should be covered by the
> extensionOptionsInternal rule.

Forgot to remove it after rebasing

https://codereview.chromium.org/431503002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_options_internal.idl (right):

https://codereview.chromium.org/431503002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/extension_options_internal.idl:9: static void
onSizeChanged();
On 2014/07/30 17:44:57, kalman wrote:
> should this have a width and height argument?

Done.

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:18:
EXTENSION_OPTIONS_ATTRIBUTE_MAXHEIGHT,
On 2014/07/30 17:44:57, kalman wrote:
> can this be a map of name --> default value (potentially null)? then you could
> use that below rather than the special logic you apply to
> autosize/minwidth/minheight.

Done.

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:59: attributeName,
this.extensionoptionsNode.getAttribute(attributeName));
On 2014/07/30 17:44:57, kalman wrote:
> indentation += 2

Done.

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:172: } else {
On 2014/07/30 17:44:57, kalman wrote:
> no else after return.

Done.

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options.js:196:
self.extensionoptionsNode.setAttribute('extension', value);
On 2014/07/30 17:44:57, kalman wrote:
> why this second case? seems like if 'extension' were in
> EXTENSION_OPTIONS_ATTRIBUTES this would be covered.

If an attribute is in EXTENSION_OPTIONS_ATTRIBUTES the attribute is propagated
to the browser plugin. I was thinking that |extension| shouldn't be included
since the browser plugin doesn't use that attribute.

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/extension_options_events.js (right):

https://codereview.chromium.org/431503002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/extension_options_events.js:12: var
SizeChangedEvent = CreateEvent('extensionOptionsInternal.onSizeChanged');
On 2014/07/30 17:44:57, kalman wrote:
> why can't this be in the EXTENSION_OPTIONS_EVENTS list?

EXTENSION_OPTIONS_EVENTS is propagated up to the DOM. I originally thought that
sizechanged wouldn't need to go that far, but it looks like I'll need it for the
WebUI integration.

[not at google - send to devlin, 2014-07-31 00:26:06]

https://codereview.chromium.org/431503002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/431503002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:368:
"extensionOptionsInternal.onSizeChanged": {
did you forget to remove it?

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

https://codereview.chromium.org/431503002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:50: }
better than using "var .. in" and then checking hasOwnProperty is to use
Object.keys(..).forEach.

better still is using utils.forEach:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

like

var utils = require('utils');
...
utils.forEach(EXTENSION_OPTIONS_ATTRIBUTES, function(name, defaultValue) {
  ...
});

https://codereview.chromium.org/431503002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:57: attributeName,
EXTENSION_OPTIONS_ATTRIBUTES.attributeName);
(you'd need EXTENSION_OPTIONS_ATTRIBUTES[attributeName] here...)

https://codereview.chromium.org/431503002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:61:
$Array.forEach(EXTENSION_OPTIONS_ATTRIBUTES, function(attributeName) {
does $Array.forEach work on objects like this?

https://codereview.chromium.org/431503002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:174:
$Array.forEach(EXTENSION_OPTIONS_ATTRIBUTES, function(attributeName) {
likewise

[ericzeng, 2014-07-31 18:29:57]

https://codereview.chromium.org/431503002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/431503002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:368:
"extensionOptionsInternal.onSizeChanged": {
On 2014/07/31 00:26:06, kalman wrote:
> did you forget to remove it?

yes

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

https://codereview.chromium.org/431503002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:50: }
On 2014/07/31 00:26:06, kalman wrote:
> better than using "var .. in" and then checking hasOwnProperty is to use
> Object.keys(..).forEach.
> 
> better still is using utils.forEach:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> 
> like
> 
> var utils = require('utils');
> ...
> utils.forEach(EXTENSION_OPTIONS_ATTRIBUTES, function(name, defaultValue) {
>   ...
> });

Done.

https://codereview.chromium.org/431503002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:57: attributeName,
EXTENSION_OPTIONS_ATTRIBUTES.attributeName);
On 2014/07/31 00:26:06, kalman wrote:
> (you'd need EXTENSION_OPTIONS_ATTRIBUTES[attributeName] here...)

Done.

https://codereview.chromium.org/431503002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:61:
$Array.forEach(EXTENSION_OPTIONS_ATTRIBUTES, function(attributeName) {
On 2014/07/31 00:26:06, kalman wrote:
> does $Array.forEach work on objects like this?

I forgot to delete this block of code, the loop above has the same functionality
(and more). But it looks like $Array.forEach does work. Which should I use?

[not at google - send to devlin, 2014-07-31 19:46:47]

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

https://codereview.chromium.org/431503002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:61:
$Array.forEach(EXTENSION_OPTIONS_ATTRIBUTES, function(attributeName) {
On 2014/07/31 18:29:57, Eric Zeng wrote:
> On 2014/07/31 00:26:06, kalman wrote:
> > does $Array.forEach work on objects like this?
> 
> I forgot to delete this block of code, the loop above has the same
functionality
> (and more). But it looks like $Array.forEach does work. Which should I use?

forEach doesn't work for objects, and EXTENSION_OPTIONS_ATTRIBUTES is an object.

[not at google - send to devlin, 2014-07-31 20:26:00]

lgtm, but wait for fady

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

https://codereview.chromium.org/431503002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:54: attributeName,
EXTENSION_OPTIONS_ATTRIBUTES[attributeName]);
use |value| not the whole "EXTENSION_OPTIONS_ATTRIBUTES[attributeName]"
expression.

[Fady Samuel, 2014-07-31 20:28:18]

I'm working on a big autosize refactor currently. Please hold off on this for a
few hours.

[Fady Samuel, 2014-08-05 22:09:40]

guest_view lgtm but my autosize patch still hasn't landed (in CQ).

[ericzeng, 2014-08-06 23:41:21]

I added some new functionality and tests that probably should
be reviewed. Now if the embedder tries to specify invalid
size constraints, the size gets reset to the defaults. The
test just makes sure that the new size is within the 
constraints. Since the actual autosizing is implemented in
the GuestViewBase, I think my tests just need to cover the
behavior from the embedder's side.

[not at google - send to devlin, 2014-08-06 23:54:49]

lgtm

[ericzeng, 2014-08-07 00:03:21]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-07 00:06:38]

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

[commit-bot: I haz the power, 2014-08-07 07:37:44]

Change committed as 287993

[Fady Samuel, 2014-08-08 19:18:15]

https://codereview.chromium.org/431503002/diff/100001/chrome/browser/guest_vi...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/431503002/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:160: bool
auto_size_enabled;
Initialize this.