Addition of 'maxSize' to experimental popup extension API.

CL adding a 'maxSize' parameter to the experimental popup extension API.  

Callers can use this optional argument to allow for popups larger than than the default 800x600 size.

Note that the implementation makes minimal changes to the existing popup code, and relies on use of the ExtensionPopupObserver to install the max size settings.

TEST=ExtensionApiTest.Popup
BUG=none


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

[Jeff Timanus, 2010-11-25 06:43:26]

Please have a look, Joi (and Travis, if you're curious).

There is nothing really out of the ordinary here - I went out of my way to
minimize the exposure of this change to the current behaviour of the popup
outside use of the extension API.

[Jói, 2010-11-25 13:50:30]

Overall looks good, I have a couple of questions.

A couple of the bots seem to be failing on the same UI test with this change,
might be related.

http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_popup_api.cc:112: const gfx::Size&
max_popup_size)
would be good to document that setting a dimension in max_popup_size to 0 makes
the popup host use the default maximum.

http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_popup_api.cc:432:
EXTENSION_FUNCTION_VALIDATE(max_size->GetInteger(kWidthKey, &max_width));
Should perhaps validate (here and below) that the width/height are positive, and
within the minimum/maximum bounds described in the API definition (or hmm, I
guess that's already done somewhere based on the definition - ignore this
comment :)

http://codereview.chromium.org/5254007/diff/5001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/5254007/diff/5001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:2355: "maximum": 1024
Do we want to specify a maximum at all? Perhaps the implicit maximum should just
be "as much as fits on the screen"?

Same for the maximum specified for the 'height' parameter.

http://codereview.chromium.org/5254007/diff/5001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/5254007/diff/5001/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:522: type: "integer",
optional: true, minimum: 32, maximum: 1024
same comment as before, do we want a maximum at all?

[Jeff Timanus, 2010-11-25 23:35:29]

I double checked the flakiness dashboard, and the WindowOpen test has been
failing previously, without the presence of my changes.

http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_popup_api.cc:112: const gfx::Size&
max_popup_size)
On 2010/11/25 13:50:30, Jói wrote:
> would be good to document that setting a dimension in max_popup_size to 0
makes
> the popup host use the default maximum.
Good point.  Done.

http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_popup_api.cc:432:
EXTENSION_FUNCTION_VALIDATE(max_size->GetInteger(kWidthKey, &max_width));
On 2010/11/25 13:50:30, Jói wrote:
> Should perhaps validate (here and below) that the width/height are positive,
and
> within the minimum/maximum bounds described in the API definition (or hmm, I
> guess that's already done somewhere based on the definition - ignore this
> comment :)

Yup!  I resisted the urge to double validate - I recalled that all parameters
are validated on the JS side, before the message is sent to the extension
function dispatcher.

http://codereview.chromium.org/5254007/diff/5001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/5254007/diff/5001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:2355: "maximum": 1024
On 2010/11/25 13:50:30, Jói wrote:
> Do we want to specify a maximum at all? Perhaps the implicit maximum should
just
> be "as much as fits on the screen"?
> 
> Same for the maximum specified for the 'height' parameter.

I chose this maximum value arbitrarily, after pondering what you mentioned. 
I'll change the code to enforce that the also limit its bounds by the resolution
of the monitor in which the anchor point resides.

http://codereview.chromium.org/5254007/diff/5001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/5254007/diff/5001/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:522: type: "integer",
optional: true, minimum: 32, maximum: 1024
On 2010/11/25 13:50:30, Jói wrote:
> same comment as before, do we want a maximum at all?

Done.

http://codereview.chromium.org/5254007/diff/5001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/popup/popup_main/dom_ui.html (right):

http://codereview.chromium.org/5254007/diff/5001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/popup/popup_main/dom_ui.html:72: function
showNoFocusShift() {
Oops.  Inserted an extra space here.   Corrected.

[Jói, 2010-11-25 23:43:49]

I still think you should just remove the hardcoded maximum, or make it
very large (4K x 4K?), then clip to the monitor.

On Thu, Nov 25, 2010 at 6:35 PM,  <twiz@chromium.org> wrote:
> I double checked the flakiness dashboard, and the WindowOpen test has been
> failing previously, without the presence of my changes.
>
>
>
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_popup_api.cc (right):
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_popup_api.cc:112: const gfx::Size&
> max_popup_size)
> On 2010/11/25 13:50:30, Jói wrote:
>>
>> would be good to document that setting a dimension in max_popup_size
>
> to 0 makes
>>
>> the popup host use the default maximum.
>
> Good point.  Done.
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_popup_api.cc:432:
> EXTENSION_FUNCTION_VALIDATE(max_size->GetInteger(kWidthKey,
> &max_width));
> On 2010/11/25 13:50:30, Jói wrote:
>>
>> Should perhaps validate (here and below) that the width/height are
>
> positive, and
>>
>> within the minimum/maximum bounds described in the API definition (or
>
> hmm, I
>>
>> guess that's already done somewhere based on the definition - ignore
>
> this
>>
>> comment :)
>
> Yup!  I resisted the urge to double validate - I recalled that all
> parameters are validated on the JS side, before the message is sent to
> the extension function dispatcher.
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/common/extensions/api...
> File chrome/common/extensions/api/extension_api.json (right):
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/common/extensions/api...
> chrome/common/extensions/api/extension_api.json:2355: "maximum": 1024
> On 2010/11/25 13:50:30, Jói wrote:
>>
>> Do we want to specify a maximum at all? Perhaps the implicit maximum
>
> should just
>>
>> be "as much as fits on the screen"?
>
>> Same for the maximum specified for the 'height' parameter.
>
> I chose this maximum value arbitrarily, after pondering what you
> mentioned.  I'll change the code to enforce that the also limit its
> bounds by the resolution of the monitor in which the anchor point
> resides.
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/renderer/resources/ex...
> File chrome/renderer/resources/extension_process_bindings.js (right):
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/renderer/resources/ex...
> chrome/renderer/resources/extension_process_bindings.js:522: type:
> "integer", optional: true, minimum: 32, maximum: 1024
> On 2010/11/25 13:50:30, Jói wrote:
>>
>> same comment as before, do we want a maximum at all?
>
> Done.
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/test/data/extensions/...
> File chrome/test/data/extensions/api_test/popup/popup_main/dom_ui.html
> (right):
>
>
http://codereview.chromium.org/5254007/diff/5001/chrome/test/data/extensions/...
> chrome/test/data/extensions/api_test/popup/popup_main/dom_ui.html:72:
> function showNoFocusShift() {
> Oops.  Inserted an extra space here.   Corrected.
>
> http://codereview.chromium.org/5254007/
>

[Jói, 2010-11-26 19:01:05]

LGTM

one tiny optional nit:

http://codereview.chromium.org/5254007/diff/20001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/popup/popup_main/dom_ui_popup_sizing.html
(right):

http://codereview.chromium.org/5254007/diff/20001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/popup/popup_main/dom_ui_popup_sizing.html:98:
// TODO: Fix the order of the onload and layout processing.
should this be a TODO(twiz@chromium.org) ?

[Jeff Timanus, 2010-11-26 19:05:49]

http://codereview.chromium.org/5254007/diff/20001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/popup/popup_main/dom_ui_popup_sizing.html
(right):

http://codereview.chromium.org/5254007/diff/20001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/popup/popup_main/dom_ui_popup_sizing.html:98:
// TODO: Fix the order of the onload and layout processing.
On 2010/11/26 19:01:05, Jói wrote:
> should this be a mailto:TODO(twiz@chromium.org) ?

May as well be.  I don't know that I'll be the one to make this change, however.
 It is due to the complicated interaction of webkit and the Views Window
implementation.