Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
141433011:
Cache feedback reports to disk in case of send failure. (Closed)
|
Created:
6 years, 11 months ago by rkc Modified:
6 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCache feedback reports to disk in case of send failure.
R=zork@chromium.org
BUG=249853
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246992
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247772
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248648
Patch Set 1 #
Total comments: 12
Patch Set 2 : #Patch Set 3 : #
Total comments: 26
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Tests fix. #Patch Set 7 : Race condition fix in tests. #Patch Set 8 : #Patch Set 9 : rebase #
Total comments: 20
Patch Set 10 : #
Total comments: 6
Patch Set 11 : #Messages
Total messages: 41 (0 generated)
https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... File chrome/browser/feedback/feedback_profile_observer.cc (right): https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_profile_observer.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2014 https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... File chrome/browser/feedback/feedback_report.cc (right): https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2014 https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.cc:44: upload_at_(upload_at), nit: indent https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.cc:50: std::string upload_time = base::Int64ToString(upload_at.ToInternalValue()); Is there a reason to use the upload time, like to make sure that reports from the same time hash to the same filename? If not, maybe use a GUID instead? https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.cc:90: !name.empty(); nit: indent. https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... File chrome/browser/feedback/feedback_report.h (right): https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2014
https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... File chrome/browser/feedback/feedback_profile_observer.cc (right): https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_profile_observer.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/01/23 05:21:01, Zachary Kuznia wrote: > 2014 Done. https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... File chrome/browser/feedback/feedback_report.cc (right): https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/01/23 05:21:01, Zachary Kuznia wrote: > 2014 Done. https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.cc:44: upload_at_(upload_at), On 2014/01/23 05:21:01, Zachary Kuznia wrote: > nit: indent Done. https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.cc:50: std::string upload_time = base::Int64ToString(upload_at.ToInternalValue()); On 2014/01/23 05:21:01, Zachary Kuznia wrote: > Is there a reason to use the upload time, like to make sure that reports from > the same time hash to the same filename? If not, maybe use a GUID instead? It's unique and it can help in diagnosing issues, since the reportfilename1 < reportfilename2 will always mean that report 1 was queued for sending before report 2. https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.cc:90: !name.empty(); On 2014/01/23 05:21:01, Zachary Kuznia wrote: > nit: indent. Done. https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... File chrome/browser/feedback/feedback_report.h (right): https://codereview.chromium.org/141433011/diff/1/chrome/browser/feedback/feed... chrome/browser/feedback/feedback_report.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/01/23 05:21:01, Zachary Kuznia wrote: > 2014 Done.
lgtm
Adding thestig@ for chrome_browser_main owner's review.
The code looks like it's cross-platform, but the associated bug says this is a CrOS feature. Shouldn't the code reflect that?
I didn't realize the bug was marked CrOS only; this is supposed to be for all platforms (the code it replaces, is also for all platforms). Changed the bug to reflect this.
Nits. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... File chrome/browser/feedback/feedback_profile_observer.cc (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.cc:21: static base::LazyInstance<feedback::FeedbackProfileObserver>::Leaky You don't need static inside an anonymous namespace. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.cc:46: switch (type) { Don't bother with the switch(). Just DCHECK_EQ(chrome::NOTIFICATION_PROFILE_CREATED, type); https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.cc:61: feedback::FeedbackUploader *uploader = nit: feedback::FeedbackUploader* https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... File chrome/browser/feedback/feedback_profile_observer.h (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.h:8: #include <queue> neither queue or string are actually needed https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.h:13: #include "base/memory/scoped_ptr.h" No scoped_ptrs, weak_ptrs, time or timer objects here. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.h:18: #include "components/browser_context_keyed_service/browser_context_keyed_service.h" not needed https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.cc (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:31: // We need this function to post a task to delete file (which returns a bool I think the correct way to do this is with base::IgnoreResult() https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:42: content::BrowserContext* context, nit: this fits on the previous line; align the other parameters with this one; 4 space indent the initializer list on its own line. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:52: content::BrowserThread::PostBlockingPoolTask( since you already declared "using content::BrowserThread", you can omit content:: here and below. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.h (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:14: #include "base/memory/scoped_ptr.h" nit: not used https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:18: class SequencedTaskRunner; nit: not used
On 2014/01/23 22:22:46, Rahul Chaturvedi wrote: > I didn't realize the bug was marked CrOS only; this is supposed to be for all > platforms (the code it replaces, is also for all platforms). > > Changed the bug to reflect this. Thanks. Just curious, is the code used on Android?
chrome/browser/chrome_browser_main.cc lgtm https://codereview.chromium.org/141433011/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1236: feedback::FeedbackProfileObserver::Initialize(); Can do this in PreProfileInit() instead? Up near line 1050.
Addressed comments. This code (though compiled in) will never run on Android since there is no way to send a Feedback Report via the Chrome Feedback feature on Android. Android has their own feedback mechanism exposed to each app; Chrome on Android uses that instead. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1236: feedback::FeedbackProfileObserver::Initialize(); On 2014/01/23 22:40:59, Lei Zhang wrote: > Can do this in PreProfileInit() instead? Up near line 1050. I am a little concerned about moving it that late. The comment near PreProfileInit mentions that the profile creation is done in that step; I specially wanted to be before that step. Also, the comment right below (// Profile Creation) made it seem like this would be the perfect place to initialize this observer. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... File chrome/browser/feedback/feedback_profile_observer.cc (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.cc:21: static base::LazyInstance<feedback::FeedbackProfileObserver>::Leaky On 2014/01/23 22:28:41, Lei Zhang wrote: > You don't need static inside an anonymous namespace. Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.cc:46: switch (type) { On 2014/01/23 22:28:41, Lei Zhang wrote: > Don't bother with the switch(). Just > DCHECK_EQ(chrome::NOTIFICATION_PROFILE_CREATED, type); Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.cc:61: feedback::FeedbackUploader *uploader = On 2014/01/23 22:28:41, Lei Zhang wrote: > nit: feedback::FeedbackUploader* Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... File chrome/browser/feedback/feedback_profile_observer.h (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.h:8: #include <queue> On 2014/01/23 22:28:41, Lei Zhang wrote: > neither queue or string are actually needed Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.h:13: #include "base/memory/scoped_ptr.h" On 2014/01/23 22:28:41, Lei Zhang wrote: > No scoped_ptrs, weak_ptrs, time or timer objects here. Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_profile_observer.h:18: #include "components/browser_context_keyed_service/browser_context_keyed_service.h" On 2014/01/23 22:28:41, Lei Zhang wrote: > not needed Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.cc (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:31: // We need this function to post a task to delete file (which returns a bool On 2014/01/23 22:28:41, Lei Zhang wrote: > I think the correct way to do this is with base::IgnoreResult() Ah, I knew there had to be a better way to do this. Thanks! Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:42: content::BrowserContext* context, On 2014/01/23 22:28:41, Lei Zhang wrote: > nit: this fits on the previous line; align the other parameters with this one; 4 > space indent the initializer list on its own line. Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:52: content::BrowserThread::PostBlockingPoolTask( On 2014/01/23 22:28:41, Lei Zhang wrote: > since you already declared "using content::BrowserThread", you can omit > content:: here and below. Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.h (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:14: #include "base/memory/scoped_ptr.h" On 2014/01/23 22:28:41, Lei Zhang wrote: > nit: not used Done. https://codereview.chromium.org/141433011/diff/120001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:18: class SequencedTaskRunner; On 2014/01/23 22:28:41, Lei Zhang wrote: > nit: not used Done.
https://codereview.chromium.org/141433011/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1236: feedback::FeedbackProfileObserver::Initialize(); On 2014/01/23 22:57:21, Rahul Chaturvedi wrote: > On 2014/01/23 22:40:59, Lei Zhang wrote: > > Can do this in PreProfileInit() instead? Up near line 1050. > > I am a little concerned about moving it that late. The comment near > PreProfileInit mentions that the profile creation is done in that step; I > specially wanted to be before that step. > > Also, the comment right below (// Profile Creation) made it seem like this would > be the perfect place to initialize this observer. PreProfileInit() is called earlier on line 1201 before the current location. The "diagram" near line 1012 should show you what the process is like.
https://codereview.chromium.org/141433011/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/141433011/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1236: feedback::FeedbackProfileObserver::Initialize(); On 2014/01/23 23:50:39, Lei Zhang wrote: > On 2014/01/23 22:57:21, Rahul Chaturvedi wrote: > > On 2014/01/23 22:40:59, Lei Zhang wrote: > > > Can do this in PreProfileInit() instead? Up near line 1050. > > > > I am a little concerned about moving it that late. The comment near > > PreProfileInit mentions that the profile creation is done in that step; I > > specially wanted to be before that step. > > > > Also, the comment right below (// Profile Creation) made it seem like this > would > > be the perfect place to initialize this observer. > > PreProfileInit() is called earlier on line 1201 before the current location. The > "diagram" near line 1012 should show you what the process is like. Done.
lgtm++
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/141433011/130013
Retried try job too often on win_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/141433011/130013
Message was sent while issue was closed.
Change committed as 246992
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/133193004/ by hclam@chromium.org. The reason for reverting is: This change is causing memory error on this bot: http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28... .
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/141433011/680001
Retried try job too often on mac_rel for step(s) remoting_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/141433011/680001
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/141433011/680001
Message was sent while issue was closed.
Change committed as 247772
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/141433011/830001
The CQ bit was unchecked by thestig@chromium.org
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
Please get your code re-reviewed after making non-trivial changes. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_uploader_unittest.cc (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_uploader_unittest.cc:59: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); No, Please don't do this. See ToTT episode 303.
https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.cc (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:46: kFeedbackReportFilenamePrefix + base::GenerateGUID()); Consider using file_util::GetUniquePathNumber() instead of GenerateGUID(). https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:72: base::AutoLock locker(file_access_lock_); You shouldn't need a lock. Instead, get a SequenceToken and use that token whenever you PostTask() to the blocking pool. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.h (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:26: // for it is created automatically. This backup is deleted once this object The statement about the backup being deleted doesn't hold. What happens if a FeedbackReport gets deleted without anyone calling DeleteReportOnDisk() ? https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:28: class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> { This class doesn't need to be RefCountedThreadSafe. Since both WriteReportOnBlockingPool() and DeleteReportOnBlockingPool() are essentially fire and forget, you can just move them into an anonymous namespace and not be part of FeedbackReport. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:38: base::Time upload_at() { return upload_at_; } make the method const, return a const-ref https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:39: const std::string& data() { return data_; } make the method const https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_uploader_unittest.cc (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_uploader_unittest.cc:59: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); If you explain what you are waiting for, I might be able to suggest a better solution.
https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.cc (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:46: kFeedbackReportFilenamePrefix + base::GenerateGUID()); On 2014/02/01 01:37:22, Lei Zhang wrote: > Consider using file_util::GetUniquePathNumber() instead of GenerateGUID(). That would involve moving getting the filename onto the blocking pool (since GetUniquePathNumber interacts with the file system). I really want to have the filename here itself and using GenerateGUID does the job. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.cc:72: base::AutoLock locker(file_access_lock_); On 2014/02/01 01:37:22, Lei Zhang wrote: > You shouldn't need a lock. Instead, get a SequenceToken and use that token > whenever you PostTask() to the blocking pool. Done. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.h (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:26: // for it is created automatically. This backup is deleted once this object On 2014/02/01 01:37:22, Lei Zhang wrote: > The statement about the backup being deleted doesn't hold. What happens if a > FeedbackReport gets deleted without anyone calling DeleteReportOnDisk() ? Fixed. Done. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:28: class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> { On 2014/02/01 01:37:22, Lei Zhang wrote: > This class doesn't need to be RefCountedThreadSafe. Since both > WriteReportOnBlockingPool() and DeleteReportOnBlockingPool() are essentially > fire and forget, you can just move them into an anonymous namespace and not be > part of FeedbackReport. I use its ref-counted behavior to not have to manage it's lifetime while the report is being moved around on the reports queue; not particularly for the PostTask methods. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:38: base::Time upload_at() { return upload_at_; } On 2014/02/01 01:37:22, Lei Zhang wrote: > make the method const, return a const-ref Done. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:39: const std::string& data() { return data_; } On 2014/02/01 01:37:22, Lei Zhang wrote: > make the method const Done. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_uploader_unittest.cc (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_uploader_unittest.cc:59: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); On 2014/02/01 01:37:22, Lei Zhang wrote: > If you explain what you are waiting for, I might be able to suggest a better > solution. This was to ensure that the timestamps of each report added were different, not to actually 'wait' for anything. The reason for doing this was that even though the case was handled correctly, it caused flakiness in the tests, since the reports could now be processed out of order. Fixed the test completion criteria to fix that flakiness (doesn't check by order anymore), hence removed these sleeps. Done. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_uploader_unittest.cc:59: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); On 2014/02/01 01:13:45, Lei Zhang wrote: > No, Please don't do this. See ToTT episode 303. This wasn't for 'timing' anything, it was to ensure we get different timestamps per report. Explained in detail below.
https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.h (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:28: class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> { On 2014/02/03 22:13:11, Rahul Chaturvedi wrote: > On 2014/02/01 01:37:22, Lei Zhang wrote: > > This class doesn't need to be RefCountedThreadSafe. Since both > > WriteReportOnBlockingPool() and DeleteReportOnBlockingPool() are essentially > > fire and forget, you can just move them into an anonymous namespace and not be > > part of FeedbackReport. > > I use its ref-counted behavior to not have to manage it's lifetime while the > report is being moved around on the reports queue; not particularly for the > PostTask methods. Is there any reason you need to use refcounting to keep track of FeedbackReports? Why not just store a vector of FeedbackReport* in FeedbackUploader? https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... File chrome/browser/feedback/feedback_report.cc (right): https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... chrome/browser/feedback/feedback_report.cc:35: const std::string& data) { I'd DCHECK(reports_path.IsParent(file));
https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_uploader_unittest.cc (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_uploader_unittest.cc:59: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); On 2014/02/03 22:13:11, Rahul Chaturvedi wrote: > On 2014/02/01 01:13:45, Lei Zhang wrote: > > No, Please don't do this. See ToTT episode 303. > > This wasn't for 'timing' anything, it was to ensure we get different timestamps > per report. Explained in detail below. Ok, if you really need to do it, please add a comment to explain why it's there. Otherwise it raises a red flag. https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... File chrome/browser/feedback/feedback_uploader_unittest.cc (right): https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... chrome/browser/feedback/feedback_uploader_unittest.cc:68: if (dispatched_reports_.count(report_data) == 0) { if (ContainsKey(dispatched_reports_, report_data)) https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... chrome/browser/feedback/feedback_uploader_unittest.cc:86: if (dispatched_reports_count_ >= expected_reports_) return (dispatched_reports_count_ >= expected_reports_);
https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_report.h (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_report.h:28: class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> { On 2014/02/03 22:26:51, Lei Zhang wrote: > On 2014/02/03 22:13:11, Rahul Chaturvedi wrote: > > On 2014/02/01 01:37:22, Lei Zhang wrote: > > > This class doesn't need to be RefCountedThreadSafe. Since both > > > WriteReportOnBlockingPool() and DeleteReportOnBlockingPool() are > essentially > > > fire and forget, you can just move them into an anonymous namespace and not > be > > > part of FeedbackReport. > > > > I use its ref-counted behavior to not have to manage it's lifetime while the > > report is being moved around on the reports queue; not particularly for the > > PostTask methods. > > Is there any reason you need to use refcounting to keep track of > FeedbackReports? Why not just store a vector of FeedbackReport* in > FeedbackUploader? As discussed offline, changing to RefCounted versus RCTS. https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... File chrome/browser/feedback/feedback_uploader_unittest.cc (right): https://codereview.chromium.org/141433011/diff/830001/chrome/browser/feedback... chrome/browser/feedback/feedback_uploader_unittest.cc:59: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); On 2014/02/03 22:29:41, Lei Zhang wrote: > On 2014/02/03 22:13:11, Rahul Chaturvedi wrote: > > On 2014/02/01 01:13:45, Lei Zhang wrote: > > > No, Please don't do this. See ToTT episode 303. > > > > This wasn't for 'timing' anything, it was to ensure we get different > timestamps > > per report. Explained in detail below. > > Ok, if you really need to do it, please add a comment to explain why it's there. > Otherwise it raises a red flag. Don't need to do it anymore since I changed the check. Removed it. https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... File chrome/browser/feedback/feedback_report.cc (right): https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... chrome/browser/feedback/feedback_report.cc:35: const std::string& data) { On 2014/02/03 22:26:51, Lei Zhang wrote: > I'd DCHECK(reports_path.IsParent(file)); Done. https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... File chrome/browser/feedback/feedback_uploader_unittest.cc (right): https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... chrome/browser/feedback/feedback_uploader_unittest.cc:68: if (dispatched_reports_.count(report_data) == 0) { On 2014/02/03 22:29:41, Lei Zhang wrote: > if (ContainsKey(dispatched_reports_, report_data)) Done. https://codereview.chromium.org/141433011/diff/1000001/chrome/browser/feedbac... chrome/browser/feedback/feedback_uploader_unittest.cc:86: if (dispatched_reports_count_ >= expected_reports_) On 2014/02/03 22:29:41, Lei Zhang wrote: > return (dispatched_reports_count_ >= expected_reports_); Done.
lgtm++
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rkc@chromium.org/141433011/1070001
Message was sent while issue was closed.
Change committed as 248648 |
