Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672313004:
Test the platform notification context synchronize operation (Closed)
|
Created:
3 years, 10 months ago by Miguel Garcia Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, awdf+watch_chromium.org, Peter Beverloo, mlamouri+watch-notifications_chromium.org, jam, darin-cc_chromium.org, einbinder+watch-test-runner_chromium.org, jochen+watch_chromium.org, mlamouri+watch-test-runner_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTest the platform notification context synchronize operation
- Extract the mock implementation used for layout tests
- Add the ability to synchronize notifications to the mock implementation
- Create a small class with a custom content browser client
- Use all of that in the test
BUG=596161
Review-Url: https://codereview.chromium.org/2672313004
Cr-Commit-Position: refs/heads/master@{#449282}
Committed: https://chromium.googlesource.com/chromium/src/+/f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c
Patch Set 1 #Patch Set 2 : - #
Total comments: 10
Patch Set 3 : review #Patch Set 4 : rebase #Patch Set 5 : compile #
Depends on Patchset: Messages
Total messages: 41 (26 generated)
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
miguelg@chromium.org changed reviewers: + peter@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2672313004/diff/20001/content/browser/notific... File content/browser/notifications/platform_notification_context_unittest.cc (right): https://codereview.chromium.org/2672313004/diff/20001/content/browser/notific... content/browser/notifications/platform_notification_context_unittest.cc:516: notification_resources.action_icons.resize(notification_data.actions.size()); Why is this line necessary? Neither |notification_data| nor |notification_resources| have actions at this point - this should be a no-op? https://codereview.chromium.org/2672313004/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_notification_manager.cc (right): https://codereview.chromium.org/2672313004/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_notification_manager.cc:31: // Notifications will never outlive the lifetime of running layout tests. Why do we have to override this? It seems like tests that rely on *not* being able to get the displayed notifications are just wrong. Can follow up later on if removing this causes test failures. https://codereview.chromium.org/2672313004/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_notification_manager.h (right): https://codereview.chromium.org/2672313004/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_notification_manager.h:24: bool GetDisplayedNotifications( nit: // MockPlatformNotificationService overrides. https://codereview.chromium.org/2672313004/diff/20001/content/test/mock_platf... File content/test/mock_platform_notification_service.cc (right): https://codereview.chromium.org/2672313004/diff/20001/content/test/mock_platf... content/test/mock_platform_notification_service.cc:89: for (auto kv : persistent_notifications_) { auto kv -> const auto& pair (using just "auto" will actually result in copies) https://codereview.chromium.org/2672313004/diff/20001/content/test/mock_platf... content/test/mock_platform_notification_service.cc:89: for (auto kv : persistent_notifications_) { nit: drop the {} for one-line statements
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
miguelg@chromium.org changed reviewers: + sky@chromium.org
+Sky for OWNERS on the two new files in content/test. https://codereview.chromium.org/2672313004/diff/20001/content/browser/notific... File content/browser/notifications/platform_notification_context_unittest.cc (right): https://codereview.chromium.org/2672313004/diff/20001/content/browser/notific... content/browser/notifications/platform_notification_context_unittest.cc:516: notification_resources.action_icons.resize(notification_data.actions.size()); On 2017/02/08 14:05:11, Peter Beverloo wrote: > Why is this line necessary? Neither |notification_data| nor > |notification_resources| have actions at this point - this should be a no-op? it's not needed. https://codereview.chromium.org/2672313004/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_notification_manager.cc (right): https://codereview.chromium.org/2672313004/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_notification_manager.cc:31: // Notifications will never outlive the lifetime of running layout tests. On 2017/02/08 14:05:11, Peter Beverloo wrote: > Why do we have to override this? It seems like tests that rely on *not* being > able to get the displayed notifications are just wrong. Can follow up later on > if removing this causes test failures. Sure, I know for a fact at least two tests in platform_notification_context_unittest fail if you allow synchronization (since they just check that the database works properly so I feel it's sort of ok). I was trying to not break too much stuff at once so I decided to keep this behaviour. Will follow up with a CL removing it and see what breaks. https://codereview.chromium.org/2672313004/diff/20001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_notification_manager.h (right): https://codereview.chromium.org/2672313004/diff/20001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_notification_manager.h:24: bool GetDisplayedNotifications( On 2017/02/08 14:05:11, Peter Beverloo wrote: > nit: > > // MockPlatformNotificationService overrides. Done. https://codereview.chromium.org/2672313004/diff/20001/content/test/mock_platf... File content/test/mock_platform_notification_service.cc (right): https://codereview.chromium.org/2672313004/diff/20001/content/test/mock_platf... content/test/mock_platform_notification_service.cc:89: for (auto kv : persistent_notifications_) { On 2017/02/08 14:05:11, Peter Beverloo wrote: > auto kv -> const auto& pair > > (using just "auto" will actually result in copies) Done. https://codereview.chromium.org/2672313004/diff/20001/content/test/mock_platf... content/test/mock_platform_notification_service.cc:89: for (auto kv : persistent_notifications_) { On 2017/02/08 14:05:11, Peter Beverloo wrote: > nit: drop the {} for one-line statements Done.
LGTM
The CQ bit was unchecked by miguelg@chromium.org
The CQ bit was checked by miguelg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by peter@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
content/shell/browser/layout_test/layout_test_notification_manager.h:
While running git apply --index -p1;
error: patch failed:
content/shell/browser/layout_test/layout_test_notification_manager.h:5
error: content/shell/browser/layout_test/layout_test_notification_manager.h:
patch does not apply
Patch:
content/shell/browser/layout_test/layout_test_notification_manager.h
Index: content/shell/browser/layout_test/layout_test_notification_manager.h
diff --git
a/content/shell/browser/layout_test/layout_test_notification_manager.h
b/content/shell/browser/layout_test/layout_test_notification_manager.h
index
b7108d1aac8ebef67356d55795c472005a040251..84fd4df4eaddac569067e904aa3d206aa58468e7
100644
--- a/content/shell/browser/layout_test/layout_test_notification_manager.h
+++ b/content/shell/browser/layout_test/layout_test_notification_manager.h
@@ -5,100 +5,29 @@
#ifndef CONTENT_SHELL_BROWSER_LAYOUT_TEST_LAYOUT_TEST_NOTIFICATION_MANAGER_H_
#define CONTENT_SHELL_BROWSER_LAYOUT_TEST_LAYOUT_TEST_NOTIFICATION_MANAGER_H_
-#include <stdint.h>
#include <string>
-#include <unordered_map>
-#include "base/callback.h"
#include "base/macros.h"
-#include "base/memory/weak_ptr.h"
-#include "content/public/browser/platform_notification_service.h"
+#include "content/test/mock_platform_notification_service.h"
#include
"third_party/WebKit/public/platform/modules/permissions/permission_status.mojom.h"
#include "url/gurl.h"
namespace content {
-class DesktopNotificationDelegate;
-struct NotificationResources;
-struct PlatformNotificationData;
-
// Responsible for tracking active notifications and allowed origins for the
// Web Notification API when running layout tests.
-class LayoutTestNotificationManager : public PlatformNotificationService {
+class LayoutTestNotificationManager : public MockPlatformNotificationService {
public:
LayoutTestNotificationManager();
~LayoutTestNotificationManager() override;
- // Simulates a click on the notification titled |title|. |action_index|
- // indicates which action was clicked, or -1 if the main notification body
was
- // clicked. |reply| indicates the user reply, if any.
- // Must be called on the UI thread.
- void SimulateClick(const std::string& title,
- int action_index,
- const base::NullableString16& reply);
-
- // Simulates the closing a notification titled |title|. Must be called on
- // the UI thread.
- void SimulateClose(const std::string& title, bool by_user);
-
- // PlatformNotificationService implementation.
- blink::mojom::PermissionStatus CheckPermissionOnUIThread(
- BrowserContext* browser_context,
- const GURL& origin,
- int render_process_id) override;
- blink::mojom::PermissionStatus CheckPermissionOnIOThread(
- ResourceContext* resource_context,
- const GURL& origin,
- int render_process_id) override;
- void DisplayNotification(
- BrowserContext* browser_context,
- const std::string& notification_id,
- const GURL& origin,
- const PlatformNotificationData& notification_data,
- const NotificationResources& notification_resources,
- std::unique_ptr<DesktopNotificationDelegate> delegate,
- base::Closure* cancel_callback) override;
- void DisplayPersistentNotification(
- BrowserContext* browser_context,
- const std::string& notification_id,
- const GURL& service_worker_scope,
- const GURL& origin,
- const PlatformNotificationData& notification_data,
- const NotificationResources& notification_resources) override;
- void ClosePersistentNotification(BrowserContext* browser_context,
- const std::string& notification_id)
override;
+ // MockPlatformNotificationService overrides.
bool GetDisplayedNotifications(
BrowserContext* browser_context,
std::set<std::string>* displayed_notifications) override;
private:
- // Structure to represent the information of a persistent notification.
- struct PersistentNotification {
- BrowserContext* browser_context = nullptr;
- GURL origin;
- };
-
- // Closes the notification titled |title|. Must be called on the UI thread.
- void Close(const std::string& title);
-
- // Fakes replacing the notification identified by |notification_id|. Both
- // persistent and non-persistent notifications will be considered for this.
- void ReplaceNotificationIfNeeded(const std::string& notification_id);
-
- // Checks if |origin| has permission to display notifications. May be called
- // on both the IO and the UI threads.
- blink::mojom::PermissionStatus CheckPermission(const GURL& origin);
-
- std::unordered_map<std::string, PersistentNotification>
- persistent_notifications_;
- std::unordered_map<std::string, std::unique_ptr<DesktopNotificationDelegate>>
- non_persistent_notifications_;
-
- // Mapping of titles to notification ids giving test a usable identifier.
- std::unordered_map<std::string, std::string> notification_id_map_;
-
- base::WeakPtrFactory<LayoutTestNotificationManager> weak_factory_;
-
+ blink::mojom::PermissionStatus CheckPermission(const GURL& origin) override;
DISALLOW_COPY_AND_ASSIGN(LayoutTestNotificationManager);
};
The CQ bit was checked by miguelg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
content/shell/browser/layout_test/layout_test_notification_manager.h:
While running git apply --index -p1;
error: patch failed:
content/shell/browser/layout_test/layout_test_notification_manager.h:5
error: content/shell/browser/layout_test/layout_test_notification_manager.h:
patch does not apply
Patch:
content/shell/browser/layout_test/layout_test_notification_manager.h
Index: content/shell/browser/layout_test/layout_test_notification_manager.h
diff --git
a/content/shell/browser/layout_test/layout_test_notification_manager.h
b/content/shell/browser/layout_test/layout_test_notification_manager.h
index
b7108d1aac8ebef67356d55795c472005a040251..84fd4df4eaddac569067e904aa3d206aa58468e7
100644
--- a/content/shell/browser/layout_test/layout_test_notification_manager.h
+++ b/content/shell/browser/layout_test/layout_test_notification_manager.h
@@ -5,100 +5,29 @@
#ifndef CONTENT_SHELL_BROWSER_LAYOUT_TEST_LAYOUT_TEST_NOTIFICATION_MANAGER_H_
#define CONTENT_SHELL_BROWSER_LAYOUT_TEST_LAYOUT_TEST_NOTIFICATION_MANAGER_H_
-#include <stdint.h>
#include <string>
-#include <unordered_map>
-#include "base/callback.h"
#include "base/macros.h"
-#include "base/memory/weak_ptr.h"
-#include "content/public/browser/platform_notification_service.h"
+#include "content/test/mock_platform_notification_service.h"
#include
"third_party/WebKit/public/platform/modules/permissions/permission_status.mojom.h"
#include "url/gurl.h"
namespace content {
-class DesktopNotificationDelegate;
-struct NotificationResources;
-struct PlatformNotificationData;
-
// Responsible for tracking active notifications and allowed origins for the
// Web Notification API when running layout tests.
-class LayoutTestNotificationManager : public PlatformNotificationService {
+class LayoutTestNotificationManager : public MockPlatformNotificationService {
public:
LayoutTestNotificationManager();
~LayoutTestNotificationManager() override;
- // Simulates a click on the notification titled |title|. |action_index|
- // indicates which action was clicked, or -1 if the main notification body
was
- // clicked. |reply| indicates the user reply, if any.
- // Must be called on the UI thread.
- void SimulateClick(const std::string& title,
- int action_index,
- const base::NullableString16& reply);
-
- // Simulates the closing a notification titled |title|. Must be called on
- // the UI thread.
- void SimulateClose(const std::string& title, bool by_user);
-
- // PlatformNotificationService implementation.
- blink::mojom::PermissionStatus CheckPermissionOnUIThread(
- BrowserContext* browser_context,
- const GURL& origin,
- int render_process_id) override;
- blink::mojom::PermissionStatus CheckPermissionOnIOThread(
- ResourceContext* resource_context,
- const GURL& origin,
- int render_process_id) override;
- void DisplayNotification(
- BrowserContext* browser_context,
- const std::string& notification_id,
- const GURL& origin,
- const PlatformNotificationData& notification_data,
- const NotificationResources& notification_resources,
- std::unique_ptr<DesktopNotificationDelegate> delegate,
- base::Closure* cancel_callback) override;
- void DisplayPersistentNotification(
- BrowserContext* browser_context,
- const std::string& notification_id,
- const GURL& service_worker_scope,
- const GURL& origin,
- const PlatformNotificationData& notification_data,
- const NotificationResources& notification_resources) override;
- void ClosePersistentNotification(BrowserContext* browser_context,
- const std::string& notification_id)
override;
+ // MockPlatformNotificationService overrides.
bool GetDisplayedNotifications(
BrowserContext* browser_context,
std::set<std::string>* displayed_notifications) override;
private:
- // Structure to represent the information of a persistent notification.
- struct PersistentNotification {
- BrowserContext* browser_context = nullptr;
- GURL origin;
- };
-
- // Closes the notification titled |title|. Must be called on the UI thread.
- void Close(const std::string& title);
-
- // Fakes replacing the notification identified by |notification_id|. Both
- // persistent and non-persistent notifications will be considered for this.
- void ReplaceNotificationIfNeeded(const std::string& notification_id);
-
- // Checks if |origin| has permission to display notifications. May be called
- // on both the IO and the UI threads.
- blink::mojom::PermissionStatus CheckPermission(const GURL& origin);
-
- std::unordered_map<std::string, PersistentNotification>
- persistent_notifications_;
- std::unordered_map<std::string, std::unique_ptr<DesktopNotificationDelegate>>
- non_persistent_notifications_;
-
- // Mapping of titles to notification ids giving test a usable identifier.
- std::unordered_map<std::string, std::string> notification_id_map_;
-
- base::WeakPtrFactory<LayoutTestNotificationManager> weak_factory_;
-
+ blink::mojom::PermissionStatus CheckPermission(const GURL& origin) override;
DISALLOW_COPY_AND_ASSIGN(LayoutTestNotificationManager);
};
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2672313004/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2672313004/#ps80001 (title: "compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486640986052420,
"parent_rev": "0284803bf5636ccc4789b933121664939478bad9", "commit_rev":
"f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c"}
Message was sent while issue was closed.
Description was changed from ========== Test the platform notification context synchronize operation - Extract the mock implementation used for layout tests - Add the ability to synchronize notifications to the mock implementation - Create a small class with a custom content browser client - Use all of that in the test BUG=596161 ========== to ========== Test the platform notification context synchronize operation - Extract the mock implementation used for layout tests - Add the ability to synchronize notifications to the mock implementation - Create a small class with a custom content browser client - Use all of that in the test BUG=596161 Review-Url: https://codereview.chromium.org/2672313004 Cr-Commit-Position: refs/heads/master@{#449282} Committed: https://chromium.googlesource.com/chromium/src/+/f3f824cd5032eac3c694bd9b7a5c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f3f824cd5032eac3c694bd9b7a5c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
