Implement chrome://crashes

Implement chrome://crashes

Creates a new internal page for listing recent reported crashes, to make it easier for users to report crashes, or get crash IDs when asked in bug reports.

Won't yet display any crashes on Windows, since the breakpad side is not yet done on Windows.

BUG=41106
TEST=Visit chrome://crashes; depending on the build type, platform, and metric pref state, it should display crashes or an informative message.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75433

[arv (Not doing code reviews), 2011-02-18 00:31:20]

I wont be able to get to this until next week.

erik








On Thu, Feb 17, 2011 at 15:16,  <stuartmorgan@chromium.org> wrote:
> Reviewers: arv,
>
> Description:
> Implement chrome://crashes
>
> Creates a new internal page for listing recent reported crashes, to make it
> easier for users to report crashes, or get crash IDs when asked in bug
> reports.
>
> Won't yet display any crashes on Windows, since the breakpad side is not yet
> done on Windows.
>
> BUG=41106
> TEST=Visit chrome://crashes; depending on the build type, platform, and
> metric
> pref state, it should display crashes or an informative message.
>
> Please review this at http://codereview.chromium.org/6545001/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/app/chromium_strings.grd
>  M chrome/app/generated_resources.grd
>  M chrome/app/google_chrome_strings.grd
>  M chrome/browser/browser_resources.grd
>  A chrome/browser/crash_upload_list.h
>  A chrome/browser/crash_upload_list.cc
>  A chrome/browser/dom_ui/crashes_ui.h
>  A chrome/browser/dom_ui/crashes_ui.cc
>  M chrome/browser/dom_ui/web_ui_factory.cc
>  A chrome/browser/resources/crashes.css
>  A chrome/browser/resources/crashes.html
>  A chrome/browser/resources/crashes.js
>  M chrome/chrome_browser.gypi
>  M chrome/common/url_constants.h
>  M chrome/common/url_constants.cc
>
>
>

[stuartmorgan, 2011-02-18 16:51:37]

Nico, any chance you could take a look today so I can land this before the next
dev channel?

(arv, I'd still appreciate it if you could take a look after that, and I can
address anything you find in a follow-up)

[Nico, 2011-02-18 18:16:01]

LG assuming that one can't use our templating framework asynchronously.

http://codereview.chromium.org/6545001/diff/6009/chrome/app/google_chrome_str...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/app/google_chrome_str...
chrome/app/google_chrome_strings.grd:518: This page only shows information on
your recent crashes if you <ph name="BEGIN_LINK">&lt;a
href="http://google.com/support/chrome/bin/answer.py?answer=96817"&gt;</ph>enable
crash reporting<ph name="END_LINK">&lt;/a&gt;</ph>.
eek &lt;a&gt; :-)

Do you use the BEGIN_LINK / END_LINK stuff anywhere? Other places use this to
create the <a>/</a> in code, but you embed it, so you might not need it.

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.cc:54: if (components.size() != 2)
If this happens in practice, add a comment about when. If not, add a NOTREACHED.
(?)

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list.h (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.h:17: struct CrashInfo {
erg would probably like it if you would have explicit out-of-line
constructor/destructors for this. You can send a tryjob to linux_clang and
search for "[chromium-style]" to see if his plugin complains about this (these
warnings will become hard errors on linux soon)

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.h:24: // Invoked when the crash list has been
loaded.
Maybe mention which thread the delegate will be called on.

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
File chrome/browser/resources/crashes.css (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
chrome/browser/resources/crashes.css:43: color: #666666;
#666

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
chrome/browser/resources/crashes.css:47: border-bottom: 1px solid #bbbbbb;
#BBB

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
File chrome/browser/resources/crashes.js (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
chrome/browser/resources/crashes.js:37: }
crashSection.innerHTML = "";

?

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
chrome/browser/resources/crashes.js:62: crashSection.appendChild(crashBlock);
Have you considered using our html templating engine to create this? See e.g.
c/b/resources/flags.html:


      <div jsdisplay="flagsExperiments.length > 0">
      <div class="experiment" jsselect="flagsExperiments">
        <table width="100%" cellpadding="2" cellspacing="0">
        <tr class="experiment-enabled">
        <td valign="top">
          <div class="experiment-text">
            <div>
              <span class="experiment-name" dir="ltr"
                    jscontent="name">NAME</span>
              <div>
                <span dir="ltr" jsvalues=".innerHTML:description">
              </div>

etc. I don't know if there's a good way to run this asynchronously – just
wondering if you've investigated this.

[stuartmorgan, 2011-02-18 19:41:21]

http://codereview.chromium.org/6545001/diff/6009/chrome/app/google_chrome_str...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/app/google_chrome_str...
chrome/app/google_chrome_strings.grd:518: This page only shows information on
your recent crashes if you <ph name="BEGIN_LINK">&lt;a
href="http://google.com/support/chrome/bin/answer.py?answer=96817"&gt;</ph>enable
crash reporting<ph name="END_LINK">&lt;/a&gt;</ph>.
Per irc, this is for the localization system. (This is the way all the other
examples of links are done in the file.)

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.cc:54: if (components.size() != 2)
On 2011/02/18 18:16:01, Nico wrote:
> If this happens in practice, add a comment about when. If not, add a
NOTREACHED.
> (?)

Done.

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list.h (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.h:17: struct CrashInfo {
On 2011/02/18 18:16:01, Nico wrote:
> erg would probably like it if you would have explicit out-of-line
> constructor/destructors for this.

Whoops, I forgot about that email. Thanks for the reminder; done.

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.h:24: // Invoked when the crash list has been
loaded.
On 2011/02/18 18:16:01, Nico wrote:
> Maybe mention which thread the delegate will be called on.

Done.

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
File chrome/browser/resources/crashes.css (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
chrome/browser/resources/crashes.css:43: color: #666666;
On 2011/02/18 18:16:01, Nico wrote:
> #666

Done.

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
chrome/browser/resources/crashes.css:47: border-bottom: 1px solid #bbbbbb;
On 2011/02/18 18:16:01, Nico wrote:
> #BBB

Done.

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
File chrome/browser/resources/crashes.js (right):

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
chrome/browser/resources/crashes.js:37: }
On 2011/02/18 18:16:01, Nico wrote:
> crashSection.innerHTML = "";
> 
> ?

Done.

http://codereview.chromium.org/6545001/diff/6009/chrome/browser/resources/cra...
chrome/browser/resources/crashes.js:62: crashSection.appendChild(crashBlock);
On 2011/02/18 18:16:01, Nico wrote:
> I don't know if there's a good way to run this asynchronously

I don't see a way to do it offhand. I can revisit later if I'm missing
something.

[Nico, 2011-02-18 19:46:10]

SLG

[arv (Not doing code reviews), 2011-02-20 23:44:00]

LGTM too. There are a few markup errors but nothing that should cause any major
breakage.

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
File chrome/browser/resources/crashes.css (right):

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
chrome/browser/resources/crashes.css:5: .hidden {
FYI, WebKit now supports the hidden boolean attribute (and DOM property) which
also does display none for you.

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
File chrome/browser/resources/crashes.html (right):

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
chrome/browser/resources/crashes.html:12: <header><h1
i18n-content="crashesTitle"></header>
Missing end tag for h1

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
chrome/browser/resources/crashes.html:16: <p id="noCrashes"
i18n-content="noCrashesMessage" class="hidden"></div>
Wrong end tag.

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
chrome/browser/resources/crashes.html:16: <p id="noCrashes"
i18n-content="noCrashesMessage" class="hidden"></div>
or with hidden attribute

<p id="noCrashes" i18n-content="noCrashesMessage" hidden></p>

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
File chrome/browser/resources/crashes.js (right):

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
chrome/browser/resources/crashes.js:25: if (enabled) {
If you used HTML5 hidden property:

$('enabledMode').hidden = !enabled;
$('disabledMode').hidden = enabled;

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
chrome/browser/resources/crashes.js:35: crashSection.innerHTML = '';
I prefer textContent = '' but no big deal.

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/resources/cra...
chrome/browser/resources/crashes.js:40: var crashBlock =
document.createElement('div');
An alternative might have been to put this as markup in the original HTML inside
a display none div. Then you could have grabbed it and cloned it and modified
the text.

In this case the dom is relatively small so it is easy enough to manage.

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/webui/crashes...
File chrome/browser/webui/crashes_ui.cc (right):

http://codereview.chromium.org/6545001/diff/3034/chrome/browser/webui/crashes...
chrome/browser/webui/crashes_ui.cc:137: 
Remove empty lines in here?