Polish UI of bookmark app creation.

Polish UI of bookmark app creation.

This change bundles together a few minor UI improvements to the bookmark
app creation flow:
- The language for opening as a window vs. a tab is now always 'open as
window', instead of sometimes being 'open as tab'.
- The menu item is now called 'Add to XXXX' where XXXX is the platform
specific shelf / dock / taskbar / desktop.
- The app is now added to the shelf / dock / taskbar / desktop
- Empty or only-whitespace names can no longer be provided.

BUG=452830, 450102, 447385, 447384
Committed: https://crrev.com/e22df36a446da603acd37115a0bea62bba7c8a73
Cr-Commit-Position: refs/heads/master@{#315184}

[benwells, 2015-02-06 15:29:46]

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

[benwells, 2015-02-06 15:29:46]

sashab for initial review.

[sashab, 2015-02-06 15:35:44]

Publishing some small comments early; still reviewing.

https://codereview.chromium.org/907473002/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/907473002/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2219: +        <message
name="IDS_ADD_TO_TASKBAR" desc="Button text for adding a bookmark app to the
taskbar.">
Could these all have the same message ID? Maybe IDS_ADD_TO_LOCAL_LAUNCHER.

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

https://codereview.chromium.org/907473002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:424: // Mac.
nit: "Pin the app to the shelf/taskbar/dock on Ash/Windows/Mac". Or even just
"Pin the app to its relevant launcher depending on the OS."

[benwells, 2015-02-06 15:38:50]

https://codereview.chromium.org/907473002/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/907473002/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2219: +        <message
name="IDS_ADD_TO_TASKBAR" desc="Button text for adding a bookmark app to the
taskbar.">
On 2015/02/06 15:35:44, sasha_b wrote:
> Could these all have the same message ID? Maybe IDS_ADD_TO_LOCAL_LAUNCHER.

They could, and I started doing it like that, but I think that is more
confusing. Like this it is pretty clear what each string ID is for.

[sashab, 2015-02-06 15:45:22]

lgtm, although not super familiar with the code in this area

https://codereview.chromium.org/907473002/diff/40001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/907473002/diff/40001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/wrench_menu_model.cc:248: int string_id =
IDS_ADD_TO_DESKTOP;
Not needed if they all use the same string ID.

[benwells, 2015-02-06 15:51:08]

New patchsets have been uploaded after l-g-t-m from sashab@chromium.org

[benwells, 2015-02-06 15:52:17]

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

https://codereview.chromium.org/907473002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:424: // Mac.
On 2015/02/06 15:35:44, sasha_b wrote:
> nit: "Pin the app to the shelf/taskbar/dock on Ash/Windows/Mac". Or even just
> "Pin the app to its relevant launcher depending on the OS."

Done.

https://codereview.chromium.org/907473002/diff/40001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/907473002/diff/40001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/wrench_menu_model.cc:248: int string_id =
IDS_ADD_TO_DESKTOP;
On 2015/02/06 15:45:22, sasha_b wrote:
> Not needed if they all use the same string ID.

Yes I understand. As described earlier I think it is clearer like this (in
particular, for translators). Let's see what owners think, maybe they'll have an
opinion...

[benwells, 2015-02-06 15:52:43]

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

[benwells, 2015-02-06 15:53:03]

+thestig for owners review

[Dan Beam, 2015-02-06 16:29:28]

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

https://codereview.chromium.org/907473002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:248:
base::TrimWhitespace(title, base::TRIM_ALL, &title);
drive-by nit:

  base::string16 GetTrimmedTitle() {
    base::string16 title(title_tf_->text());
    base::TrimWhitespace(title, base::TRIM_ALL, &title);
    return title;
  }

  ...
  web_app_info_.title = GetTrimmedTitle();
  ...
  add_button_->SetEnabled(!GetTrimmedTitle().empty());

https://codereview.chromium.org/907473002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:249:
add_button_->SetEnabled(!title.empty());
nit: is this UI only written on views?  seems like this could be at some
platform-independent layer to avoid re-writing.

[benwells, 2015-02-06 16:49:29]

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

https://codereview.chromium.org/907473002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:248:
base::TrimWhitespace(title, base::TRIM_ALL, &title);
On 2015/02/06 16:29:28, Dan Beam wrote:
> drive-by nit:
> 
>   base::string16 GetTrimmedTitle() {
>     base::string16 title(title_tf_->text());
>     base::TrimWhitespace(title, base::TRIM_ALL, &title);
>     return title;
>   }
> 
>   ...
>   web_app_info_.title = GetTrimmedTitle();
>   ...
>   add_button_->SetEnabled(!GetTrimmedTitle().empty());

Done.

https://codereview.chromium.org/907473002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:249:
add_button_->SetEnabled(!title.empty());
On 2015/02/06 16:29:28, Dan Beam wrote:
> nit: is this UI only written on views?  seems like this could be at some
> platform-independent layer to avoid re-writing.

This is also on cocoa. I haven't made this change there yet as I don't have easy
access to a mac. When we do that we can extract this out.

[Lei Zhang, 2015-02-06 22:41:28]

lgtm

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

https://codereview.chromium.org/907473002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:230:
DCHECK(sender == title_tf_);
DCHECK_EQ()

https://codereview.chromium.org/907473002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.h (right):

https://codereview.chromium.org/907473002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.h:64: // Overidden
from views::TextfieldController:
Overridden

[benwells, 2015-02-07 03:15:47]

New patchsets have been uploaded after l-g-t-m from thestig@chromium.org

[benwells, 2015-02-07 03:17:38]

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

https://codereview.chromium.org/907473002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.cc:230:
DCHECK(sender == title_tf_);
On 2015/02/06 22:41:27, Lei Zhang wrote:
> DCHECK_EQ()

Done.

https://codereview.chromium.org/907473002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/bookmark_app_bubble_view.h (right):

https://codereview.chromium.org/907473002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/bookmark_app_bubble_view.h:64: // Overidden
from views::TextfieldController:
On 2015/02/06 22:41:28, Lei Zhang wrote:
> Overridden

Done.

[benwells, 2015-02-07 03:17:46]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2015-02-07 03:18:39]

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

[commit-bot: I haz the power, 2015-02-07 04:34:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-07 04:35:57]

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