chrome://crashes: add a link on CrOS for triggering crash uploading

chrome://crashes: add a link on CrOS for triggering crash uploading

The CrOS crash page is not clear in terms of when crashes will be uploaded.
Add a link so people can click to automatically trigger the system.

BUG=chromium:210624
TEST=loaded on a new system, went to chrome://crashes, clicked new link and saw system crash reporter run

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261068

[Lei Zhang, 2013-10-08 18:05:07]

https://codereview.chromium.org/26536002/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/26536002/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:6813: +        Start Uploading Crashes
Start uploading crashes ?

https://codereview.chromium.org/26536002/diff/1/chrome/browser/resources/cras...
File chrome/browser/resources/crashes.js (right):

https://codereview.chromium.org/26536002/diff/1/chrome/browser/resources/cras...
chrome/browser/resources/crashes.js:108: if (!enabled)
Don't you return no matter what?

https://codereview.chromium.org/26536002/diff/1/chrome/browser/ui/webui/crash...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/26536002/diff/1/chrome/browser/ui/webui/crash...
chrome/browser/ui/webui/crashes_ui.cc:210: return true;
remove

[vapier, 2013-10-08 18:10:15]

https://codereview.chromium.org/26536002/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/26536002/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:6813: +        Start Uploading Crashes
Done.

https://codereview.chromium.org/26536002/diff/1/chrome/browser/resources/cras...
File chrome/browser/resources/crashes.js (right):

https://codereview.chromium.org/26536002/diff/1/chrome/browser/resources/cras...
chrome/browser/resources/crashes.js:108: if (!enabled)
yes.  this makes more sense when you see a follow up CL where i add more logic
after this :).  i'll move it to that CL though.

https://codereview.chromium.org/26536002/diff/1/chrome/browser/ui/webui/crash...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/26536002/diff/1/chrome/browser/ui/webui/crash...
chrome/browser/ui/webui/crashes_ui.cc:210: return true;
oops, left debug code in

[zel, 2013-10-08 20:00:17]

...delegating my part to xiyuan@

[Lei Zhang, 2013-10-08 20:08:57]

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
File chrome/browser/resources/crashes.html (right):

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
chrome/browser/resources/crashes.html:15: <a id="uploadCrashesLink"
href="#upload_crashes"><span i18n-content="uploadCrashesLinkText"></span></a>
Probably should be a <button> with the appropriate css to make it look like a
link, rather than a bogus <a>. There's the link-button and navbar-link hiding in
some css file around here.

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
File chrome/browser/resources/crashes.js (right):

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
chrome/browser/resources/crashes.js:105: $('uploadCrashesLink').onclick =
requestCrashUpload;
Can't you just set this once and be done with it?

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/ui/webui/cr...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/ui/webui/cr...
chrome/browser/ui/webui/crashes_ui.cc:140: void
CrashesDOMHandler::HandleRequestUploads(const ListValue* args) {
Do we care if a user double/triple clicks the link and triggers this multiple
times?

[vapier, 2013-10-08 20:16:24]

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
File chrome/browser/resources/crashes.html (right):

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
chrome/browser/resources/crashes.html:15: <a id="uploadCrashesLink"
href="#upload_crashes"><span i18n-content="uploadCrashesLinkText"></span></a>
it is a valid <a>, it just links to an anchor point that isn't currently defined
;)

i'll research existing link-button code

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
File chrome/browser/resources/crashes.js (right):

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
chrome/browser/resources/crashes.js:105: $('uploadCrashesLink').onclick =
requestCrashUpload;
i can add logic to initialize only once

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/ui/webui/cr...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/ui/webui/cr...
chrome/browser/ui/webui/crashes_ui.cc:140: void
CrashesDOMHandler::HandleRequestUploads(const ListValue* args) {
nope.  the system crash sender already handles that since it has to anyways --
it could be run via a cronjob, via crosh ("upload_crashes"), or via a developer
on the command line (`crash_sender`).  this is just adding another trigger
point.

the exact status of the system crash sender is tracked only in /var/log/messages
atm.  a follow up CL is designed for exposing that state in the chrome://crashes
page (idle/running/etc...).  but i think just getting a clickable link for
people is worth doing now independently of that plumbing.

[xiyuan, 2013-10-08 20:20:10]

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
File chrome/browser/resources/crashes.js (right):

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
chrome/browser/resources/crashes.js:97: refreshCrashListId =
setInterval(requestCrashes, 5000);
Since requestCrashes clears the interval, it seems we only need a one shot
timer. Why not use window.setTimeout?

[vapier, 2014-03-25 20:49:07]

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
File chrome/browser/resources/crashes.js (right):

https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
chrome/browser/resources/crashes.js:97: refreshCrashListId =
setInterval(requestCrashes, 5000);
Done

[Lei Zhang, 2014-03-25 20:54:54]

On 2014/03/25 20:49:07, vapier wrote:
>
https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
> File chrome/browser/resources/crashes.js (right):
> 
>
https://codereview.chromium.org/26536002/diff/4001/chrome/browser/resources/c...
> chrome/browser/resources/crashes.js:97: refreshCrashListId =
> setInterval(requestCrashes, 5000);
> Done

You probably need to upload a new patch set. Patch set 2 is ~6 months old.

[vapier, 2014-03-25 21:03:27]

that's why patchset 3 is here now :)

[vapier, 2014-03-25 21:20:44]

PTAL -- patchset 5 should address everybody's feedback thus far

[xiyuan, 2014-03-25 21:30:58]

https://codereview.chromium.org/26536002/diff/95002/chrome/browser/resources/...
File chrome/browser/resources/crashes.js (right):

https://codereview.chromium.org/26536002/diff/95002/chrome/browser/resources/...
chrome/browser/resources/crashes.js:22: function updateCrashList(enabled,
dynamic_backend, crashes, version) {
dynamic_backend -> dynamicBackend

https://codereview.chromium.org/26536002/diff/95002/chrome/browser/resources/...
chrome/browser/resources/crashes.js:32: $('crashUploadStatus').hidden =
!enabled;
nit: we can get rid "if"
  $('crashUploadStatus').hidden = !dynamicBackend || !enabled;

https://codereview.chromium.org/26536002/diff/95002/chrome/browser/resources/...
chrome/browser/resources/crashes.js:102: clearInterval(refreshCrashListId);
if (refreshCrashListId)
  clearTimeout(refreshCrashListId);

[vapier, 2014-03-25 21:35:45]

https://codereview.chromium.org/26536002/diff/95002/chrome/browser/resources/...
File chrome/browser/resources/crashes.js (right):

https://codereview.chromium.org/26536002/diff/95002/chrome/browser/resources/...
chrome/browser/resources/crashes.js:22: function updateCrashList(enabled,
dynamic_backend, crashes, version) {
Done.

https://codereview.chromium.org/26536002/diff/95002/chrome/browser/resources/...
chrome/browser/resources/crashes.js:32: $('crashUploadStatus').hidden =
!enabled;
sure.  i plan on adding more UI elements, but they'll be inside this one div
element, so shouldn't need to toggle any others.

https://codereview.chromium.org/26536002/diff/95002/chrome/browser/resources/...
chrome/browser/resources/crashes.js:102: clearInterval(refreshCrashListId);
any particular reason for the if ?  when i tested things, it didn't seem to care
when you pass undefined.  i'd also point out that it ignores cases where the
timeout has already passed and so the id is no longer really valid.  didn't seem
worth it to handle these cases since the code behaves fine.

[Lei Zhang, 2014-03-25 21:50:10]

https://codereview.chromium.org/26536002/diff/55001/chromeos/dbus/fake_debug_...
File chromeos/dbus/fake_debug_daemon_client.h (right):

https://codereview.chromium.org/26536002/diff/55001/chromeos/dbus/fake_debug_...
chromeos/dbus/fake_debug_daemon_client.h:49: virtual void UploadCrashes()
OVERRIDE {}
nit: move the empty impl to the .cc file.

[vapier, 2014-03-31 20:25:22]

afaik, the latest patchset addresses all concerns raised

https://codereview.chromium.org/26536002/diff/55001/chromeos/dbus/fake_debug_...
File chromeos/dbus/fake_debug_daemon_client.h (right):

https://codereview.chromium.org/26536002/diff/55001/chromeos/dbus/fake_debug_...
chromeos/dbus/fake_debug_daemon_client.h:49: virtual void UploadCrashes()
OVERRIDE {}
Done.

[xiyuan, 2014-03-31 20:29:17]

lgtm

https://codereview.chromium.org/26536002/diff/275001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/26536002/diff/275001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/crashes_ui.cc:162: 
nit: keep only one empty line

[Lei Zhang, 2014-03-31 20:40:51]

chrome/ lgtm

[vapier, 2014-03-31 21:53:28]

https://codereview.chromium.org/26536002/diff/275001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/26536002/diff/275001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/crashes_ui.cc:162: 
Done.

[vapier, 2014-03-31 21:53:31]

The CQ bit was checked by vapier@chromium.org

[commit-bot: I haz the power, 2014-03-31 21:54:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vapier@chromium.org/26536002/295001

[commit-bot: I haz the power, 2014-03-31 22:27:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 22:27:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[vapier, 2014-04-01 22:02:45]

+derat for chromeos/ approval

[Daniel Erat, 2014-04-01 22:06:25]

lgtm for chromeos/

[vapier, 2014-04-01 22:16:50]

The CQ bit was checked by vapier@chromium.org

[commit-bot: I haz the power, 2014-04-01 22:31:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vapier@chromium.org/26536002/295001

[commit-bot: I haz the power, 2014-04-02 02:02:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vapier@chromium.org/26536002/295001

[commit-bot: I haz the power, 2014-04-02 04:21:55]

Change committed as 261068