In chrome://crashes, re-read the crash list on page reload

In chrome://crashes, re-read the crash list on page reload

BUG=210624
TEST=built for lumpy, add new entry to uploads.log, hit f5, see entry show up

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

[achaulk, 2013-08-16 18:47:17]

I'm not sure if there's a race or not with multiple async loads happening at the
same time. I wasn't able to trigger anything by spamming f5 anyway

[Lei Zhang, 2013-08-20 07:01:47]

I defer to Stuart.

[achaulk, 2013-08-26 16:36:38]

ping

[achaulk, 2013-09-03 18:24:41]

ping

[stuartmorgan, 2013-09-04 23:40:45]

Sorry for the delay. So I take it the issue here is that CrashesDOMHandler is
longer-lived than the page itself in this case? (I haven't done WebUI work in ~3
years, but that class of issues sounds familiar.)

Changing the list class itself to handle being spammed with load requests feels
kind of hacky. How about instead having the handler keep track of whether it's
waiting for a crash list load, and if so, coalescing (by just dropping the new
incoming requests). If it's not, it can reset the list object, and start over.

Also, have you checked whether the removal of the initial
LoadUploadListAsynchronously() makes the page seem jankier? You're now starting
the load later in the page lifecycle for the common case of not reloading.

[vapier, 2013-09-14 04:43:39]

i posted a slightly different variant at
https://codereview.chromium.org/23650005/ (as i didn't realize this one was
posted)

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

https://codereview.chromium.org/23020015/diff/1/chrome/browser/ui/webui/crash...
chrome/browser/ui/webui/crashes_ui.cc:91: bool list_available_;
pretty sure this is now unused

https://codereview.chromium.org/23020015/diff/1/chrome/browser/upload_list.cc
File chrome/browser/upload_list.cc (right):

https://codereview.chromium.org/23020015/diff/1/chrome/browser/upload_list.cc...
chrome/browser/upload_list.cc:57: uploads_.clear();
i'm not sure this is correct.  the upload list class is used in places other
than just the crash reporter list.

[achaulk1, 2013-09-16 18:06:20]

The issue is that a reload of the page doesn't re-read the crash list, you have
to hard close the tab and then make a new one.

Though I do like vapier's version a little more

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

https://codereview.chromium.org/23020015/diff/1/chrome/browser/ui/webui/crash...
chrome/browser/ui/webui/crashes_ui.cc:91: bool list_available_;
On 2013/09/14 04:43:39, vapier wrote:
> pretty sure this is now unused

Ah, yes it is

https://codereview.chromium.org/23020015/diff/1/chrome/browser/upload_list.cc
File chrome/browser/upload_list.cc (right):

https://codereview.chromium.org/23020015/diff/1/chrome/browser/upload_list.cc...
chrome/browser/upload_list.cc:57: uploads_.clear();
On 2013/09/14 04:43:39, vapier wrote:
> i'm not sure this is correct.  the upload list class is used in places other
> than just the crash reporter list.

It is, but they use the list in the same way that crash reporting did - load
once

[vapier, 2013-09-16 21:02:40]

https://codereview.chromium.org/23020015/diff/1/chrome/browser/upload_list.cc
File chrome/browser/upload_list.cc (right):

https://codereview.chromium.org/23020015/diff/1/chrome/browser/upload_list.cc...
chrome/browser/upload_list.cc:57: uploads_.clear();
if people are happy with changing the upload list for everyone, then that works
for me too.  i guess the existing code doesn't make much sense unless the source
list itself is fully cleared and only new entries get added.

it would make the calling code simpler imo ... otherwise you have to delete &
create a new object every time, or add a new func to the upload_list class that
lets you clear existing entries.

[achaulk1, 2013-09-16 22:37:43]

Changed to keep the initial async load, if the file load beats the JS request,
the UI gets updated from HandleRequestCrashes, otherwise, and for every
subsequent attempt it's done from OnUploadListAvailable

[vapier, 2013-09-16 22:50:06]

lgtm

[achaulk1, 2013-09-17 16:45:16]

Any of the Chrome people want to comment?

[Lei Zhang, 2013-09-17 20:33:15]

https://codereview.chromium.org/23020015/diff/13001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/23020015/diff/13001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/crashes_ui.cc:113: void
CrashesDOMHandler::HandleRequestCrashes(const ListValue* args) {
I think this should be:

  if (first_load_) {
    first_load_ = false;
    if (list_available_) {
      UpdateUI();
    }
  } else {
    list_available_ = false;
    upload_list_->LoadUploadListAsynchronously();
  }

As is, if the load from RegisterMessages() happens too slowly, the else case
below will be triggered and another load request will be sent.

[achaulk1, 2013-09-17 21:21:51]

https://codereview.chromium.org/23020015/diff/13001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/crashes_ui.cc (right):

https://codereview.chromium.org/23020015/diff/13001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/crashes_ui.cc:113: void
CrashesDOMHandler::HandleRequestCrashes(const ListValue* args) {
On 2013/09/17 20:33:15, Lei Zhang wrote:
> I think this should be:
> 
>   if (first_load_) {
>     first_load_ = false;
>     if (list_available_) {
>       UpdateUI();
>     }
>   } else {
>     list_available_ = false;
>     upload_list_->LoadUploadListAsynchronously();
>   }
> 
> As is, if the load from RegisterMessages() happens too slowly, the else case
> below will be triggered and another load request will be sent.

Ah, you're right it would do that.

[Lei Zhang, 2013-09-17 21:51:44]

lgtm

[commit-bot: I haz the power, 2013-09-17 22:05:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achaulk@chromium.org/23020015/20001

[commit-bot: I haz the power, 2013-09-17 22:17:57]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-18 21:27:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achaulk@chromium.org/23020015/20001

[commit-bot: I haz the power, 2013-09-18 22:43:11]

Change committed as 223964