Add convert_web_app.*, a utility to convert web apps declared

Add convert_web_app.*, a utility to convert web apps declared
in pure HTML into the extension-based web apps used internally
by Chrome.

BUG=49233
TEST=unit_tests --gtest_filter=ExtensionFromWebApp.*

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

[Aaron Boodman, 2010-10-28 23:42:51]

Let me know if you'd rather me find someone else for these reviews.

[Paweł Hajdan Jr., 2010-10-29 10:30:43]

Drive-by with some test comments. No need to wait for another review by me.

http://codereview.chromium.org/4139008/diff/2001/3003
File chrome/browser/extensions/convert_web_app_unittest.cc (right):

http://codereview.chromium.org/4139008/diff/2001/3003#newcode37
chrome/browser/extensions/convert_web_app_unittest.cc:37: LOG(ERROR) << "Could
not get test data directory.";
nit: ADD_FAILURE instead (you can still use << for the message), to make sure we
notice those.

http://codereview.chromium.org/4139008/diff/2001/3003#newcode51
chrome/browser/extensions/convert_web_app_unittest.cc:51: LOG(ERROR) << "Could
not read test icon.";
nit: Same here.

http://codereview.chromium.org/4139008/diff/2001/3003#newcode59
chrome/browser/extensions/convert_web_app_unittest.cc:59: if
(result.data.isNull())
nit: And here (EXPECT_FALSE).

http://codereview.chromium.org/4139008/diff/2001/3003#newcode73
chrome/browser/extensions/convert_web_app_unittest.cc:73: }
nit: // namespace

http://codereview.chromium.org/4139008/diff/2001/3003#newcode101
chrome/browser/extensions/convert_web_app_unittest.cc:101:
EXPECT_EQ("lJqm1+jncOHClAuwif1QxNJKfeV9Fbl9IBZx7FkNwkA=",
extension->public_key());
nit: 80cols

[Erik does not do reviews, 2010-10-29 17:37:20]

http://codereview.chromium.org/4139008/diff/2001/3001
File chrome/browser/extensions/convert_web_app.cc (right):

http://codereview.chromium.org/4139008/diff/2001/3001#newcode53
chrome/browser/extensions/convert_web_app.cc:53: // really needed, but it seems
like useful information.
neat idea.

I'm a little nervous about not having a version in the manifest.  It seems like
it might somehow be possible to go backwards due to caches, etc.  Also, it means
that it's harder for developers to debug problems that users are seeing with
their manifest.

http://codereview.chromium.org/4139008/diff/2001/3001#newcode57
chrome/browser/extensions/convert_web_app.cc:57: return
base::StringPrintf("%i.%.2i.%.2i",
maybe add a timestamp as a final part of the version?  it will make it easier to
see if/when the manifest was actually updated.

http://codereview.chromium.org/4139008/diff/2001/3001#newcode86
chrome/browser/extensions/convert_web_app.cc:86: // TODO(aa):
kConvertedFromWebApp?
This seems like a good idea.  if not in this CL, please fill out the comment a
bit more.

http://codereview.chromium.org/4139008/diff/2001/3003
File chrome/browser/extensions/convert_web_app_unittest.cc (right):

http://codereview.chromium.org/4139008/diff/2001/3003#newcode83
chrome/browser/extensions/convert_web_app_unittest.cc:83:
web_app.urls.push_back(GURL("http://aaronboodman.com/gearpad"));
The different URL here (missing "/") looks funny.  Was this intentional?

http://codereview.chromium.org/4139008/diff/2001/3003#newcode95
chrome/browser/extensions/convert_web_app_unittest.cc:95: EXPECT_EQ("", error);
doesn't look like anything sets error

http://codereview.chromium.org/4139008/diff/2001/3003#newcode123
chrome/browser/extensions/convert_web_app_unittest.cc:123: }
the directory created by this test isn't cleaned up (also in the next one)

http://codereview.chromium.org/4139008/diff/2001/3003#newcode136
chrome/browser/extensions/convert_web_app_unittest.cc:136: EXPECT_EQ("", error);
doesn't look like anything sets error

http://codereview.chromium.org/4139008/diff/2001/3003#newcode149
chrome/browser/extensions/convert_web_app_unittest.cc:149: EXPECT_EQ(0u,
extension->api_permissions().size());
maybe also add a test for icons.size()

http://codereview.chromium.org/4139008/diff/2001/3003#newcode155
chrome/browser/extensions/convert_web_app_unittest.cc:155:
TEST(ExtensionFromWebApp, Errors) {
Doing anything for this in this CL?

[Aaron Boodman, 2010-10-29 22:39:52]

http://codereview.chromium.org/4139008/diff/2001/3001
File chrome/browser/extensions/convert_web_app.cc (right):

http://codereview.chromium.org/4139008/diff/2001/3001#newcode53
chrome/browser/extensions/convert_web_app.cc:53: // really needed, but it seems
like useful information.
On 2010/10/29 17:37:20, Erik Kay wrote:
> neat idea.
> 
> I'm a little nervous about not having a version in the manifest.  It seems
like
> it might somehow be possible to go backwards due to caches, etc.  Also, it
means
> that it's harder for developers to debug problems that users are seeing with
> their manifest.

I don't think that the app declaration (that's what I'm calling it to avoid
conflict with html5's existing app cache manifest) adds anything that is
particularly bad if it goes backwards.

The only real problem is when you have local state, but web developers already
have that problem presumably.

Can you explain your concern with debugging?

http://codereview.chromium.org/4139008/diff/2001/3001#newcode57
chrome/browser/extensions/convert_web_app.cc:57: return
base::StringPrintf("%i.%.2i.%.2i",
On 2010/10/29 17:37:20, Erik Kay wrote:
> maybe add a timestamp as a final part of the version?  it will make it easier
to
> see if/when the manifest was actually updated.

Unfortunately base::Version uses short ints internally so a full timestamp is
not possible in one component. I've changed it to:

<year>.<month><day>.<upper 16 bytes of timestamp>.<lower 16 bytes of timestamp>

WDYT?

http://codereview.chromium.org/4139008/diff/2001/3001#newcode86
chrome/browser/extensions/convert_web_app.cc:86: // TODO(aa):
kConvertedFromWebApp?
On 2010/10/29 17:37:20, Erik Kay wrote:
> This seems like a good idea.  if not in this CL, please fill out the comment a
> bit more.

I wanted to avoid the mistake we made with kConvertedFromUserScript and not put
this state in the manifest. So I was going to look at putting it in the prefs
instead. I just removed the comment so that I don't forget to later.

http://codereview.chromium.org/4139008/diff/2001/3003
File chrome/browser/extensions/convert_web_app_unittest.cc (right):

http://codereview.chromium.org/4139008/diff/2001/3003#newcode83
chrome/browser/extensions/convert_web_app_unittest.cc:83:
web_app.urls.push_back(GURL("http://aaronboodman.com/gearpad"));
On 2010/10/29 17:37:20, Erik Kay wrote:
> The different URL here (missing "/") looks funny.  Was this intentional?

Doesn't really matter one way or the other, but I changed it for you.

http://codereview.chromium.org/4139008/diff/2001/3003#newcode95
chrome/browser/extensions/convert_web_app_unittest.cc:95: EXPECT_EQ("", error);
On 2010/10/29 17:37:20, Erik Kay wrote:
> doesn't look like anything sets error

Whoops! thanks.

http://codereview.chromium.org/4139008/diff/2001/3003#newcode123
chrome/browser/extensions/convert_web_app_unittest.cc:123: }
On 2010/10/29 17:37:20, Erik Kay wrote:
> the directory created by this test isn't cleaned up (also in the next one)

Done.

http://codereview.chromium.org/4139008/diff/2001/3003#newcode136
chrome/browser/extensions/convert_web_app_unittest.cc:136: EXPECT_EQ("", error);
On 2010/10/29 17:37:20, Erik Kay wrote:
> doesn't look like anything sets error

Done.

http://codereview.chromium.org/4139008/diff/2001/3003#newcode149
chrome/browser/extensions/convert_web_app_unittest.cc:149: EXPECT_EQ(0u,
extension->api_permissions().size());
On 2010/10/29 17:37:20, Erik Kay wrote:
> maybe also add a test for icons.size()

Done.

http://codereview.chromium.org/4139008/diff/2001/3003#newcode155
chrome/browser/extensions/convert_web_app_unittest.cc:155:
TEST(ExtensionFromWebApp, Errors) {
On 2010/10/29 17:37:20, Erik Kay wrote:
> Doing anything for this in this CL?

Whoops, no. Removed.

[Erik does not do reviews, 2010-10-29 23:01:57]

http://codereview.chromium.org/4139008/diff/2001/3001
File chrome/browser/extensions/convert_web_app.cc (right):

http://codereview.chromium.org/4139008/diff/2001/3001#newcode53
chrome/browser/extensions/convert_web_app.cc:53: // really needed, but it seems
like useful information.
On 2010/10/29 22:39:53, Aaron Boodman wrote:
> On 2010/10/29 17:37:20, Erik Kay wrote:
> > neat idea.
> > 
> > I'm a little nervous about not having a version in the manifest.  It seems
> like
> > it might somehow be possible to go backwards due to caches, etc.  Also, it
> means
> > that it's harder for developers to debug problems that users are seeing with
> > their manifest.
> 
> I don't think that the app declaration (that's what I'm calling it to avoid
> conflict with html5's existing app cache manifest) adds anything that is
> particularly bad if it goes backwards.
> 
> The only real problem is when you have local state, but web developers already
> have that problem presumably.
> 
> Can you explain your concern with debugging?

I push out a new version that fixes a bug in my extent or adds a new permission.
 User complains that it's still not working for them.  If there was a version
number, I could ask them to check to see what version they had (we could expose
that like we do with extensions).  Without that, I'm not sure what the dev can
figure out.

http://codereview.chromium.org/4139008/diff/2001/3001#newcode57
chrome/browser/extensions/convert_web_app.cc:57: return
base::StringPrintf("%i.%.2i.%.2i",
On 2010/10/29 22:39:53, Aaron Boodman wrote:
> On 2010/10/29 17:37:20, Erik Kay wrote:
> > maybe add a timestamp as a final part of the version?  it will make it
easier
> to
> > see if/when the manifest was actually updated.
> 
> Unfortunately base::Version uses short ints internally so a full timestamp is
> not possible in one component. I've changed it to:
> 
> <year>.<month><day>.<upper 16 bytes of timestamp>.<lower 16 bytes of
timestamp>
> 
> WDYT?

Hmm.  I was going to suggest you just take the lower 16 bits, but seconds in a
day needs 17 bits.  It seems like the full 32 bit value is overkill.  What if we
do minutes instead (timetstamp % 1440)?

http://codereview.chromium.org/4139008/diff/2001/3001#newcode86
chrome/browser/extensions/convert_web_app.cc:86: // TODO(aa):
kConvertedFromWebApp?
On 2010/10/29 22:39:53, Aaron Boodman wrote:
> On 2010/10/29 17:37:20, Erik Kay wrote:
> > This seems like a good idea.  if not in this CL, please fill out the comment
a
> > bit more.
> 
> I wanted to avoid the mistake we made with kConvertedFromUserScript and not
put
> this state in the manifest. So I was going to look at putting it in the prefs
> instead. I just removed the comment so that I don't forget to later.

good point.  I forgot about the problem that caused.  your plan sgtm.

http://codereview.chromium.org/4139008/diff/23001/24003
File chrome/browser/extensions/convert_web_app.h (right):

http://codereview.chromium.org/4139008/diff/23001/24003#newcode26
chrome/browser/extensions/convert_web_app.h:26: // <year>.<month><day>.<upper 16
bytes of unix timestamp>.<lower 16 bytes>
bytes -> bits

[Erik does not do reviews, 2010-10-29 23:52:44]

LGTM - remaining changes aren't substantial