Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268783002:
Manual crash uploads for mac and win (Closed)
|
Created:
4 years, 4 months ago by gayane -on leave until 09-2017 Modified:
4 years, 3 months ago CC:
chromium-reviews, jam, sdefresne+watch_chromium.org, darin-cc_chromium.org, sadrul, kalyank, Ilya Sherman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionManual crash uploads for mac and win
Adding functionally for requesting upload for a crash report from
about:crashes page and propagating the request to crashpad.
BUG=620762
Committed: https://crrev.com/f132d5d5801871170f5d87aabb9bad3339c9c022
Cr-Commit-Position: refs/heads/master@{#416945}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address Jesse's comments #
Total comments: 4
Patch Set 3 : policy restrictions + show crashes when crash uploads disabled #
Total comments: 18
Patch Set 4 : Mark's comments #Patch Set 5 : fixes for windows #
Total comments: 12
Patch Set 6 : fix comments and formatting #
Total comments: 4
Patch Set 7 : fixing comments #
Total comments: 12
Patch Set 8 : dbeam's comments fixed #
Total comments: 4
Patch Set 9 : fix js formatting #Messages
Total messages: 50 (19 generated)
Description was changed from ========== Manual uploads for mac and win BUG= ========== to ========== Manual uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 ==========
gayane@chromium.org changed reviewers: + jwd@chromium.org
jwd@ please have a look on the initial CL for manual uploads from crash UI. Note, windows part is not tested yet.
On 2016/08/22 21:50:08, gayane wrote: > jwd@ please have a look on the initial CL for manual uploads from crash UI. > Note, windows part is not tested yet. nit: can you change the title to mention it's crash uploads.
https://codereview.chromium.org/2268783002/diff/1/chrome/browser/crash_upload... File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/1/chrome/browser/crash_upload... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:49: GetProcAddress(elf_module, "RequestSingleCrashUploadImpl")); Can you pull the string out into a constant in the anonymous namespace, and add some comment to it, similar to https://cs.chromium.org/chromium/src/chrome/browser/metrics/chrome_metrics_se... https://codereview.chromium.org/2268783002/diff/1/chrome/browser/crash_upload... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:114: // exe. I don't think this is a good way of explaining it. At least, I find it confusing. I understood it that it lives in the chrome_elf.dll, not chrome.exe, but I don't actually have a good understanding. I think it's probably sufficient to say that crash reporting is handled by chrome_elf.dll, and we can't call crash_reporter::RequestSingleCrashUpload directly. I'd not mention the pointer details here. https://codereview.chromium.org/2268783002/diff/1/components/crash/content/ap... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/1/components/crash/content/ap... components/crash/content/app/crashpad.cc:334: // See CrashUploadListCrashpad. Note that we do not pass an std::vector here, I think this note about the vector and returned pointer aren't relavent for this function. https://codereview.chromium.org/2268783002/diff/1/components/crash_strings.grdp File components/crash_strings.grdp (right): https://codereview.chromium.org/2268783002/diff/1/components/crash_strings.gr... components/crash_strings.grdp:40: <message name="IDS_CRASH_UPLOAD_NOW_LINK_TEXT" desc="Link text for manual uploads of crash reports"> nit: just one report, right? https://codereview.chromium.org/2268783002/diff/1/components/upload_list/uplo... File components/upload_list/upload_list.cc (right): https://codereview.chromium.org/2268783002/diff/1/components/upload_list/uplo... components/upload_list/upload_list.cc:147: // Manual uploads for not uploaded crash reports are not availble for non availble -> available https://codereview.chromium.org/2268783002/diff/1/components/upload_list/uplo... components/upload_list/upload_list.cc:149: NOTREACHED(); I wonder if this function should be defined here at all. You could make RequestSingleCrashUploadAsync virtual with the NOTREACHED, and just implement it properly in the crashpad version, and have a RequestSingleCrashUpload defined there only, no?
Description was changed from ========== Manual uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 ========== to ========== Manual crash uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 ==========
thanks for having a look https://codereview.chromium.org/2268783002/diff/1/chrome/browser/crash_upload... File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/1/chrome/browser/crash_upload... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:49: GetProcAddress(elf_module, "RequestSingleCrashUploadImpl")); On 2016/08/23 19:14:30, jwd wrote: > Can you pull the string out into a constant in the anonymous namespace, and add > some comment to it, similar to > https://cs.chromium.org/chromium/src/chrome/browser/metrics/chrome_metrics_se... > Done. https://codereview.chromium.org/2268783002/diff/1/chrome/browser/crash_upload... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:114: // exe. On 2016/08/23 19:14:30, jwd wrote: > I don't think this is a good way of explaining it. At least, I find it > confusing. I understood it that it lives in the chrome_elf.dll, not chrome.exe, > but I don't actually have a good understanding. > > I think it's probably sufficient to say that crash reporting is handled by > chrome_elf.dll, and we can't call crash_reporter::RequestSingleCrashUpload > directly. > > I'd not mention the pointer details here. Done. https://codereview.chromium.org/2268783002/diff/1/components/crash/content/ap... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/1/components/crash/content/ap... components/crash/content/app/crashpad.cc:334: // See CrashUploadListCrashpad. Note that we do not pass an std::vector here, On 2016/08/23 19:14:30, jwd wrote: > I think this note about the vector and returned pointer aren't relavent for this > function. Opps. copy paste. https://codereview.chromium.org/2268783002/diff/1/components/crash_strings.grdp File components/crash_strings.grdp (right): https://codereview.chromium.org/2268783002/diff/1/components/crash_strings.gr... components/crash_strings.grdp:40: <message name="IDS_CRASH_UPLOAD_NOW_LINK_TEXT" desc="Link text for manual uploads of crash reports"> On 2016/08/23 19:14:30, jwd wrote: > nit: just one report, right? Done. https://codereview.chromium.org/2268783002/diff/1/components/upload_list/uplo... File components/upload_list/upload_list.cc (right): https://codereview.chromium.org/2268783002/diff/1/components/upload_list/uplo... components/upload_list/upload_list.cc:147: // Manual uploads for not uploaded crash reports are not availble for non On 2016/08/23 19:14:30, jwd wrote: > availble -> available Done. https://codereview.chromium.org/2268783002/diff/1/components/upload_list/uplo... components/upload_list/upload_list.cc:149: NOTREACHED(); On 2016/08/23 19:14:30, jwd wrote: > I wonder if this function should be defined here at all. You could make > RequestSingleCrashUploadAsync virtual with the NOTREACHED, and just implement it > properly in the crashpad version, and have a RequestSingleCrashUpload defined > there only, no? initially i did it like you suggested, but then I thought that if we are going to add Android part then we will just need to implementRequestSingleCrashUpload in here and Async will be common for all the platforms.
https://codereview.chromium.org/2268783002/diff/1/components/upload_list/uplo... File components/upload_list/upload_list.cc (right): https://codereview.chromium.org/2268783002/diff/1/components/upload_list/uplo... components/upload_list/upload_list.cc:149: NOTREACHED(); On 2016/08/23 20:57:45, gayane wrote: > On 2016/08/23 19:14:30, jwd wrote: > > I wonder if this function should be defined here at all. You could make > > RequestSingleCrashUploadAsync virtual with the NOTREACHED, and just implement > it > > properly in the crashpad version, and have a RequestSingleCrashUpload defined > > there only, no? > > initially i did it like you suggested, but then I thought that if we are going > to add Android part then we will just need to implementRequestSingleCrashUpload > in here and Async will be common for all the platforms. Ok, can you add a comment that this is here to support android later? https://codereview.chromium.org/2268783002/diff/20001/chrome/browser/crash_up... File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/20001/chrome/browser/crash_up... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:19: // chrome_elf dll. Can you mention where the function is located too please. https://codereview.chromium.org/2268783002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/20001/components/crash/conten... components/crash/content/app/crashpad.cc:333: // This helper is invoked by code in chrome.dll to request single crash report nit: request a single
Addressed the comments. Also changed the code - to show crash reports when crash reporting is disabled - not allow manual uploads if crash reporting is disabled by policy https://codereview.chromium.org/2268783002/diff/20001/chrome/browser/crash_up... File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/20001/chrome/browser/crash_up... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:19: // chrome_elf dll. On 2016/08/24 15:09:20, jwd wrote: > Can you mention where the function is located too please. Done. https://codereview.chromium.org/2268783002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/20001/components/crash/conten... components/crash/content/app/crashpad.cc:333: // This helper is invoked by code in chrome.dll to request single crash report On 2016/08/24 15:09:20, jwd wrote: > nit: request a single Done.
gayane@chromium.org changed reviewers: + mark@chromium.org
mark@chromium.org: Please have a look
https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/crash_up... File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/crash_up... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:46: void RequestSingleCrashUploadThunk(const std::string& local_id) { This whole function should probably just follow the model of GetReportsThunk() above. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/crash_up... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:51: static RequestSingleCrashUploadPointer request_single_crash_upload = This type wasn’t ever declared. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/crash_up... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:53: GetProcAddress(elf_module, kRequestSingleCrashUploadFunc); No reason for this to be a constant as opposed to just writing it inline here. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:176: bool support_manual_uploads = false; Put “const bool support_manual_uploads = …” in both branches of the #if that follows, so that it’s impossible to accidentally change the value. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:180: // Allow manual uploads only if crash uploads are not disabled by policy. s/Windows and Mac/Crashpad-using/ https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:182: crash_reporting_enabled || IsMetricsReportingPolicyManaged(); We want to support manual uploads if the user has disabled crash reporting too. That’s a major reason to do this, in fact. But we don’t want to support it if policy says “no”. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:212: void CrashesDOMHandler::HandleRequestSingleCrashUpload( I think it’s a good idea to check that policy says it’s OK to do the upload here, once C++ has control again. Otherwise, we’re trusting the (user-inspectable) JS code to maintain the correct value of support_manual_uploads. Dangerous. https://codereview.chromium.org/2268783002/diff/40001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/40001/components/crash/conten... components/crash/content/app/crashpad.cc:281: if (pending_report.upload_explicitly_requested) Seems like a good case for the ?: operator. https://codereview.chromium.org/2268783002/diff/40001/components/crash/conten... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/2268783002/diff/40001/components/crash/conten... components/crash/content/app/crashpad.h:72: enum class ReportUploadState { NotUploaded, Pending, Uploaded, UserRequested }; Pending_UserExplicitlyRequested This isn’t persisted to disk or anything, so you can put Pending_UserExplicitlyRequested next to Pending.
Thanks for the comments. Please have one more look https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/crash_up... File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/crash_up... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:46: void RequestSingleCrashUploadThunk(const std::string& local_id) { On 2016/08/25 20:25:11, Mark Mentovai wrote: > This whole function should probably just follow the model of GetReportsThunk() > above. Done. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/crash_up... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:51: static RequestSingleCrashUploadPointer request_single_crash_upload = On 2016/08/25 20:25:11, Mark Mentovai wrote: > This type wasn’t ever declared. Done. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/crash_up... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:53: GetProcAddress(elf_module, kRequestSingleCrashUploadFunc); On 2016/08/25 20:25:11, Mark Mentovai wrote: > No reason for this to be a constant as opposed to just writing it inline here. Done. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:176: bool support_manual_uploads = false; On 2016/08/25 20:25:11, Mark Mentovai wrote: > Put “const bool support_manual_uploads = …” in both branches of the #if that > follows, so that it’s impossible to accidentally change the value. Done. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:180: // Allow manual uploads only if crash uploads are not disabled by policy. On 2016/08/25 20:25:11, Mark Mentovai wrote: > s/Windows and Mac/Crashpad-using/ Done. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:182: crash_reporting_enabled || IsMetricsReportingPolicyManaged(); On 2016/08/25 20:25:11, Mark Mentovai wrote: > We want to support manual uploads if the user has disabled crash reporting too. > That’s a major reason to do this, in fact. But we don’t want to support it if > policy says “no”. Thanks. There was a logic problem here. Now manual uploads would be off only if crash reporting is disabled and policy is on. https://codereview.chromium.org/2268783002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:212: void CrashesDOMHandler::HandleRequestSingleCrashUpload( On 2016/08/25 20:25:11, Mark Mentovai wrote: > I think it’s a good idea to check that policy says it’s OK to do the upload > here, once C++ has control again. Otherwise, we’re trusting the > (user-inspectable) JS code to maintain the correct value of > support_manual_uploads. Dangerous. Done. https://codereview.chromium.org/2268783002/diff/40001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/40001/components/crash/conten... components/crash/content/app/crashpad.cc:281: if (pending_report.upload_explicitly_requested) On 2016/08/25 20:25:11, Mark Mentovai wrote: > Seems like a good case for the ?: operator. Done. https://codereview.chromium.org/2268783002/diff/40001/components/crash/conten... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/2268783002/diff/40001/components/crash/conten... components/crash/content/app/crashpad.h:72: enum class ReportUploadState { NotUploaded, Pending, Uploaded, UserRequested }; On 2016/08/25 20:25:11, Mark Mentovai wrote: > Pending_UserExplicitlyRequested > > This isn’t persisted to disk or anything, so you can put > Pending_UserExplicitlyRequested next to Pending. Done.
Description was changed from ========== Manual crash uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 ========== to ========== Manual crash uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 ==========
PTAL, I have tested on windows and fixed a few things.
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping
LGTM with nit https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:188: // Don't show crash reports list if crash reporting is disbled for platforms disbled -> disabled
LGTM https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:189: // where manual uploads are not supported. This isn’t about manual uploads so much as it’s about Crashpad. It’s just coincidence that Crashpad is the only implementation that supports manual upload. Reword this to say something like “disabled for non-Crashpad-using platforms.” https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:218: // Don't allow manual uploads only if crash uploads are not disabled by Double-negatives are hard to parse. How about “Only allow manual uploads if crash uploads aren’t disabled by policy.” ? https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:222: return; {} because the if() took up more than one line and it otherwise becomes difficult to follow all of that raggedy indentation. https://codereview.chromium.org/2268783002/diff/80001/components/crash_string... File components/crash_strings.grdp (right): https://codereview.chromium.org/2268783002/diff/80001/components/crash_string... components/crash_strings.grdp:41: Upload now I’m surprised that we call this “upload” in user-facing strings, but this isn’t the first instance in the file where we do that, so I guess it’s OK.
Thanks. Please have one more look. https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:188: // Don't show crash reports list if crash reporting is disbled for platforms On 2016/09/01 18:25:27, jwd wrote: > disbled -> disabled Done. https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:189: // where manual uploads are not supported. On 2016/09/01 21:25:19, Mark Mentovai wrote: > This isn’t about manual uploads so much as it’s about Crashpad. It’s just > coincidence that Crashpad is the only implementation that supports manual > upload. Reword this to say something like “disabled for non-Crashpad-using > platforms.” Its just we are enabling manual uploads for Android as well and that change is on its way https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:218: // Don't allow manual uploads only if crash uploads are not disabled by On 2016/09/01 21:25:19, Mark Mentovai wrote: > Double-negatives are hard to parse. How about “Only allow manual uploads if > crash uploads aren’t disabled by policy.” ? Done. https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:222: return; On 2016/09/01 21:25:19, Mark Mentovai wrote: > {} because the if() took up more than one line and it otherwise becomes > difficult to follow all of that raggedy indentation. Done. https://codereview.chromium.org/2268783002/diff/80001/components/crash_string... File components/crash_strings.grdp (right): https://codereview.chromium.org/2268783002/diff/80001/components/crash_string... components/crash_strings.grdp:41: Upload now On 2016/09/01 21:25:19, Mark Mentovai wrote: > I’m surprised that we call this “upload” in user-facing strings, but this isn’t > the first instance in the file where we do that, so I guess it’s OK. would you prefer "Send now" ?
gayane@chromium.org changed reviewers: + pkl@chromium.org
pkl@chromium.org: Please have a look for OWNERS review for ios/chrome/browser/ui/webui/crashes_ui.cc
LGTM https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/crashes_ui.cc:189: // where manual uploads are not supported. gayane wrote: > On 2016/09/01 21:25:19, Mark Mentovai wrote: > > This isn’t about manual uploads so much as it’s about Crashpad. It’s just > > coincidence that Crashpad is the only implementation that supports manual > > upload. Reword this to say something like “disabled for non-Crashpad-using > > platforms.” > > Its just we are enabling manual uploads for Android as well and that change is > on its way OK, but this comment about platforms without manual uploads still doesn’t have anything to do with showing the list. https://codereview.chromium.org/2268783002/diff/80001/components/crash_string... File components/crash_strings.grdp (right): https://codereview.chromium.org/2268783002/diff/80001/components/crash_string... components/crash_strings.grdp:41: Upload now gayane wrote: > On 2016/09/01 21:25:19, Mark Mentovai wrote: > > I’m surprised that we call this “upload” in user-facing strings, but this > isn’t > > the first instance in the file where we do that, so I guess it’s OK. > > would you prefer "Send now" ? Yeah, that’s even better than what I was thinking. https://codereview.chromium.org/2268783002/diff/100001/chrome/browser/crash_u... File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/100001/chrome/browser/crash_u... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:112: // On Windows, crash reporting is handled by chrome_elf.dll, thats why we that's https://codereview.chromium.org/2268783002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/crashes_ui.cc:183: // can manually uploads those reports. upload (no s)
pkl@chromium.org changed reviewers: + eugenebut@chromium.org
LGTM for ios/chrome/browser/ui/webui/crashes_ui.cc but +eugenebut for real owners approval.
ios lgtm
On 2016/09/01 23:24:42, Mark Mentovai wrote: > LGTM > > https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/crashes_ui.cc (right): > > https://codereview.chromium.org/2268783002/diff/80001/chrome/browser/ui/webui... > chrome/browser/ui/webui/crashes_ui.cc:189: // where manual uploads are not > supported. > gayane wrote: > > On 2016/09/01 21:25:19, Mark Mentovai wrote: > > > This isn’t about manual uploads so much as it’s about Crashpad. It’s just > > > coincidence that Crashpad is the only implementation that supports manual > > > upload. Reword this to say something like “disabled for non-Crashpad-using > > > platforms.” > > > > Its just we are enabling manual uploads for Android as well and that change is > > on its way > > OK, but this comment about platforms without manual uploads still doesn’t have > anything to do with showing the list. I rephrased the comment a bit. Showing the list of crashes when crash reporting is disabled is connected to manual uploads in a way that, for platforms that support manual uploads now it will show a message saying crash reporting is disabled but will also show list of crashes. > https://codereview.chromium.org/2268783002/diff/80001/components/crash_string... > File components/crash_strings.grdp (right): > > https://codereview.chromium.org/2268783002/diff/80001/components/crash_string... > components/crash_strings.grdp:41: Upload now > gayane wrote: > > On 2016/09/01 21:25:19, Mark Mentovai wrote: > > > I’m surprised that we call this “upload” in user-facing strings, but this > > isn’t > > > the first instance in the file where we do that, so I guess it’s OK. > > > > would you prefer "Send now" ? > > Yeah, that’s even better than what I was thinking. Done.
https://codereview.chromium.org/2268783002/diff/100001/chrome/browser/crash_u... File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2268783002/diff/100001/chrome/browser/crash_u... chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:112: // On Windows, crash reporting is handled by chrome_elf.dll, thats why we On 2016/09/01 23:24:41, Mark Mentovai wrote: > that's Done. https://codereview.chromium.org/2268783002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/crashes_ui.cc:183: // can manually uploads those reports. On 2016/09/01 23:24:41, Mark Mentovai wrote: > upload (no s) Done.
LGTM
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, pkl@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2268783002/#ps120001 (title: "fixing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
gayane@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@chromium.org: PTAL for owners approval for chrome/browser/ui/webui/crashes_ui.cc
https://codereview.chromium.org/2268783002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/crashes_ui.cc:176: base::ListValue crash_list; bool upload_list = crash_reporting_enabled; bool supports_manual_uploads = false; #if defined(OS_WIN) || defined(OS_MACOSX) supports_manual_uploads = !IsMetricsReportingPolicyManaged(); upload_list = true; #endif if (upload_list) crash::UploadListToValue(upload_list_.get(), &crash_list); https://codereview.chromium.org/2268783002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/crashes_ui.cc:216: if (!args || !args->GetString(0, &local_id)) do you want this to trigger a NOTREACHED()/DCHECK()? https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... File components/crash/core/browser/resources/crashes.js (right): https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... components/crash/core/browser/resources/crashes.js:104: } else if (crash['state'] == 'pending_user_requested') { it's more common to use object.propName instead of object['propName'] in JS and generates less strings (I think) https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... components/crash/core/browser/resources/crashes.js:105: var user_requested = document.createElement('p'); jsVarNamesLikeThis https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... components/crash/core/browser/resources/crashes.js:130: } }; https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... components/crash/core/browser/resources/crashes.js:132: crashBlock.appendChild(uploadNowLinkBlock) );
thanks dbeam@ PTAL https://codereview.chromium.org/2268783002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/crashes_ui.cc (right): https://codereview.chromium.org/2268783002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/crashes_ui.cc:176: base::ListValue crash_list; On 2016/09/02 22:01:12, Dan Beam wrote: > bool upload_list = crash_reporting_enabled; > bool supports_manual_uploads = false; > > #if defined(OS_WIN) || defined(OS_MACOSX) > supports_manual_uploads = !IsMetricsReportingPolicyManaged(); > upload_list = true; > #endif > > if (upload_list) > crash::UploadListToValue(upload_list_.get(), &crash_list); Done. https://codereview.chromium.org/2268783002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/crashes_ui.cc:216: if (!args || !args->GetString(0, &local_id)) On 2016/09/02 22:01:12, Dan Beam wrote: > do you want this to trigger a NOTREACHED()/DCHECK()? Done. https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... File components/crash/core/browser/resources/crashes.js (right): https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... components/crash/core/browser/resources/crashes.js:104: } else if (crash['state'] == 'pending_user_requested') { On 2016/09/02 22:01:12, Dan Beam wrote: > it's more common to use object.propName instead of object['propName'] in JS and > generates less strings (I think) Done. https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... components/crash/core/browser/resources/crashes.js:105: var user_requested = document.createElement('p'); On 2016/09/02 22:01:12, Dan Beam wrote: > jsVarNamesLikeThis Done. https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... components/crash/core/browser/resources/crashes.js:130: } On 2016/09/02 22:01:12, Dan Beam wrote: > }; Done. https://codereview.chromium.org/2268783002/diff/120001/components/crash/core/... components/crash/core/browser/resources/crashes.js:132: crashBlock.appendChild(uploadNowLinkBlock) On 2016/09/02 22:01:12, Dan Beam wrote: > ); Done.
lgtm https://codereview.chromium.org/2268783002/diff/140001/components/crash/core/... File components/crash/core/browser/resources/crashes.js (right): https://codereview.chromium.org/2268783002/diff/140001/components/crash/core/... components/crash/core/browser/resources/crashes.js:105: var userRequested = document.createElement('p'); indent off https://codereview.chromium.org/2268783002/diff/140001/components/crash/core/... components/crash/core/browser/resources/crashes.js:109: } else { was this supposed to be an else if ?
More changes for crashes.js file https://codereview.chromium.org/2268783002/diff/140001/components/crash/core/... File components/crash/core/browser/resources/crashes.js (right): https://codereview.chromium.org/2268783002/diff/140001/components/crash/core/... components/crash/core/browser/resources/crashes.js:105: var userRequested = document.createElement('p'); On 2016/09/06 20:05:43, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/2268783002/diff/140001/components/crash/core/... components/crash/core/browser/resources/crashes.js:109: } else { On 2016/09/06 20:05:43, Dan Beam wrote: > was this supposed to be an else if ? Done.
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, pkl@chromium.org, eugenebut@chromium.org, jwd@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2268783002/#ps160001 (title: "fix js formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Manual crash uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 ========== to ========== Manual crash uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Manual crash uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 ========== to ========== Manual crash uploads for mac and win Adding functionally for requesting upload for a crash report from about:crashes page and propagating the request to crashpad. BUG=620762 Committed: https://crrev.com/f132d5d5801871170f5d87aabb9bad3339c9c022 Cr-Commit-Position: refs/heads/master@{#416945} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f132d5d5801871170f5d87aabb9bad3339c9c022 Cr-Commit-Position: refs/heads/master@{#416945} |
