Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(11)

Issue 2797573002: Fix startForegroundService API calls. (Closed)

Created:
3 years, 8 months ago by David Trainor- moved to gerrit
Modified:
3 years, 8 months ago
CC:
agrieve+watch_chromium.org, chromium-reviews, David Trainor- moved to gerrit, feature-media-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix startForegroundService API calls. Migrate to a new way of calling this, which does not involve sending the notification to the start call. Instead just make sure we move the service to the foreground shortly after launching it. BUG=707932 Review-Url: https://codereview.chromium.org/2797573002 Cr-Commit-Position: refs/heads/master@{#461844} Committed: https://chromium.googlesource.com/chromium/src/+/81033db0364139db8ee059623d8892f2b00a39ba

Patch Set 1 #

Patch Set 2 : Removed commented out code #

Total comments: 1

Patch Set 3 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -15 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/AppHooks.java View 1 chunk +3 lines, -9 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java View 1 2 4 chunks +12 lines, -3 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java View 2 chunks +2 lines, -3 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 16 (10 generated)
David Trainor- moved to gerrit
nyquist@ ptal at all zqzhang@ can you take a look at the MediaNotificationManager? Should we ...
3 years, 8 months ago (2017-04-04 00:07:09 UTC) #2
nyquist
lgtm https://codereview.chromium.org/2797573002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java (right): https://codereview.chromium.org/2797573002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java#newcode461 chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:461: mShouldMakeForeground = false; Optional nit: For my own ...
3 years, 8 months ago (2017-04-04 05:45:44 UTC) #3
Zhiqiang Zhang (Slow)
MediaNotificationManager lgtm +mlamouri@ for rubber-stamping
3 years, 8 months ago (2017-04-04 10:28:53 UTC) #5
mlamouri (slow - plz ping)
lgtm
3 years, 8 months ago (2017-04-04 12:50:24 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2797573002/40001
3 years, 8 months ago (2017-04-04 21:10:03 UTC) #13
commit-bot: I haz the power
3 years, 8 months ago (2017-04-04 21:24:24 UTC) #16
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/81033db0364139db8ee059623d88...

Powered by Google App Engine
This is Rietveld 408576698