Issue 29371739: Issue 4722 - Remove old check for Chrome notification API (Closed)
|
Created:
Jan. 13, 2017, 11:27 a.m. by kzar Modified:
Jan. 17, 2017, 6:59 a.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 4722 - Remove old check for Chrome notification API
Patch Set 1 #
Total comments: 4
Patch Set 2 : Check for chrome directly instead of platform #Patch Set 3 : Remove chrome check #
Total comments: 2
Patch Set 4 : Inline chrome.notification check #MessagesTotal messages: 8
Patch Set 1 (Not tested yet.)
https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHel... lib/notificationHelper.js:37: let canUseChromeNotifications = platform == "chromium" && "notifications" in chrome; Any reason to still check for the platform? This would exclude Firefox and Microsoft Edge, regardless whether they implement the notifications API.
Patch Set 2 : Check for chrome directly instead of platform https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHel... lib/notificationHelper.js:37: let canUseChromeNotifications = platform == "chromium" && "notifications" in chrome; On 2017/01/13 11:43:23, Sebastian Noack wrote: > Any reason to still check for the platform? This would exclude Firefox and > Microsoft Edge, regardless whether they implement the notifications API. Not really but IMO that change is for when we switch to the browser API, since `chrome` will be undefined on Edge anyway. I guess I could simply test for the presence of chrome for now.
https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHel... lib/notificationHelper.js:37: let canUseChromeNotifications = platform == "chromium" && "notifications" in chrome; On 2017/01/16 04:15:37, kzar wrote: > On 2017/01/13 11:43:23, Sebastian Noack wrote: > > Any reason to still check for the platform? This would exclude Firefox and > > Microsoft Edge, regardless whether they implement the notifications API. > > Not really but IMO that change is for when we switch to the browser API, since > `chrome` will be undefined on Edge anyway. I guess I could simply test for the > presence of chrome for now. Well, when we switch to the browser.* API we would use browser.notfication instead of chrome.notifcation here of course. So I don't see any reason to check for the existence of "chrome" either. Or is there any reason we have to exclude Edge here regardless whether it has browser.notifcations or not?
Patch Set 3 : Remove chrome check https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHel... lib/notificationHelper.js:37: let canUseChromeNotifications = platform == "chromium" && "notifications" in chrome; On 2017/01/16 13:39:59, Sebastian Noack wrote: > On 2017/01/16 04:15:37, kzar wrote: > > On 2017/01/13 11:43:23, Sebastian Noack wrote: > > > Any reason to still check for the platform? This would exclude Firefox and > > > Microsoft Edge, regardless whether they implement the notifications API. > > > > Not really but IMO that change is for when we switch to the browser API, since > > `chrome` will be undefined on Edge anyway. I guess I could simply test for the > > presence of chrome for now. > > Well, when we switch to the browser.* API we would use browser.notfication > instead of chrome.notifcation here of course. So I don't see any reason to check > for the existence of "chrome" either. Or is there any reason we have to exclude > Edge here regardless whether it has browser.notifcations or not? I guess not, Done.
https://codereview.adblockplus.org/29371739/diff/29372042/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29372042/lib/notificationHel... lib/notificationHelper.js:36: let canUseChromeNotifications = "notifications" in chrome; How about inlining this check, now. It isn't any more verbose, but potentially easier to understand, than the variable name.
Patch Set 4 : Inline chrome.notification check https://codereview.adblockplus.org/29371739/diff/29372042/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29372042/lib/notificationHel... lib/notificationHelper.js:36: let canUseChromeNotifications = "notifications" in chrome; On 2017/01/16 14:08:28, Sebastian Noack wrote: > How about inlining this check, now. It isn't any more verbose, but potentially > easier to understand, than the variable name. Done.
LGTM 
|
