Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1148633007:
Hooked the trace event argument whitelist up to the background_trace_manager (Closed)
|
Created:
5 years, 7 months ago by oystein (OOO til 10th of July) Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHooked the trace event argument whitelist up to the BackgroundTracingManager
R=dsinclair,nduca,davidben,shatch
BUG=466769
Committed: https://crrev.com/af2b800518e5f6a4b0704aa2ba936dc4ae569369
Cr-Commit-Position: refs/heads/master@{#333214}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 8
Patch Set 3 : Changed bool param to enum #
Total comments: 4
Patch Set 4 : Moved tracing enabled callback to its own testing function #
Total comments: 18
Patch Set 5 : Review fixes #
Total comments: 2
Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : All review fixes together now #Patch Set 9 : Buildfix #
Messages
Total messages: 36 (15 generated)
oysteine@chromium.org changed reviewers: + simonhatch@chromium.org
https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... content/browser/tracing/background_tracing_manager_browsertest.cc:188: // NavigateToURL(shell(), GetTestUrl("", "title.html")); nit: Get rid of this. https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... content/browser/tracing/background_tracing_manager_impl.cc:238: trace_options.enable_argument_filter = requires_anonymized_data_; Am I taking crazy pills, or is this not actually used. https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... File content/browser/tracing/background_tracing_manager_impl.h (right): https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... content/browser/tracing/background_tracing_manager_impl.h:24: const base::Closure& enabled_callback, Is this needed? Just for testing? https://codereview.chromium.org/1148633007/diff/1/content/public/browser/back... File content/public/browser/background_tracing_config.h (right): https://codereview.chromium.org/1148633007/diff/1/content/public/browser/back... content/public/browser/background_tracing_config.h:32: bool compress_trace; Like we talked about, don't change interface just for testing. https://codereview.chromium.org/1148633007/diff/1/content/public/browser/back... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1148633007/diff/1/content/public/browser/back... content/public/browser/background_tracing_manager.h:42: typedef base::Callback<void(const scoped_refptr<base::RefCountedString>&, Intentional change?
https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... content/browser/tracing/background_tracing_manager_browsertest.cc:166: NoWhitelistedArgsStripped) { Should you write 2 tests, enabled and disabled. Since that trace_options in the impl didn't seem to be used, would have shown up.
https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... content/browser/tracing/background_tracing_manager_browsertest.cc:166: NoWhitelistedArgsStripped) { On 2015/05/22 22:25:05, shatch wrote: > Should you write 2 tests, enabled and disabled. Since that trace_options in the > impl didn't seem to be used, would have shown up. Test improved now, would've caught it. https://codereview.chromium.org/1148633007/diff/1/content/browser/tracing/bac... content/browser/tracing/background_tracing_manager_browsertest.cc:188: // NavigateToURL(shell(), GetTestUrl("", "title.html")); On 2015/05/22 22:21:37, shatch wrote: > nit: Get rid of this. Done.
Patchset #2 (id:20001) has been deleted
oysteine@chromium.org changed reviewers: + davidben@chromium.org, dsinclair@chromium.org, nduca@chromium.org
+dsinclair/nduca for tracing +davidben for content/public ptal!
dsinclair/nduca: ptal :)
https://codereview.chromium.org/1148633007/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1148633007/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_browsertest.cc:248: // rather than return potential PII. We crash the process? That seems .... bad? https://codereview.chromium.org/1148633007/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_impl.h (right): https://codereview.chromium.org/1148633007/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_impl.h:25: bool) override; Can you add a parameter name to bool, this isn't very descriptive ... Or better yet, make it an enum so we'll see ANONYMOUS_DATA_ONLY or something instead of true/false https://codereview.chromium.org/1148633007/diff/40001/content/public/browser/... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1148633007/diff/40001/content/public/browser/... content/public/browser/background_tracing_manager.h:62: const base::Closure& enabled_callback, What is this for? I see it pass around all over the place but never used. Can we remove it until we have a CL which needs it?
https://codereview.chromium.org/1148633007/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1148633007/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_browsertest.cc:248: // rather than return potential PII. On 2015/06/01 15:09:37, dsinclair wrote: > We crash the process? That seems .... bad? It's a CHECK() in the childprocess; shouldn't ever happen, this is just a safeguard in case someone removes it and we start getting PII data. https://codereview.chromium.org/1148633007/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_impl.h (right): https://codereview.chromium.org/1148633007/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_manager_impl.h:25: bool) override; On 2015/06/01 15:09:37, dsinclair wrote: > Can you add a parameter name to bool, this isn't very descriptive ... Or better > yet, make it an enum so we'll see ANONYMOUS_DATA_ONLY or something instead of > true/false Done. https://codereview.chromium.org/1148633007/diff/40001/content/public/browser/... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1148633007/diff/40001/content/public/browser/... content/public/browser/background_tracing_manager.h:62: const base::Closure& enabled_callback, On 2015/06/01 15:09:37, dsinclair wrote: > What is this for? I see it pass around all over the place but never used. Can we > remove it until we have a CL which needs it? It's used in the BackgroundTracingManagerBrowserTest.NoWhitelistedArgsStripped browsertest added in this CL, to wait until tracing is fully enabled before we start emitting events.
https://codereview.chromium.org/1148633007/diff/40001/content/public/browser/... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1148633007/diff/40001/content/public/browser/... content/public/browser/background_tracing_manager.h:62: const base::Closure& enabled_callback, On 2015/06/01 18:23:39, Oystein wrote: > On 2015/06/01 15:09:37, dsinclair wrote: > > What is this for? I see it pass around all over the place but never used. Can > we > > remove it until we have a CL which needs it? > > It's used in the BackgroundTracingManagerBrowserTest.NoWhitelistedArgsStripped > browsertest added in this CL, to wait until tracing is fully enabled before we > start emitting events. If it's only for testing, it's too bad we have to have it in the public API. Is there any way we can move this to a testing only setting that we use instead? https://codereview.chromium.org/1148633007/diff/60001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_impl.cc (left): https://codereview.chromium.org/1148633007/diff/60001/content/browser/tracing... content/browser/tracing/background_tracing_manager_impl.cc:235: TracingController::EnableRecordingDoneCallback()); Where does this callback get set, I'm not seeing it by grepping around. https://codereview.chromium.org/1148633007/diff/60001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/1148633007/diff/60001/content/browser/tracing... content/browser/tracing/background_tracing_manager_impl.cc:304: BackgroundTracingConfig* BackgroundTracingConfig::FromDict( Why the ownership change?
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Thanks dan! https://codereview.chromium.org/1148633007/diff/40001/content/public/browser/... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1148633007/diff/40001/content/public/browser/... content/public/browser/background_tracing_manager.h:62: const base::Closure& enabled_callback, On 2015/06/01 18:50:05, dsinclair wrote: > On 2015/06/01 18:23:39, Oystein wrote: > > On 2015/06/01 15:09:37, dsinclair wrote: > > > What is this for? I see it pass around all over the place but never used. > Can > > we > > > remove it until we have a CL which needs it? > > > > It's used in the BackgroundTracingManagerBrowserTest.NoWhitelistedArgsStripped > > browsertest added in this CL, to wait until tracing is fully enabled before we > > start emitting events. > > If it's only for testing, it's too bad we have to have it in the public API. Is > there any way we can move this to a testing only setting that we use instead? Done. https://codereview.chromium.org/1148633007/diff/60001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_impl.cc (left): https://codereview.chromium.org/1148633007/diff/60001/content/browser/tracing... content/browser/tracing/background_tracing_manager_impl.cc:235: TracingController::EnableRecordingDoneCallback()); On 2015/06/01 18:50:05, dsinclair wrote: > Where does this callback get set, I'm not seeing it by grepping around. It's the same one from the other review comments that's only set for testing; I moved it into a member now. https://codereview.chromium.org/1148633007/diff/60001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/1148633007/diff/60001/content/browser/tracing... content/browser/tracing/background_tracing_manager_impl.cc:304: BackgroundTracingConfig* BackgroundTracingConfig::FromDict( On 2015/06/01 18:50:05, dsinclair wrote: > Why the ownership change? I think simonhatch@ already changed this to return a scoped_ptr, in another CL.
lgtm
The CQ bit was checked by oysteine@chromium.org
The CQ bit was unchecked by oysteine@chromium.org
On 2015/06/01 20:52:11, dsinclair wrote: > lgtm Thanks! Friendly ping to davidben@ for content/public
Sorry about the delay. https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_browsertest.cc (left): https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:104: config.Pass(), upload_config_wrapper.get_receive_callback(), true); Why does both true and false (here and DisableScenarioWhenIdle) convert to NO_DATA_FILTERING? https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:45: stream.total_out = stream.avail_out = output_buffer_length; Are you supposed to be setting total_in and total_out? This says: The fields total_in and total_out can be used for statistics or progress reports. After compression, total_in holds the total size of the uncompressed data and may be saved for use in the decompressor (particularly if the decompressor wants to decompress everything in a single step). net/filter/gzip_filter.cc doesn't set it. And poking around codesearch, I think zlib sets it for you: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/zlib/i... https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:47: stream.next_out = (Bytef*)&output_str[0]; You can use vector_as_array from base/stl_util.h https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:51: stream.opaque = Z_NULL; You're already initializing it with zeros, right? https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:53: int result = inflateInit2(&stream, 16 + MAX_WBITS); Where does 16 + MAX_WBITS come from? https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:56: bool success = (result != Z_STREAM_ERROR); result == Z_STREAM_END? It seems there are other possible error codes like Z_BUF_ERROR. Or better: EXPECT_EQ(Z_STREAM_END, result); https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:60: last_file_contents_ = &output_str[0]; This is rather confusing since you're writing a pointer into a std::string. Switch it to last_file_contents_.assign? It's also assuming that inflate writes a NUL-terminated string, and I would think it doesn't. (I think you're actually getting the NUL terminator from initializing the buffer to zeros.) I think you actually want: last_file_contents_.assign(vector_as_array(&output_str), bytes_written); where bytes_written is the number of bytes that inflate actually wrote. I'm having a hard time understanding inflate's API, but going by gzip_filter.cc, it seems you're supposed to do this: https://code.google.com/p/chromium/codesearch#chromium/src/net/filter/gzip_fi... https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:202: { Why an extra level of scope here? https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:253: { Ditto. https://codereview.chromium.org/1148633007/diff/120001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1148633007/diff/120001/content/public/browser... content/public/browser/background_tracing_manager.h:66: DataFiltering data_filtering) = 0; For a follow-up since this was like this before: I believe methods of Foo in //content that are only called elsewhere in //content just get put on the FooImpl and then we cast to FooImpl.
On 2015/06/02 22:52:02, David Benjamin wrote: > Sorry about the delay. Np, thanks for the review! :) https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_browsertest.cc (left): https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:104: config.Pass(), upload_config_wrapper.get_receive_callback(), true); On 2015/06/02 22:52:02, David Benjamin wrote: > Why does both true and false (here and DisableScenarioWhenIdle) convert to > NO_DATA_FILTERING? The parameter didn't actually do anything before this CL, and this test in particular shouldn't test any filtering. https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:45: stream.total_out = stream.avail_out = output_buffer_length; On 2015/06/02 22:52:02, David Benjamin wrote: > Are you supposed to be setting total_in and total_out? This says: > > The fields total_in and total_out can be used for statistics or progress > reports. After compression, total_in holds the total size of the uncompressed > data and may be saved for use in the decompressor (particularly if the > decompressor wants to decompress everything in a single step). > > net/filter/gzip_filter.cc doesn't set it. And poking around codesearch, I think > zlib sets it for you: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/zlib/i... Done. https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:51: stream.opaque = Z_NULL; On 2015/06/02 22:52:01, David Benjamin wrote: > You're already initializing it with zeros, right? Removed. https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:53: int result = inflateInit2(&stream, 16 + MAX_WBITS); On 2015/06/02 22:52:02, David Benjamin wrote: > Where does 16 + MAX_WBITS come from? zlib.h:771: (inflateInit2 docs) "The windowBits parameter is the base two logarithm of the maximum window size (the size of the history buffer)." (...) "windowBits can also be greater than 15 for optional gzip decoding. Add 32 to windowBits to enable zlib and gzip decoding with automatic header detection, or add 16 to decode only the gzip format (the zlib format will return a Z_DATA_ERROR). If a gzip stream is being decoded, strm->adler is a crc32 instead of an adler32." Added a comment to explain this. https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:56: bool success = (result != Z_STREAM_ERROR); On 2015/06/02 22:52:01, David Benjamin wrote: > result == Z_STREAM_END? It seems there are other possible error codes like > Z_BUF_ERROR. Or better: > EXPECT_EQ(Z_STREAM_END, result); Done. https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:60: last_file_contents_ = &output_str[0]; On 2015/06/02 22:52:02, David Benjamin wrote: > This is rather confusing since you're writing a pointer into a std::string. > Switch it to last_file_contents_.assign? > > It's also assuming that inflate writes a NUL-terminated string, and I would > think it doesn't. (I think you're actually getting the NUL terminator from > initializing the buffer to zeros.) I think you actually want: > > last_file_contents_.assign(vector_as_array(&output_str), bytes_written); > > where bytes_written is the number of bytes that inflate actually wrote. I'm > having a hard time understanding inflate's API, but going by gzip_filter.cc, it > seems you're supposed to do this: > > https://code.google.com/p/chromium/codesearch#chromium/src/net/filter/gzip_fi... Ah good catch; yeah you're right, it was indeed relying on the vector initialization. Done! https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:202: { On 2015/06/02 22:52:02, David Benjamin wrote: > Why an extra level of scope here? Whoops, copy and paste artifact from the existing tests :/. Removed, sorry. https://codereview.chromium.org/1148633007/diff/120001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:253: { On 2015/06/02 22:52:01, David Benjamin wrote: > Ditto. Done.
davidben: ping :)
lgtm. Just two random style comments. https://codereview.chromium.org/1148633007/diff/140001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1148633007/diff/140001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:40: const size_t output_buffer_length = 10 * 1024 * 1024; Style nit: kOutputBufferLength https://codereview.chromium.org/1148633007/diff/140001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_impl.h (right): https://codereview.chromium.org/1148633007/diff/140001/content/browser/tracin... content/browser/tracing/background_tracing_manager_impl.h:25: bool) override; Style: parameters should be named. I don't think Chromium omits them like Blink does. https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
On 2015/06/05 16:40:07, David Benjamin wrote: > lgtm. Just two random style comments. > Awesome, thanks! Both style issues fixed.
The CQ bit was checked by oysteine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1148633007/#ps200001 (title: "All review fixes together now")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1148633007/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
Patchset #9 (id:220001) has been deleted
Patchset #10 (id:260001) has been deleted
Patchset #9 (id:240001) has been deleted
The CQ bit was checked by oysteine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1148633007/#ps280001 (title: "Buildfix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1148633007/280001
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/af2b800518e5f6a4b0704aa2ba936dc4ae569369 Cr-Commit-Position: refs/heads/master@{#333214} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
