Show error for invalid url in chrome.app.window.create()

Show error for invalid url in chrome.app.window.create()

Show error when an http:// URL is passed as parameter
to chrome.app.window.create().

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

[Nikhil, 2014-06-17 14:53:23]

PTAL. Thanks!

Documentation doesn't say anything on constraints for URL passed in parameter so
I'm checking only for scheme.

If you've any review comments then please let me know. I'll incorporate them.

[benwells, 2014-06-18 03:22:18]

https://codereview.chromium.org/343433005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/343433005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/app_window/app_window_api.cc:163:
WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_WARNING,
Errors from APIs are generally communicated via chrome.runtime.lastError, which
API users should be checking for. E.g. see below where the id is too long, which
is a similar case.

[Nikhil, 2014-06-18 07:52:20]

On 2014/06/18 03:22:18, benwells wrote:
>
https://codereview.chromium.org/343433005/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/app_window/app_window_api.cc (right):
> 
>
https://codereview.chromium.org/343433005/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/app_window/app_window_api.cc:163:
> WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_WARNING,
> Errors from APIs are generally communicated via chrome.runtime.lastError,
which
> API users should be checking for. E.g. see below where the id is too long,
which
> is a similar case.

Thanks for reviewing :)

I've one query regarding expected behavior that whether we should log a warning
or consider this as an error case.

* In the bug description, it says that we should display warning for invalid URL
and so, I used WriteToConsole(). This logs a warning and allows window to get
created. But URL isn't loaded. WriteToConsole() is also used to log a warning
when 'singleton' option is passed in parameters.

* If I return an error similar to the case where id is too long then window is
not created and an error is logged. The documentation isn't clear on constraints
for URL passed in parameter.

Please let me know if you think we should consider this as an error scenario.
I'll update the bug description and prepare the patch to make suggested change.

[benwells, 2014-06-18 08:31:04]

On 2014/06/18 07:52:20, Nikhil wrote:
> On 2014/06/18 03:22:18, benwells wrote:
> >
>
https://codereview.chromium.org/343433005/diff/1/chrome/browser/extensions/ap...
> > File chrome/browser/extensions/api/app_window/app_window_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/343433005/diff/1/chrome/browser/extensions/ap...
> > chrome/browser/extensions/api/app_window/app_window_api.cc:163:
> > WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_WARNING,
> > Errors from APIs are generally communicated via chrome.runtime.lastError,
> which
> > API users should be checking for. E.g. see below where the id is too long,
> which
> > is a similar case.
> 
> Thanks for reviewing :)
> 

Thanks for contributing :)

> I've one query regarding expected behavior that whether we should log a
warning
> or consider this as an error case.
> 
> * In the bug description, it says that we should display warning for invalid
URL
> and so, I used WriteToConsole(). This logs a warning and allows window to get
> created. But URL isn't loaded. WriteToConsole() is also used to log a warning
> when 'singleton' option is passed in parameters.
> 
> * If I return an error similar to the case where id is too long then window is
> not created and an error is logged. The documentation isn't clear on
constraints
> for URL passed in parameter.
> 
> Please let me know if you think we should consider this as an error scenario.
> I'll update the bug description and prepare the patch to make suggested
change.

Hmm I see. I think we should make it an error, unless someone can think of a
reason why showing the window anyway with dud contents is a good idea.

[Nikhil, 2014-06-18 08:52:39]

Made the changes to communicate error. PTAL. Thanks!

https://codereview.chromium.org/343433005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/343433005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/app_window/app_window_api.cc:163:
WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_WARNING,
On 2014/06/18 03:22:17, benwells wrote:
> Errors from APIs are generally communicated via chrome.runtime.lastError,
which
> API users should be checking for. E.g. see below where the id is too long,
which
> is a similar case.

Done.

[benwells, 2014-06-18 22:53:37]

https://codereview.chromium.org/343433005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/343433005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:164: if
(GURL(params->url).has_scheme()) {
Looking 10 lines or so up, this is considered fine for component apps. Your
change will break those apps. You should combine this code with the bit above
somehow.

[Nikhil, 2014-06-19 08:26:28]

Refactored code to combine both cases. PTAL. Thanks!

https://codereview.chromium.org/343433005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/343433005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:164: if
(GURL(params->url).has_scheme()) {
On 2014/06/18 22:53:37, benwells wrote:
> Looking 10 lines or so up, this is considered fine for component apps. Your
> change will break those apps. You should combine this code with the bit above
> somehow.

Done.

[benwells, 2014-06-19 08:40:32]

lgtm

https://codereview.chromium.org/343433005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/343433005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:56: "Url passed
should be local for security reasons.";
Nit: please rephrase as "The URL used for window creation must ..."

[Nikhil, 2014-06-19 08:55:58]

Rephrased error message. PTAL.

https://codereview.chromium.org/343433005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/343433005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:56: "Url passed
should be local for security reasons.";
On 2014/06/19 08:40:32, benwells wrote:
> Nit: please rephrase as "The URL used for window creation must ..."

Done.

[benwells, 2014-06-19 09:02:20]

On 2014/06/19 08:55:58, Nikhil wrote:
> Rephrased error message. PTAL.
> 
>
https://codereview.chromium.org/343433005/diff/40001/chrome/browser/extension...
> File chrome/browser/extensions/api/app_window/app_window_api.cc (right):
> 
>
https://codereview.chromium.org/343433005/diff/40001/chrome/browser/extension...
> chrome/browser/extensions/api/app_window/app_window_api.cc:56: "Url passed
> should be local for security reasons.";
> On 2014/06/19 08:40:32, benwells wrote:
> > Nit: please rephrase as "The URL used for window creation must ..."
> 
> Done.

Still lgtm

[Nikhil, 2014-06-19 09:05:48]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-06-19 09:07:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/343433005/60001

[commit-bot: I haz the power, 2014-06-19 15:32:31]

Change committed as 278389