Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11218002:
Add a command line option to save autofill upload payloads to disk. (Closed)
|
Created:
8 years, 2 months ago by Albert Bodenhamer Modified:
8 years, 2 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, dhollowa+watch_chromium.org, browser-components-watch_chromium.org, dyu1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd logging of autofill upload request.
BUG=156455
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163130
Patch Set 1 #Patch Set 2 : Fix incorrectly removed line #
Total comments: 2
Patch Set 3 : Switch to log approach #Patch Set 4 : Fix includes #
Total comments: 1
Patch Set 5 : Review feedback #Messages
Total messages: 13 (0 generated)
Ilya, can you take a look?
I think we can make this even simpler -- see inline. WDYT? https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/auto... File chrome/browser/autofill/autofill_download.cc (right): https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/auto... chrome/browser/autofill/autofill_download.cc:180: } Rather than adding a new command-line switch and saving to a file, how about just using VLOGs, which can be redirected to a file if need be? https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/auto... File chrome/browser/autofill/autofill_download.h (right): https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/auto... chrome/browser/autofill/autofill_download.h:106: const std::string& form_xml); nit: Can these be tucked into the anonymous namespace in the implementation file? If so, that's generally preferred to private static member functions.
On Wed, Oct 17, 2012 at 4:37 PM, <isherman@chromium.org> wrote: > I think we can make this even simpler -- see inline. WDYT? > > > https://chromiumcodereview.**appspot.com/11218002/diff/** > 2001/chrome/browser/autofill/**autofill_download.cc<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.cc> > File chrome/browser/autofill/**autofill_download.cc (right): > > https://chromiumcodereview.**appspot.com/11218002/diff/** > 2001/chrome/browser/autofill/**autofill_download.cc#**newcode180<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.cc#newcode180> > chrome/browser/autofill/**autofill_download.cc:180: } > Rather than adding a new command-line switch and saving to a file, how > about just using VLOGs, which can be redirected to a file if need be? > Doing the output with a VLOG would mean other log entries could contaminate the output and anyone using this to construct mappings would have to manually slice and dice the log output into something usable on the server. We could insert sentinels of some sort into the log output then write a script to post process it into something useful. That seems heavier than this though. Thoughts? > > https://chromiumcodereview.**appspot.com/11218002/diff/** > 2001/chrome/browser/autofill/**autofill_download.h<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.h> > File chrome/browser/autofill/**autofill_download.h (right): > > https://chromiumcodereview.**appspot.com/11218002/diff/** > 2001/chrome/browser/autofill/**autofill_download.h#newcode106<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.h#newcode106> > chrome/browser/autofill/**autofill_download.h:106: const std::string& > form_xml); > nit: Can these be tucked into the anonymous namespace in the > implementation file? If so, that's generally preferred to private > static member functions. > > Sure. Makes sense. > https://chromiumcodereview.**appspot.com/11218002/<https://chromiumcodereview... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
On 2012/10/17 23:47:29, Albert Bodenhamer wrote: > On Wed, Oct 17, 2012 at 4:37 PM, <mailto:isherman@chromium.org> wrote: > > > I think we can make this even simpler -- see inline. WDYT? > > > > > > https://chromiumcodereview.**appspot.com/11218002/diff/** > > > 2001/chrome/browser/autofill/**autofill_download.cc<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.cc> > > File chrome/browser/autofill/**autofill_download.cc (right): > > > > https://chromiumcodereview.**appspot.com/11218002/diff/** > > > 2001/chrome/browser/autofill/**autofill_download.cc#**newcode180<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.cc#newcode180> > > chrome/browser/autofill/**autofill_download.cc:180: } > > Rather than adding a new command-line switch and saving to a file, how > > about just using VLOGs, which can be redirected to a file if need be? > > > > Doing the output with a VLOG would mean other log entries could contaminate > the output and anyone using this to construct mappings would have to > manually slice and dice the log output into something usable on the server. > > We could insert sentinels of some sort into the log output then write a > script to post process it into something useful. That seems heavier than > this though. > > Thoughts? VLOG actually allows you to restrict output to a specific file (using --vmodule), so I don't think noise from other random logs should be much of an issue. Does that address your concern, or are you concerned about unrelated logging from this same file as well?
On Wed, Oct 17, 2012 at 4:54 PM, <isherman@chromium.org> wrote: > On 2012/10/17 23:47:29, Albert Bodenhamer wrote: > > On Wed, Oct 17, 2012 at 4:37 PM, <mailto:isherman@chromium.org> wrote: >> > > > I think we can make this even simpler -- see inline. WDYT? >> > >> > >> > https://chromiumcodereview.**a**ppspot.com/11218002/diff/**<http://appspot.co... >> > >> > > 2001/chrome/browser/autofill/****autofill_download.cc<https://** > chromiumcodereview.appspot.**com/11218002/diff/2001/chrome/** > browser/autofill/autofill_**download.cc<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.cc> > > > >> > File chrome/browser/autofill/****autofill_download.cc (right): >> > >> > https://chromiumcodereview.**a**ppspot.com/11218002/diff/**<http://appspot.co... >> > >> > > 2001/chrome/browser/autofill/****autofill_download.cc#****newcode180< > https://**chromiumcodereview.appspot.**com/11218002/diff/2001/chrome/** > browser/autofill/autofill_**download.cc#newcode180<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.cc#newcode180> > > > >> > chrome/browser/autofill/****autofill_download.cc:180: } >> >> > Rather than adding a new command-line switch and saving to a file, how >> > about just using VLOGs, which can be redirected to a file if need be? >> > >> > > Doing the output with a VLOG would mean other log entries could >> contaminate >> the output and anyone using this to construct mappings would have to >> manually slice and dice the log output into something usable on the >> server. >> > > We could insert sentinels of some sort into the log output then write a >> script to post process it into something useful. That seems heavier than >> this though. >> > > Thoughts? >> > > VLOG actually allows you to restrict output to a specific file (using > --vmodule), so I don't think noise from other random logs should be much > of an > issue. Does that address your concern, or are you concerned about > unrelated > logging from this same file as well? > Good point. I hadn't seen --vmodule. The output would still be concatenated and we'd still need to filter out timestamp, line#, etc. but that should be manageable. I'll play with it a bit. > > https://chromiumcodereview.**appspot.com/11218002/<https://chromiumcodereview... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
I went with the logging approach you suggested. PTAL. On Wed, Oct 17, 2012 at 5:09 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > On Wed, Oct 17, 2012 at 4:54 PM, <isherman@chromium.org> wrote: > >> On 2012/10/17 23:47:29, Albert Bodenhamer wrote: >> >> On Wed, Oct 17, 2012 at 4:37 PM, <mailto:isherman@chromium.org> wrote: >>> >> >> > I think we can make this even simpler -- see inline. WDYT? >>> > >>> > >>> > https://chromiumcodereview.**a**ppspot.com/11218002/diff/**<http://appspot.co... >>> > >>> >> >> 2001/chrome/browser/autofill/****autofill_download.cc<https://** >> chromiumcodereview.appspot.**com/11218002/diff/2001/chrome/** >> browser/autofill/autofill_**download.cc<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.cc> >> > >> >>> > File chrome/browser/autofill/****autofill_download.cc (right): >>> > >>> > https://chromiumcodereview.**a**ppspot.com/11218002/diff/**<http://appspot.co... >>> > >>> >> >> 2001/chrome/browser/autofill/****autofill_download.cc#****newcode180< >> https://**chromiumcodereview.appspot.**com/11218002/diff/2001/chrome/** >> browser/autofill/autofill_**download.cc#newcode180<https://chromiumcodereview.appspot.com/11218002/diff/2001/chrome/browser/autofill/autofill_download.cc#newcode180> >> > >> >>> > chrome/browser/autofill/****autofill_download.cc:180: } >>> >>> > Rather than adding a new command-line switch and saving to a file, how >>> > about just using VLOGs, which can be redirected to a file if need be? >>> > >>> >> >> Doing the output with a VLOG would mean other log entries could >>> contaminate >>> the output and anyone using this to construct mappings would have to >>> manually slice and dice the log output into something usable on the >>> server. >>> >> >> We could insert sentinels of some sort into the log output then write a >>> script to post process it into something useful. That seems heavier than >>> this though. >>> >> >> Thoughts? >>> >> >> VLOG actually allows you to restrict output to a specific file (using >> --vmodule), so I don't think noise from other random logs should be much >> of an >> issue. Does that address your concern, or are you concerned about >> unrelated >> logging from this same file as well? >> > > Good point. I hadn't seen --vmodule. The output would still be > concatenated and we'd still need to filter out timestamp, line#, etc. but > that should be manageable. I'll play with it a bit. > > > >> >> https://chromiumcodereview.**appspot.com/11218002/<https://chromiumcodereview... >> > > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> > org > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
LGTM, thanks :) https://chromiumcodereview.appspot.com/11218002/diff/6002/chrome/browser/auto... File chrome/browser/autofill/autofill_download.cc (right): https://chromiumcodereview.appspot.com/11218002/diff/6002/chrome/browser/auto... chrome/browser/autofill/autofill_download.cc:46: const std::string& form_xml) { nit: Please either wrap all the function params to be indented four spaces, or indent this to line up with the opening paren.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abodenha@chromium.org/11218002/15001
Sorry for I got bad news for ya. Compile failed with a clobber build. Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abodenha@chromium.org/11218002/15001
Sorry for I got bad news for ya. Compile failed with a clobber build. Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abodenha@chromium.org/11218002/15001
Change committed as 163130 |
