Add generateAppForLink function in chrome.management

Reland: Add generateAppForLink function in chrome.management

This function takes a url and a title as input, generates and
returns a bookmark app.

R=benwells@chromium.org,kalman@chromium.org,calamity@chromium.org
BUG=370350
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274732

[wjywbs, 2014-05-06 06:02:15]

Please take a look. The design doc is at

https://docs.google.com/document/d/1lSfKLt6E-vOgYpBiiNxFhCI4FXWFX3woiirWfxkBZ...

Thanks.

[wjywbs, 2014-05-11 20:21:09]

The development of this function is finished. Now it observes
NOTIFICATION_CRX_INSTALLER_DONE of the CrxInstaller. Please take a look. Thanks.

I wrote the "Reasons for not taking a launch type argument" part in the design
doc for comments. @kalman, what's your opinion about this?

[not at google - send to devlin, 2014-05-12 17:55:42]

No opinion, defer to BenW.

[calamity, 2014-05-13 01:59:04]

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:791:
installer->InstallWebApp(*web_app);
You can use BookmarkAppHelper::CreateOrUpdateBookmarkApp() here.

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:832:
&cancelable_task_tracker_);
Given that this will only ever grab 16x16 icons, we should consider generating
larger icons as with streamlined hosted apps. See
BookmarkAppHelper::GenerateContainerIcon().

[wjywbs, 2014-05-20 00:01:58]

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:791:
installer->InstallWebApp(*web_app);
On 2014/05/13 01:59:04, calamity wrote:
> You can use BookmarkAppHelper::CreateOrUpdateBookmarkApp() here.

I think I need to register for the notifications using the |installer| above
this line. If I use CreateOrUpdateBookmarkApp, the CrxInstaller will be created
inside that function, and I won't be able to register for the notifications.

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:832:
&cancelable_task_tracker_);
On 2014/05/13 01:59:04, calamity wrote:
> Given that this will only ever grab 16x16 icons, we should consider generating
> larger icons as with streamlined hosted apps. See
> BookmarkAppHelper::GenerateContainerIcon().

I didn't find this BookmarkAppHelper::GenerateContainerIcon function. Can you
please tell me where it is? Or should I create an instance of BookmarkAppHelper
and completely use it to generate web apps instead?

[wjywbs, 2014-05-20 00:01:58]

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:791:
installer->InstallWebApp(*web_app);
On 2014/05/13 01:59:04, calamity wrote:
> You can use BookmarkAppHelper::CreateOrUpdateBookmarkApp() here.

I think I need to register for the notifications using the |installer| above
this line. If I use CreateOrUpdateBookmarkApp, the CrxInstaller will be created
inside that function, and I won't be able to register for the notifications.

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:832:
&cancelable_task_tracker_);
On 2014/05/13 01:59:04, calamity wrote:
> Given that this will only ever grab 16x16 icons, we should consider generating
> larger icons as with streamlined hosted apps. See
> BookmarkAppHelper::GenerateContainerIcon().

I didn't find this BookmarkAppHelper::GenerateContainerIcon function. Can you
please tell me where it is? Or should I create an instance of BookmarkAppHelper
and completely use it to generate web apps instead?

[calamity, 2014-05-20 01:39:35]

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:791:
installer->InstallWebApp(*web_app);
On 2014/05/20 00:01:59, wjywbs wrote:
> On 2014/05/13 01:59:04, calamity wrote:
> > You can use BookmarkAppHelper::CreateOrUpdateBookmarkApp() here.
> 
> I think I need to register for the notifications using the |installer| above
> this line. If I use CreateOrUpdateBookmarkApp, the CrxInstaller will be
created
> inside that function, and I won't be able to register for the notifications.

Ah ok.

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:832:
&cancelable_task_tracker_);
On 2014/05/20 00:01:59, wjywbs wrote:
> On 2014/05/13 01:59:04, calamity wrote:
> > Given that this will only ever grab 16x16 icons, we should consider
generating
> > larger icons as with streamlined hosted apps. See
> > BookmarkAppHelper::GenerateContainerIcon().
> 
> I didn't find this BookmarkAppHelper::GenerateContainerIcon function. Can you
> please tell me where it is? Or should I create an instance of
BookmarkAppHelper
> and completely use it to generate web apps instead?

It recently got renamed to GenerateIcon(). It's in
chrome/browser/extensions/bookmark_app_helper.cc . You may need to extract
methods to get the correct letter and background color from the URL and favicon
respectively.

SkColor GetIconColorForGeneratedIcon(const SkBitmap& base_icon);
char GetIconLetterForGeneratedIcon(const GURL& url);

[wjywbs, 2014-05-20 04:31:50]

PTAL. Thanks.

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:832:
&cancelable_task_tracker_);
On 2014/05/20 01:39:35, calamity wrote:
> It recently got renamed to GenerateIcon(). It's in
> chrome/browser/extensions/bookmark_app_helper.cc . You may need to extract
> methods to get the correct letter and background color from the URL and
favicon
> respectively.
> 
> SkColor GetIconColorForGeneratedIcon(const SkBitmap& base_icon);
> char GetIconLetterForGeneratedIcon(const GURL& url);

Done.

[calamity, 2014-05-21 08:37:46]

Cool, lgtm. You'll need benwells@ to do the OWNERS review.

[benwells, 2014-05-22 02:49:24]

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:751: case
chrome::NOTIFICATION_CRX_INSTALLER_DONE: {
Could we push all this code into BookmarkAppHelper? How much similarity is there
between this and the stuff there already / in the other bits of chrome that uses
it?

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:766: // Fall through
if the information does not match.
Is there any way this can happen, or would this indicate a bug in this API? If
it would be a bug in the API we should DCHECK / NOTREACHED or something like
that.

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.h:195: public
content::NotificationObserver {
This indentation looks wrong. Did you run git cl format?

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.h:223: // Overridden
from content::NotificationObserver:
Methods should go before fields. Put this up after the struct.

https://codereview.chromium.org/266353006/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/management/test/generateAppForLink.js
(right):

https://codereview.chromium.org/266353006/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/management/test/generateAppForLink.js:12:
url, title, callback(function(data) { arg(data) }));
Can't this just be callback(arg)?

I'd prefer it to be testGenerateAppForLink(url, title, error, func) and make the
typeof check to be just if (error != null) or something.

[wjywbs, 2014-05-22 03:21:35]

PTAL, thanks.

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:751: case
chrome::NOTIFICATION_CRX_INSTALLER_DONE: {
On 2014/05/22 02:49:25, benwells wrote:
> Could we push all this code into BookmarkAppHelper? How much similarity is
there
> between this and the stuff there already / in the other bits of chrome that
uses
> it?

BookmarkAppHelper needs a content::WebContents, and uses the WebContents to
download the favicon. I think there's no WebContents in the extension function.
If NULL is passed in as the WebContents, the favicon downloader will not run the
callback to continue to install the web app. BookmarkAppHelper doesn't add the
16*16 favicon either, so it probably needs a lot of changes.

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:766: // Fall through
if the information does not match.
On 2014/05/22 02:49:25, benwells wrote:
> Is there any way this can happen, or would this indicate a bug in this API? If
> it would be a bug in the API we should DCHECK / NOTREACHED or something like
> that.

I don't think this can happen and it didn't happen in the testing. I'll change
it to a NOTREACHED.

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.h:195: public
content::NotificationObserver {
Done.

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.h:223: // Overridden
from content::NotificationObserver:
Done.

https://codereview.chromium.org/266353006/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/management/test/generateAppForLink.js
(right):

https://codereview.chromium.org/266353006/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/management/test/generateAppForLink.js:12:
url, title, callback(function(data) { arg(data) }));
Done.

[benwells, 2014-05-25 23:59:43]

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:751: case
chrome::NOTIFICATION_CRX_INSTALLER_DONE: {
On 2014/05/22 03:21:35, wjywbs wrote:
> On 2014/05/22 02:49:25, benwells wrote:
> > Could we push all this code into BookmarkAppHelper? How much similarity is
> there
> > between this and the stuff there already / in the other bits of chrome that
> uses
> > it?
> 
> BookmarkAppHelper needs a content::WebContents, and uses the WebContents to
> download the favicon. I think there's no WebContents in the extension
function.
> If NULL is passed in as the WebContents, the favicon downloader will not run
the
> callback to continue to install the web app. BookmarkAppHelper doesn't add the
> 16*16 favicon either, so it probably needs a lot of changes.

You would need to make it so BookmarkAppHelper can be constructed with a null
WebContents, in which case it would act differently.

Is there a reason why you add the 16x16 icon and BookmarkAppHelper doesn't?
Ideally the action of creating a URL app would be the same whether via this API
or the existing UI.

https://codereview.chromium.org/266353006/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:872: error_ =
keys::kNoFaviconServiceToGenerateAppForLink;
This error is unactionable for an extension developer - it would be a bug in
chrome for this to ever happen (except for maybe in tests).

This should just be a DCHECK(favicon_service).

[wjywbs, 2014-05-26 03:22:24]

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:751: case
chrome::NOTIFICATION_CRX_INSTALLER_DONE: {
On 2014/05/25 23:59:43, benwells wrote:
> You would need to make it so BookmarkAppHelper can be constructed with a null
> WebContents, in which case it would act differently.
> 
> Is there a reason why you add the 16x16 icon and BookmarkAppHelper doesn't?
> Ideally the action of creating a URL app would be the same whether via this
API
> or the existing UI.

I looked at BookmarkAppHelper again and merging the changes would increase the
complexity a lot. It would also need to revert some changes that calamity@
suggested. I'm not sure if I should continue.

The 16x16 favicon is added to be compatible with chrome://apps page and other
existing extensions. That page only read and use 16x16 favicons when displaying
and generating bookmark apps.

https://codereview.chromium.org/266353006/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/266353006/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:872: error_ =
keys::kNoFaviconServiceToGenerateAppForLink;
On 2014/05/25 23:59:43, benwells wrote:
> This error is unactionable for an extension developer - it would be a bug in
> chrome for this to ever happen (except for maybe in tests).
> 
> This should just be a DCHECK(favicon_service).

Done.

[calamity, 2014-05-26 06:39:52]

On 2014/05/26 03:22:24, wjywbs wrote:
>
https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
> File chrome/browser/extensions/api/management/management_api.cc (right):
> 
>
https://codereview.chromium.org/266353006/diff/40001/chrome/browser/extension...
> chrome/browser/extensions/api/management/management_api.cc:751: case
> chrome::NOTIFICATION_CRX_INSTALLER_DONE: {
> On 2014/05/25 23:59:43, benwells wrote:
> > You would need to make it so BookmarkAppHelper can be constructed with a
null
> > WebContents, in which case it would act differently.
> > 
> > Is there a reason why you add the 16x16 icon and BookmarkAppHelper doesn't?
> > Ideally the action of creating a URL app would be the same whether via this
> API
> > or the existing UI.
> 
> I looked at BookmarkAppHelper again and merging the changes would increase the
> complexity a lot. It would also need to revert some changes that calamity@
> suggested. I'm not sure if I should continue.
> 
> The 16x16 favicon is added to be compatible with chrome://apps page and other
> existing extensions. That page only read and use 16x16 favicons when
displaying
> and generating bookmark apps.
> 

I think you can actually do this relatively simply.

First off, make it not instantiate favicon_downloader_ in the BookmarkAppHelper
constructor if the WebContents is NULL. Then in Create(), if favicon_downloader_
is false, you can just call OnIconsDownloaded(true, const std::map<GURL,
std::vector<SkBitmap> >()); .

Your code would then end up being:

<Create WebAppInfo with icons from favicon service>

helper_ = new BookmarkAppHelper(service, web_app_info, NULL);
helper_->Create(&ManagementGenerateAppForLinkFunction::OnBookmarkAppCreated); //
I think you'll need a weak ptr factory for this.

OnBookmarkAppCreated would perform the actions your Observe method currently
does.

[wjywbs, 2014-05-26 15:54:36]

I merged the changes into BookmarkAppHelper. It turned out to be simple. Please
review again. Thanks a lot.

[benwells, 2014-05-27 03:24:04]

On 2014/05/26 15:54:36, wjywbs wrote:
> I merged the changes into BookmarkAppHelper. It turned out to be simple.
Please
> review again. Thanks a lot.

calamity - could you take a look first?

[calamity, 2014-05-27 05:32:11]

Great! This is much cleaner.

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:258: SkBitmap* icon =
&web_app_info_.icons[0].data;
Just use a const SkBitmap& here.

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:262: 
It seems a little messy to just take the first icon. I think it's legitimate to
use web_app_info_.icons to fill in any icons sizes that are missing.

You can just add all the valid bitmaps from web_app_info_ to downloaded_icons
above. You should then call web_app_info_.icons.clear() to avoid duplicating a
bitmap.

You should add the bitmaps to the end of downloaded_icons so that downloaded
icons are preferred.

[wjywbs, 2014-05-27 06:22:31]

PTAL, thanks.

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:258: SkBitmap* icon =
&web_app_info_.icons[0].data;
On 2014/05/27 05:32:12, calamity wrote:
> Just use a const SkBitmap& here.

Done.

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:262: 
On 2014/05/27 05:32:12, calamity wrote:
> It seems a little messy to just take the first icon. I think it's legitimate
to
> use web_app_info_.icons to fill in any icons sizes that are missing.
> 
> You can just add all the valid bitmaps from web_app_info_ to downloaded_icons
> above. You should then call web_app_info_.icons.clear() to avoid duplicating a
> bitmap.
> 
> You should add the bitmaps to the end of downloaded_icons so that downloaded
> icons are preferred.

Done, but can we clear the web_app_info_.icons? It may contains the url of the
icons and empty bitmap. Is the |url| in IconInfo used for sync? Or how about add
all other sizes of the icons that do not exist in |resized_bitmaps|?

[calamity, 2014-05-27 07:18:47]

New changes here lgtm.

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:262: 
On 2014/05/27 06:22:32, wjywbs wrote:
> On 2014/05/27 05:32:12, calamity wrote:
> > It seems a little messy to just take the first icon. I think it's legitimate
> to
> > use web_app_info_.icons to fill in any icons sizes that are missing.
> > 
> > You can just add all the valid bitmaps from web_app_info_ to
downloaded_icons
> > above. You should then call web_app_info_.icons.clear() to avoid duplicating
a
> > bitmap.
> > 
> > You should add the bitmaps to the end of downloaded_icons so that downloaded
> > icons are preferred.
> 
> Done, but can we clear the web_app_info_.icons? It may contains the url of the
> icons and empty bitmap. Is the |url| in IconInfo used for sync? Or how about
add
> all other sizes of the icons that do not exist in |resized_bitmaps|?

Clearing the icons should be fine.

The icon URL isn't used for anything at the moment, it's mostly a container to
pass through information into this class.

Icons aren't synced right now. Using the icon URL was considered but there is a
blob sync API in development that looks suitable for syncing icons directly.

Adding to |resized_bitmaps| isn't great because the subsequent code assumes that
only valid icon sizes will be in there. If web_app_info_ for some reason has an
18x18 icon, it should be discarded or resized to 16x16 before use which will
currently happen in ConstrainBitmapsToSizes.

[benwells, 2014-05-27 22:05:53]

this is looking really good, just one more question..

https://codereview.chromium.org/266353006/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/266353006/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.h:209: struct
AppInstallInfo {
Why do you need this struct? Seems like having a title_ and launch_url_ field is
equivalent to the way it is now.

You could also bind these into the OnFaviconForApp callback but that is probably
unnecessary.

[wjywbs, 2014-05-27 22:20:20]

PTAL, thanks.

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/266353006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:262: 
On 2014/05/27 07:18:48, calamity wrote:
> Clearing the icons should be fine.
> 
> The icon URL isn't used for anything at the moment, it's mostly a container to
> pass through information into this class.
> 
> Icons aren't synced right now. Using the icon URL was considered but there is
a
> blob sync API in development that looks suitable for syncing icons directly.
> 
> Adding to |resized_bitmaps| isn't great because the subsequent code assumes
that
> only valid icon sizes will be in there. If web_app_info_ for some reason has
an
> 18x18 icon, it should be discarded or resized to 16x16 before use which will
> currently happen in ConstrainBitmapsToSizes.

I added "web_app_info_.icons.clear();".

https://codereview.chromium.org/266353006/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/266353006/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.h:209: struct
AppInstallInfo {
On 2014/05/27 22:05:54, benwells wrote:
> Why do you need this struct? Seems like having a title_ and launch_url_ field
is
> equivalent to the way it is now.
> 
> You could also bind these into the OnFaviconForApp callback but that is
probably
> unnecessary.

Done.

[benwells, 2014-05-28 01:04:17]

lgtm

https://codereview.chromium.org/266353006/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/266353006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.h:216: GURL launch_url_;
Nit: no need for the blank line between these two. You can also remove the blank
line between OnFavicon.. and FinishCreate...

[wjywbs, 2014-05-28 01:19:00]

PTAL. Thanks. I'm going to rebase the histograms again and commit later when the
histograms conflicts are resolved.

@isherman: Please review the histograms.

https://codereview.chromium.org/266353006/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/266353006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.h:216: GURL launch_url_;
On 2014/05/28 01:04:18, benwells wrote:
> Nit: no need for the blank line between these two. You can also remove the
blank
> line between OnFavicon.. and FinishCreate... 

Done.

[Ilya Sherman, 2014-05-28 06:31:04]

Histograms LGTM.  Apologies for the delayed review -- I was traveling.

[wjywbs, 2014-05-29 20:56:08]

The CQ bit was checked by wjywbs@gmail.com

[commit-bot: I haz the power, 2014-05-29 20:57:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjywbs@gmail.com/266353006/180001

[wjywbs, 2014-05-29 21:38:40]

The CQ bit was unchecked by wjywbs@gmail.com

[wjywbs, 2014-05-29 23:17:31]

The CreateBookmarkApp unit test used NULL as WebContents in BookmarkAppHelper
and now failed, because we changed the logic of BookmarkAppHelper. I fixed that
unit test. @calamity, can you please take a look? Thanks.

[calamity, 2014-05-30 01:44:10]

That looks fine but you should also add a test for the new logic that checks
that the input WebAppInfo's icons get used in the case of a NULL webcontents.

[wjywbs, 2014-05-30 01:50:09]

On 2014/05/30 01:44:10, calamity wrote:
> That looks fine but you should also add a test for the new logic that checks
> that the input WebAppInfo's icons get used in the case of a NULL webcontents.

I think that case is already in management_apitest.cc and generateAppForLink.js?

[calamity, 2014-05-30 04:20:59]

I think the BookmarkAppHelper's test should reflect the fact that the
WebAppInfo's icons are now used.

https://codereview.chromium.org/266353006/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper_unittest.cc (right):

https://codereview.chromium.org/266353006/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper_unittest.cc:139:
web_app_info.description = base::UTF8ToUTF16(kAppDescription);
WebApplicationInfo::IconsInfo icon_info;
icon_info.data = CreateSquareBitmapWithColor(kIconSizeLarge, SK_ColorRED);
web_app_info.icons.push_back(icon_info);

https://codereview.chromium.org/266353006/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper_unittest.cc:164: extension,
kIconSizeSmall, ExtensionIconSet::MATCH_EXACTLY).empty());
EXPECT_FALSE(
    IconsInfo::GetIconResource(
        extension, kIconSizeLarge, ExtensionIconSet::MATCH_EXACTLY).empty());

[wjywbs, 2014-05-30 14:43:03]

I updated the unit test. I also added icon size checks in generateAppForLink.js.
Thanks.

https://codereview.chromium.org/266353006/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper_unittest.cc (right):

https://codereview.chromium.org/266353006/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper_unittest.cc:56: 
Creating TestWebContents also creates a RenderProcessHost that needs to be
deleted. Otherwise the later tests will crash by accessing the freed |profile_|
in ExtensionService::NotifyExtensionLoaded line 996.

[wjywbs, 2014-06-03 03:14:08]

Can I proceed and commit this? Thanks.

[benwells, 2014-06-03 03:46:37]

On 2014/06/03 03:14:08, wjywbs wrote:
> Can I proceed and commit this? Thanks.

calamity?

[calamity, 2014-06-03 05:48:16]

slgtm!

[wjywbs, 2014-06-03 13:42:00]

The CQ bit was checked by wjywbs@gmail.com

[commit-bot: I haz the power, 2014-06-03 13:44:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjywbs@gmail.com/266353006/240001

[commit-bot: I haz the power, 2014-06-03 16:40:38]

Change committed as 274549

[wjywbs, 2014-06-03 23:01:05]

I'm going to reland this CL. If this fix fails on the main builders again, I'll
redo the unit test. Thanks.

https://codereview.chromium.org/266353006/diff/260001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper_unittest.cc (right):

https://codereview.chromium.org/266353006/diff/260001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper_unittest.cc:66: host->Cleanup();
This commit was reverted because the "delete host" here hit a DCHECK in debug
trybots. I changed "delete host" to "host->Cleanup()" to fix this.

[wjywbs, 2014-06-03 23:03:15]

The CQ bit was checked by wjywbs@gmail.com

[commit-bot: I haz the power, 2014-06-03 23:03:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjywbs@gmail.com/266353006/260001

[commit-bot: I haz the power, 2014-06-04 08:58:07]

Change committed as 274732

[wjywbs, 2014-06-17 02:49:49]

@calamity, can you please take a look at #10 in the bug? Thanks.
https://code.google.com/p/chromium/issues/detail?id=370350#c10