Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(19)

Issue 29371739: Issue 4722 - Remove old check for Chrome notification API (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 10 months ago by kzar
Modified:
2 years, 10 months ago
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.)
2 years, 10 months ago (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 ...
2 years, 10 months ago (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): ...
2 years, 10 months ago (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 ...
2 years, 10 months ago (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 ...
2 years, 10 months ago (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 ...
2 years, 10 months ago (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 ...
2 years, 10 months ago (2017-01-16 14:28:42 UTC) #7
Sebastian Noack
2 years, 10 months ago (2017-01-16 16:01:58 UTC) #8
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5