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

Issue 19370004: Prevent unnecessary dismissal retries (Closed)

Created:
7 years, 5 months ago by vadimt
Modified:
7 years, 5 months ago
CC:
chromium-reviews, arv+watch_chromium.org, stromme, govind1
Visibility:
Public.

Description

Prevent unnecessary dismissal retries. We stop retrying a dismissal if: 1. The response from the server indicates that retrying is hopeless; 2. We kept retrying for 1 day. BUG=164227 TEST=Disconnect network and dismiss a notification. Observe the console output. Make sure that dismissal attempts continue for 1 day and then stop. (Of course, you don't have to run this test on every Canary release :)) Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212497

Patch Set 1 #

Total comments: 6

Patch Set 2 : skare@ notes #

Patch Set 3 : rgustafson's notes #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -5 lines) Patch
M chrome/browser/resources/google_now/background.js View 1 2 4 chunks +25 lines, -5 lines 1 comment Download

Messages

Total messages: 12 (0 generated)
vadimt
7 years, 5 months ago (2013-07-16 17:47:02 UTC) #1
skare_
https://codereview.chromium.org/19370004/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19370004/diff/1/chrome/browser/resources/google_now/background.js#newcode65 chrome/browser/resources/google_now/background.js:65: var MAXIMUM_DISMISSAL_AGE = 24 * 60 * 1000; // ...
7 years, 5 months ago (2013-07-16 23:30:13 UTC) #2
vadimt
:) https://codereview.chromium.org/19370004/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19370004/diff/1/chrome/browser/resources/google_now/background.js#newcode65 chrome/browser/resources/google_now/background.js:65: var MAXIMUM_DISMISSAL_AGE = 24 * 60 * 1000; ...
7 years, 5 months ago (2013-07-16 23:54:49 UTC) #3
skare_
lgtm
7 years, 5 months ago (2013-07-17 00:07:35 UTC) #4
rgustafson
lgtm https://codereview.chromium.org/19370004/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19370004/diff/1/chrome/browser/resources/google_now/background.js#newcode502 chrome/browser/resources/google_now/background.js:502: // doesn't have chances for successful completion. nit: ...
7 years, 5 months ago (2013-07-17 20:25:36 UTC) #5
vadimt
https://codereview.chromium.org/19370004/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19370004/diff/1/chrome/browser/resources/google_now/background.js#newcode502 chrome/browser/resources/google_now/background.js:502: // doesn't have chances for successful completion. On 2013/07/17 ...
7 years, 5 months ago (2013-07-18 19:37:59 UTC) #6
vadimt
arv@, please provide OWNER's approval
7 years, 5 months ago (2013-07-18 19:38:21 UTC) #7
arv (Not doing code reviews)
LGTM https://codereview.chromium.org/19370004/diff/10001/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/19370004/diff/10001/chrome/browser/resources/google_now/background.js#newcode65 chrome/browser/resources/google_now/background.js:65: var MAXIMUM_DISMISSAL_AGE_MS = 24 * 60 * 60 ...
7 years, 5 months ago (2013-07-18 19:46:14 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/19370004/10001
7 years, 5 months ago (2013-07-18 19:55:08 UTC) #9
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 5 months ago (2013-07-18 20:54:22 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/19370004/10001
7 years, 5 months ago (2013-07-18 21:09:13 UTC) #11
commit-bot: I haz the power
7 years, 5 months ago (2013-07-19 01:50:18 UTC) #12
Message was sent while issue was closed.
Change committed as 212497

Powered by Google App Engine
This is Rietveld 408576698