app_list: DriveAppConverter to install local URL app from DriveAppInfo.

app_list: DriveAppConverter to install local URL app from DriveAppInfo.

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

[calamity, 2014-05-19 04:01:07]

Mostly lg. Just a couple of nits and questions.

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
File chrome/browser/ui/app_list/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter.cc:28: class
DriveAppConverter::IconFetcher : public net::URLFetcherDelegate,
Class comment. It would good to mention somewhere that DriveAppInfo only expects
single images. (i.e it won't unpack a favicon.ico into all its resolutions)

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter.cc:60:
fetcher->GetResponseCode() != 200) {
s/200/net::HTTP_OK/

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
File chrome/browser/ui/app_list/drive/drive_app_converter.h (right):

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter.h:29: typedef
base::Callback<void(DriveAppConverter*, bool success)>
Any reason not to return a const DriveAppConverter* here? It would prevent
Start() from being called in a completion callback.

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter.h:46: void
OnIconFetchComplete(IconFetcher* fetcher);
const IconFetcher* .

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
File chrome/browser/ui/app_list/drive/drive_app_converter_browsertest.cc
(right):

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter_browsertest.cc:72:
app_icons.push_back(std::make_pair(66, GetTestUrl("extensions/icon3.png")));
Do we need to account for the case where the same icon size is provided twice?

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter_browsertest.cc:105:
EXPECT_TRUE(app->is_hosted_app());
Also check app->from_bookmark() is true.

[xiyuan, 2014-05-19 06:08:14]

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
File chrome/browser/ui/app_list/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter.cc:28: class
DriveAppConverter::IconFetcher : public net::URLFetcherDelegate,
On 2014/05/19 04:01:08, calamity wrote:
> Class comment. It would good to mention somewhere that DriveAppInfo only
expects
> single images. (i.e it won't unpack a favicon.ico into all its resolutions)

Done.

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter.cc:60:
fetcher->GetResponseCode() != 200) {
On 2014/05/19 04:01:08, calamity wrote:
> s/200/net::HTTP_OK/

Done.

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
File chrome/browser/ui/app_list/drive/drive_app_converter.h (right):

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter.h:29: typedef
base::Callback<void(DriveAppConverter*, bool success)>
On 2014/05/19 04:01:08, calamity wrote:
> Any reason not to return a const DriveAppConverter* here? It would prevent
> Start() from being called in a completion callback.

Done.

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter.h:46: void
OnIconFetchComplete(IconFetcher* fetcher);
On 2014/05/19 04:01:08, calamity wrote:
> const IconFetcher* .

Done.

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
File chrome/browser/ui/app_list/drive/drive_app_converter_browsertest.cc
(right):

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter_browsertest.cc:72:
app_icons.push_back(std::make_pair(66, GetTestUrl("extensions/icon3.png")));
On 2014/05/19 04:01:08, calamity wrote:
> Do we need to account for the case where the same icon size is provided twice?

Added code in converter class to only pick the first resource for a certain icon
size. And added a test case here.

https://codereview.chromium.org/292523004/diff/1/chrome/browser/ui/app_list/d...
chrome/browser/ui/app_list/drive/drive_app_converter_browsertest.cc:105:
EXPECT_TRUE(app->is_hosted_app());
On 2014/05/19 04:01:08, calamity wrote:
> Also check app->from_bookmark() is true.

Done.

[calamity, 2014-05-19 06:13:27]

lgtm

[xiyuan, 2014-05-19 06:25:48]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-05-19 06:25:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiyuan@chromium.org/292523004/20001

[commit-bot: I haz the power, 2014-05-19 09:25:15]

Change committed as 271355