Make quota requests from background pages not crash

Make quota requests from background pages not crash


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

[tzik, 2012-09-18 04:59:18]

We need NULL check for tab_contents here, since browser action popup doesn't
have it.
Could you take a look?

[kinuko, 2012-09-18 05:03:35]

lgtm

http://codereview.chromium.org/10948002/diff/1/chrome/browser/chrome_quota_pe...
File chrome/browser/chrome_quota_permission_context.cc (right):

http://codereview.chromium.org/10948002/diff/1/chrome/browser/chrome_quota_pe...
chrome/browser/chrome_quota_permission_context.cc:142: tab_contents ?
tab_contents->infobar_tab_helper() : NULL;
nit: indent

[Nico, 2012-09-18 05:13:53]

test?

[tzik, 2012-09-18 05:33:39]

On 2012/09/18 05:13:53, Nico wrote:
> test?

OK, I'll add one.

[tzik, 2012-09-19 12:08:54]

I added a regression test. Could you take a look again?
For comparison, test only version of the CL is available at
http://codereview.chromium.org/10943026/
Crash on QuotaPermissionBrowserTest should be fixed after this CL.

http://codereview.chromium.org/10948002/diff/1/chrome/browser/chrome_quota_pe...
File chrome/browser/chrome_quota_permission_context.cc (right):

http://codereview.chromium.org/10948002/diff/1/chrome/browser/chrome_quota_pe...
chrome/browser/chrome_quota_permission_context.cc:142: tab_contents ?
tab_contents->infobar_tab_helper() : NULL;
On 2012/09/18 05:03:35, kinuko wrote:
> nit: indent

Thanks. I fixed it and my .emacs.

[kinuko, 2012-09-20 04:43:57]

http://codereview.chromium.org/10948002/diff/6003/chrome/browser/chrome_quota...
File chrome/browser/chrome_quota_permission_context_browsertest.cc (right):

http://codereview.chromium.org/10948002/diff/6003/chrome/browser/chrome_quota...
chrome/browser/chrome_quota_permission_context_browsertest.cc:13: #include
"chrome/test/base/ui_test_utils.h"
this looks a bit too many includes for this small test to me.

http://codereview.chromium.org/10948002/diff/6003/chrome/browser/chrome_quota...
chrome/browser/chrome_quota_permission_context_browsertest.cc:15: using
extensions::Extension;
do we need this?

[tzik, 2012-09-20 05:18:37]

http://codereview.chromium.org/10948002/diff/6003/chrome/browser/chrome_quota...
File chrome/browser/chrome_quota_permission_context_browsertest.cc (right):

http://codereview.chromium.org/10948002/diff/6003/chrome/browser/chrome_quota...
chrome/browser/chrome_quota_permission_context_browsertest.cc:13: #include
"chrome/test/base/ui_test_utils.h"
On 2012/09/20 04:43:57, kinuko wrote:
> this looks a bit too many includes for this small test to me.

Ah, I forgot to drop that. Done.

http://codereview.chromium.org/10948002/diff/6003/chrome/browser/chrome_quota...
chrome/browser/chrome_quota_permission_context_browsertest.cc:15: using
extensions::Extension;
On 2012/09/20 04:43:57, kinuko wrote:
> do we need this?

Done. Removed.

[tzik, 2012-09-21 04:58:46]

Could you take another look, Nico?

[Nico, 2012-09-21 05:10:21]

I'd change the description to be more descriptive about what it does ("Make
quota requests from background pages not crash"), other than that LGTM. Thanks
for adding a test!

[tzik, 2012-09-21 05:21:54]

On 2012/09/21 05:10:21, Nico wrote:
> I'd change the description to be more descriptive about what it does ("Make
> quota requests from background pages not crash"), other than that LGTM. Thanks
> for adding a test!

Thanks! Done.

[commit-bot: I haz the power, 2012-09-21 05:22:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10948002/15002

[commit-bot: I haz the power, 2012-09-21 09:16:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10948002/15002

[commit-bot: I haz the power, 2012-09-22 08:48:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10948002/29002

[commit-bot: I haz the power, 2012-09-22 11:04:06]

Change committed as 158184