Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10332115:
Fixed GetDebugLogs functionality. (Closed)
|
Created:
8 years, 7 months ago by ygorshenin1 Modified:
8 years, 7 months ago CC:
chromium-reviews, eroman, Nikita (slow) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFixed GetDebugLogs functionality.
BUG=chromium:127779
TEST=Manual testing on the Alex device.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137648
Patch Set 1 #Patch Set 2 : Fixed fileshelf directory, simplified methods call chain for the GetDebugLogs. #
Total comments: 5
Patch Set 3 : Refactored net_internals::GetDebugLogs flow. #
Total comments: 6
Patch Set 4 : Comments are adjusted. #
Total comments: 4
Patch Set 5 : Fixed comments. #Messages
Total messages: 11 (0 generated)
You should have someone familiar with the underlying classes review this as well. I'm unfamiliar with the thread safety requirements of chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->GetDebugLogs(), and none seem to be documented. 7 functions to store debug logs just seems really weird and overly complicated to me. We have: OnStoreDebugLogs (UI) StoreDebugLogs (UI) CreateDebugLog (FILE) DBusCallGetDebugLogFile (UI) DBusCallGetDebugLogFileCompleted (UI) CreateDebugLogFileCompleted (FILE) OnStoreDebugLogsCompleted (UI) There should at least be some comments on all this thread hopping, and why it is necessary.
+ Sam for DebugDaemonClient::GetDebugLogs call. PTAL.
http://codereview.chromium.org/10332115/diff/3001/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/10332115/diff/3001/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:199: if (!PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS_SAFE, &fileshelf_dir)) { should this be download_util::GetDefaultDownloadDirectory()? http://codereview.chromium.org/10332115/diff/3001/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:213: log_path, flags, &created, &error); feels like this belongs in download_util (just a comment) http://codereview.chromium.org/10332115/diff/3001/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:242: BrowserThread::PostTask( I think it's cleaner to make this a call to BrowserThread::PostTaskAndReply as it eliminates the explicit return hand-off to the UI thread; doing this might also allow you to move much of this code to debug_daemon_client.cc It was also my understanding that it was desirable to not explicitly push file ops to the FILE thread but instead send them to the WorkerPool (as I did for my equivalent changes); it should be simple to do that
Thanks, PTAL. http://codereview.chromium.org/10332115/diff/3001/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/10332115/diff/3001/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:199: if (!PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS_SAFE, &fileshelf_dir)) { On 2012/05/14 15:52:38, Sam Leffler wrote: > should this be download_util::GetDefaultDownloadDirectory()? Done. http://codereview.chromium.org/10332115/diff/3001/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:242: BrowserThread::PostTask( Thanks! It's a cool idea to use WorkerPool for file ops. It requires a helper class, but code became more readable and understandable. On 2012/05/14 15:52:38, Sam Leffler wrote: > I think it's cleaner to make this a call to BrowserThread::PostTaskAndReply as > it eliminates the explicit return hand-off to the UI thread; doing this might > also allow you to move much of this code to debug_daemon_client.cc > > It was also my understanding that it was desirable to not explicitly push file > ops to the FILE thread but instead send them to the WorkerPool (as I did for my > equivalent changes); it should be simple to do that
lgtm w/ nits http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:224: // Closes file handle, so, should be called on the FILE thread. nit: called on a workerpool thread so comment needs adjusting http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:230: // on the FILE thread. workerpool thread
Thanks. Matt, do you have any comments? http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:224: // Closes file handle, so, should be called on the FILE thread. On 2012/05/15 16:42:28, Sam Leffler wrote: > nit: called on a workerpool thread so comment needs adjusting Done. http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:230: // on the FILE thread. On 2012/05/15 16:42:28, Sam Leffler wrote: > workerpool thread Done.
LGTM. http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:196: base::PLATFORM_FILE_WRITE; nit: For legibility, may want to align the two flags at base:: http://codereview.chromium.org/10332115/diff/12002/chrome/browser/ui/webui/ne... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/10332115/diff/12002/chrome/browser/ui/webui/ne... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:260: void FillDebugLogFile(const StoreDebugLogsCallback& callback, nit: Think this function name is a little weird. Maybe WriteDebugLogToFile? Or PopulateDebugLogFile, or somesuch. http://codereview.chromium.org/10332115/diff/12002/chrome/browser/ui/webui/ne... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:284: // |callback| upon completion. Suggest an overview either here or up top. Something along the lines of: "The file is created on the worker pool, then writing to it is triggered from the UI thread, and finally it is closed (on success) or deleted (on failure) on the worker pool, prior to calling |callback|."
Many thanks! http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/10332115/diff/3002/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:196: base::PLATFORM_FILE_WRITE; On 2012/05/15 17:25:28, Matt Menke wrote: > nit: For legibility, may want to align the two flags at base:: Done. http://codereview.chromium.org/10332115/diff/12002/chrome/browser/ui/webui/ne... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/10332115/diff/12002/chrome/browser/ui/webui/ne... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:260: void FillDebugLogFile(const StoreDebugLogsCallback& callback, On 2012/05/15 17:25:28, Matt Menke wrote: > nit: Think this function name is a little weird. Maybe WriteDebugLogToFile? > Or PopulateDebugLogFile, or somesuch. Done. http://codereview.chromium.org/10332115/diff/12002/chrome/browser/ui/webui/ne... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:284: // |callback| upon completion. On 2012/05/15 17:25:28, Matt Menke wrote: > Suggest an overview either here or up top. Something along the lines of: > > "The file is created on the worker pool, then writing to it is triggered from > the UI thread, and finally it is closed (on success) or deleted (on failure) on > the worker pool, prior to calling |callback|." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/10332115/11002
Change committed as 137648 |
