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

Issue 2949263003: Remove call to GetBlockingPool in RLZ (Closed)

Created:
3 years, 6 months ago by Roger Tawa OOO till Jul 10th
Modified:
3 years, 4 months ago
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove call to GetBlockingPool in RLZ. BUG=667892, 618530 Review-Url: https://codereview.chromium.org/2949263003 Cr-Commit-Position: refs/heads/master@{#491013} Committed: https://chromium.googlesource.com/chromium/src/+/c4a9c77f6e6c59b02fbb09db8da441f630b9ac54

Patch Set 1 : rebased #

Patch Set 2 : rebased #

Total comments: 17

Patch Set 3 : Address review comments #

Patch Set 4 : Fix typo in comment #

Patch Set 5 : rebased #

Patch Set 6 : rebased #

Patch Set 7 : Address review comments, rebase #

Patch Set 8 : rebased #

Total comments: 22

Patch Set 9 : Address review comments #

Patch Set 10 : rebased #

Patch Set 11 : Use PostDelayedTaskWithTraits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -155 lines) Patch
M chrome/browser/rlz/chrome_rlz_tracker_delegate.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/rlz/chrome_rlz_tracker_delegate.cc View 2 chunks +2 lines, -8 lines 0 comments Download
M chrome/browser/rlz/chrome_rlz_tracker_delegate_unittest.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M components/rlz/rlz_tracker.h View 1 2 3 4 5 6 7 8 3 chunks +11 lines, -5 lines 0 comments Download
M components/rlz/rlz_tracker.cc View 1 2 3 4 5 6 7 8 12 chunks +38 lines, -30 lines 0 comments Download
M components/rlz/rlz_tracker_delegate.h View 2 chunks +0 lines, -9 lines 0 comments Download
M components/rlz/rlz_tracker_unittest.cc View 5 chunks +3 lines, -9 lines 0 comments Download
M ios/chrome/browser/rlz/rlz_tracker_delegate_impl.h View 1 chunk +0 lines, -1 line 0 comments Download
M ios/chrome/browser/rlz/rlz_tracker_delegate_impl.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M rlz/lib/financial_ping.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +148 lines, -88 lines 0 comments Download

Messages

Total messages: 98 (80 generated)
Roger Tawa OOO till Jul 10th
Hi Gab, Let me know if this looks correct. Thanks.
3 years, 5 months ago (2017-07-13 12:37:33 UTC) #33
gab
Thanks for working on this, comments below. I don't think it works as is, but ...
3 years, 5 months ago (2017-07-17 16:44:20 UTC) #44
Roger Tawa OOO till Jul 10th
Why don't you think this works? All tests pass, I built the code with rlz ...
3 years, 5 months ago (2017-07-17 19:19:37 UTC) #49
gab
https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc#newcode230 rlz/lib/financial_ping.cc:230: : event_(event) {} On 2017/07/17 19:19:37, Roger Tawa OOO ...
3 years, 5 months ago (2017-07-17 21:05:20 UTC) #52
Roger Tawa OOO till Jul 10th
Thanks Gab, see below. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc#newcode232 rlz/lib/financial_ping.cc:232: void OnURLFetchComplete(const net::URLFetcher* source) override ...
3 years, 5 months ago (2017-07-21 13:53:42 UTC) #56
gab
https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc#newcode232 rlz/lib/financial_ping.cc:232: void OnURLFetchComplete(const net::URLFetcher* source) override { On 2017/07/21 13:53:42, ...
3 years, 5 months ago (2017-07-21 22:39:17 UTC) #59
gab
@rogerta: did my previous reply make sense to you? Happy to discuss further if not, ...
3 years, 4 months ago (2017-07-26 00:32:58 UTC) #65
Roger Tawa OOO till Jul 10th
Hi Gab, Used CreateSequencedTaskRunnerWithTraits() as you suggested to make a new sequence. This required a ...
3 years, 4 months ago (2017-07-26 21:01:31 UTC) #68
gab
lgtm w/ comments, this is great, thanks! https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tracker.cc File components/rlz/rlz_tracker.cc (right): https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tracker.cc#newcode519 components/rlz/rlz_tracker.cc:519: void RLZTracker::ClearRlzStateImpl() ...
3 years, 4 months ago (2017-07-27 18:29:38 UTC) #75
Roger Tawa OOO till Jul 10th
Thanks, all comments addressed. Will commit after rebase. https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tracker.cc File components/rlz/rlz_tracker.cc (right): https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tracker.cc#newcode519 components/rlz/rlz_tracker.cc:519: void ...
3 years, 4 months ago (2017-07-28 18:43:07 UTC) #76
Roger Tawa OOO till Jul 10th
I forgot to respond to one comment earlier. While RLZ is not enabled in chrome.exe ...
3 years, 4 months ago (2017-07-28 18:52:50 UTC) #79
gab
https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc#newcode242 rlz/lib/financial_ping.cc:242: response_.clear(); On 2017/07/28 18:43:06, Roger Tawa OOO till Jul ...
3 years, 4 months ago (2017-07-28 19:28:58 UTC) #82
Roger Tawa OOO till Jul 10th
https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc#newcode242 rlz/lib/financial_ping.cc:242: response_.clear(); On 2017/07/28 19:28:57, gab wrote: > On 2017/07/28 ...
3 years, 4 months ago (2017-07-28 19:42:17 UTC) #83
Roger Tawa OOO till Jul 10th
Salut David, Can you do an owner review for: ios/chrome/browser/rlz/rlz_tracker_delegate_impl.h|cc Thanks.
3 years, 4 months ago (2017-07-28 20:01:45 UTC) #87
gab
On 2017/07/28 19:42:17, Roger Tawa OOO till Jul 10th wrote: > https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc > File rlz/lib/financial_ping.cc ...
3 years, 4 months ago (2017-07-28 20:27:24 UTC) #88
rohitrao (ping after 24h)
ios/ lgtm
3 years, 4 months ago (2017-08-01 14:31:34 UTC) #92
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/2949263003/360001
3 years, 4 months ago (2017-08-01 14:48:49 UTC) #95
commit-bot: I haz the power
3 years, 4 months ago (2017-08-01 16:40:52 UTC) #98
Message was sent while issue was closed.
Committed patchset #11 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/c4a9c77f6e6c59b02fbb09db8da4...

Powered by Google App Engine
This is Rietveld 408576698