Get web application list using Drive V2 API.

Get web application list using Drive V2 API.

In Drive V2 API, getting drive application list has a separate API call,
which was formally included in account metadata in WAPI.

BUG=chromium:127728
TEST=none

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

[kochi, 2012-08-10 06:10:25]

Hi Satoru,

Could you review this?
Thanks,

[satorux1, 2012-08-10 17:26:02]

http://codereview.chromium.org/10829276/diff/6002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/drive_webapps_registry.cc (right):

http://codereview.chromium.org/10829276/diff/6002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:200: it !=
applist->items().end(); ++it) {
matter of taste, but the following is more concise. 

  for (size_t i = 0; i <  applist->items().size(); ++i)

http://codereview.chromium.org/10829276/diff/6002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:201: const AppResource*
app = *it;
maybe  const AppResource& app = *applist->items()[i];

http://codereview.chromium.org/10829276/diff/6002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:207: for
(ScopedVector<DriveAppIcon>::const_iterator it = app->icons().begin();
|it| is already used in the outer loop. please use a different name. I'd go with
size_t i and size_t j.

http://codereview.chromium.org/10829276/diff/6002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:209: DriveAppIcon* icon
= *it;
const reference?

http://codereview.chromium.org/10829276/diff/6002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/drive_webapps_registry.h (right):

http://codereview.chromium.org/10829276/diff/6002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry.h:68: virtual void
UpdateFromFeed(const AccountMetadataFeed* metadata) = 0;
if it's const, can you make it const AccountMetadataFeed& ?

http://codereview.chromium.org/10829276/diff/6002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry.h:72: virtual void
UpdateFromApplicationList(const AppList* applist) = 0;
ditto.

[satorux1, 2012-08-10 17:41:43]

BTW, there are several M22 bugs related to the webapp stuff. We might need to
wait until these issues are fixed. I'll keep you updated on the progress


http://code.google.com/p/chromium/issues/list?can=2&q=Feature%3DFileManager-D...

[kochi, 2012-08-13 09:12:24]

I'll keep an eye on changes about drive_webapps_registry
chagnes.  Thanks for the notice!

https://chromiumcodereview.appspot.com/10829276/diff/6002/chrome/browser/chro...
File chrome/browser/chromeos/gdata/drive_webapps_registry.cc (right):

https://chromiumcodereview.appspot.com/10829276/diff/6002/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:200: it !=
applist->items().end(); ++it) {
On 2012/08/10 17:26:02, satorux1 wrote:
> matter of taste, but the following is more concise. 
> 
>   for (size_t i = 0; i <  applist->items().size(); ++i)

Done.

The same is done for UpdateFeed().

https://chromiumcodereview.appspot.com/10829276/diff/6002/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:201: const AppResource*
app = *it;
On 2012/08/10 17:26:02, satorux1 wrote:
> maybe  const AppResource& app = *applist->items()[i];

Done.

The same is done for UpdateFeed().

https://chromiumcodereview.appspot.com/10829276/diff/6002/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:207: for
(ScopedVector<DriveAppIcon>::const_iterator it = app->icons().begin();
This |it| removed in favor of size_t loop.

https://chromiumcodereview.appspot.com/10829276/diff/6002/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:209: DriveAppIcon* icon
= *it;
On 2012/08/10 17:26:02, satorux1 wrote:
> const reference?

Done.

https://chromiumcodereview.appspot.com/10829276/diff/6002/chrome/browser/chro...
File chrome/browser/chromeos/gdata/drive_webapps_registry.h (right):

https://chromiumcodereview.appspot.com/10829276/diff/6002/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_webapps_registry.h:68: virtual void
UpdateFromFeed(const AccountMetadataFeed* metadata) = 0;
On 2012/08/10 17:26:02, satorux1 wrote:
> if it's const, can you make it const AccountMetadataFeed& ?

Done.

https://chromiumcodereview.appspot.com/10829276/diff/6002/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_webapps_registry.h:72: virtual void
UpdateFromApplicationList(const AppList* applist) = 0;
On 2012/08/10 17:26:02, satorux1 wrote:
> ditto.

Done.

[satorux1, 2012-08-13 17:14:48]

http://codereview.chromium.org/10829276/diff/11001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/drive_webapps_registry.cc (right):

http://codereview.chromium.org/10829276/diff/11001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:193: void
DriveWebAppsRegistry::UpdateFromApplicationList(const AppList& applist) {
Can you write a unit test for this? We have one for UpdateFromFeed()

[kochi, 2012-08-14 09:21:10]

Thanks, added a unittest.

https://chromiumcodereview.appspot.com/10829276/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/drive_webapps_registry.cc (right):

https://chromiumcodereview.appspot.com/10829276/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/gdata/drive_webapps_registry.cc:193: void
DriveWebAppsRegistry::UpdateFromApplicationList(const AppList& applist) {
On 2012/08/13 17:14:48, satorux1 wrote:
> Can you write a unit test for this? We have one for UpdateFromFeed()

Done.

[satorux1, 2012-08-14 13:04:55]

LGTM. I think it's safe to submit. I had some chat with gspencer@ and it sounded
like he would not need to change this code.

http://codereview.chromium.org/10829276/diff/3007/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/drive_webapps_registry_unittest.cc (left):

http://codereview.chromium.org/10829276/diff/3007/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry_unittest.cc:25: if (arg1 ==
arg2)
Glad to see this gone!

http://codereview.chromium.org/10829276/diff/3007/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/drive_webapps_registry_unittest.cc (right):

http://codereview.chromium.org/10829276/diff/3007/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry_unittest.cc:40:
.AppendASCII(prefix.c_str())
indentation is wrong? please remove .c_str()

http://codereview.chromium.org/10829276/diff/3007/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_webapps_registry_unittest.cc:41:
.AppendASCII(filename.c_str());
remove c_str()

[satorux1, 2012-08-14 13:06:42]

please submit after fixing nits. thanks.

[kochi, 2012-08-15 03:33:02]

Nits fixed.
Thanks for the review!

https://chromiumcodereview.appspot.com/10829276/diff/3007/chrome/browser/chro...
File chrome/browser/chromeos/gdata/drive_webapps_registry_unittest.cc (right):

https://chromiumcodereview.appspot.com/10829276/diff/3007/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_webapps_registry_unittest.cc:40:
.AppendASCII(prefix.c_str())
On 2012/08/14 13:04:55, satorux1 wrote:
> indentation is wrong? please remove .c_str()

Done.

https://chromiumcodereview.appspot.com/10829276/diff/3007/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_webapps_registry_unittest.cc:41:
.AppendASCII(filename.c_str());
On 2012/08/14 13:04:55, satorux1 wrote:
> remove c_str()

Done.

[kochi, 2012-08-15 04:39:16]

(+gspencer)
Submitted.