NUX Email - Add icons to the bookmark cache for the selected email.

CL also includes a few minor UI fixes to match mocks and a11y.

Bug: 832933
Change-Id: Ibb9fd594b80d79692593e65f00636fbd7115eb3e
Reviewed-on: https://chromium-review.googlesource.com/1212265
Commit-Queue: Hector Carmona <hcarmona@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590165}

chrome/browser/ui/webui/welcome_ui.cc

@@ -130,13 +130,9 @@ WelcomeUI::WelcomeUI(content::WebUI* web_ui, const GURL& url)
 
   if (onboard_during_nux &&
       base::FeatureList::IsEnabled(nux::kNuxEmailFeature)) {
-    content::BrowserContext* browser_context =
-        web_ui->GetWebContents()->GetBrowserContext();
     web_ui->AddMessageHandler(std::make_unique<nux::EmailHandler>(
-        profile->GetPrefs(),
-        FaviconServiceFactory::GetForProfile(
-            profile, ServiceAccessType::EXPLICIT_ACCESS),
-        BookmarkModelFactory::GetForBrowserContext(browser_context)));
+        profile->GetPrefs(), FaviconServiceFactory::GetForProfile(
+                                 profile, ServiceAccessType::EXPLICIT_ACCESS)));
 
     nux::EmailHandler::AddSources(html_source, profile->GetPrefs());
   }

components/nux/email/email_handler.cc

@@ -9,7 +9,6 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/common/bookmark_pref_names.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/grit/components_resources.h"
@@ -37,9 +36,9 @@ enum class EmailProviders {
 
 struct EmailProviderType {
   const EmailProviders id;
-  const char* name;
+  const char* name;  // Icon in WebUI should use name to match CSS.
   const char* url;
-  const int icon;
+  const int icon;  // Corresponds with resource ID, used for bookmark cache.
 };
 
 const char* kEmailInteractionHistogram =
@@ -50,12 +49,14 @@ const char* kEmailInteractionHistogram =
 // TODO(hcarmona): populate with icon ids.
 const EmailProviderType kEmail[] = {
     {EmailProviders::kGmail, "Gmail",
-     "https://accounts.google.com/b/0/AddMailService", 0},
-    {EmailProviders::kYahoo, "Yahoo", "https://mail.yahoo.com", 0},
+     "https://accounts.google.com/b/0/AddMailService", IDR_NUX_EMAIL_GMAIL_1X},
+    {EmailProviders::kYahoo, "Yahoo", "https://mail.yahoo.com",
+     IDR_NUX_EMAIL_YAHOO_1X},
     {EmailProviders::kOutlook, "Outlook", "https://login.live.com/login.srf?",
-     0},
-    {EmailProviders::kAol, "AOL", "https://mail.aol.com", 0},
-    {EmailProviders::kiCloud, "iCloud", "https://www.icloud.com/mail", 0},
+     IDR_NUX_EMAIL_OUTLOOK_1X},
+    {EmailProviders::kAol, "AOL", "https://mail.aol.com", IDR_NUX_EMAIL_AOL_1X},
+    {EmailProviders::kiCloud, "iCloud", "https://www.icloud.com/mail",
+     IDR_NUX_EMAIL_ICLOUD_1X},
 };
 
 constexpr const int kEmailIconSize = 48;  // Pixels.
@@ -64,22 +65,15 @@ static_assert(base::size(kEmail) == (size_t)EmailProviders::kCount,
               "names and histograms must match");
 
 EmailHandler::EmailHandler(PrefService* prefs,
-                           favicon::FaviconService* favicon_service,
-                           bookmarks::BookmarkModel* bookmark_model)
-    : prefs_(prefs),
-      favicon_service_(favicon_service),
-      bookmark_model_(bookmark_model) {}
+                           favicon::FaviconService* favicon_service)
+    : prefs_(prefs), favicon_service_(favicon_service) {}
 
 EmailHandler::~EmailHandler() {}
 
 void EmailHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
-      "rejectEmails", base::BindRepeating(&EmailHandler::HandleRejectEmails,
-                                          base::Unretained(this)));
-
-  web_ui()->RegisterMessageCallback(
-      "addEmails", base::BindRepeating(&EmailHandler::HandleAddEmails,
-                                       base::Unretained(this)));
+      "cacheEmailIcon", base::BindRepeating(&EmailHandler::HandleCacheEmailIcon,
+                                            base::Unretained(this)));
 
   web_ui()->RegisterMessageCallback(
       "toggleBookmarkBar",
@@ -87,47 +81,28 @@ void EmailHandler::RegisterMessages() {
                           base::Unretained(this)));
 }
 
-void EmailHandler::HandleRejectEmails(const base::ListValue* args) {
-  /* TODO(hcarmona): Add histograms and uncomment this code.
-  UMA_HISTOGRAM_ENUMERATION(kEmailInteractionHistogram,
-                            EmailInteraction::kNoThanks,
-                            EmailInteraction::kCount);
-  */
-}
+void EmailHandler::HandleCacheEmailIcon(const base::ListValue* args) {
+  int emailId;
+  args->GetInteger(0, &emailId);
 
-void EmailHandler::HandleAddEmails(const base::ListValue* args) {
-  // Add bookmarks for all selected emails.
-  int bookmarkIndex = 0;
-  for (size_t i = 0; i < (size_t)EmailProviders::kCount; ++i) {
-    bool selected = false;
-    CHECK(args->GetBoolean(i, &selected));
-    if (selected) {
-      /* TODO(hcarmona): Add histograms and uncomment this code.
-      UMA_HISTOGRAM_ENUMERATION("FirstRun.NewUserExperience.EmailSelection",
-                                (EmailProviders)i,
-                                EmailProviders::kCount);
-      */
-      GURL app_url = GURL(kEmail[i].url);
-      bookmark_model_->AddURL(bookmark_model_->bookmark_bar_node(),
-                              bookmarkIndex++,
-                              base::ASCIIToUTF16(kEmail[i].name), app_url);
-
-      // Preload the favicon cache with Chrome-bundled images. Otherwise, the
-      // pre-populated bookmarks don't have favicons and look bad. Favicons are
-      // updated automatically when a user visits a site.
-      favicon_service_->MergeFavicon(
-          app_url, app_url, favicon_base::IconType::kFavicon,
-          ui::ResourceBundle::GetSharedInstance().LoadDataResourceBytes(
-              kEmail[i].icon),
-          gfx::Size(kEmailIconSize, kEmailIconSize));
+  const EmailProviderType* selectedEmail = NULL;
+  for (size_t i = 0; i < base::size(kEmail); i++) {
+    if ((int)kEmail[i].id == emailId) {
+      selectedEmail = &kEmail[i];
+      break;
     }
   }
-
-  /* TODO(hcarmona): Add histograms and uncomment this code.
-  UMA_HISTOGRAM_ENUMERATION(kEmailInteractionHistogram,
-                            EmailInteraction::kGetStarted,
-                            EmailInteraction::kCount);
-  */
+  CHECK(selectedEmail);  // WebUI should not be able to pass non-existent ID.
+
+  // Preload the favicon cache with Chrome-bundled images. Otherwise, the
+  // pre-populated bookmarks don't have favicons and look bad. Favicons are
+  // updated automatically when a user visits a site.
+  GURL app_url = GURL(selectedEmail->url);
+  favicon_service_->MergeFavicon(
+      app_url, app_url, favicon_base::IconType::kFavicon,
+      ui::ResourceBundle::GetSharedInstance().LoadDataResourceBytes(
+          selectedEmail->icon),
+      gfx::Size(kEmailIconSize, kEmailIconSize));
 }
 
 void EmailHandler::HandleToggleBookmarkBar(const base::ListValue* args) {
@@ -143,6 +118,12 @@ void EmailHandler::AddSources(content::WebUIDataSource* html_source,
   html_source->AddLocalizedString("getStarted", IDS_NUX_EMAIL_GET_STARTED);
   html_source->AddLocalizedString("welcomeTitle", IDS_NUX_EMAIL_WELCOME_TITLE);
   html_source->AddLocalizedString("emailPrompt", IDS_NUX_EMAIL_PROMPT);
+  html_source->AddLocalizedString("bookmarkAdded",
+                                  IDS_NUX_EMAIL_BOOKMARK_ADDED);
+  html_source->AddLocalizedString("bookmarkRemoved",
+                                  IDS_NUX_EMAIL_BOOKMARK_REMOVED);
+  html_source->AddLocalizedString("bookmarkReplaced",
+                                  IDS_NUX_EMAIL_BOOKMARK_REPLACED);
 
   // Add required resources.
   html_source->AddResourcePath("email", IDR_NUX_EMAIL_HTML);

components/nux/email/email_handler.h

@@ -11,10 +11,6 @@
 
 class PrefService;
 
-namespace bookmarks {
-class BookmarkModel;
-}  // namespace bookmarks
-
 namespace content {
 class WebUIDataSource;
 }  // namespace content
@@ -38,17 +34,14 @@ enum class EmailInteraction {
 
 class EmailHandler : public content::WebUIMessageHandler {
  public:
-  EmailHandler(PrefService* prefs,
-               favicon::FaviconService* favicon_service,
-               bookmarks::BookmarkModel* bookmark_model);
+  EmailHandler(PrefService* prefs, favicon::FaviconService* favicon_service);
   ~EmailHandler() override;
 
   // WebUIMessageHandler:
   void RegisterMessages() override;
 
   // Callbacks for JS APIs.
-  void HandleRejectEmails(const base::ListValue* args);
-  void HandleAddEmails(const base::ListValue* args);
+  void HandleCacheEmailIcon(const base::ListValue* args);
   void HandleToggleBookmarkBar(const base::ListValue* args);
 
   // Adds webui sources.
@@ -62,9 +55,6 @@ class EmailHandler : public content::WebUIMessageHandler {
   // Weak reference.
   favicon::FaviconService* favicon_service_;
 
-  // Weak reference.
-  bookmarks::BookmarkModel* bookmark_model_;
-
   DISALLOW_COPY_AND_ASSIGN(EmailHandler);
 };
 

components/nux/email/resources/email_chooser.html

@@ -3,6 +3,8 @@
 <link rel="import" href="chrome://resources/cr_elements/icons.html">
 <link rel="import" href="chrome://resources/cr_elements/shared_vars_css.html">
 <link rel="import" href="chrome://resources/cr_elements/paper_button_style_css.html">
+<link rel="import" href="chrome://resources/html/i18n_behavior.html">
+<link rel="import" href="chrome://resources/polymer/v1_0/iron-a11y-announcer/iron-a11y-announcer.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
 <link rel="import" href="chrome://welcome/email/nux_email_proxy.html">
@@ -11,7 +13,7 @@
   <template>
     <style include="paper-button-style">
       :host {
-        display: initial;
+        display: block;
         white-space: nowrap;
       }
 
@@ -34,7 +36,7 @@
         outline: 0;
       }
 
-      .option:not(:first-child) {
+      .option:not(:first-of-type) {
         margin-inline-start: 24px;
       }
 

components/nux/email/resources/email_chooser.js

@@ -19,6 +19,9 @@ nux_email.EmailProviderModel;
 
 Polymer({
   is: 'email-chooser',
+
+  behaviors: [I18nBehavior],
+
   properties: {
     emailList: Array,
 
@@ -29,7 +32,7 @@ Polymer({
     },
 
     /** @private */
-    gotStarted_: Boolean,
+    finalized_: Boolean,
 
     /** @private {nux_email.EmailProviderModel} */
     selectedEmailProvider_: {
@@ -42,6 +45,13 @@ Polymer({
   /** @private {NuxEmailProxy} */
   browserProxy_: null,
 
+  /** @override */
+  attached: function() {
+    Polymer.RenderStatus.afterNextRender(this, function() {
+      Polymer.IronA11yAnnouncer.requestAvailability();
+    });
+  },
+
   /** @override */
   ready: function() {
     this.browserProxy_ = nux.NuxEmailProxyImpl.getInstance();
@@ -50,13 +60,16 @@ Polymer({
     this.emailList = this.browserProxy_.getEmailList();
 
     window.addEventListener('beforeunload', () => {
-      // Only need to clean up if user didn't choose "Get Started".
-      if (this.gotStarted_)
+      // Only need to clean up if user didn't interact with the buttons.
+      if (this.finalized_)
         return;
 
-      this.revertBookmark_();
+      if (this.selectedEmailProvider_) {
+        this.browserProxy_.recordProviderSelected(
+            this.selectedEmailProvider_.id);
+      }
+
       this.browserProxy_.recordFinalize();
-      this.browserProxy_.toggleBookmarkBar(this.bookmarkBarWasShown_);
     });
   },
 
@@ -125,24 +138,42 @@ Polymer({
     if (newEmail) {
       this.browserProxy_.toggleBookmarkBar(true);
       this.browserProxy_.addBookmark(
-          {title: newEmail.name, url: newEmail.url, parentId: '1'}, results => {
+          {
+            title: newEmail.name,
+            url: newEmail.url,
+            parentId: '1',
+          },
+          newEmail.id, results => {
             this.selectedEmailProvider_.bookmarkId = results.id;
           });
     } else {
       this.browserProxy_.toggleBookmarkBar(this.bookmarkBarWasShown_);
     }
+
+    // Announcements are mutually exclusive, so keeping separate.
+    if (prevEmail && newEmail) {
+      this.fire('iron-announce', {text: this.i18n('bookmarkReplaced')});
+    } else if (prevEmail) {
+      this.fire('iron-announce', {text: this.i18n('bookmarkRemoved')});
+    } else if (newEmail) {
+      this.fire('iron-announce', {text: this.i18n('bookmarkAdded')});
+    }
   },
 
   /** @private */
   onNoThanksClicked_: function() {
+    this.finalized_ = true;
+    this.revertBookmark_();
+    this.browserProxy_.toggleBookmarkBar(this.bookmarkBarWasShown_);
     this.browserProxy_.recordNoThanks();
     window.location.replace('chrome://newtab');
   },
 
   /** @private */
   onGetStartedClicked_: function() {
-    this.gotStarted_ = true;
-    this.browserProxy_.recordGetStarted(this.selectedEmailProvider_.id);
+    this.finalized_ = true;
+    this.browserProxy_.recordProviderSelected(this.selectedEmailProvider_.id);
+    this.browserProxy_.recordGetStarted();
     window.location.replace(this.selectedEmailProvider_.url);
   },
 

components/nux/email/resources/nux_email.html

@@ -7,7 +7,6 @@
   <script src="strings.js"></script>
   <link rel="import" href="chrome://resources/html/polymer.html">
   <link rel="import" href="chrome://welcome/email/email_chooser.html">
-  <link rel="import" href="chrome://welcome/email/nux_email_proxy.html">
   <link rel="stylesheet" href="chrome://resources/css/text_defaults_md.css">
   <style>
     body {
@@ -21,21 +20,24 @@
         :host {
           align-items: center;
           display: flex;
-          height: 100vh;
-          justify-content: space-around;
+          height: fit-content;
+          margin: auto;
+          min-height: 100vh;
+          width: fit-content;
         }
 
         .email-ask {
           text-align: center;
-          width: 800px;
         }
 
         .email-logo {
-          background: red; /* TODO(scottchen): replace with logo. */
-          height: 36px;
+          content: -webkit-image-set(
+            url('chrome://welcome/logo.png') 1x,
+            url('chrome://welcome/logo2x.png') 2x);
+          height: 48px;
           margin: auto;
           margin-bottom: 16px;
-          width: 36px;
+          width: 48px;
         }
 
         h1 {
@@ -61,7 +63,7 @@
         }
       </style>
       <div class="email-ask">
-        <div class="email-logo"></div>
+        <div class="email-logo" alt=""></div>
         <h1>$i18n{welcomeTitle}</h1>
         <h2>$i18n{emailPrompt}</h2>
         <email-chooser id="emailChooser"></email-chooser>

components/nux/email/resources/nux_email_proxy.js

@@ -44,15 +44,34 @@ cr.define('nux', function() {
 
     /**
      * @param {!bookmarkData} data
+     * @param {number} emailProviderId
      * @param {!Function} callback
      */
-    addBookmark(data, callback) {}
+    addBookmark(data, emailProviderId, callback) {}
 
     /** @param {boolean} show */
     toggleBookmarkBar(show) {}
 
     /** @return {!Array<Object>} Array of email providers. */
     getEmailList() {}
+
+    recordPageInitialized() {}
+
+    recordClickedOption() {}
+
+    recordClickedDisabledButton() {}
+
+    /**
+     * @param {number} providerId This should match one of the histogram enum
+     *     value for NuxEmailProvidersSelections.
+     */
+    recordProviderSelected(providerId) {}
+
+    recordNoThanks() {}
+
+    recordGetStarted() {}
+
+    recordFinalize() {}
   }
 
   /** @implements {NuxEmailProxy} */
@@ -63,9 +82,9 @@ cr.define('nux', function() {
     }
 
     /** @override */
-    addBookmark(data, callback) {
+    addBookmark(data, emailProviderId, callback) {
       chrome.bookmarks.create(data, callback);
-      // TODO(scottchen): request C++ to cache favicon
+      chrome.send('cacheEmailIcon', [emailProviderId]);
     }
 
     /** @override */
@@ -88,6 +107,7 @@ cr.define('nux', function() {
       return emailList;
     }
 
+    /** @override */
     recordPageInitialized() {
       chrome.metricsPrivate.recordEnumerationValue(
           INTERACTION_METRIC_NAME, NuxEmailProvidersInteractions.PageShown,
@@ -100,47 +120,46 @@ cr.define('nux', function() {
       this.lastPart = 'AndNavigatedAway';
     }
 
+    /** @override */
     recordClickedOption() {
       // Only overwrite this.firstPart if it's not overwritten already
       if (this.firstPart == 'DidNothing')
         this.firstPart = 'ChoseAnOption';
     }
 
+    /** @override */
     recordClickedDisabledButton() {
       // Only overwrite this.firstPart if it's not overwritten already
       if (this.firstPart == 'DidNothing')
         this.firstPart = 'ClickedDisabledNextButton';
     }
 
+    /** @override */
+    recordProviderSelected(providerId) {
+      chrome.metricsPrivate.recordEnumerationValue(
+          SELECTION_METRIC_NAME, providerId,
+          loadTimeData.getInteger('email_count'));
+    }
+
+    /** @override */
     recordNoThanks() {
       this.lastPart = 'AndChoseSkip';
       this.recordFinalize();
     }
 
-    /**
-     * @param {number} providerId This should match one of the histogram enum
-     *     value for NuxEmailProvidersSelections.
-     */
-    recordGetStarted(providerId) {
-      chrome.metricsPrivate.recordEnumerationValue(
-          SELECTION_METRIC_NAME, providerId,
-          loadTimeData.getInteger('email_count'));
-
+    /** @override */
+    recordGetStarted() {
       this.lastPart = 'AndChoseNext';
       this.recordFinalize();
     }
 
+    /** @override */
     recordFinalize() {
-      // Don't record again if it's already recorded before unloading.
-      if (this.finalized)
-        return;
-
       let finalValue = this.firstPart + this.lastPart;
 
       chrome.metricsPrivate.recordEnumerationValue(
           INTERACTION_METRIC_NAME, NuxEmailProvidersInteractions[finalValue],
           INTERACTION_METRIC_COUNT);
-      this.finalized = true;
     }
   }
 

components/nux_email_strings.grdp

@@ -9,4 +9,13 @@
   <message name="IDS_NUX_EMAIL_WELCOME_TITLE" desc="Text shown to welcome the users to chrome.">
     Make Chrome your own
   </message>
+  <message name="IDS_NUX_EMAIL_BOOKMARK_ADDED" desc="String read for accessibility to inform the user a bookmark was added.">
+    Bookmark added
+  </message>
+  <message name="IDS_NUX_EMAIL_BOOKMARK_REMOVED" desc="String read for accessibility to inform the user a bookmark was removed.">
+    Bookmark removed
+  </message>
+  <message name="IDS_NUX_EMAIL_BOOKMARK_REPLACED" desc="String read for accessibility to inform the user a bookmark was replaced.">
+    Bookmark replaced
+  </message>
 </grit-part>