Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
456223002:
Add NotifyOnShowSettings implementation of notification provider API (Closed)
|
Created:
6 years, 4 months ago by liyanhou Modified:
6 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd NotifyOnShowSettings implementation of notification
provider API and test.
When an extension/app with notificationProvider permission
shows a user that certain notifiers have advanced settings.
When the user clicks and chooses to see advanced settings of
one notifier, NotifyOnShowSettings can be used to inform the
notifier that a user requested advanced settings, so the
notifier can do something else to display its advanced
settings.
BUG=397197
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289823
Patch Set 1 : #
Total comments: 5
Patch Set 2 : address comments #Patch Set 3 : rebase #
Total comments: 4
Patch Set 4 : add some new fields in API, add more tests #Patch Set 5 : add comments in IDL file #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : fix and refactor browser test #Patch Set 8 : style fix #Patch Set 9 : Move AddListeners before calling functions that would cause those events #
Total comments: 6
Patch Set 10 : address comments (JavaScript format) #
Messages
Total messages: 22 (0 generated)
Can you take a look at this? Thank you so much!
Please explain a bit more in the changelist description. I wasn't able to get a good sense of what the changelist does from the description and title alone. Perhaps a bit of establishing context would help. https://codereview.chromium.org/456223002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:116: createNotification(id1, content) You probably don't need all these catch statements. Each catch will catch anything in the promise chain that failed before it. So, instead of alternating then and catch, just do as many thens as you want, and then catch. You might not even catch until the end unless there was some resource that needed to be cleaned up before you proceeded.
petewil@, I added more detailed descriptions for this CL. https://codereview.chromium.org/456223002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:116: createNotification(id1, content) On 2014/08/11 17:53:50, Pete Williamson wrote: > You probably don't need all these catch statements. Each catch will catch > anything in the promise chain that failed before it. So, instead of alternating > then and catch, just do as many thens as you want, and then catch. You might > not even catch until the end unless there was some resource that needed to be > cleaned up before you proceeded. I was doing that for more detailed error messages to show which function fails in case of failure. Do you think it would be better to use less catches?
lgtm https://codereview.chromium.org/456223002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:116: createNotification(id1, content) On 2014/08/11 18:24:06, liyanhou wrote: > On 2014/08/11 17:53:50, Pete Williamson wrote: > > You probably don't need all these catch statements. Each catch will catch > > anything in the promise chain that failed before it. So, instead of > alternating > > then and catch, just do as many thens as you want, and then catch. You might > > not even catch until the end unless there was some resource that needed to be > > cleaned up before you proceeded. > > I was doing that for more detailed error messages to show which function fails > in case of failure. Do you think it would be better to use less catches? OK, in that case these are fine. I just wanted to see if this was intended.
kalman@chromium.org: Could you take a look at notification_provider_api.cc and notification_provider_apitest.cc? Btw, the change in notification_provider_apitest.cc is the same as in CL 442513002. Depends on which CL gets committed first, I will update the other one.
I'd rather you add an OWNERS file and I review that instead :)
(rubberstamp lgtm in the meantime)
The CQ bit was checked by liyanhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/liyanhou@chromium.org/456223002/20001
not lgtm Please fix the spelling in the cl subject and description and also address my comment about the settings controller. https://codereview.chromium.org/456223002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/notification_provider/notification_provider_api.cc (right): https://codereview.chromium.org/456223002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:197: new MessageCenterSettingsController(profile_info_cache)); I'd rather that you use the settings controller that already exists in the NotificationUIManager instead of creating a new one here.
The CQ bit was unchecked by liyanhou@chromium.org
dewittj@ Addressed comment. Could you take another look at this? Thank you! https://codereview.chromium.org/456223002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/notification_provider/notification_provider_api.cc (right): https://codereview.chromium.org/456223002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:197: new MessageCenterSettingsController(profile_info_cache)); On 2014/08/11 21:16:16, dewittj wrote: > I'd rather that you use the settings controller that already exists in the > NotificationUIManager instead of creating a new one here. Done.
https://codereview.chromium.org/456223002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/notification_provider/notification_provider_api.cc (right): https://codereview.chromium.org/456223002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:219: message_center::MessageCenter::Get()->GetNotifierSettingsProvider(); This is better, but it feels like you are skipping a layer by going directly to the message center instead of using the notification UI manager. Can we add a TODO to refactor the NotificationUIManager API so that it allows grabbing a settings provider? https://codereview.chromium.org/456223002/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:118: createNotification(id1, content) it would be better if this test also verified that the various events in the notifications API actually fired.
Added some new fields in the API. Please take another look. Thank you! Sorry that patch 5 is not passing the trybots now, but patch 4 passed the trybots, and patch 5 only has some extra comments compared to patch 4. https://codereview.chromium.org/456223002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/notification_provider/notification_provider_api.cc (right): https://codereview.chromium.org/456223002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:219: message_center::MessageCenter::Get()->GetNotifierSettingsProvider(); On 2014/08/12 18:21:05, dewittj wrote: > This is better, but it feels like you are skipping a layer by going directly to > the message center instead of using the notification UI manager. Can we add a > TODO to refactor the NotificationUIManager API so that it allows grabbing a > settings provider? Done. https://codereview.chromium.org/456223002/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:118: createNotification(id1, content) On 2014/08/12 18:21:05, dewittj wrote: > it would be better if this test also verified that the various events in the > notifications API actually fired. Done.
https://codereview.chromium.org/456223002/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:192: .catch(function() { return allEventesReceived( Same comment as the other CL; this doesn't actually /wait/ for the events to happen, it just hopes that they have happened by the time line 192 rolls around. Need to refactor this. I also recommend splitting this into multiple test functions for clarity.
Addressed comments, fixed and refactored browser test. Could you take another look? Thank you! https://codereview.chromium.org/456223002/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:192: .catch(function() { return allEventesReceived( On 2014/08/13 21:35:45, dewittj wrote: > Same comment as the other CL; this doesn't actually /wait/ for the events to > happen, it just hopes that they have happened by the time line 192 rolls around. > Need to refactor this. I also recommend splitting this into multiple test > functions for clarity. Done.
lgtm + nits https://codereview.chromium.org/456223002/diff/200001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/200001/chrome/test/data/extens... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:81: senderId, nit: only indent the params 4 spaces https://codereview.chromium.org/456223002/diff/200001/chrome/test/data/extens... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:101: reject(new Error("Unable to send onShowSettings message")); nit: indentation is weird here. Pretty sure git-cl format doesn't run on JS. https://codereview.chromium.org/456223002/diff/200001/chrome/test/data/extens... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:121: .catch(function() { failTest("notifications.create"); }) I think that all of these catch/then chains need to be indented 4 spaces..
https://codereview.chromium.org/456223002/diff/200001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js (right): https://codereview.chromium.org/456223002/diff/200001/chrome/test/data/extens... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:81: senderId, On 2014/08/14 20:54:58, dewittj wrote: > nit: only indent the params 4 spaces Done. https://codereview.chromium.org/456223002/diff/200001/chrome/test/data/extens... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:101: reject(new Error("Unable to send onShowSettings message")); On 2014/08/14 20:54:58, dewittj wrote: > nit: indentation is weird here. Pretty sure git-cl format doesn't run on JS. Done. https://codereview.chromium.org/456223002/diff/200001/chrome/test/data/extens... chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:121: .catch(function() { failTest("notifications.create"); }) On 2014/08/14 20:54:58, dewittj wrote: > I think that all of these catch/then chains need to be indented 4 spaces.. Done.
The CQ bit was checked by liyanhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/liyanhou@chromium.org/456223002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
Message was sent while issue was closed.
Committed patchset #10 (220001) as 289823 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
