Add deprecation warning banner to App Launcher on Mac.

Add deprecation warning banner to App Launcher on Mac.

Given limited usage of the Chrome App Launcher on Windows, Mac and
Linux, we're planning to remove the Launcher on these platforms later
this year to remove unnecessary complexity in the browser. This does not
affect Chrome OS.

After the removal, users will still be able to access their Chrome Apps
through the Apps bookmark, or system shortcuts.

BUG=576531
Committed: https://crrev.com/192a6f001ffe62cca3b685fcd7ca5e43b7896695
Cr-Commit-Position: refs/heads/master@{#382758}

[tapted, 2016-03-18 01:44:57]

Description was changed from

==========
nit


cl format


nit logging


Real prgoress


First cut


constants -> functions

BUG=576887
==========

to

==========
nit


cl format


nit logging


Real prgoress


First cut


constants -> functions

BUG=576531
==========

[tapted, 2016-03-21 04:51:19]

Description was changed from

==========
nit


cl format


nit logging


Real prgoress


First cut


constants -> functions

BUG=576531
==========

to

==========
Mac: Allow the Cocoa app launcher to show a message

BUG=576531
==========

[tapted, 2016-03-21 04:51:19]

tapted@chromium.org changed reviewers:
+ mgiuca@chromium.org

[tapted, 2016-03-21 05:45:10]

Description was changed from

==========
Mac: Allow the Cocoa app launcher to show a message

BUG=576531
==========

to

==========
Add deprecation warning banner to App Launcher on Mac.

Given limited usage of the Chrome App Launcher on Windows, Mac and
Linux, we're planning to remove the Launcher on these platforms later
this year to remove unnecessary complexity in the browser. This does not
affect Chrome OS.

After the removal, users will still be able to access their Chrome Apps
through the Apps bookmark, or system shortcuts.

BUG=576531
==========

[Matt Giuca, 2016-03-22 03:28:06]

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:886: Use the <ph
name="APPS_ICON_IMAGE">$1<ex>[inline-image] Apps</ex></ph> shortcut in the
Chromium bookmarks bar to continue accessing your apps.
I think you can drop "[inline-image]". The translator doesn't need to know that
and it might just confuse.

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:13301: +           
https://support.google.com/chrome/answer/3060053?hl=[GRITLANGCODE]
Is hl= necessary?

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/google_chro...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/google_chro...
chrome/app/google_chrome_strings.grd:887: Use the <ph
name="APPS_ICON_IMAGE">$1<ex>[inline-image] Apps</ex></ph> shortcut in the
Chrome bookmarks bar to continue accessing your apps.
Ditto.

https://codereview.chromium.org/1802473002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/1802473002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_list_view_delegate.cc:800: return
l10n_util::GetStringUTF16(IDS_APP_LIST_MESSAGE_TEXT);
base::string16 GetMessageText(size_t* offset) const {
  l10n_util::GetStringFUTF16(IDS_APP_LIST_MESSAGE_TEXT, base::string16(),
offset);
}

(see my later comments)

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
ui/app_list/app_list_view_delegate.h:173: virtual base::string16
GetMessageText() const;
// Returns the message text (with the placeholder symbol removed). |offset| is
set to the index where the placeholder was in the string.
virtual base::string16 GetMessageText(size_t* offset) const;

See my comment in app_list_view_controller.mm.

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/cocoa/app_...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/cocoa/app_...
ui/app_list/cocoa/app_list_view_controller.mm:244: size_t messageBreak =
message_full.find(base::ASCIIToUTF16("$1"));
I don't think you should be parsing this yourself. l10n has utilities for doing
this. See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_...
: GetStringFUTF16(int, const string16&, size_t*).

It's hard here because you've built these abstractions to get across the
chrome/browser boundary but I think you should still use GetStringFUTF16 to
parse this. See my suggestion on GetMessageText.

[Matt Giuca, 2016-03-22 03:50:20]

Here is a patch to change the GetMessageText() things (I did it myself because I
want to use it now).

diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.cc
b/chrome/browser/ui/app_list/app_list_view_delegate.cc
index 2e6e388..69486c2 100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.cc
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.cc
@@ -796,8 +796,10 @@ base::string16 AppListViewDelegate::GetMessageTitle() const
{
   return l10n_util::GetStringUTF16(IDS_APP_LIST_MESSAGE_TITLE);
 }
 
-base::string16 AppListViewDelegate::GetMessageText() const {
-  return l10n_util::GetStringUTF16(IDS_APP_LIST_MESSAGE_TEXT);
+base::string16 AppListViewDelegate::GetMessageText(
+    size_t* message_break) const {
+  return l10n_util::GetStringFUTF16(IDS_APP_LIST_MESSAGE_TEXT,
base::string16(),
+                                    message_break);
 }
 
 base::string16 AppListViewDelegate::GetAppsShortcutName() const {
diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.h
b/chrome/browser/ui/app_list/app_list_view_delegate.h
index 3b92d3d4..4453257 100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.h
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.h
@@ -122,7 +122,7 @@ class AppListViewDelegate : public
app_list::AppListViewDelegate,
   void RemoveObserver(app_list::AppListViewDelegateObserver* observer)
override;
 #if !defined(OS_CHROMEOS)
   base::string16 GetMessageTitle() const override;
-  base::string16 GetMessageText() const override;
+  base::string16 GetMessageText(size_t* message_break) const override;
   base::string16 GetAppsShortcutName() const override;
   base::string16 GetLearnMoreText() const override;
   base::string16 GetLearnMoreLink() const override;
diff --git a/ui/app_list/app_list_view_delegate.cc
b/ui/app_list/app_list_view_delegate.cc
index d7d7bee..2ec5b4a 100644
--- a/ui/app_list/app_list_view_delegate.cc
+++ b/ui/app_list/app_list_view_delegate.cc
@@ -17,7 +17,9 @@ AppListViewDelegate::User::~User() {}
 base::string16 AppListViewDelegate::GetMessageTitle() const {
   return base::string16();
 }
-base::string16 AppListViewDelegate::GetMessageText() const {
+base::string16 AppListViewDelegate::GetMessageText(
+    size_t* message_break) const {
+  *message_break = 0;
   return base::string16();
 }
 base::string16 AppListViewDelegate::GetAppsShortcutName() const {
diff --git a/ui/app_list/app_list_view_delegate.h
b/ui/app_list/app_list_view_delegate.h
index 6254633..2a2beca 100644
--- a/ui/app_list/app_list_view_delegate.h
+++ b/ui/app_list/app_list_view_delegate.h
@@ -170,7 +170,10 @@ class APP_LIST_EXPORT AppListViewDelegate {
   // Methods to retrieve properties of the message displayed on the app
launcher
   // above the apps grid.
   virtual base::string16 GetMessageTitle() const;
-  virtual base::string16 GetMessageText() const;
+  // Returns the message text (with the placeholder symbol removed).
+  // |message_break| is set to the index where the placeholder was in the
+  // string.
+  virtual base::string16 GetMessageText(size_t* message_break) const;
   virtual base::string16 GetAppsShortcutName() const;
   virtual base::string16 GetLearnMoreText() const;
   virtual base::string16 GetLearnMoreLink() const;
diff --git a/ui/app_list/cocoa/app_list_view_controller.mm
b/ui/app_list/cocoa/app_list_view_controller.mm
index 2137c60..8861579 100644
--- a/ui/app_list/cocoa/app_list_view_controller.mm
+++ b/ui/app_list/cocoa/app_list_view_controller.mm
@@ -236,15 +236,14 @@ void AppListModelObserverBridge::OnShutdown() {
   NSFont* titleFont = rb.GetFontWithDelta(2).GetNativeFont();
 
   base::string16 title = delegate_->GetMessageTitle();
-  base::string16 message_full = delegate_->GetMessageText();
+  size_t messageBreak;
+  base::string16 message_full = delegate_->GetMessageText(&messageBreak);
   base::string16 shortcutName = delegate_->GetAppsShortcutName();
   base::string16 learnMore = delegate_->GetLearnMoreText();
   base::string16 learnMoreUrl = delegate_->GetLearnMoreLink();
 
-  size_t messageBreak = message_full.find(base::ASCIIToUTF16("$1"));
-  DCHECK_NE(base::string16::npos, messageBreak);
   base::string16 messagePre = message_full.substr(0, messageBreak);
-  base::string16 messagePost = message_full.substr(messageBreak + 2);
+  base::string16 messagePost = message_full.substr(messageBreak);
 
   NSURL* linkURL = [NSURL
URLWithString:base::SysUTF16ToNSString(learnMoreUrl)];
   gfx::ImageSkia* icon = delegate_->GetAppsIcon();

[Matt Giuca, 2016-03-22 03:56:16]

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
ui/app_list/app_list_view_delegate.h:172: virtual base::string16
GetMessageTitle() const;
Also I think these should be =0 and add overrides in AppListTestViewDelegate.
(thoughts?)

[tapted, 2016-03-22 04:19:50]

(no new code yet Coming Real Soon)

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:13301: +           
https://support.google.com/chrome/answer/3060053?hl=[GRITLANGCODE]
On 2016/03/22 03:28:05, Matt Giuca wrote:
> Is hl= necessary?

yup - it's how you get a translated page. E.g.
https://support.google.com/chrome/answer/3060053?hl=ja is in Japanese

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
ui/app_list/app_list_view_delegate.h:172: virtual base::string16
GetMessageTitle() const;
On 2016/03/22 03:56:15, Matt Giuca wrote:
> Also I think these should be =0 and add overrides in AppListTestViewDelegate.
> (thoughts?)

There's precedent for impure virtuals already for the observers above. I don't
think there's any upside to making these pure - just makes the CL bigger and the
#ifdefs more confusing.

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/cocoa/app_...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/cocoa/app_...
ui/app_list/cocoa/app_list_view_controller.mm:244: size_t messageBreak =
message_full.find(base::ASCIIToUTF16("$1"));
On 2016/03/22 03:28:06, Matt Giuca wrote:
> I don't think you should be parsing this yourself. l10n has utilities for
doing
> this. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_...
> : GetStringFUTF16(int, const string16&, size_t*).
> 
> It's hard here because you've built these abstractions to get across the
> chrome/browser boundary but I think you should still use GetStringFUTF16 to
> parse this. See my suggestion on GetMessageText.

oh groovy - I'll patch in your thingy.

[Matt Giuca, 2016-03-22 04:23:43]

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
ui/app_list/app_list_view_delegate.h:172: virtual base::string16
GetMessageTitle() const;
On 2016/03/22 04:19:50, tapted wrote:
> On 2016/03/22 03:56:15, Matt Giuca wrote:
> > Also I think these should be =0 and add overrides in
AppListTestViewDelegate.
> > (thoughts?)
> 
> There's precedent for impure virtuals already for the observers above. I don't
> think there's any upside to making these pure - just makes the CL bigger and
the
> #ifdefs more confusing.

That isn't precedent in my mind -- observers should always be impure. Do these
methods make sense if they are not implemented? If not, they should be pure.

Not that it matters; this code will be short lived. Happy to leave it as-is.

[tapted, 2016-03-22 04:58:15]

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:886: Use the <ph
name="APPS_ICON_IMAGE">$1<ex>[inline-image] Apps</ex></ph> shortcut in the
Chromium bookmarks bar to continue accessing your apps.
On 2016/03/22 03:28:05, Matt Giuca wrote:
> I think you can drop "[inline-image]". The translator doesn't need to know
that
> and it might just confuse.

Done.

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/google_chro...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1802473002/diff/100001/chrome/app/google_chro...
chrome/app/google_chrome_strings.grd:887: Use the <ph
name="APPS_ICON_IMAGE">$1<ex>[inline-image] Apps</ex></ph> shortcut in the
Chrome bookmarks bar to continue accessing your apps.
On 2016/03/22 03:28:06, Matt Giuca wrote:
> Ditto.

Done.

https://codereview.chromium.org/1802473002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/1802473002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_list_view_delegate.cc:800: return
l10n_util::GetStringUTF16(IDS_APP_LIST_MESSAGE_TEXT);
On 2016/03/22 03:28:06, Matt Giuca wrote:
> base::string16 GetMessageText(size_t* offset) const {
>   l10n_util::GetStringFUTF16(IDS_APP_LIST_MESSAGE_TEXT, base::string16(),
> offset);
> }
> 
> (see my later comments)

Acknowledged.

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/app_list_v...
ui/app_list/app_list_view_delegate.h:173: virtual base::string16
GetMessageText() const;
On 2016/03/22 03:28:06, Matt Giuca wrote:
> // Returns the message text (with the placeholder symbol removed). |offset| is
> set to the index where the placeholder was in the string.
> virtual base::string16 GetMessageText(size_t* offset) const;
> 
> See my comment in app_list_view_controller.mm.

Acknowledged.

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/cocoa/app_...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://codereview.chromium.org/1802473002/diff/100001/ui/app_list/cocoa/app_...
ui/app_list/cocoa/app_list_view_controller.mm:244: size_t messageBreak =
message_full.find(base::ASCIIToUTF16("$1"));
On 2016/03/22 04:19:50, tapted wrote:
> On 2016/03/22 03:28:06, Matt Giuca wrote:
> > I don't think you should be parsing this yourself. l10n has utilities for
> doing
> > this. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_...
> > : GetStringFUTF16(int, const string16&, size_t*).
> > 
> > It's hard here because you've built these abstractions to get across the
> > chrome/browser boundary but I think you should still use GetStringFUTF16 to
> > parse this. See my suggestion on GetMessageText.
> 
> oh groovy - I'll patch in your thingy.

Done.

[Matt Giuca, 2016-03-22 05:01:52]

LGTM other than .mm files. I don't think I'm qualified to comment on those so
please get a Cocoa person to review.

And thanks for doing this; it is very helpful to build on for the Views version.

[tapted, 2016-03-22 06:19:40]

tapted@chromium.org changed reviewers:
+ rsesek@chromium.org

[tapted, 2016-03-22 06:19:41]

+rsesek - could you please sanity-check the cocoa changes in ui/app_list/cocoa?

Mocks and screenshots are at http://crbug.com/576531#c5

[Matt Giuca, 2016-03-22 07:45:30]

https://codereview.chromium.org/1802473002/diff/140001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1802473002/diff/140001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:883: The Chromium App Launcher is going away
Can we add a period after this sentence please? (It is a full sentence.)

(The mocks don't have it, but I pointed it out and Ani said to add one;
https://crbug.com/576887#c13.)

https://codereview.chromium.org/1802473002/diff/140001/chrome/app/google_chro...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1802473002/diff/140001/chrome/app/google_chro...
chrome/app/google_chrome_strings.grd:884: The Chrome App Launcher is going away
Ditto.

[tapted, 2016-03-22 07:49:17]

https://codereview.chromium.org/1802473002/diff/140001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/1802473002/diff/140001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:883: The Chromium App Launcher is going away
On 2016/03/22 07:45:30, Matt Giuca wrote:
> Can we add a period after this sentence please? (It is a full sentence.)
> 
> (The mocks don't have it, but I pointed it out and Ani said to add one;
> https://crbug.com/576887#c13.)

Done.

https://codereview.chromium.org/1802473002/diff/140001/chrome/app/google_chro...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1802473002/diff/140001/chrome/app/google_chro...
chrome/app/google_chrome_strings.grd:884: The Chrome App Launcher is going away
On 2016/03/22 07:45:30, Matt Giuca wrote:
> Ditto.

Done.

[Robert Sesek, 2016-03-22 14:24:11]

LGTM, though don't feel strongly about the comment since the code is
short-lived.

https://codereview.chromium.org/1802473002/diff/160001/ui/app_list/cocoa/app_...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://codereview.chromium.org/1802473002/diff/160001/ui/app_list/cocoa/app_...
ui/app_list/cocoa/app_list_view_controller.mm:231: if (![AppsGridController
hasFewerRows])
This is large enough to be its own method.

[tapted, 2016-03-23 00:27:22]

Thanks Robert!

https://codereview.chromium.org/1802473002/diff/160001/ui/app_list/cocoa/app_...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://codereview.chromium.org/1802473002/diff/160001/ui/app_list/cocoa/app_...
ui/app_list/cocoa/app_list_view_controller.mm:231: if (![AppsGridController
hasFewerRows])
On 2016/03/22 14:24:11, Robert Sesek wrote:
> This is large enough to be its own method.

Done.

[tapted, 2016-03-23 00:32:46]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-03-23 00:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1802473002/#ps180001
(title: "Separate function")

[commit-bot: I haz the power, 2016-03-23 00:33:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802473002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802473002/180001

[commit-bot: I haz the power, 2016-03-23 01:22:02]

Description was changed from

==========
Add deprecation warning banner to App Launcher on Mac.

Given limited usage of the Chrome App Launcher on Windows, Mac and
Linux, we're planning to remove the Launcher on these platforms later
this year to remove unnecessary complexity in the browser. This does not
affect Chrome OS.

After the removal, users will still be able to access their Chrome Apps
through the Apps bookmark, or system shortcuts.

BUG=576531
==========

to

==========
Add deprecation warning banner to App Launcher on Mac.

Given limited usage of the Chrome App Launcher on Windows, Mac and
Linux, we're planning to remove the Launcher on these platforms later
this year to remove unnecessary complexity in the browser. This does not
affect Chrome OS.

After the removal, users will still be able to access their Chrome Apps
through the Apps bookmark, or system shortcuts.

BUG=576531
==========

[commit-bot: I haz the power, 2016-03-23 01:22:03]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-23 01:23:59]

Description was changed from

==========
Add deprecation warning banner to App Launcher on Mac.

Given limited usage of the Chrome App Launcher on Windows, Mac and
Linux, we're planning to remove the Launcher on these platforms later
this year to remove unnecessary complexity in the browser. This does not
affect Chrome OS.

After the removal, users will still be able to access their Chrome Apps
through the Apps bookmark, or system shortcuts.

BUG=576531
==========

to

==========
Add deprecation warning banner to App Launcher on Mac.

Given limited usage of the Chrome App Launcher on Windows, Mac and
Linux, we're planning to remove the Launcher on these platforms later
this year to remove unnecessary complexity in the browser. This does not
affect Chrome OS.

After the removal, users will still be able to access their Chrome Apps
through the Apps bookmark, or system shortcuts.

BUG=576531

Committed: https://crrev.com/192a6f001ffe62cca3b685fcd7ca5e43b7896695
Cr-Commit-Position: refs/heads/master@{#382758}
==========

[commit-bot: I haz the power, 2016-03-23 01:24:00]

Patchset 10 (id:??) landed as
https://crrev.com/192a6f001ffe62cca3b685fcd7ca5e43b7896695
Cr-Commit-Position: refs/heads/master@{#382758}