Add a delete and mark as read/unread button to Read later menu items.

This change is behind the kReadLater feature flag and uses styles shared
with the Tab Search feature. The delete button removes entries from the
reading list model and the mark as read/unread button updates the
status. Both buttons will trigger an update to the menu items so changes
are seen as they are triggered.

Bug: 1109316
Change-Id: Iaa093427c4f9be78901833eca3fd1dabec8ec1b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422246
Commit-Queue: Caroline Rising <corising@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Reviewed-by: dpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810050}

chrome/app/generated_resources.grd

@@ -5693,6 +5693,12 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_READ_LATER_MENU_READ_HEADER" desc="Header for section of read Read later items.">
         PAGES YOU'VE READ
       </message>
+      <message name="IDS_READ_LATER_MENU_TOOLTIP_MARK_AS_READ" desc="Tooltip for the button to mark a read later entry as read.">
+        Mark as read
+      </message>
+      <message name="IDS_READ_LATER_MENU_TOOLTIP_MARK_AS_UNREAD" desc="Tooltip for the button to mark a read later entry as unread.">
+        Mark as unread
+      </message>
 
       <!--Tooltip strings-->
       <message name="IDS_TOOLTIP_BACK" desc="The tooltip for back button">

chrome/app/generated_resources_grd/IDS_READ_LATER_MENU_TOOLTIP_MARK_AS_READ.png.sha1

+12b98f99d75698c0a0ef26743f25c7dbd3457cd2
\ No newline at end of file

chrome/app/generated_resources_grd/IDS_READ_LATER_MENU_TOOLTIP_MARK_AS_UNREAD.png.sha1

+5f987e96bfd6c48895c012a4abdd40306e50e24d
\ No newline at end of file

chrome/browser/resources/read_later/BUILD.gn

@@ -31,7 +31,9 @@ js_library("read_later_api_proxy") {
 
 js_library("read_later_item") {
   deps = [
+    ":read_later_api_proxy",
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
+    "//ui/webui/resources/cr_elements/cr_icon_button:cr_icon_button.m",
   ]
 }
 

chrome/browser/resources/read_later/app.html

@@ -35,13 +35,11 @@
   <div class="sub-heading">$i18n{unreadHeader}</div>
   <template id="ureadItemsList" is="dom-repeat" items="[[unreadItems_]]">
     <read-later-item data-url$="[[item.url]]" class="mwb-list-item"
-        data="[[item]]" on-click="onItemClick_">
-    </read-later-item>
+        data="[[item]]"></read-later-item>
   </template>
   <div class="sub-heading">$i18n{readHeader}</div>
   <template id="readItemsList" is="dom-repeat" items="[[readItems_]]">
     <read-later-item data-url$="[[item.url]]" class="mwb-list-item"
-        data="[[item]]" on-click="onItemClick_">
-    </read-later-item>
+        data="[[item]]"></read-later-item>
   </template>
 </div>

chrome/browser/resources/read_later/app.js

@@ -34,27 +34,40 @@ export class ReadLaterAppElement extends PolymerElement {
     super();
     /** @private {!ReadLaterApiProxy} */
     this.apiProxy_ = ReadLaterApiProxyImpl.getInstance();
+
+    /** @private {?number} */
+    this.listenerId_ = null;
   }
 
   /** @override */
   ready() {
     super.ready();
+    this.updateItems_();
+  }
+
+  /** @override */
+  connectedCallback() {
+    super.connectedCallback();
+    const callbackRouter = this.apiProxy_.getCallbackRouter();
+    this.listenerId_ =
+        callbackRouter.itemsChanged.addListener(() => this.updateItems_());
+  }
 
+  /** @override */
+  disconnectedCallback() {
+    super.disconnectedCallback();
+    this.apiProxy_.getCallbackRouter().removeListener(
+        /** @type {number} */ (this.listenerId_));
+    this.listenerId_ = null;
+  }
+
+  /** @private */
+  updateItems_() {
     this.apiProxy_.getReadLaterEntries().then(({entries}) => {
       this.unreadItems_ = entries.unreadEntries;
       this.readItems_ = entries.readEntries;
     });
   }
-
-  /**
-   * @param {!Event} e
-   * @private
-   */
-  onItemClick_(e) {
-    const item =
-        /** @type {!ReadLaterItemElement} */ (e.currentTarget);
-    this.apiProxy_.openSavedEntry(item.data.url);
-  }
 }
 
 customElements.define(ReadLaterAppElement.is, ReadLaterAppElement);

chrome/browser/resources/read_later/read_later_api_proxy.js

@@ -18,6 +18,18 @@ export class ReadLaterApiProxy {
 
   /** @param {!url.mojom.Url} url */
   openSavedEntry(url) {}
+
+  /**
+   * @param {!url.mojom.Url} url
+   * @param {boolean} read
+   */
+  updateReadStatus(url, read) {}
+
+  /** @param {!url.mojom.Url} url */
+  removeEntry(url) {}
+
+  /** @return {!readLater.mojom.PageCallbackRouter} */
+  getCallbackRouter() {}
 }
 
 /** @implements {ReadLaterApiProxy} */
@@ -44,6 +56,21 @@ export class ReadLaterApiProxyImpl {
   openSavedEntry(url) {
     this.handler.openSavedEntry(url);
   }
+
+  /** @override */
+  updateReadStatus(url, read) {
+    this.handler.updateReadStatus(url, read);
+  }
+
+  /** @override */
+  removeEntry(url) {
+    this.handler.removeEntry(url);
+  }
+
+  /** @override */
+  getCallbackRouter() {
+    return this.callbackRouter;
+  }
 }
 
 addSingletonGetter(ReadLaterApiProxyImpl);

chrome/browser/resources/read_later/read_later_item.html

 <style>
+  :host(:hover) .button-container,
+  :host(.selected) .button-container {
+    display: flex;
+  }
+
+  .button-container {
+    display: none;
+  }
+
   .text-container {
     flex-grow: 1;
     overflow: hidden;
@@ -22,6 +31,12 @@
     color: var(--cr-secondary-text-color);
     font-size: var(--mwb-secondary-text-font-size);
   }
+
+  cr-icon-button {
+    --cr-icon-button-margin-end: calc(var(--mwb-icon-size) / 4);
+    --cr-icon-button-margin-start: calc(var(--mwb-icon-size) / 4);
+    --cr-icon-button-size: var(--mwb-icon-size);
+  }
 </style>
 
 <div class="text-container">
@@ -30,3 +45,16 @@
     [[data.displayUrl]] - [[data.displayTimeSinceUpdate]]
   </div>
 </div>
+<div class="button-container">
+  <cr-icon-button id="updateStatusButton"
+      aria-label="[[getUpdateStatusButtonTooltip_(
+      '$i18n{tooltipMarkAsUnread}', '$i18n{tooltipMarkAsRead}', data.read)]]"
+      iron-icon="cr:check" title="[[getUpdateStatusButtonTooltip_(
+      '$i18n{tooltipMarkAsUnread}', '$i18n{tooltipMarkAsRead}', data.read)]]"
+      on-click="onUpdateStatusClick_">
+  </cr-icon-button>
+  <cr-icon-button id="deleteButton" aria-label="$i18n{tooltipDelete}"
+      iron-icon="cr:close" title="$i18n{tooltipDelete}"
+      on-click="onItemDeleteClick_">
+  </cr-icon-button>
+</div>

chrome/browser/resources/read_later/read_later_item.js

@@ -2,11 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'chrome://resources/cr_elements/cr_icon_button/cr_icon_button.m.js';
+import 'chrome://resources/cr_elements/cr_icons_css.m.js';
+import 'chrome://resources/cr_elements/icons.m.js';
 import 'chrome://resources/cr_elements/mwb_shared_vars.js';
 import 'chrome://resources/cr_elements/shared_vars_css.m.js';
 
 import {html, PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
+import {ReadLaterApiProxy, ReadLaterApiProxyImpl} from './read_later_api_proxy.js';
+
 export class ReadLaterItemElement extends PolymerElement {
   static get is() {
     return 'read-later-item';
@@ -22,6 +27,51 @@ export class ReadLaterItemElement extends PolymerElement {
       data: Object,
     };
   }
+
+  constructor() {
+    super();
+    /** @private {!ReadLaterApiProxy} */
+    this.apiProxy_ = ReadLaterApiProxyImpl.getInstance();
+  }
+
+  /** @override */
+  ready() {
+    super.ready();
+    this.addEventListener('click', this.onClick_);
+  }
+
+  /** @private */
+  onClick_() {
+    this.apiProxy_.openSavedEntry(this.data.url);
+  }
+
+  /**
+   * @param {!Event} e
+   * @private
+   */
+  onUpdateStatusClick_(e) {
+    e.stopPropagation();
+    this.apiProxy_.updateReadStatus(this.data.url, !this.data.read);
+  }
+
+  /**
+   * @param {!Event} e
+   * @private
+   */
+  onItemDeleteClick_(e) {
+    e.stopPropagation();
+    this.apiProxy_.removeEntry(this.data.url);
+  }
+
+  /**
+   * @param {string} markAsUnreadTooltip
+   * @param {string} markAsReadTooltip
+   * @return {string} The appropriate tooltip for the current state
+   * @private
+   */
+  getUpdateStatusButtonTooltip_(markAsUnreadTooltip, markAsReadTooltip) {
+    return this.data.read ? markAsUnreadTooltip : markAsReadTooltip;
+  }
 }
 
 customElements.define(ReadLaterItemElement.is, ReadLaterItemElement);

chrome/browser/ui/webui/read_later/read_later.mojom

@@ -18,6 +18,8 @@ struct ReadLaterEntry {
   url.mojom.Url url;
   string display_url;
   int64 update_time;
+  // Whether the entry has been read.
+  bool read;
   string display_time_since_update;
 };
 
@@ -45,4 +47,7 @@ interface PageHandler {
 
 // WebUI-side handler for requests from the browser.
 interface Page {
+  // Callback when any item in read later is
+  // changed/removed.
+  ItemsChanged();
 };

chrome/browser/ui/webui/read_later/read_later_page_handler.cc

@@ -85,10 +85,12 @@ void ReadLaterPageHandler::OpenSavedEntry(const GURL& url) {
 
 void ReadLaterPageHandler::UpdateReadStatus(const GURL& url, bool read) {
   reading_list_model_->SetReadStatus(url, read);
+  page_->ItemsChanged();
 }
 
 void ReadLaterPageHandler::RemoveEntry(const GURL& url) {
   reading_list_model_->RemoveEntryByURL(url);
+  page_->ItemsChanged();
 }
 
 read_later::mojom::ReadLaterEntryPtr ReadLaterPageHandler::GetEntryData(
@@ -105,6 +107,7 @@ read_later::mojom::ReadLaterEntryPtr ReadLaterPageHandler::GetEntryData(
           url_formatter::kFormatUrlTrimAfterHost,
       net::UnescapeRule::NORMAL, nullptr, nullptr, nullptr));
   entry_data->update_time = entry->UpdateTime();
+  entry_data->read = entry->IsRead();
   entry_data->display_time_since_update =
       GetTimeSinceLastUpdate(entry->UpdateTime());
 

chrome/browser/ui/webui/read_later/read_later_page_handler_unittest.cc

@@ -42,6 +42,8 @@ class MockPage : public read_later::mojom::Page {
     return receiver_.BindNewPipeAndPassRemote();
   }
   mojo::Receiver<read_later::mojom::Page> receiver_{this};
+
+  MOCK_METHOD0(ItemsChanged, void());
 };
 
 void ExpectNewReadLaterEntry(const read_later::mojom::ReadLaterEntry* entry,
@@ -104,8 +106,9 @@ class TestReadLaterPageHandlerTest : public BrowserWithTestWindowTest {
                                         base::ASCIIToUTF16(title));
   }
 
- private:
   testing::StrictMock<MockPage> page_;
+
+ private:
   std::unique_ptr<TestReadLaterPageHandler> handler_;
   ReadingListModel* model_;
 };
@@ -156,6 +159,7 @@ TEST_F(TestReadLaterPageHandlerTest, OpenSavedEntry) {
 
 TEST_F(TestReadLaterPageHandlerTest, UpdateReadStatus) {
   handler()->UpdateReadStatus(GURL(kTabUrl3), true);
+  EXPECT_CALL(page_, ItemsChanged()).Times(1);
 
   // Get Read later entries.
   read_later::mojom::PageHandler::GetReadLaterEntriesCallback callback1 =
@@ -177,6 +181,7 @@ TEST_F(TestReadLaterPageHandlerTest, UpdateReadStatus) {
 
 TEST_F(TestReadLaterPageHandlerTest, RemoveEntry) {
   handler()->RemoveEntry(GURL(kTabUrl3));
+  EXPECT_CALL(page_, ItemsChanged()).Times(1);
 
   // Get Read later entries.
   read_later::mojom::PageHandler::GetReadLaterEntriesCallback callback1 =

chrome/browser/ui/webui/read_later/read_later_ui.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/webui/read_later/read_later_ui.h"
 
+#include <string>
 #include <utility>
 
 #include "chrome/browser/ui/webui/read_later/read_later_page_handler.h"
@@ -16,12 +17,21 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/base/webui/web_ui_util.h"
 
 namespace {
 constexpr char kGeneratedPath[] =
     "@out_folder@/gen/chrome/browser/resources/read_later/";
+
+void AddLocalizedString(content::WebUIDataSource* source,
+                        const std::string& message,
+                        int id) {
+  base::string16 str = l10n_util::GetStringUTF16(id);
+  base::Erase(str, '&');
+  source->AddString(message, str);
 }
+}  // namespace
 
 ReadLaterUI::ReadLaterUI(content::WebUI* web_ui)
     : ui::MojoWebUIController(web_ui) {
@@ -30,11 +40,16 @@ ReadLaterUI::ReadLaterUI(content::WebUI* web_ui)
   source->AddResourcePath("read_later.mojom-lite.js",
                           IDR_READ_LATER_MOJO_LITE_JS);
   static constexpr webui::LocalizedString kLocalizedStrings[] = {
+      {"readHeader", IDS_READ_LATER_MENU_READ_HEADER},
       {"title", IDS_READ_LATER_MENU_TITLE},
+      {"tooltipDelete", IDS_DELETE},
+      {"tooltipMarkAsRead", IDS_READ_LATER_MENU_TOOLTIP_MARK_AS_READ},
+      {"tooltipMarkAsUnread", IDS_READ_LATER_MENU_TOOLTIP_MARK_AS_UNREAD},
       {"unreadHeader", IDS_READ_LATER_MENU_UNREAD_HEADER},
-      {"readHeader", IDS_READ_LATER_MENU_READ_HEADER},
   };
-  AddLocalizedStringsBulk(source, kLocalizedStrings);
+  for (const auto& str : kLocalizedStrings)
+    AddLocalizedString(source, str.name, str.id);
+
   webui::SetupWebUIDataSource(
       source, base::make_span(kReadLaterResources, kReadLaterResourcesSize),
       kGeneratedPath, IDR_READ_LATER_HTML);

chrome/test/data/webui/read_later/read_later_app_test.js

@@ -5,7 +5,7 @@
 import {ReadLaterAppElement} from 'chrome://read-later/app.js';
 import {ReadLaterApiProxy, ReadLaterApiProxyImpl} from 'chrome://read-later/read_later_api_proxy.js';
 
-import {assertEquals} from '../chai_assert.js';
+import {assertEquals, assertFalse, assertTrue} from '../chai_assert.js';
 import {flushTasks} from '../test_util.m.js';
 
 import {TestReadLaterApiProxy} from './test_read_later_api_proxy.js';
@@ -46,6 +46,7 @@ suite('ReadLaterAppTest', () => {
           url: 'https://www.google.com',
           displayUrl: 'google.com',
           updateTime: 0,
+          read: false,
           displayTimeSinceUpdate: '2 minutes ago',
         },
         {
@@ -53,6 +54,7 @@ suite('ReadLaterAppTest', () => {
           url: 'https://www.apple.com',
           displayUrl: 'apple.com',
           updateTime: 0,
+          read: false,
           displayTimeSinceUpdate: '20 minutes ago',
         },
       ],
@@ -62,6 +64,7 @@ suite('ReadLaterAppTest', () => {
           url: 'https://www.bing.com',
           displayUrl: 'bing.com',
           updateTime: 0,
+          read: true,
           displayTimeSinceUpdate: '5 minutes ago',
         },
         {
@@ -69,6 +72,7 @@ suite('ReadLaterAppTest', () => {
           url: 'https://www.yahoo.com',
           displayUrl: 'yahoo.com',
           updateTime: 0,
+          read: true,
           displayTimeSinceUpdate: '7 minutes ago',
         },
       ]
@@ -97,10 +101,48 @@ suite('ReadLaterAppTest', () => {
     assertEntryURLs(queryItems(), urls);
   });
 
-  test('click passes correct url', async () => {
+  test('click on item passes correct url', async () => {
     const expectedUrl = 'https://www.apple.com';
     clickItem(expectedUrl);
     const url = await testProxy.whenCalled('openSavedEntry');
     assertEquals(url, expectedUrl);
   });
+
+  test('Click on item mark as read button triggers actions', async () => {
+    const expectedUrl = 'https://www.apple.com';
+
+    const readLaterItem =
+        readLaterApp.shadowRoot.querySelector(`[data-url="${expectedUrl}"]`);
+    const readLaterItemUpdateStatusButton =
+        readLaterItem.shadowRoot.querySelector('#updateStatusButton');
+    readLaterItemUpdateStatusButton.click();
+    const [url, read] = await testProxy.whenCalled('updateReadStatus');
+    assertEquals(expectedUrl, url);
+    assertTrue(read);
+  });
+
+  test('Click on item mark as unread button triggers actions', async () => {
+    const expectedUrl = 'https://www.bing.com';
+
+    const readLaterItem =
+        readLaterApp.shadowRoot.querySelector(`[data-url="${expectedUrl}"]`);
+    const readLaterItemUpdateStatusButton =
+        readLaterItem.shadowRoot.querySelector('#updateStatusButton');
+    readLaterItemUpdateStatusButton.click();
+    const [url, read] = await testProxy.whenCalled('updateReadStatus');
+    assertEquals(expectedUrl, url);
+    assertFalse(read);
+  });
+
+  test('Click on item delete button triggers actions', async () => {
+    const expectedUrl = 'https://www.apple.com';
+
+    const readLaterItem =
+        readLaterApp.shadowRoot.querySelector(`[data-url="${expectedUrl}"]`);
+    const readLaterItemDeleteButton =
+        readLaterItem.shadowRoot.querySelector('#deleteButton');
+    readLaterItemDeleteButton.click();
+    const url = await testProxy.whenCalled('removeEntry');
+    assertEquals(expectedUrl, url);
+  });
 });

chrome/test/data/webui/read_later/test_read_later_api_proxy.js

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'chrome://read-later/read_later.mojom-lite.js';
+
 import {ReadLaterApiProxy} from 'chrome://read-later/read_later_api_proxy.js';
 import {TestBrowserProxy} from '../test_browser_proxy.m.js';
 
@@ -11,8 +13,13 @@ export class TestReadLaterApiProxy extends TestBrowserProxy {
     super([
       'getReadLaterEntries',
       'openSavedEntry',
+      'updateReadStatus',
+      'removeEntry',
     ]);
 
+    /** @type {!readLater.mojom.PageCallbackRouter} */
+    this.callbackRouter = new readLater.mojom.PageCallbackRouter();
+
     /** @private {!readLater.mojom.ReadLaterEntriesByStatus} */
     this.entries_;
   }
@@ -28,6 +35,21 @@ export class TestReadLaterApiProxy extends TestBrowserProxy {
     this.methodCalled('openSavedEntry', url);
   }
 
+  /** @override */
+  updateReadStatus(url, read) {
+    this.methodCalled('updateReadStatus', [url, read]);
+  }
+
+  /** @override */
+  removeEntry(url) {
+    this.methodCalled('removeEntry', url);
+  }
+
+  /** @override */
+  getCallbackRouter() {
+    return this.callbackRouter;
+  }
+
   /** @param {!readLater.mojom.ReadLaterEntriesByStatus} entries */
   setEntries(entries) {
     this.entries_ = entries;