Implement web app definition parsing.

Implement web app definition parsing.

This required moving some code from webkit/glue to
chrome/common/web_apps.cc so that it could rely on
chrome/common/json_schema_validator.h.

BUG=49233
TEST=Convered by unit tests.

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

[Aaron Boodman, 2010-11-15 00:38:56]

The only real change here is the addition of ParseWebAppFromDefinitionFile().
The rest is just moving code and adjusting call istes.

[Paweł Hajdan Jr., 2010-11-15 08:05:31]

Drive-by with a minor test comment. No need to wait for another review by me.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps_unitt...
File chrome/common/web_apps_unittest.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps_unitt...
chrome/common/web_apps_unittest.cc:22: PathService::Get(chrome::DIR_TEST_DATA,
&path);
Check return value of this.

[Erik does not do reviews, 2010-11-15 19:45:36]

I didn't get through the test code yet, but thought this was useful until I
finish.

http://codereview.chromium.org/4979003/diff/4001/chrome/browser/extensions/co...
File chrome/browser/extensions/convert_web_app.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/browser/extensions/co...
chrome/browser/extensions/convert_web_app.cc:120: // Add the permissions. In web
apps, these are called 'privileges'.
I'm not sure we want to make this change just yet.  In Mozilla's stuff, they
used two different terms ("capabilities" and "privileges"), so it looked like
they hadn't made up their minds.  It seems worthwhile to chat with them before
we change something so we don't have to just go and change it again.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc
File chrome/common/web_apps.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:46: return 0;
the header indicates a size < 0 if it's invalid

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:47: // Make sure all chars are from 0-9.
nit: newline above comment

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:55: return output;
do we want a min/max value for icon sizes?

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:76: icon_sizes.size() != 1) {
so this means we're explicitly not allowing things like .ico files with multiple
sizes?

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:134: // If is_any is true, it must occur by itself.
Without displaying error messages to the developer, these errors may wind up
being hard to debug.  Maybe this function should take an error out param?

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:251: CHECK(privileges_value->GetString(i,
&privilege));
isn't this covered by the schema validation?

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:263: CHECK(urls_value->GetString(i, &url_string));
again, is this redundant?

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:283: if (!base::StringToInt(*iter, &size) || size < 0
|| size > 128)
128 seems too small as a max.  512 is pretty standard as a large size (e.g.
apple and win7).  Also, it looks like this restriction isn't put on the link
rel=icon tag.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h
File chrome/common/web_apps.h (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h#new...
chrome/common/web_apps.h:75: gfx::Size ParseIconSize(const string16& text);
It seems odd for all of these functions to be naked like this.  typically,
they'd be inside of a namespace or static inside of a class.  Can't we just make
the struct above into a class?

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h#new...
chrome/common/web_apps.h:88: // function returns true.
it seems like there needs to be some way to detect this last case (no web app
present) from the caller.  maybe a bool in the struct?

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps_unitt...
File chrome/common/web_apps_unittest.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps_unitt...
chrome/common/web_apps_unittest.cc:25: ADD_FAILURE() << L"Path does not exist: "
<< path.ToWStringHack();
I think you can just use value() instead of ToWStringHack because of the
overloads here.

http://codereview.chromium.org/4979003/diff/4001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:3918:
ParseWebAppFromWebDocument(webview()->mainFrame(), &app_info, &error);
Since this can fail, don't we need to punt if it returns false and error is set?
 Is there going to be a way to pass error up to the browser?

http://codereview.chromium.org/4979003/diff/4001/chrome/test/data/web_app_inf...
File chrome/test/data/web_app_info/full.json (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/test/data/web_app_inf...
chrome/test/data/web_app_info/full.json:11: // We also ignore this because it is
larger than 128.
I thought our JSON parser barfed on comments

[Aaron Boodman, 2010-11-15 19:52:34]

quick meta-repy.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc
File chrome/common/web_apps.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:46: return 0;
On 2010/11/15 19:45:36, Erik Kay wrote:
> the header indicates a size < 0 if it's invalid

I forgot to mention this in the review, but ParseSingleIconSize, AddInstallIcon,
and AddIconSizes were just moved directly from dom_operations.cc. I just moved
them here for consistency (if ParseWebAppFromDefinitionFile is here, it makes
sense to me that ParseWebAppFromDocument also is).

But we don't actually use any of these functions in the definition file branch.
They are only used in the case where the user selects tools > install as web
app.

[Aaron Boodman, 2010-11-15 19:54:00]

On 2010/11/15 19:52:34, Aaron Boodman wrote:
> They are only used in the case where the user selects tools > install as web
> app.

err, tools > create application shortcuts

[Aaron Boodman, 2010-11-16 03:44:44]

http://codereview.chromium.org/4979003/diff/4001/chrome/browser/extensions/co...
File chrome/browser/extensions/convert_web_app.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/browser/extensions/co...
chrome/browser/extensions/convert_web_app.cc:120: // Add the permissions. In web
apps, these are called 'privileges'.
On 2010/11/15 19:45:36, Erik Kay wrote:
> I'm not sure we want to make this change just yet.  In Mozilla's stuff, they
> used two different terms ("capabilities" and "privileges"), so it looked like
> they hadn't made up their minds.  It seems worthwhile to chat with them before
> we change something so we don't have to just go and change it again.

I decided to land these changes in a different order for unrelated reasons, so I
took the changes to convert_web_app.* out of this CL. But I made this change in
web_apps.*.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc
File chrome/common/web_apps.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:46: return 0;
On 2010/11/15 19:52:34, Aaron Boodman wrote:
> On 2010/11/15 19:45:36, Erik Kay wrote:
> > the header indicates a size < 0 if it's invalid
> 
> I forgot to mention this in the review, but ParseSingleIconSize,
AddInstallIcon,
> and AddIconSizes were just moved directly from dom_operations.cc. I just moved
> them here for consistency (if ParseWebAppFromDefinitionFile is here, it makes
> sense to me that ParseWebAppFromDocument also is).
> 
> But we don't actually use any of these functions in the definition file
branch.
> They are only used in the case where the user selects tools > install as web
> app.

Not my code, but fixed.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:47: // Make sure all chars are from 0-9.
On 2010/11/15 19:45:36, Erik Kay wrote:
> nit: newline above comment

Done.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:55: return output;
On 2010/11/15 19:45:36, Erik Kay wrote:
> do we want a min/max value for icon sizes?

<shrug> -- this code is only used by the create application shortcuts feature. I
don't really want to change its behavior in this CL.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:76: icon_sizes.size() != 1) {
On 2010/11/15 19:45:36, Erik Kay wrote:
> so this means we're explicitly not allowing things like .ico files with
multiple
> sizes?

Also only used by create application shortcuts feature. To answer the question,
our initial implementation won't support formats like ico that contain multiple
sizes.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:134: // If is_any is true, it must occur by itself.
On 2010/11/15 19:45:36, Erik Kay wrote:
> Without displaying error messages to the developer, these errors may wind up
> being hard to debug.  Maybe this function should take an error out param?

This is only used by the create application shortcuts feature. I don't want to
make changes to that functionality in this CL.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:251: CHECK(privileges_value->GetString(i,
&privilege));
On 2010/11/15 19:45:36, Erik Kay wrote:
> isn't this covered by the schema validation?

The CHECK is just a quick assertion. I prefer that we crash with a backtrace
rather than end up with bad data persisted forever.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:283: if (!base::StringToInt(*iter, &size) || size < 0
|| size > 128)
On 2010/11/15 19:45:36, Erik Kay wrote:
> 128 seems too small as a max.  512 is pretty standard as a large size (e.g.
> apple and win7).  Also, it looks like this restriction isn't put on the link
> rel=icon tag.

The extension system doesn't currently support any larger than 128, so I thought
it did not make sense to download any sizes larger than that. Do you think that
we should, just so that we have them for future applications..? That would
require some other changes in extensions land.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h
File chrome/common/web_apps.h (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h#new...
chrome/common/web_apps.h:75: gfx::Size ParseIconSize(const string16& text);
On 2010/11/15 19:45:36, Erik Kay wrote:
> It seems odd for all of these functions to be naked like this.  typically,
> they'd be inside of a namespace or static inside of a class.  Can't we just
make
> the struct above into a class?

It is a bit odd. I moved this code from its previous home and tried to perturb
its structure as little as possible to prevent having this change spiral out of
control.

Just now I tried changing it into a proper class (with getters and setters) and
it requires me touching a lot more code than I'd like to.

These functions used to be in the webkit_glue namespace. I could move them into
something like web_apps if you'd prefer.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h#new...
chrome/common/web_apps.h:88: // function returns true.
On 2010/11/15 19:45:36, Erik Kay wrote:
> it seems like there needs to be some way to detect this last case (no web app
> present) from the caller.  maybe a bool in the struct?

The current call sites don't care about that case.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps_unitt...
File chrome/common/web_apps_unittest.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps_unitt...
chrome/common/web_apps_unittest.cc:22: PathService::Get(chrome::DIR_TEST_DATA,
&path);
On 2010/11/15 08:05:31, Paweł Hajdan Jr. wrote:
> Check return value of this.

Done.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps_unitt...
chrome/common/web_apps_unittest.cc:25: ADD_FAILURE() << L"Path does not exist: "
<< path.ToWStringHack();
On 2010/11/15 19:45:36, Erik Kay wrote:
> I think you can just use value() instead of ToWStringHack because of the
> overloads here.

Done.

http://codereview.chromium.org/4979003/diff/4001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:3918:
ParseWebAppFromWebDocument(webview()->mainFrame(), &app_info, &error);
On 2010/11/15 19:45:36, Erik Kay wrote:
> Since this can fail, don't we need to punt if it returns false and error is
set?
>  Is there going to be a way to pass error up to the browser?

No, the calling code in the browser process just defaults everything that it
doesn't find in app_info.

http://codereview.chromium.org/4979003/diff/4001/chrome/test/data/web_app_inf...
File chrome/test/data/web_app_info/full.json (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/test/data/web_app_inf...
chrome/test/data/web_app_info/full.json:11: // We also ignore this because it is
larger than 128.
On 2010/11/15 19:45:36, Erik Kay wrote:
> I thought our JSON parser barfed on comments

The one in v8 does (as per the spec), but the one that we wrote (in base/json)
accepts them, which I think is quite nice.

[Erik does not do reviews, 2010-11-16 23:43:02]

LGTM with namespace addition

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc
File chrome/common/web_apps.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.cc#ne...
chrome/common/web_apps.cc:283: if (!base::StringToInt(*iter, &size) || size < 0
|| size > 128)
On 2010/11/16 03:44:45, Aaron Boodman wrote:
> On 2010/11/15 19:45:36, Erik Kay wrote:
> > 128 seems too small as a max.  512 is pretty standard as a large size (e.g.
> > apple and win7).  Also, it looks like this restriction isn't put on the link
> > rel=icon tag.
> 
> The extension system doesn't currently support any larger than 128, so I
thought
> it did not make sense to download any sizes larger than that. Do you think
that
> we should, just so that we have them for future applications..? That would
> require some other changes in extensions land.

I forgot that we had that hard-coded.  I'll file a separate bug for supporting
512 icons in the extensions system.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h
File chrome/common/web_apps.h (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h#new...
chrome/common/web_apps.h:75: gfx::Size ParseIconSize(const string16& text);
On 2010/11/16 03:44:45, Aaron Boodman wrote:
> On 2010/11/15 19:45:36, Erik Kay wrote:
> > It seems odd for all of these functions to be naked like this.  typically,
> > they'd be inside of a namespace or static inside of a class.  Can't we just
> make
> > the struct above into a class?
> 
> It is a bit odd. I moved this code from its previous home and tried to perturb
> its structure as little as possible to prevent having this change spiral out
of
> control.
> 
> Just now I tried changing it into a proper class (with getters and setters)
and
> it requires me touching a lot more code than I'd like to.
> 
> These functions used to be in the webkit_glue namespace. I could move them
into
> something like web_apps if you'd prefer.

yeah.  for now a namespace would help.

http://codereview.chromium.org/4979003/diff/4001/chrome/common/web_apps.h#new...
chrome/common/web_apps.h:88: // function returns true.
On 2010/11/16 03:44:45, Aaron Boodman wrote:
> On 2010/11/15 19:45:36, Erik Kay wrote:
> > it seems like there needs to be some way to detect this last case (no web
app
> > present) from the caller.  maybe a bool in the struct?
> 
> The current call sites don't care about that case.

OK.

http://codereview.chromium.org/4979003/diff/4001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:3918:
ParseWebAppFromWebDocument(webview()->mainFrame(), &app_info, &error);
On 2010/11/16 03:44:45, Aaron Boodman wrote:
> On 2010/11/15 19:45:36, Erik Kay wrote:
> > Since this can fail, don't we need to punt if it returns false and error is
> set?
> >  Is there going to be a way to pass error up to the browser?
> 
> No, the calling code in the browser process just defaults everything that it
> doesn't find in app_info.

ok

http://codereview.chromium.org/4979003/diff/4001/chrome/test/data/web_app_inf...
File chrome/test/data/web_app_info/full.json (right):

http://codereview.chromium.org/4979003/diff/4001/chrome/test/data/web_app_inf...
chrome/test/data/web_app_info/full.json:11: // We also ignore this because it is
larger than 128.
On 2010/11/16 03:44:45, Aaron Boodman wrote:
> On 2010/11/15 19:45:36, Erik Kay wrote:
> > I thought our JSON parser barfed on comments
> 
> The one in v8 does (as per the spec), but the one that we wrote (in base/json)
> accepts them, which I think is quite nice.

agree.  that always bugged me about the JSON spec.

http://codereview.chromium.org/4979003/diff/28001/chrome/common/web_app_schem...
File chrome/common/web_app_schema.json (right):

http://codereview.chromium.org/4979003/diff/28001/chrome/common/web_app_schem...
chrome/common/web_app_schema.json:44: "type": "string"
minLength: 1 here too?