Make sure we don't "hang" when the bookmarks extension API does not work. Als...

Make sure we don't "hang" when the bookmarks extension API does not work. Also, make sure we don't "hang" when the chrome url override failed.

BUG=43448
TEST=Manually


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

[Erik does not do reviews, 2010-05-13 19:31:57]

http://codereview.chromium.org/2108001/diff/1/3
File chrome/browser/dom_ui/bookmarks_ui.cc (left):

http://codereview.chromium.org/2108001/diff/1/3#oldcode32
chrome/browser/dom_ui/bookmarks_ui.cc:32: NOTREACHED() << "We should never get
here since the extension should have"
It seems like we should leave the NOTREACHED in here, since this is still a bug,
right?  Do we have any idea of how this is happening?  Maybe we should add a
histogram somewhere to measure this.

http://codereview.chromium.org/2108001/diff/1/4
File chrome/browser/resources/bookmarks.html (right):

http://codereview.chromium.org/2108001/diff/1/4#newcode2
chrome/browser/resources/bookmarks.html:2: <!-- chrome://bookmarks/ placeholder
-->
Maybe we should have some error text?

[arv (Not doing code reviews), 2010-05-13 21:05:02]

http://codereview.chromium.org/2108001/diff/1/3
File chrome/browser/dom_ui/bookmarks_ui.cc (left):

http://codereview.chromium.org/2108001/diff/1/3#oldcode32
chrome/browser/dom_ui/bookmarks_ui.cc:32: NOTREACHED() << "We should never get
here since the extension should have"
On 2010/05/13 19:31:57, Erik Kay wrote:
> It seems like we should leave the NOTREACHED in here, since this is still a
bug,
> right?  Do we have any idea of how this is happening?  Maybe we should add a
> histogram somewhere to measure this.

I'm putting back the NOTREACHED

http://codereview.chromium.org/2108001/diff/1/4
File chrome/browser/resources/bookmarks.html (right):

http://codereview.chromium.org/2108001/diff/1/4#newcode2
chrome/browser/resources/bookmarks.html:2: <!-- chrome://bookmarks/ placeholder
-->
On 2010/05/13 19:31:57, Erik Kay wrote:
> Maybe we should have some error text?

Yeah, I'm not sure what to show here though. I'll look into reusing the "This
webpage is not available." error page.

[arv (Not doing code reviews), 2010-05-13 23:45:01]

This patch fixes a bug where we passed in the wrong error code. Now, when we
SendResponse(requires_id, NULL) we get the default error page :-)

Did you still want me to add the histogram?

[Erik does not do reviews, 2010-05-14 00:31:27]

On Thu, May 13, 2010 at 4:45 PM, <arv@chromium.org> wrote:

> This patch fixes a bug where we passed in the wrong error code. Now, when
> we
> SendResponse(requires_id, NULL) we get the default error page :-)
>
> Did you still want me to add the histogram?


In that case, it seems unnecessary.

What about the !chrome.bookmarks case?  Do we know why that happens?

Erik

[eroman, 2010-05-14 01:11:38]

lgtm

[arv (Not doing code reviews), 2010-05-14 01:19:21]

On Thu, May 13, 2010 at 17:31, Erik Kay <erikkay@chromium.org> wrote:
> What about the !chrome.bookmarks case?  Do we know why that happens?

This CL is a stop gap so that we can see which case people are hitting.

I'm planning to look at the issue regarding the failing extensions API
as well but I'd rather get this in first.

-- 
erik

[Erik does not do reviews, 2010-05-14 04:19:15]

lgtm

On Thu, May 13, 2010 at 6:18 PM, Erik Arvidsson <arv@chromium.org> wrote:

> On Thu, May 13, 2010 at 17:31, Erik Kay <erikkay@chromium.org> wrote:
> > What about the !chrome.bookmarks case?  Do we know why that happens?
>
> This CL is a stop gap so that we can see which case people are hitting.
>
> I'm planning to look at the issue regarding the failing extensions API
> as well but I'd rather get this in first.
>
> --
> erik
>