Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9600060:
SetPrinterInfo (Closed)
|
Created:
8 years, 9 months ago by Vitaly Buka (NO REVIEWS) Modified:
8 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Scott Hess - ex-Googler, Lei Zhang Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded SetPrinterInfo to include information about printer driver. This information will be added by Chrome (in different CL) just before performing error-prone printer related operations.
BUG=108194
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127105
Patch Set 1 : #Patch Set 2 : #
Total comments: 16
Patch Set 3 : Comments #
Total comments: 2
Patch Set 4 : ClearCrashKey #
Total comments: 2
Patch Set 5 : arraysize #Patch Set 6 : #
Messages
Total messages: 33 (0 generated)
lgtm from my side Could you please add more informative description? I am not familiar with breakpad. Get somebody from breakpad OWNERS to take a look as well.
fyi When we discussed on a previous thread we'd talked about getting someone to take a good look at the privacy side of things before we collect any new info in the crash reports or UMA. I don't think there's a problem, but I want to make sure we have a definitive sign off before we submit this. I'll poke the other thread and make sure everyone's on the same page. On Wed, Mar 7, 2012 at 10:53 AM, <gene@chromium.org> wrote: > lgtm from my side > Could you please add more informative description? > I am not familiar with breakpad. Get somebody from breakpad OWNERS to take > a > look as well. > > https://chromiumcodereview.**appspot.com/9600060/<https://chromiumcodereview.... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
Done. On 2012/03/07 18:53:36, gene wrote: > lgtm from my side > Could you please add more informative description? > I am not familiar with breakpad. Get somebody from breakpad OWNERS to take a > look as well.
I removed content changes. I don't need them.
You should have thestig take a look at linux changes, and shess for the mac changes. the windows changes LGTM with the comment. http://codereview.chromium.org/9600060/diff/43001/chrome/app/breakpad_win.cc File chrome/app/breakpad_win.cc (right): http://codereview.chromium.org/9600060/diff/43001/chrome/app/breakpad_win.cc#... chrome/app/breakpad_win.cc:282: base::StringPrintf(L"prn-info-%i", i).c_str(), L"")); size_t is an unsigned type, i don't think it is correct to use the %i format string. Would be best to just make "i" an int counter. Also note that for consistency with the other entries you should use i+1. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... File chrome/common/child_process_logging.h (right): http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging.h:90: // separated by ';' up to kMaxReportedPrinterRecords strings. There is also a limit on the length of these strings (for breakpad windows it is like 63) http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... File chrome/common/child_process_logging_posix.cc (right): http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_posix.cc:107: void SetPrinterInfo(const char* printer_info) { This is using a different format than the mac/windows one. Why? Will be easier for searching in dremel if they use the same format.
http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... File chrome/common/child_process_logging.h (right): http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging.h:51: extern char g_prn_info[]; nit: alphabetical order. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... File chrome/common/child_process_logging_posix.cc (right): http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_posix.cc:38: char g_printer_info[256] = ""; This is unused. You need to put this into the crash report in breakpad_linux.cc.
Generally LGTM, with minor OSX suggestions. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... File chrome/common/child_process_logging_mac.mm (right): http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_mac.mm:171: DCHECK(info.size() <= kMaxReportedPrinterRecords); DCHECK_LE(). http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_mac.mm:172: for (size_t i = 0; i < std::min(kMaxReportedPrinterRecords, info.size()); Since your vector is local, rather than using std::min() maybe you could just remove the excess elements? Actually, another point might obviate that suggestion: Could this be called multiple times? If so, you should clear the blank elements in case they were previously set, so maybe the loop could just process through kMaxReportedPrinterRecords and make the set/clear decision inside the loop. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_mac.mm:184: SetCrashKeyValue(key, value); If you're going to change this line, then pull this entire body into SetNumberOfViews() and remove the Impl(). The Impl() pattern is an historical artifact in the OSX version of this code.
http://codereview.chromium.org/9600060/diff/43001/chrome/app/breakpad_win.cc File chrome/app/breakpad_win.cc (right): http://codereview.chromium.org/9600060/diff/43001/chrome/app/breakpad_win.cc#... chrome/app/breakpad_win.cc:282: base::StringPrintf(L"prn-info-%i", i).c_str(), L"")); On 2012/03/14 02:12:49, eroman wrote: > size_t is an unsigned type, i don't think it is correct to use the %i format > string. Would be best to just make "i" an int counter. > > Also note that for consistency with the other entries you should use i+1. Done. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... File chrome/common/child_process_logging.h (right): http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging.h:51: extern char g_prn_info[]; On 2012/03/14 04:20:05, Lei Zhang wrote: > nit: alphabetical order. Done. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging.h:90: // separated by ';' up to kMaxReportedPrinterRecords strings. On 2012/03/14 02:12:49, eroman wrote: > There is also a limit on the length of these strings (for breakpad windows it is > like 63) Done. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... File chrome/common/child_process_logging_mac.mm (right): http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_mac.mm:171: DCHECK(info.size() <= kMaxReportedPrinterRecords); On 2012/03/14 04:36:07, shess wrote: > DCHECK_LE(). Done. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_mac.mm:172: for (size_t i = 0; i < std::min(kMaxReportedPrinterRecords, info.size()); On 2012/03/14 04:36:07, shess wrote: > Since your vector is local, rather than using std::min() maybe you could just > remove the excess elements? > > Actually, another point might obviate that suggestion: Could this be called > multiple times? If so, you should clear the blank elements in case they were > previously set, so maybe the loop could just process through > kMaxReportedPrinterRecords and make the set/clear decision inside the loop. Done. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_mac.mm:184: SetCrashKeyValue(key, value); Sorry, it was not intended. Probably changed editing function above. On 2012/03/14 04:36:07, shess wrote: > If you're going to change this line, then pull this entire body into > SetNumberOfViews() and remove the Impl(). The Impl() pattern is an historical > artifact in the OSX version of this code. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... File chrome/common/child_process_logging_posix.cc (right): http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_posix.cc:38: char g_printer_info[256] = ""; On 2012/03/14 04:20:05, Lei Zhang wrote: > This is unused. You need to put this into the crash report in breakpad_linux.cc. Done. http://codereview.chromium.org/9600060/diff/43001/chrome/common/child_process... chrome/common/child_process_logging_posix.cc:107: void SetPrinterInfo(const char* printer_info) { Actually it's like URL and extensions here. It's divided on Win and MAC and as single string here. On 2012/03/14 02:12:49, eroman wrote: > This is using a different format than the mac/windows one. Why? > > Will be easier for searching in dremel if they use the same format.
LGTM OSX, minor ask. http://codereview.chromium.org/9600060/diff/43006/chrome/common/child_process... File chrome/common/child_process_logging_mac.mm (right): http://codereview.chromium.org/9600060/diff/43006/chrome/common/child_process... chrome/common/child_process_logging_mac.mm:175: SetCrashKeyValue(key, value); I _think_ this'll leave you with all N keys, every time, but many empty. I would prefer actually clearing the unnecessary ones, so that they don't bloat up the various data sets when people are trying to see what's going on. If empty strings are not expected in the input, I'd be fine with just testing [value length] and calling set or clear as appropriate.
http://codereview.chromium.org/9600060/diff/43006/chrome/common/child_process... File chrome/common/child_process_logging_mac.mm (right): http://codereview.chromium.org/9600060/diff/43006/chrome/common/child_process... chrome/common/child_process_logging_mac.mm:175: SetCrashKeyValue(key, value); On 2012/03/14 21:26:37, shess wrote: > I _think_ this'll leave you with all N keys, every time, but many empty. I > would prefer actually clearing the unnecessary ones, so that they don't bloat up > the various data sets when people are trying to see what's going on. If empty > strings are not expected in the input, I'd be fine with just testing [value > length] and calling set or clear as appropriate. Done.
LGTM with one small issue. http://codereview.chromium.org/9600060/diff/43013/chrome/common/child_process... File chrome/common/child_process_logging_posix.cc (right): http://codereview.chromium.org/9600060/diff/43013/chrome/common/child_process... chrome/common/child_process_logging_posix.cc:120: kPrinterInfoStrLen * kMaxReportedPrinterRecords); Isn't this the same as "arraysize(g_printer_info) - 1" ? I'd like this length to be referred to consistently with the next line.
http://codereview.chromium.org/9600060/diff/43013/chrome/common/child_process... File chrome/common/child_process_logging_posix.cc (right): http://codereview.chromium.org/9600060/diff/43013/chrome/common/child_process... chrome/common/child_process_logging_posix.cc:120: kPrinterInfoStrLen * kMaxReportedPrinterRecords); On 2012/03/14 21:52:30, Lei Zhang wrote: > Isn't this the same as "arraysize(g_printer_info) - 1" ? I'd like this length to > be referred to consistently with the next line. Done.
LGTM for OSX.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/9600060/39012
Try job failure for 9600060-39012 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/9600060/39012
Change committed as 126866
Reverted in http://codereview.chromium.org/9600060 Since the sheriff didn't add notes, I went back in the IRC logs; this was reverted due to breaking the official build.
Sorry, revert was http://src.chromium.org/viewvc/chrome?view=rev&revision=126878 On 2012/03/15 16:18:14, Scott Byer wrote: > Reverted in http://codereview.chromium.org/9600060 > > Since the sheriff didn't add notes, I went back in the IRC logs; this was > reverted due to breaking the official build.
Thanks, Scott. Sheriff sent me personal email. I guess I have fixed issue. Is any way to try on official build bot?
Not that I know of. :-( On 2012/03/15 16:30:39, Vitaly Buka wrote: > Thanks, Scott. > Sheriff sent me personal email. I guess I have fixed issue. > Is any way to try on official build bot?
There should be a try bot config that matches the build bot that broke. You'll need to run a try that specifies that config manually. See http://www.chromium.org/developers/testing/try-server-usage On Thu, Mar 15, 2012 at 9:30 AM, <vitalybuka@chromium.org> wrote: > Thanks, Scott. > Sheriff sent me personal email. I guess I have fixed issue. > Is any way to try on official build bot? > > http://codereview.chromium.**org/9600060/<http://codereview.chromium.org/9600... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
Looks like it's only public. I see no reference to official.
Yeah. I just chatted with Scott about it. There's no try for the official builder. On Thu, Mar 15, 2012 at 10:11 AM, <vitalybuka@chromium.org> wrote: > Looks like it's only public. I see no reference to official. > > http://codereview.chromium.**org/9600060/<http://codereview.chromium.org/9600... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
Just synced official code on my ubuntu and going to check there.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/9600060/38009
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/9600060/43029
Commit queue failed due to new patchset.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/9600060/44013
Change committed as 127105 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
