Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14329020:
Implementing uploading of a WebRTC diagnostic log. (Closed)
|
Created:
7 years, 8 months ago by Henrik Grunell Modified:
7 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementing uploading of a WebRTC diagnostic log.
BUG=229829
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203700
R=agl@chromium.org, joi@chromium.org, sky@chromium.org, tommi@chromium.org, vabr@chromium.org, wtc@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203711
Patch Set 1 #
Total comments: 4
Patch Set 2 : Refactoring. #Patch Set 3 : Fixed pre-submit warnings/errors (including adding bzip2 to content/browser DEPS). #
Total comments: 4
Patch Set 4 : Refactoring + cleaning code. #Patch Set 5 : Moved uploader to components (and removed log manager), partial circular buffer to base. Plus chang… #Patch Set 6 : Removed two files #
Total comments: 20
Patch Set 7 : Limit to max 5 simultaneous logs + a few other todo fixes. #Patch Set 8 : Code review. #
Total comments: 17
Patch Set 9 : Code review and rebase #Patch Set 10 : Removed move of PartialCircularBuffer since it's in another CL. #Patch Set 11 : Remove a little piece of test code. #Patch Set 12 : Upload again (error last upload). #
Total comments: 29
Patch Set 13 : Code review + use zlib instead of bzip2 #Patch Set 14 : Removed file added by mistake. #
Total comments: 2
Patch Set 15 : Code review, added url and app session id, rebase #
Total comments: 13
Patch Set 16 : Code review #Patch Set 17 : Rebased on CL that moves files to //chrome. #Patch Set 18 : Minor cleanup. #Patch Set 19 : Moved uploader to //chrome. #Patch Set 20 : Rebased. Moved uploader out of chrome namespace. #Patch Set 21 : Fixed minor rebase merge miss. #Patch Set 22 : Fixed unit test build error. #Patch Set 23 : Fixed build error on ChromeOS. #Messages
Total messages: 47 (0 generated)
My first comment is that the log uploader should probably be a component (e.g. //components/webrtc_log_uploader) since it has no real dependencies on //chrome (the few that are there seem very easy to break) and could be reused by other implementations based on content. See also nits below. Cheers, Jói https://codereview.chromium.org/14329020/diff/1/chrome/browser/webrtc_log_upl... File chrome/browser/webrtc_log_upload_manager.cc (right): https://codereview.chromium.org/14329020/diff/1/chrome/browser/webrtc_log_upl... chrome/browser/webrtc_log_upload_manager.cc:17: #include "chrome/browser/browser_process.h" Where is this used? https://codereview.chromium.org/14329020/diff/1/chrome/browser/webrtc_log_upl... chrome/browser/webrtc_log_upload_manager.cc:217: // TODO(grunell): Break out the function in cloud print. Yeah, you definitely shouldn't be including stuff from chrome/common/cloud_print :) https://codereview.chromium.org/14329020/diff/1/third_party/libjingle/overrid... File third_party/libjingle/overrides/talk/base/logging.h (right): https://codereview.chromium.org/14329020/diff/1/third_party/libjingle/overrid... third_party/libjingle/overrides/talk/base/logging.h:190: //#define LOG(sev) LOG_V(talk_base::sev) I presume this is temporary?
Thanks for looking at this Jói. I've done some refactoring to meet the
requirements of deps in browser. The only violation now is
You added one or more #includes that violate checkdeps rules.
content/browser/webrtc_log_manager.cc
Illegal include: "third_party/bzip2/bzlib.h"
Because of no rule applying.
Not sure about this and what's legal here?
Also, there's commented out code, todos and some other todos that don't have
todo comments on them. :) I'm super happoy if you want to take a high level look
before I go on and clean up for proper review.
https://chromiumcodereview.appspot.com/14329020/diff/1/third_party/libjingle/...
File third_party/libjingle/overrides/talk/base/logging.h (right):
https://chromiumcodereview.appspot.com/14329020/diff/1/third_party/libjingle/...
third_party/libjingle/overrides/talk/base/logging.h:190: //#define LOG(sev)
LOG_V(talk_base::sev)
On 2013/04/26 12:55:10, Jói wrote:
> I presume this is temporary?
Yes.
Hi Henrik, Sorry for the delay, I was on vacation for a few days. Regarding bzip, it seems that only //chrome and //chrome_frame currently include files from there. So there might be some pressure against adding that dependency to //content, although from a practical standpoint, in official builds the two are in the same binary so it should perhaps be OK. See comment below that might remove this dependency though. Cheers, Jói https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/ren... File content/browser/renderer_host/webrtc_logging_handler_host.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/ren... content/browser/renderer_host/webrtc_logging_handler_host.cc:95: // TODO(grunell): Move to chrome. Do you really want to move this fully to the embedder? That would mean that in some cases, //content is creating a compressed log file which then doesn't get uploaded, which is unnecessary work. https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/web... File content/browser/webrtc_log_manager.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/web... content/browser/webrtc_log_manager.cc:54: void WebRtcLogManager::ReadAndCompressLog(uint8* input, It almost seems strange that the content layer would stipulate the format of the compressed log file. Perhaps the embedder should be the one implementing this bit, and the SharedMemory log or tokens from that log should be sent to the embedder via the ContentClient boundary?
https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/ren... File content/browser/renderer_host/webrtc_logging_handler_host.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/ren... content/browser/renderer_host/webrtc_logging_handler_host.cc:95: // TODO(grunell): Move to chrome. On 2013/05/02 16:47:31, Jói wrote: > Do you really want to move this fully to the embedder? That would mean that in > some cases, //content is creating a compressed log file which then doesn't get > uploaded, which is unnecessary work. I've done some more changes/refactoring and cleaning in patch 4, including moving this. It uses base/prefs/pref_service.h, chrome/common/pref_names.h and chrome/common/chrome_switches.h that cannot be accessed from content, hence the move. The idea is that the host will ask the embedder (upload manager for chrome case) for a file to put the (compressed as of now) log, if it gets one logging will be enabled. Otherwise not. Side note: However due to threading requirements I actually let the host first ask if it's allowed to log, then if yes ask for a file. Doing the check in the upload manager when asking for a file would require a callback through content/public since we have to switch between threads (check prefs on UI, create file on FILE). My idea was to avoid that; let me know if that's preferable over the current solution. https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/web... File content/browser/webrtc_log_manager.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/web... content/browser/webrtc_log_manager.cc:54: void WebRtcLogManager::ReadAndCompressLog(uint8* input, On 2013/05/02 16:47:31, Jói wrote: > It almost seems strange that the content layer would stipulate the format of the > compressed log file. Perhaps the embedder should be the one implementing this > bit, and the SharedMemory log or tokens from that log should be sent to the > embedder via the ContentClient boundary? I definitely like to have this in the embedder. The only reason I put it here was it has a dep on content/common/partial_circular_buffer.h. We would have to add an interface in content/public. Is that better?
I haven't taken a look at your latest patch, this is based only on your latest comments: a) It seems fine in principle to have the host first ask if it's allowed to log, then if yes ask for a file. b) If PartialCircularBuffer starts having multiple users (e.g. both //content and //components) then it seems like a candidate to move to //base. It has no true dependencies apart from //base. Cheers, Jói On Fri, May 3, 2013 at 7:59 AM, <grunell@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/ren... > File content/browser/renderer_host/webrtc_logging_handler_host.cc > (right): > > https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/ren... > content/browser/renderer_host/webrtc_logging_handler_host.cc:95: // > TODO(grunell): Move to chrome. > On 2013/05/02 16:47:31, Jói wrote: >> >> Do you really want to move this fully to the embedder? That would mean > > that in >> >> some cases, //content is creating a compressed log file which then > > doesn't get >> >> uploaded, which is unnecessary work. > > > I've done some more changes/refactoring and cleaning in patch 4, > including moving this. It uses base/prefs/pref_service.h, > chrome/common/pref_names.h and chrome/common/chrome_switches.h that > cannot be accessed from content, hence the move. The idea is that the > host will ask the embedder (upload manager for chrome case) for a file > to put the (compressed as of now) log, if it gets one logging will be > enabled. Otherwise not. > > Side note: However due to threading requirements I actually let the host > first ask if it's allowed to log, then if yes ask for a file. Doing the > check in the upload manager when asking for a file would require a > callback through content/public since we have to switch between threads > (check prefs on UI, create file on FILE). My idea was to avoid that; let > me know if that's preferable over the current solution. > > > https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/web... > File content/browser/webrtc_log_manager.cc (right): > > https://chromiumcodereview.appspot.com/14329020/diff/8001/content/browser/web... > content/browser/webrtc_log_manager.cc:54: void > WebRtcLogManager::ReadAndCompressLog(uint8* input, > On 2013/05/02 16:47:31, Jói wrote: >> >> It almost seems strange that the content layer would stipulate the > > format of the >> >> compressed log file. Perhaps the embedder should be the one > > implementing this >> >> bit, and the SharedMemory log or tokens from that log should be sent > > to the >> >> embedder via the ContentClient boundary? > > > I definitely like to have this in the embedder. The only reason I put it > here was it has a dep on content/common/partial_circular_buffer.h. We > would have to add an interface in content/public. Is that better? > > https://chromiumcodereview.appspot.com/14329020/
On 2013/05/03 11:52:32, Jói wrote: > I haven't taken a look at your latest patch, this is based only on > your latest comments: > > a) It seems fine in principle to have the host first ask if it's > allowed to log, then if yes ask for a file. > > b) If PartialCircularBuffer starts having multiple users (e.g. both > //content and //components) then it seems like a candidate to move to > //base. It has no true dependencies apart from //base. > > Cheers, > Jói I've moved the uploader to components, the partial circular buffer to base and made changed as a consequence of that. The partial circular buffer change is ~1/3 of the files in the CL; if you think base is a good place I can create a separate CL for that. It's used by components and content/renderer as of now.
Hi Henrik, Can you explain to me why you need the partial circular buffer WebRtcLoggingHandlerImpl (rather than e.g. a plain circular buffer or just a buffer that you send to the browser side each time it fills up)? Other than this question, which impacts where the PartialCircularBuffer implementation should go, the change looks structurally right to me. I haven't yet reviewed the details of the implementation of the WebRtcLogUploader class. Cheers, Jói On Tue, May 7, 2013 at 12:31 PM, <grunell@chromium.org> wrote: > On 2013/05/03 11:52:32, Jói wrote: >> >> I haven't taken a look at your latest patch, this is based only on >> your latest comments: > > >> a) It seems fine in principle to have the host first ask if it's >> allowed to log, then if yes ask for a file. > > >> b) If PartialCircularBuffer starts having multiple users (e.g. both >> //content and //components) then it seems like a candidate to move to >> //base. It has no true dependencies apart from //base. > > >> Cheers, >> Jói > > > I've moved the uploader to components, the partial circular buffer to base > and > made changed as a consequence of that. > > The partial circular buffer change is ~1/3 of the files in the CL; if you > think > base is a good place I can create a separate CL for that. It's used by > components and content/renderer as of now. > > https://chromiumcodereview.appspot.com/14329020/
The partial circular buffer wraps at a selectable position, so it's used for keeping the first x bytes and last y bytes of log messages. This is because the most interesting parts are typically in the beginning (setup) and in the end (before a problem occurs). The reason for not using an intermediate buffer in the renderer is that in case of a renderer crash, the last messages will be lost. On 2013/05/07 15:36:40, Jói wrote: > Hi Henrik, > > Can you explain to me why you need the partial circular buffer > WebRtcLoggingHandlerImpl (rather than e.g. a plain circular buffer or > just a buffer that you send to the browser side each time it fills > up)? > > Other than this question, which impacts where the > PartialCircularBuffer implementation should go, the change looks > structurally right to me. I haven't yet reviewed the details of the > implementation of the WebRtcLogUploader class. > > Cheers, > Jói > > > On Tue, May 7, 2013 at 12:31 PM, <mailto:grunell@chromium.org> wrote: > > On 2013/05/03 11:52:32, Jói wrote: > >> > >> I haven't taken a look at your latest patch, this is based only on > >> your latest comments: > > > > > >> a) It seems fine in principle to have the host first ask if it's > >> allowed to log, then if yes ask for a file. > > > > > >> b) If PartialCircularBuffer starts having multiple users (e.g. both > >> //content and //components) then it seems like a candidate to move to > >> //base. It has no true dependencies apart from //base. > > > > > >> Cheers, > >> Jói > > > > > > I've moved the uploader to components, the partial circular buffer to base > > and > > made changed as a consequence of that. > > > > The partial circular buffer change is ~1/3 of the files in the CL; if you > > think > > base is a good place I can create a separate CL for that. It's used by > > components and content/renderer as of now. > > > > https://chromiumcodereview.appspot.com/14329020/
Thanks Henrik. What is the overall intended behavior? Do only the first and last X and Y bytes of the WebRTC log get uploaded? Or does the browser periodically upload the current shared memory buffer? On Tue, May 7, 2013 at 7:07 PM, <grunell@chromium.org> wrote: > The partial circular buffer wraps at a selectable position, so it's used for > keeping the first x bytes and last y bytes of log messages. This is because > the > most interesting parts are typically in the beginning (setup) and in the end > (before a problem occurs). > > The reason for not using an intermediate buffer in the renderer is that in > case > of a renderer crash, the last messages will be lost. > > > On 2013/05/07 15:36:40, Jói wrote: >> >> Hi Henrik, > > >> Can you explain to me why you need the partial circular buffer >> WebRtcLoggingHandlerImpl (rather than e.g. a plain circular buffer or >> just a buffer that you send to the browser side each time it fills >> up)? > > >> Other than this question, which impacts where the >> PartialCircularBuffer implementation should go, the change looks >> structurally right to me. I haven't yet reviewed the details of the >> implementation of the WebRtcLogUploader class. > > >> Cheers, >> Jói > > > >> On Tue, May 7, 2013 at 12:31 PM, <mailto:grunell@chromium.org> wrote: >> > On 2013/05/03 11:52:32, Jói wrote: >> >> >> >> I haven't taken a look at your latest patch, this is based only on >> >> your latest comments: >> > >> > >> >> a) It seems fine in principle to have the host first ask if it's >> >> allowed to log, then if yes ask for a file. >> > >> > >> >> b) If PartialCircularBuffer starts having multiple users (e.g. both >> >> //content and //components) then it seems like a candidate to move to >> >> //base. It has no true dependencies apart from //base. >> > >> > >> >> Cheers, >> >> Jói >> > >> > >> > I've moved the uploader to components, the partial circular buffer to >> > base >> > and >> > made changed as a consequence of that. >> > >> > The partial circular buffer change is ~1/3 of the files in the CL; if >> > you >> > think >> > base is a good place I can create a separate CL for that. It's used by >> > components and content/renderer as of now. >> > >> > https://chromiumcodereview.appspot.com/14329020/ > > > > > https://chromiumcodereview.appspot.com/14329020/
Only the first and last parts are uploaded. On 2013/05/08 13:37:43, Jói wrote: > Thanks Henrik. What is the overall intended behavior? Do only the > first and last X and Y bytes of the WebRTC log get uploaded? Or does > the browser periodically upload the current shared memory buffer? > > On Tue, May 7, 2013 at 7:07 PM, <mailto:grunell@chromium.org> wrote: > > The partial circular buffer wraps at a selectable position, so it's used for > > keeping the first x bytes and last y bytes of log messages. This is because > > the > > most interesting parts are typically in the beginning (setup) and in the end > > (before a problem occurs). > > > > The reason for not using an intermediate buffer in the renderer is that in > > case > > of a renderer crash, the last messages will be lost. > > > > > > On 2013/05/07 15:36:40, Jói wrote: > >> > >> Hi Henrik, > > > > > >> Can you explain to me why you need the partial circular buffer > >> WebRtcLoggingHandlerImpl (rather than e.g. a plain circular buffer or > >> just a buffer that you send to the browser side each time it fills > >> up)? > > > > > >> Other than this question, which impacts where the > >> PartialCircularBuffer implementation should go, the change looks > >> structurally right to me. I haven't yet reviewed the details of the > >> implementation of the WebRtcLogUploader class. > > > > > >> Cheers, > >> Jói > > > > > > > >> On Tue, May 7, 2013 at 12:31 PM, <mailto:grunell@chromium.org> wrote: > >> > On 2013/05/03 11:52:32, Jói wrote: > >> >> > >> >> I haven't taken a look at your latest patch, this is based only on > >> >> your latest comments: > >> > > >> > > >> >> a) It seems fine in principle to have the host first ask if it's > >> >> allowed to log, then if yes ask for a file. > >> > > >> > > >> >> b) If PartialCircularBuffer starts having multiple users (e.g. both > >> >> //content and //components) then it seems like a candidate to move to > >> >> //base. It has no true dependencies apart from //base. > >> > > >> > > >> >> Cheers, > >> >> Jói > >> > > >> > > >> > I've moved the uploader to components, the partial circular buffer to > >> > base > >> > and > >> > made changed as a consequence of that. > >> > > >> > The partial circular buffer change is ~1/3 of the files in the CL; if > >> > you > >> > think > >> > base is a good place I can create a separate CL for that. It's used by > >> > components and content/renderer as of now. > >> > > >> > https://chromiumcodereview.appspot.com/14329020/ > > > > > > > > > > https://chromiumcodereview.appspot.com/14329020/
Various nits and a few larger comments. I can review again once you've followed up on these. Cheers, Jói https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: no (c) https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:32: const char kTempUploadFile[] = "/tmp/chromium-webrtc-log-upload"; This isn't going to work on all platforms. I think you want file_util::CreateTemporaryFile. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:33: const char kUploadURL[] = "https://clients2.google.com/cr/report"; Is this a new API? It should probably require an API key to be included with the request, to help control abuse. I'll send you details on that. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:146: void WebRtcLogUploader::SetupMultipartFile(uint8* log_buffer, Is it possible to reuse existing code to create a multipart file? A quick grep of the //net code for "multipart" seems to indicate we might only have multipart support there for downloads, but I think it would be worth checking with e.g. wtc@ to see if there is opportunity for reuse. If reuse is not possible, then someone from the net team familiar with the the HTTP multipart format should review this implementation; again wtc@ could probably help find an appropriate reviewer. This code should have unit tests, but wait to add them until you've resolved whether you can reuse code or not. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:248: uint32 read = read_pcb.Read(intermediate_buffer.get(), This and the stream.next_in, stream.avail_in setup below seems to be duplicated in the while loop; can you avoid duplication if you make it a do/while loop? https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.h (right): https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: I think we no longer include the (c) https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:30: class WEBRTC_LOG_UPLOADER_EXPORT WebRtcLogUploader Since this class is decoding a binary buffer created by the renderer process, I think you should get a security review of it. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:42: // TODO(grunell): Take a scoped_refptr instead. Are you planning to fix this before committing, or in a follow-up patch? https://chromiumcodereview.appspot.com/14329020/diff/29001/content/renderer/m... File content/renderer/media/webrtc_logging_handler_impl.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/29001/content/renderer/m... content/renderer/media/webrtc_logging_handler_impl.cc:51: length, indentation is off
https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/05/08 14:14:22, Jói wrote: > nit: no (c) Removed. Though my conclusion from the bug and the pretty recent thread (started Feb 20) about this that both are OK (maybe without (c) is preferred). The presubmit script still allows it, i.e. step 5 in the plan has been skipped. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:32: const char kTempUploadFile[] = "/tmp/chromium-webrtc-log-upload"; On 2013/05/08 14:14:22, Jói wrote: > This isn't going to work on all platforms. I think you want > file_util::CreateTemporaryFile. Done. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:33: const char kUploadURL[] = "https://clients2.google.com/cr/report"; On 2013/05/08 14:14:22, Jói wrote: > Is this a new API? It should probably require an API key to be included with the > request, to help control abuse. I'll send you details on that. This is the ordinary crash server. See chrome/app/breakpad_linux.cc and chrome/tools/crash_service/crash_service.cc https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:146: void WebRtcLogUploader::SetupMultipartFile(uint8* log_buffer, On 2013/05/08 14:14:22, Jói wrote: > Is it possible to reuse existing code to create a multipart file? A quick grep > of the //net code for "multipart" seems to indicate we might only have multipart > support there for downloads, but I think it would be worth checking with e.g. > wtc@ to see if there is opportunity for reuse. > > If reuse is not possible, then someone from the net team familiar with the the > HTTP multipart format should review this implementation; again wtc@ could > probably help find an appropriate reviewer. > > This code should have unit tests, but wait to add them until you've resolved > whether you can reuse code or not. Yeah, couldn't really find anything reusable when I checked some time ago. I'll check with wtc@. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:248: uint32 read = read_pcb.Read(intermediate_buffer.get(), On 2013/05/08 14:14:22, Jói wrote: > This and the stream.next_in, stream.avail_in setup below seems to be duplicated > in the while loop; can you avoid duplication if you make it a do/while loop? In case the intermediate buffer isn't filled, that is we won't have any more data that this to compress more than this, my understanding is that we should go directly to the BZ_FINISH action. The documentation isn't entirely clear if one BZ_RUN action is OK, but that's how I interpret it. Do you prefer a do-while loop with a break in it? https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.h (right): https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/05/08 14:14:22, Jói wrote: > nit: I think we no longer include the (c) Done. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:30: class WEBRTC_LOG_UPLOADER_EXPORT WebRtcLogUploader On 2013/05/08 14:14:22, Jói wrote: > Since this class is decoding a binary buffer created by the renderer process, I > think you should get a security review of it. Yes. We have a launch bug for this so it should be taken care of through that. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:42: // TODO(grunell): Take a scoped_refptr instead. On 2013/05/08 14:14:22, Jói wrote: > Are you planning to fix this before committing, or in a follow-up patch? Before committing. Done already.
Thanks Henrik. I will wait until you've had a chance to speak to wtc@ and act on his feedback if any, before reviewing again. Cheers, Jói https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:248: uint32 read = read_pcb.Read(intermediate_buffer.get(), On 2013/05/08 16:05:05, Henrik Grunell wrote: > On 2013/05/08 14:14:22, Jói wrote: > > This and the stream.next_in, stream.avail_in setup below seems to be > duplicated > > in the while loop; can you avoid duplication if you make it a do/while loop? > > In case the intermediate buffer isn't filled, that is we won't have any more > data that this to compress more than this, my understanding is that we should go > directly to the BZ_FINISH action. The documentation isn't entirely clear if one > BZ_RUN action is OK, but that's how I interpret it. > > Do you prefer a do-while loop with a break in it? I see. I think a do-while loop with a break will avoid the code duplication, which makes it easier to read and maintain. You might want to document that you're intentionally skipping the BZ_RUN if BZ_FINISH would be the next action, and rewriting the loop this way will give you a natural way to document that (at the break).
Wan-Teh doesn't know of any multipart code in net, none of us can find anything searching for it. My conclusion is there is no reusable code for this. (Other than possibly putting the function in cloud print I've copied in say net.) I'll change to a do-while-break in the next patch. On 2013/05/08 17:00:31, Jói wrote: > Thanks Henrik. I will wait until you've had a chance to speak to wtc@ and act on > his feedback if any, before reviewing again. > > Cheers, > Jói > > https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... > File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): > > https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... > components/webrtc_log_uploader/webrtc_log_uploader.cc:248: uint32 read = > read_pcb.Read(intermediate_buffer.get(), > On 2013/05/08 16:05:05, Henrik Grunell wrote: > > On 2013/05/08 14:14:22, Jói wrote: > > > This and the stream.next_in, stream.avail_in setup below seems to be > > duplicated > > > in the while loop; can you avoid duplication if you make it a do/while loop? > > > > In case the intermediate buffer isn't filled, that is we won't have any more > > data that this to compress more than this, my understanding is that we should > go > > directly to the BZ_FINISH action. The documentation isn't entirely clear if > one > > BZ_RUN action is OK, but that's how I interpret it. > > > > Do you prefer a do-while loop with a break in it? > > I see. I think a do-while loop with a break will avoid the code duplication, > which makes it easier to read and maintain. You might want to document that > you're intentionally skipping the BZ_RUN if BZ_FINISH would be the next action, > and rewriting the loop this way will give you a natural way to document that (at > the break).
OK, thanks Henrik. If there's a function copied from Cloud Print (a DRY violation) then that definitely needs to go somewhere that both pieces of code can use it. Somewhere in //net/base may be appropriate (e.g. I recall a MIME utilities file), Wan-Teh could answer for that. Cheers, Jói On Fri, May 10, 2013 at 8:05 AM, <grunell@chromium.org> wrote: > Wan-Teh doesn't know of any multipart code in net, none of us can find > anything > searching for it. My conclusion is there is no reusable code for this. > (Other > than possibly putting the function in cloud print I've copied in say net.) > > I'll change to a do-while-break in the next patch. > > > On 2013/05/08 17:00:31, Jói wrote: >> >> Thanks Henrik. I will wait until you've had a chance to speak to wtc@ and >> act > > on >> >> his feedback if any, before reviewing again. > > >> Cheers, >> Jói > > > > https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... >> >> File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): > > > > https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... >> >> components/webrtc_log_uploader/webrtc_log_uploader.cc:248: uint32 read = >> read_pcb.Read(intermediate_buffer.get(), >> On 2013/05/08 16:05:05, Henrik Grunell wrote: >> > On 2013/05/08 14:14:22, Jói wrote: >> > > This and the stream.next_in, stream.avail_in setup below seems to be >> > duplicated >> > > in the while loop; can you avoid duplication if you make it a do/while > > loop? >> >> > >> > In case the intermediate buffer isn't filled, that is we won't have any >> > more >> > data that this to compress more than this, my understanding is that we > > should >> >> go >> > directly to the BZ_FINISH action. The documentation isn't entirely clear >> > if >> one >> > BZ_RUN action is OK, but that's how I interpret it. >> > >> > Do you prefer a do-while loop with a break in it? > > >> I see. I think a do-while loop with a break will avoid the code >> duplication, >> which makes it easier to read and maintain. You might want to document >> that >> you're intentionally skipping the BZ_RUN if BZ_FINISH would be the next > > action, >> >> and rewriting the loop this way will give you a natural way to document >> that > > (at >> >> the break). > > > > > https://chromiumcodereview.appspot.com/14329020/
On 2013/05/10 10:43:57, Jói wrote: > > If there's a function copied from Cloud Print (a DRY violation) then > that definitely needs to go somewhere that both pieces of code can use > it. Somewhere in //net/base may be appropriate (e.g. I recall a MIME > utilities file), Wan-Teh could answer for that. I seem to remember someone wants to move the MIME utilities file out of net because it is not used by net itself. So the function for creating a multipart file would have the same problem. I don't have a better idea though. Perhaps somewhere in Blink, or content/browser/net or chrome/browser/net?
Hi Henrik, I checked that what WebRtcLogUploader produces is a valid multipart body according to RFC 2046. You might want to mention the standard in your comments so that anybody modifying the code in the future can make sure to keep the output valid. Other than that, I read the CL through, checked for general issues, and left a couple of comments. What I did not do is a review of the code structure (what is in which file or directory), or anything specific to the parts of Chromium you touched -- I rely on you getting OWNERS reviews for that. Please let me know if you want me to review more. Otherwise I'll take another look once you address my comments. Thanks :) Vaclav https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:69: CHECK(url_request_context_.get()); nit: Put this CHECK into the if branch above? https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:141: content_type.append(kMultipartBoundary); |content_type| looks like a composition of compile-time constants. Could it be a compile- or object-constant here instead of composing it in every UploadLog call? https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:192: const std::string& value) { nit: correct indenting https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:204: std::stringstream ss; I don't think the Style Guide allows streams http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream.... Could you just append to a string? https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.h (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:48: void UploadLog(scoped_ptr<base::SharedMemory> shared_memory, uint32 length); If the contract is that ApplyForStartLogging() needs to be called before calling UploadLog(...), it should be noted in the comment here. https://chromiumcodereview.appspot.com/14329020/diff/34005/content/public/bro... File content/public/browser/content_browser_client.h (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/content/public/bro... content/public/browser/content_browser_client.h:556: // of simultaneos logs is reached already. typo: simultaneos -> simultaneous https://chromiumcodereview.appspot.com/14329020/diff/34005/content/renderer/m... File content/renderer/media/webrtc_logging_handler_impl.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/content/renderer/m... content/renderer/media/webrtc_logging_handler_impl.cc:52: length, Please correct indentation.
> I seem to remember someone wants to move the MIME utilities file > out of net because it is not used by net itself. So the function > for creating a multipart file would have the same problem. Aren't there other parts of //net that are not used by //net itself? I'm not sure it feels right to insist that everything in a library must be used by something else in the library, especially a fairly low-level library like //net. There are certainly many bits of //base that shouldn't be in //base by that argument, but are because they're used in multiple places and belong at that layer of our layer cake. That said, it looks like the mime_util.h actually is used in //net/base by mime_sniffer.cc, filter.cc, and net_util.cc, and in //net/url_request by url_request_file_job.cc and url_request_http_job.cc. Cheers, Jói On Fri, May 10, 2013 at 7:00 PM, <wtc@chromium.org> wrote: > On 2013/05/10 10:43:57, Jói wrote: > >> If there's a function copied from Cloud Print (a DRY violation) then >> that definitely needs to go somewhere that both pieces of code can use >> it. Somewhere in //net/base may be appropriate (e.g. I recall a MIME >> utilities file), Wan-Teh could answer for that. > > > I seem to remember someone wants to move the MIME utilities file > out of net because it is not used by net itself. So the function > for creating a multipart file would have the same problem. > > I don't have a better idea though. Perhaps somewhere in Blink, > or content/browser/net or chrome/browser/net? > > https://chromiumcodereview.appspot.com/14329020/
On 2013/05/13 14:12:15, Jói wrote: > > I'm not sure it feels right to insist that everything in a library > must be used by something else in the library, especially a fairly > low-level library like //net. There are certainly many bits of //base > that shouldn't be in //base by that argument, but are because they're > used in multiple places and belong at that layer of our layer cake. Your argument about //base is sound.
OK, I take it you are agreeing. Created CL https://codereview.chromium.org/15076008/ for moving the function. On 2013/05/13 17:11:07, wtc wrote: > On 2013/05/13 14:12:15, Jói wrote: > > > > I'm not sure it feels right to insist that everything in a library > > must be used by something else in the library, especially a fairly > > low-level library like //net. There are certainly many bits of //base > > that shouldn't be in //base by that argument, but are because they're > > used in multiple places and belong at that layer of our layer cake. > > Your argument about //base is sound.
On 2013/05/13 08:55:50, vabr (Chromium) wrote: > Hi Henrik, > > I checked that what WebRtcLogUploader produces is a valid multipart body > according to RFC 2046. You might want to mention the standard in your comments > so that anybody modifying the code in the future can make sure to keep the > output valid. Thanks! Added that to a comment. > Other than that, I read the CL through, checked for general issues, and left a > couple of comments. > What I did not do is a review of the code structure (what is in which file or > directory), or anything specific to the parts of Chromium you touched -- I rely > on you getting OWNERS reviews for that. OK. > Please let me know if you want me to review more. Otherwise I'll take another > look once you address my comments. No, nothing else thanks. > Thanks :) > Vaclav >
https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:69: CHECK(url_request_context_.get()); On 2013/05/13 08:55:50, vabr (Chromium) wrote: > nit: Put this CHECK into the if branch above? Done. https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:141: content_type.append(kMultipartBoundary); On 2013/05/13 08:55:50, vabr (Chromium) wrote: > |content_type| looks like a composition of compile-time constants. Could it be a > compile- or object-constant here instead of composing it in every UploadLog > call? Yes it could (and I agree it's nicer), but the boundary (kMultipartBoundary) would be duplicated in this constant, which I'd prefer to avoid. Uploads are done rarely, so penalty-wise it's not a big thing. What do you think? https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:192: const std::string& value) { On 2013/05/13 08:55:50, vabr (Chromium) wrote: > nit: correct indenting Done. https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:204: std::stringstream ss; On 2013/05/13 08:55:50, vabr (Chromium) wrote: > I don't think the Style Guide allows streams > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream.... > Could you just append to a string? OK, sure. Done. https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.h (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:48: void UploadLog(scoped_ptr<base::SharedMemory> shared_memory, uint32 length); On 2013/05/13 08:55:50, vabr (Chromium) wrote: > If the contract is that ApplyForStartLogging() needs to be called before calling > UploadLog(...), it should be noted in the comment here. Done. https://chromiumcodereview.appspot.com/14329020/diff/34005/content/public/bro... File content/public/browser/content_browser_client.h (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/content/public/bro... content/public/browser/content_browser_client.h:556: // of simultaneos logs is reached already. On 2013/05/13 08:55:50, vabr (Chromium) wrote: > typo: simultaneos -> simultaneous Done. https://chromiumcodereview.appspot.com/14329020/diff/34005/content/renderer/m... File content/renderer/media/webrtc_logging_handler_impl.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/content/renderer/m... content/renderer/media/webrtc_logging_handler_impl.cc:52: length, On 2013/05/13 08:55:50, vabr (Chromium) wrote: > Please correct indentation. Done.
Adding owners reviewers. Please review.
sky@: chrome/*
tommi@: content/browser/renderer_host/media/*
content/renderer/media/*
Adding wtc@ for owner review of added dep on net in components/webrtc_log_uploader/DEPS.
LGTM https://codereview.chromium.org/14329020/diff/34005/components/webrtc_log_upl... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://codereview.chromium.org/14329020/diff/34005/components/webrtc_log_upl... components/webrtc_log_uploader/webrtc_log_uploader.cc:141: content_type.append(kMultipartBoundary); On 2013/05/14 13:48:50, Henrik Grunell wrote: > On 2013/05/13 08:55:50, vabr (Chromium) wrote: > > |content_type| looks like a composition of compile-time constants. Could it be > a > > compile- or object-constant here instead of composing it in every UploadLog > > call? > > Yes it could (and I agree it's nicer), but the boundary (kMultipartBoundary) > would be duplicated in this constant, which I'd prefer to avoid. Uploads are > done rarely, so penalty-wise it's not a big thing. What do you think? I see three possibilities: 1. #define kMultipartBoundaryMacro "----**--yradnuoBgoLtrapitluMklaTelgooG--**----" #define kUploadContentTypeMacro "multipart/form-data" const char kUploadContentType[] = kUploadContentTypeMacro; const char kMultipartBoundary[] = kMultipartBoundaryMacro; const char kContentType = kUploadContentTypeMacro "; boundary=" kMultipartBoundaryMacro #undef kBoundary #undef kUploadContentTypeMacro 2. Do what you do with |content_type|, but as a member variable of WebRtcLogUploader, created on construction. 3. Keep it as you have it now. Given you said that uploads are done rarely, I do not think that the options are much different. So please chose which one you like best, even if it is 3. I was merely making sure that you have considered alternatives, rather than forcing a change on you. https://codereview.chromium.org/14329020/diff/34005/components/webrtc_log_upl... components/webrtc_log_uploader/webrtc_log_uploader.cc:141: content_type.append(kMultipartBoundary); On 2013/05/14 13:48:50, Henrik Grunell wrote: > On 2013/05/13 08:55:50, vabr (Chromium) wrote: > > |content_type| looks like a composition of compile-time constants. Could it be > a > > compile- or object-constant here instead of composing it in every UploadLog > > call? > > Yes it could (and I agree it's nicer), but the boundary (kMultipartBoundary) > would be duplicated in this constant, which I'd prefer to avoid. Uploads are > done rarely, so penalty-wise it's not a big thing. What do you think?
I find the coverage of ENABLE_WEBRTC is bit confusing. Shouldn't it be every where you're using something from WebRtc?
Patch set 12 LGTM. As instructed, I only reviewed components/webrtc_log_uploader/DEPS. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/compone... File components/components.gyp (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/components/compone... components/components.gyp:16: 'webrtc_log_uploader.gypi', You probably should list this in alphabetical order. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... File components/webrtc_log_uploader/DEPS (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/DEPS:3: "+net", This LGTM.
https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:7: #if defined(USE_SYSTEM_LIBBZ2) nit: I think conditional includes usually come after the normal includes https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:32: const uint32 kIntermediateCompressionBufferSize = 256 * 1024; // 256 KB Suggest renaming to kIntermediateCompressionBufferBytes to make the unit obvious from the name. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:39: // Config getter that always returns direct settings. Can you document the motivation for this? I see it's used only for Linux and Android below. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:108: delete source; Why are you deleting the object? I don't think ownership has been passed to this method, at least I can't find documentation of that in URLFetcherDelegate. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:152: DLOG(ERROR) << "Could not create temporary file"; Suggest a more informative message, i.e. that it's the WebRtcLogUploader and perhaps the path you tried to create. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:169: DCHECK(error == base::PLATFORM_FILE_OK); DCHECK_EQ https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:171: // TODO(grunell): Correct product name. Is this TODO (and the one 2 lines down) for the current change, or a follow-up? https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:199: // TODO(grunell): Implement. I'm assuming this is needed for the current change? https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:211: base::WritePlatformFileAtCurrentPos(multipart_file, str.c_str(), str.size()); I have to wonder, since the log_buffer is in memory and makes up the bulk of the file that is uploaded, do you really need to write to a file and then upload the file? Why not just do everything in memory and then use the std::string version of URLFetcher::SetUploadData? That seems slightly less prone to error and the code would possibly be a little bit simpler. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.h (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:75: std::vector<net::URLFetcher*> url_fetchers_; A one-liner comment about ownership (or not) of these URLFetcher objects would be good. https://chromiumcodereview.appspot.com/14329020/diff/70001/content/browser/re... File content/browser/renderer_host/media/webrtc_logging_handler_host.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/content/browser/re... content/browser/renderer_host/media/webrtc_logging_handler_host.cc:50: void WebRtcLoggingHandlerHost::CheckLoggingAllowed() { The name could be a bit more descriptive; how about OpenLogIfAllowed?
- Added agl@ as reviewer of third_party/zlib/* and addition of zlib in components/webrtc_log_uploader/DEPS. - Replaced bzip2 with zlib; the former has been deleted from Chromium. - Code review fixes. https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/29001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:248: uint32 read = read_pcb.Read(intermediate_buffer.get(), On 2013/05/08 17:00:31, Jói wrote: > On 2013/05/08 16:05:05, Henrik Grunell wrote: > > On 2013/05/08 14:14:22, Jói wrote: > > > This and the stream.next_in, stream.avail_in setup below seems to be > > duplicated > > > in the while loop; can you avoid duplication if you make it a do/while loop? > > > > In case the intermediate buffer isn't filled, that is we won't have any more > > data that this to compress more than this, my understanding is that we should > go > > directly to the BZ_FINISH action. The documentation isn't entirely clear if > one > > BZ_RUN action is OK, but that's how I interpret it. > > > > Do you prefer a do-while loop with a break in it? > > I see. I think a do-while loop with a break will avoid the code duplication, > which makes it easier to read and maintain. You might want to document that > you're intentionally skipping the BZ_RUN if BZ_FINISH would be the next action, > and rewriting the loop this way will give you a natural way to document that (at > the break). Done. https://chromiumcodereview.appspot.com/14329020/diff/29001/content/renderer/m... File content/renderer/media/webrtc_logging_handler_impl.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/29001/content/renderer/m... content/renderer/media/webrtc_logging_handler_impl.cc:51: length, On 2013/05/08 14:14:22, Jói wrote: > indentation is off Done. https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/34005/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:141: content_type.append(kMultipartBoundary); On 2013/05/14 15:24:41, vabr (Chromium) wrote: > On 2013/05/14 13:48:50, Henrik Grunell wrote: > > On 2013/05/13 08:55:50, vabr (Chromium) wrote: > > > |content_type| looks like a composition of compile-time constants. Could it > be > > a > > > compile- or object-constant here instead of composing it in every UploadLog > > > call? > > > > Yes it could (and I agree it's nicer), but the boundary (kMultipartBoundary) > > would be duplicated in this constant, which I'd prefer to avoid. Uploads are > > done rarely, so penalty-wise it's not a big thing. What do you think? > > I see three possibilities: > > 1. > #define kMultipartBoundaryMacro "----**--yradnuoBgoLtrapitluMklaTelgooG--**----" > #define kUploadContentTypeMacro "multipart/form-data" > const char kUploadContentType[] = kUploadContentTypeMacro; > const char kMultipartBoundary[] = kMultipartBoundaryMacro; > const char kContentType = kUploadContentTypeMacro "; boundary=" > kMultipartBoundaryMacro > #undef kBoundary > #undef kUploadContentTypeMacro > > 2. > Do what you do with |content_type|, but as a member variable of > WebRtcLogUploader, created on construction. > > 3. > Keep it as you have it now. > > Given you said that uploads are done rarely, I do not think that the options are > much different. So please chose which one you like best, even if it is 3. I was > merely making sure that you have considered alternatives, rather than forcing a > change on you. Yes I did think of these options, I prefer 3 (simpler, less lines of code) so I'll stick to that unless someone says no. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/compone... File components/components.gyp (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/components/compone... components/components.gyp:16: 'webrtc_log_uploader.gypi', On 2013/05/14 18:49:47, wtc wrote: > > You probably should list this in alphabetical order. Done. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:7: #if defined(USE_SYSTEM_LIBBZ2) On 2013/05/14 21:30:29, Jói wrote: > nit: I think conditional includes usually come after the normal includes I've replaced bzlib with zlib; the former has been removed from Chromium. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:32: const uint32 kIntermediateCompressionBufferSize = 256 * 1024; // 256 KB On 2013/05/14 21:30:29, Jói wrote: > Suggest renaming to kIntermediateCompressionBufferBytes to make the unit obvious > from the name. Done. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:39: // Config getter that always returns direct settings. On 2013/05/14 21:30:29, Jói wrote: > Can you document the motivation for this? I see it's used only for Linux and > Android below. I'll double check with someone who knows net, background is I got a DCHECK fail in ProxyService::CreateUsingSystemProxyResolver (when starting the fetcher I think), and the function to set the proxy config service only exists on linux and android. I'll update comment (or code) after getting clearer on this. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:108: delete source; On 2013/05/14 21:30:29, Jói wrote: > Why are you deleting the object? I don't think ownership has been passed to > this method, at least I can't find documentation of that in URLFetcherDelegate. You're right, I think I got that from a unit test in a weak moment. Removed that and also |url_fetchers_| which I used (or planned to use) for something before but not anymore. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:152: DLOG(ERROR) << "Could not create temporary file"; On 2013/05/14 21:30:29, Jói wrote: > Suggest a more informative message, i.e. that it's the WebRtcLogUploader and > perhaps the path you tried to create. Done. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:169: DCHECK(error == base::PLATFORM_FILE_OK); On 2013/05/14 21:30:29, Jói wrote: > DCHECK_EQ Done. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:171: // TODO(grunell): Correct product name. On 2013/05/14 21:30:29, Jói wrote: > Is this TODO (and the one 2 lines down) for the current change, or a follow-up? I was thinking for the current change, but it can be made in a follow-up CL if that would make reviewers' life easier. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:199: // TODO(grunell): Implement. On 2013/05/14 21:30:29, Jói wrote: > I'm assuming this is needed for the current change? Same as reply above. That's my intention but could be made in follow-up. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:211: base::WritePlatformFileAtCurrentPos(multipart_file, str.c_str(), str.size()); On 2013/05/14 21:30:29, Jói wrote: > I have to wonder, since the log_buffer is in memory and makes up the bulk of the > file that is uploaded, do you really need to write to a file and then upload the > file? Why not just do everything in memory and then use the std::string version > of URLFetcher::SetUploadData? That seems slightly less prone to error and the > code would possibly be a little bit simpler. Very valid point. Here's why I chose a file: after this CL, I will add the possibility for the user to upload logs manually afterwards, for the case where the user setting doesn't allow automatic upload. Last x logs will be kept on the disk. So my plan was to prepare for that. I could use memory now and change later if you wish, but it would mean double work. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.h (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.h:75: std::vector<net::URLFetcher*> url_fetchers_; On 2013/05/14 21:30:29, Jói wrote: > A one-liner comment about ownership (or not) of these URLFetcher objects would > be good. I removed this, not used. https://chromiumcodereview.appspot.com/14329020/diff/70001/content/browser/re... File content/browser/renderer_host/media/webrtc_logging_handler_host.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/content/browser/re... content/browser/renderer_host/media/webrtc_logging_handler_host.cc:50: void WebRtcLoggingHandlerHost::CheckLoggingAllowed() { On 2013/05/14 21:30:29, Jói wrote: > The name could be a bit more descriptive; how about OpenLogIfAllowed? sgtm, done.
NACK on the zlib change. Nothing else has needed it and you're using entirely typical functions. Why is this code being different? Secondly, having __attribute__ in the zlib source is unlikely to go down well on Windows.
Henrik, sounds good to finish the TODOs in the current patch before sending out for review again. Re the memory vs. file thing; once you store the last N files, will they be stored as temporary files? (I'm assuming no.) If everything was done in memory it would be trivial to just write the memory buffer out to a file in one write operation once you need to store it, and in the case where you can upload, you would save the need to create and clean up a temporary file. Cheers, Jói On Wed, May 15, 2013 at 1:37 PM, <agl@chromium.org> wrote: > NACK on the zlib change. > > Nothing else has needed it and you're using entirely typical functions. Why > is > this code being different? > > Secondly, having __attribute__ in the zlib source is unlikely to go down > well on > Windows. > > https://chromiumcodereview.appspot.com/14329020/
lgtm for content/browser/renderer_host/media/* https://chromiumcodereview.appspot.com/14329020/diff/89001/content/browser/re... File content/browser/renderer_host/media/webrtc_logging_handler_host.h (right): https://chromiumcodereview.appspot.com/14329020/diff/89001/content/browser/re... content/browser/renderer_host/media/webrtc_logging_handler_host.h:13: } nit: } // base
looking at this change and the other ones in http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/media/webrtc..., can all these files be in src/chrome? this is because the intended consumer is really only in chrome anyways. if my quick reading is not off, it looks like the logging data comes from third_party/libjingle/overrides/talk/base/logging.h, which chrome can include as well. if that's the case, then all these logging related files in src/content should be in src/chrome instead.
- Code review fixes, including all todos (added on new though for a tbd). - Fixed ENABLE_WEBRTC usage. - Fixed zlib visibility issue - made the uploader a static lib instead of “<(component)”. - There’s a CL for moving all webrtc logging files and code to //chrome in https://chromiumcodereview.appspot.com/15741003/ - Rebase changes as well. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:39: // Config getter that always returns direct settings. On 2013/05/15 20:17:53, Henrik Grunell wrote: > On 2013/05/14 21:30:29, Jói wrote: > > Can you document the motivation for this? I see it's used only for Linux and > > Android below. > > I'll double check with someone who knows net, background is I got a DCHECK fail > in ProxyService::CreateUsingSystemProxyResolver (when starting the fetcher I > think), and the function to set the proxy config service only exists on linux > and android. I'll update comment (or code) after getting clearer on this. I've changed it so that it uses the system context request getter. (I checked with chrome-network-stack@ about this.) https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:171: // TODO(grunell): Correct product name. On 2013/05/15 20:17:53, Henrik Grunell wrote: > On 2013/05/14 21:30:29, Jói wrote: > > Is this TODO (and the one 2 lines down) for the current change, or a > follow-up? > > I was thinking for the current change, but it can be made in a follow-up CL if > that would make reviewers' life easier. It's done now. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:199: // TODO(grunell): Implement. On 2013/05/15 20:17:53, Henrik Grunell wrote: > On 2013/05/14 21:30:29, Jói wrote: > > I'm assuming this is needed for the current change? > > Same as reply above. That's my intention but could be made in follow-up. This functions has been removed; not needed. https://chromiumcodereview.appspot.com/14329020/diff/70001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:211: base::WritePlatformFileAtCurrentPos(multipart_file, str.c_str(), str.size()); On 2013/05/15 20:17:53, Henrik Grunell wrote: > On 2013/05/14 21:30:29, Jói wrote: > > I have to wonder, since the log_buffer is in memory and makes up the bulk of > the > > file that is uploaded, do you really need to write to a file and then upload > the > > file? Why not just do everything in memory and then use the std::string > version > > of URLFetcher::SetUploadData? That seems slightly less prone to error and the > > code would possibly be a little bit simpler. > > Very valid point. Here's why I chose a file: after this CL, I will add the > possibility for the user to upload logs manually afterwards, for the case where > the user setting doesn't allow automatic upload. Last x logs will be kept on the > disk. So my plan was to prepare for that. I could use memory now and change > later if you wish, but it would mean double work. Changed to writing to memory instead. https://chromiumcodereview.appspot.com/14329020/diff/89001/content/browser/re... File content/browser/renderer_host/media/webrtc_logging_handler_host.h (right): https://chromiumcodereview.appspot.com/14329020/diff/89001/content/browser/re... content/browser/renderer_host/media/webrtc_logging_handler_host.h:13: } On 2013/05/16 12:06:49, tommi wrote: > nit: > } // base Done.
Thanks for switching to doing everything in memory, looks much simpler to me. Cheers, Jói https://chromiumcodereview.appspot.com/14329020/diff/96001/chrome/browser/bro... File chrome/browser/browser_process_impl.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/96001/chrome/browser/bro... chrome/browser/browser_process_impl.cc:919: webrtc_log_uploader_.reset(new components::WebRtcLogUploader()); Suggest constructing only on demand (in the accessor), that way the object doesn't get created if logging is not enabled. https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:96: #if defined(OS_WIN) This logic for determining the product name for Crash seems like it must exist elsewhere; can we share source? https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:160: scoped_ptr<uint8[]> intermediate_buffer( As far as I can tell, this can be stack-allocated rather than heap-allocated, since its size doesn't change and is based on a constant. https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:164: IncreaseMultipartBufferSize(post_data, &stream); A better name might indicate that this sets stream up to output to post_data. How about e.g. ResizeForNextOutput or some such? https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:187: result = deflate(&stream, Z_FINISH); Will this be OK if (stream.avail_in == 0)? Seems like you can hit that condition. https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... File content/browser/renderer_host/media/webrtc_logging_handler_host.h (right): https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... content/browser/renderer_host/media/webrtc_logging_handler_host.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Looks like this file still needs to be rebased against the moved version in another (pending) change. I'll wait to do the final review until that happens, but am still looking at all files now. https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... content/browser/renderer_host/media/webrtc_logging_handler_host.h:40: void OnOpenLog(const std::string& app_session_id, const std::string& app_url); +80
Code review fixes. https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:96: #if defined(OS_WIN) On 2013/05/24 21:14:06, Jói wrote: > This logic for determining the product name for Crash seems like it must exist > elsewhere; can we share source? Yes, I've checked this with cpu@ and there isn't. (It's determined in platform specific files.) It's obviously better to put it in a shared place, though he thought it's OK to duplicate it here. https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:160: scoped_ptr<uint8[]> intermediate_buffer( On 2013/05/24 21:14:06, Jói wrote: > As far as I can tell, this can be stack-allocated rather than heap-allocated, > since its size doesn't change and is based on a constant. OK, done. https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:164: IncreaseMultipartBufferSize(post_data, &stream); On 2013/05/24 21:14:06, Jói wrote: > A better name might indicate that this sets stream up to output to post_data. > How about e.g. ResizeForNextOutput or some such? Agree. I thing the suggested name is fine. Done. https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... components/webrtc_log_uploader/webrtc_log_uploader.cc:187: result = deflate(&stream, Z_FINISH); On 2013/05/24 21:14:06, Jói wrote: > Will this be OK if (stream.avail_in == 0)? Seems like you can hit that > condition. Yes, this is OK. If there's no more input it will just do nothing. (I double checked by setting it to zero.) https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... File content/browser/renderer_host/media/webrtc_logging_handler_host.h (right): https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... content/browser/renderer_host/media/webrtc_logging_handler_host.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/05/24 21:14:06, Jói wrote: > Looks like this file still needs to be rebased against the moved version in > another (pending) change. I'll wait to do the final review until that happens, > but am still looking at all files now. Yes, now when it's been decided we'll move them, I'll rebase onto that. I'll do that in a separate patch. https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... content/browser/renderer_host/media/webrtc_logging_handler_host.h:40: void OnOpenLog(const std::string& app_session_id, const std::string& app_url); On 2013/05/24 21:14:06, Jói wrote: > +80 Done.
Thanks, fixes look good. I'll take another look once you've rebased. On Mon, May 27, 2013 at 1:00 PM, <grunell@chromium.org> wrote: > Code review fixes. > > > > https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... > File components/webrtc_log_uploader/webrtc_log_uploader.cc (right): > > https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... > components/webrtc_log_uploader/webrtc_log_uploader.cc:96: #if > defined(OS_WIN) > On 2013/05/24 21:14:06, Jói wrote: >> >> This logic for determining the product name for Crash seems like it > > must exist >> >> elsewhere; can we share source? > > > Yes, I've checked this with cpu@ and there isn't. (It's determined in > platform specific files.) It's obviously better to put it in a shared > place, though he thought it's OK to duplicate it here. > > > https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... > components/webrtc_log_uploader/webrtc_log_uploader.cc:160: > scoped_ptr<uint8[]> intermediate_buffer( > On 2013/05/24 21:14:06, Jói wrote: >> >> As far as I can tell, this can be stack-allocated rather than > > heap-allocated, >> >> since its size doesn't change and is based on a constant. > > > OK, done. > > > https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... > components/webrtc_log_uploader/webrtc_log_uploader.cc:164: > IncreaseMultipartBufferSize(post_data, &stream); > On 2013/05/24 21:14:06, Jói wrote: >> >> A better name might indicate that this sets stream up to output to > > post_data. >> >> How about e.g. ResizeForNextOutput or some such? > > > Agree. I thing the suggested name is fine. Done. > > > https://chromiumcodereview.appspot.com/14329020/diff/96001/components/webrtc_... > components/webrtc_log_uploader/webrtc_log_uploader.cc:187: result = > deflate(&stream, Z_FINISH); > On 2013/05/24 21:14:06, Jói wrote: >> >> Will this be OK if (stream.avail_in == 0)? Seems like you can hit that >> condition. > > > Yes, this is OK. If there's no more input it will just do nothing. (I > double checked by setting it to zero.) > > > https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... > File content/browser/renderer_host/media/webrtc_logging_handler_host.h > (right): > > https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... > content/browser/renderer_host/media/webrtc_logging_handler_host.h:1: // > Copyright (c) 2013 The Chromium Authors. All rights reserved. > On 2013/05/24 21:14:06, Jói wrote: >> >> Looks like this file still needs to be rebased against the moved > > version in >> >> another (pending) change. I'll wait to do the final review until that > > happens, >> >> but am still looking at all files now. > > > Yes, now when it's been decided we'll move them, I'll rebase onto that. > I'll do that in a separate patch. > > > https://chromiumcodereview.appspot.com/14329020/diff/96001/content/browser/re... > content/browser/renderer_host/media/webrtc_logging_handler_host.h:40: > void OnOpenLog(const std::string& app_session_id, const std::string& > app_url); > On 2013/05/24 21:14:06, Jói wrote: >> >> +80 > > > Done. > > https://chromiumcodereview.appspot.com/14329020/
On 2013/05/27 13:03:37, Jói wrote: > Thanks, fixes look good. I'll take another look once you've rebased. I've now rebased. Since the uploader uses the partial circular buffer which now is in //chrome, does it make sense to move the uploader to //chrome as well? Otherwise I'd have to either read the buffer and compress in //chrome thus introduce yet another object and essentially moving half the uploader anyway. Then I think all can as well move. Or hack away with the pointers and lengths given to the uploader which seems like overkill for this case and not the nicest solution.
The PartialCircularBuffer could live in //components/webrtc_log_uploader and be used from there by //chrome. It's up to you though. I generally recommend doing new features as components, since it prevents dependency creep (on the various internals of //chrome) and makes them trivially reusable in other top-level applications, but in your case if it seems very unlikely that it will need to be used elsewhere and you are willing to do the work to move it later if necessary, then you could put all of it in //chrome. Cheers, Jói On Tue, May 28, 2013 at 10:08 AM, <grunell@chromium.org> wrote: > On 2013/05/27 13:03:37, Jói wrote: >> >> Thanks, fixes look good. I'll take another look once you've rebased. > > > I've now rebased. Since the uploader uses the partial circular buffer which > now > is in //chrome, does it make sense to move the uploader to //chrome as well? > > Otherwise I'd have to either read the buffer and compress in //chrome thus > introduce yet another object and essentially moving half the uploader > anyway. > Then I think all can as well move. > > Or hack away with the pointers and lengths given to the uploader which seems > like overkill for this case and not the nicest solution. > > https://chromiumcodereview.appspot.com/14329020/
I've moved the uploader to //chrome. As you said it's unlikely it will be used by someone else and I'm happy to move it later if necessary. On 2013/05/28 10:50:18, Jói wrote: > The PartialCircularBuffer could live in > //components/webrtc_log_uploader and be used from there by //chrome. > > It's up to you though. I generally recommend doing new features as > components, since it prevents dependency creep (on the various > internals of //chrome) and makes them trivially reusable in other > top-level applications, but in your case if it seems very unlikely > that it will need to be used elsewhere and you are willing to do the > work to move it later if necessary, then you could put all of it in > //chrome. > > Cheers, > Jói > > > On Tue, May 28, 2013 at 10:08 AM, <mailto:grunell@chromium.org> wrote: > > On 2013/05/27 13:03:37, Jói wrote: > >> > >> Thanks, fixes look good. I'll take another look once you've rebased. > > > > > > I've now rebased. Since the uploader uses the partial circular buffer which > > now > > is in //chrome, does it make sense to move the uploader to //chrome as well? > > > > Otherwise I'd have to either read the buffer and compress in //chrome thus > > introduce yet another object and essentially moving half the uploader > > anyway. > > Then I think all can as well move. > > > > Or hack away with the pointers and lengths given to the uploader which seems > > like overkill for this case and not the nicest solution. > > > > https://chromiumcodereview.appspot.com/14329020/
LGTM for DEPS.
LGTM
chrom/browser LGTM (I didn't review c/b/media as you got other reviewers to review it)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/14329020/144001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/14329020/148001
Message was sent while issue was closed.
Change committed as 203700
Message was sent while issue was closed.
Committed patchset #23 manually as r203711 (presubmit successful). |
