Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -17 lines) Patch
M lib/notificationHelper.js View 1 2 3 3 chunks +2 lines, -17 lines 0 comments Download

Messages

Total messages: 8
kzar
Patch Set 1 (Not tested yet.)
Jan. 13, 2017, 11:28 a.m. (2017-01-13 11:28:29 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHelper.js#newcode37 lib/notificationHelper.js:37: let canUseChromeNotifications = platform == "chromium" && "notifications" in ...
Jan. 13, 2017, 11:43 a.m. (2017-01-13 11:43:23 UTC) #2
kzar
Patch Set 2 : Check for chrome directly instead of platform https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHelper.js File lib/notificationHelper.js (right): ...
Jan. 16, 2017, 4:15 a.m. (2017-01-16 04:15:37 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHelper.js#newcode37 lib/notificationHelper.js:37: let canUseChromeNotifications = platform == "chromium" && "notifications" in ...
Jan. 16, 2017, 1:39 p.m. (2017-01-16 13:39:59 UTC) #4
kzar
Patch Set 3 : Remove chrome check https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29371740/lib/notificationHelper.js#newcode37 lib/notificationHelper.js:37: let canUseChromeNotifications ...
Jan. 16, 2017, 1:50 p.m. (2017-01-16 13:50:22 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29371739/diff/29372042/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29372042/lib/notificationHelper.js#newcode36 lib/notificationHelper.js:36: let canUseChromeNotifications = "notifications" in chrome; How about inlining ...
Jan. 16, 2017, 2:08 p.m. (2017-01-16 14:08:28 UTC) #6
kzar
Patch Set 4 : Inline chrome.notification check https://codereview.adblockplus.org/29371739/diff/29372042/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371739/diff/29372042/lib/notificationHelper.js#newcode36 lib/notificationHelper.js:36: let canUseChromeNotifications ...
Jan. 16, 2017, 2:28 p.m. (2017-01-16 14:28:42 UTC) #7
Sebastian Noack
Jan. 16, 2017, 4:01 p.m. (2017-01-16 16:01:58 UTC) #8
LGTM

Powered by Google App Engine
This is Rietveld