Create bookmark apps at the end of the process.

Create bookmark apps at the end of the process.

Prior to this change bookmark apps were created as soon as the user
started the process. If the user quit the process the app was then
removed. This made the flow feel strange, and also caused any matching
app that previously existed to be deleted if the user quit the process.

BUG=438019
Committed: https://crrev.com/da4b8ca729ece8bf351dd68c7288f3e3805da013
Cr-Commit-Position: refs/heads/master@{#314720}

[benwells, 2015-02-02 20:46:18]

benwells@chromium.org changed reviewers:
+ calamity@chromium.org

[calamity, 2015-02-03 07:29:49]

lgtm.

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:393: extensions::LaunchType
launch_type = web_app_info_.open_as_window
Comment about what these lines do.

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/bookmark_app_helper.h (right):

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.h:43: // All existing icons from
WebApplicationInfo will also be used.
Mention that the dialog will be created.

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc (right):

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:68: const
BrowserWindow::ShowBookmarkAppBubbleCallback& callback) {
There's no protection here against showing 2 dialogs anymore. Is this
intentional?

[benwells, 2015-02-03 08:54:13]

benwells@chromium.org changed reviewers:
+ thestig@chromium.org

[benwells, 2015-02-03 08:54:13]

+thestig for owners review for non-c/b/extensions stuff (but look at that too if
you want, of course).

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:393: extensions::LaunchType
launch_type = web_app_info_.open_as_window
On 2015/02/03 07:29:49, calamity wrote:
> Comment about what these lines do.

Done.

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/bookmark_app_helper.h (right):

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.h:43: // All existing icons from
WebApplicationInfo will also be used.
On 2015/02/03 07:29:49, calamity wrote:
> Mention that the dialog will be created.

Done.

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc (right):

https://codereview.chromium.org/899443002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:68: const
BrowserWindow::ShowBookmarkAppBubbleCallback& callback) {
On 2015/02/03 07:29:49, calamity wrote:
> There's no protection here against showing 2 dialogs anymore. Is this
> intentional?

Yes. I don't think there is any need for this protection. If it is needed, the
protection should probably  be provided at a higher level.

Let me know if you can see a problem without the protection.

[Lei Zhang, 2015-02-03 18:37:49]

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc (right):

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:46: int size =
static_cast<int>(dip_size_ * scale);
Can this overflow? I'm guessing no because the two numbers are small.

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:47: for (auto
icon_info : info_.icons) {
const auto& ?

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:69:
BookmarkAppBubbleView* bookmark_app_bubble =
Who frees this? Can you add a comment to explain the object lifetime?

[mitchellj, 2015-02-03 23:43:46]

mitchelljones@chromium.org changed reviewers:
+ mitchelljones@chromium.org

[mitchellj, 2015-02-03 23:43:47]

Tested this out on Mac, seems to be good.

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:516: [open_as_window_checkbox
setState:web_app_info.open_as_window];
Don't we still need to set |open_as_window| (or at least set the state of the
checkbox) according to the user's preferences? Since currently |open_as_window|
is set to false in the constructor.

[benwells, 2015-02-04 16:01:00]

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:516: [open_as_window_checkbox
setState:web_app_info.open_as_window];
On 2015/02/03 23:43:47, mitchellj wrote:
> Don't we still need to set |open_as_window| (or at least set the state of the
> checkbox) according to the user's preferences? Since currently
|open_as_window|
> is set to false in the constructor.

Oh yeah, good catch. That logic is about to all disappear and I half removed
it.... I now set the field in bookmark_app_helper.

Thanks for testing!

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc (right):

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:46: int size =
static_cast<int>(dip_size_ * scale);
On 2015/02/03 18:37:49, Lei Zhang wrote:
> Can this overflow? I'm guessing no because the two numbers are small.

That's right, but I've changed it to use the safe utility function we have for
this.

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:47: for (auto
icon_info : info_.icons) {
On 2015/02/03 18:37:49, Lei Zhang wrote:
> const auto& ?

Done.

https://codereview.chromium.org/899443002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:69:
BookmarkAppBubbleView* bookmark_app_bubble =
On 2015/02/03 18:37:49, Lei Zhang wrote:
> Who frees this? Can you add a comment to explain the object lifetime?

Comment added.

[Lei Zhang, 2015-02-04 19:21:31]

lgtm

[benwells, 2015-02-05 02:10:44]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2015-02-05 02:11:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/899443002/120001

[commit-bot: I haz the power, 2015-02-05 02:15:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-05 02:17:21]

Patchset 7 (id:??) landed as
https://crrev.com/da4b8ca729ece8bf351dd68c7288f3e3805da013
Cr-Commit-Position: refs/heads/master@{#314720}