Site Settings Desktop: Fix crash when deleting storage for a site.

Site Settings Desktop: Fix crash when deleting storage for a site.

BUG=646801
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/8b38a4bfb8ceae1cd31616c146c699616fa26f94
Cr-Commit-Position: refs/heads/master@{#420133}

[Finnur, 2016-09-20 11:21:46]

Description was changed from

==========
Site Settings Desktop: Fix crash when deleting storage for a site.

BUG=646801
==========

to

==========
Site Settings Desktop: Fix crash when deleting storage for a site.

BUG=646801
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-09-20 11:29:58]

Patchset #1 (id:1) has been deleted

[Finnur, 2016-09-20 11:36:59]

Patchset #1 (id:20001) has been deleted

[Finnur, 2016-09-20 11:37:26]

finnur@chromium.org changed reviewers:
+ dschuyler@chromium.org

[Finnur, 2016-09-20 11:37:27]

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.js (right):

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:79:
this.toUrl(this.site.origin).href, this.storageType_);
I found while testing this, that if patterns are entered we need to convert them
to url for the deletion to work.

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:86: if
(event.detail.origin == this.toUrl(this.site.origin).href) {
And when the delete-complete event comes in, we need to make sure we don't
compare the origin against a pattern -- otherwise we ignore the event.

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:324:
CHECK(args->GetDouble(1, &storage_type));
This was the source of the crash. When I checked the code in 
initially, I was reading a double here but in a subsequent CL
(that converted the content settings enum to string) I accidentally 
changed this to GetString (mistakenly thinking this was a 
content settings type). But this is a storage type -- as should be 
obvious now.

[Finnur, 2016-09-20 13:00:26]

The CQ bit was checked by finnur@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 13:00:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 13:58:07]

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

[commit-bot: I haz the power, 2016-09-20 13:58:08]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-09-20 18:28:51]

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:324:
CHECK(args->GetDouble(1, &storage_type));
On 2016/09/20 11:37:27, Finnur wrote:
> This was the source of the crash. When I checked the code in 
> initially, I was reading a double here but in a subsequent CL
> (that converted the content settings enum to string) I accidentally 
> changed this to GetString (mistakenly thinking this was a 
> content settings type). But this is a storage type -- as should be 
> obvious now.

Would it be ok to GetInteger here instead of GetDouble?

[Dan Beam, 2016-09-20 19:01:59]

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:324:
CHECK(args->GetDouble(1, &storage_type));
On 2016/09/20 18:28:51, dschuyler wrote:
> On 2016/09/20 11:37:27, Finnur wrote:
> > This was the source of the crash. When I checked the code in 
> > initially, I was reading a double here but in a subsequent CL
> > (that converted the content settings enum to string) I accidentally 
> > changed this to GetString (mistakenly thinking this was a 
> > content settings type). But this is a storage type -- as should be 
> > obvious now.
> 
> Would it be ok to GetInteger here instead of GetDouble?

I don't think that will work

[dschuyler, 2016-09-20 19:17:13]

On 2016/09/20 19:01:59, Dan Beam wrote:
>
https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):
> 
>
https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/settings/site_settings_handler.cc:324:
> CHECK(args->GetDouble(1, &storage_type));
> On 2016/09/20 18:28:51, dschuyler wrote:
> > On 2016/09/20 11:37:27, Finnur wrote:
> > > This was the source of the crash. When I checked the code in 
> > > initially, I was reading a double here but in a subsequent CL
> > > (that converted the content settings enum to string) I accidentally 
> > > changed this to GetString (mistakenly thinking this was a 
> > > content settings type). But this is a storage type -- as should be 
> > > obvious now.
> > 
> > Would it be ok to GetInteger here instead of GetDouble?
> 
> I don't think that will work

Ah, and that would be because JS only uses doubles? - rather than
there being a limitation on the C++ side?

[Finnur, 2016-09-20 23:05:08]

I may be wrong, but, as I recall, GetInteger also works (and is how it was
initially before Dan pointed out that JS uses doubles). GetDouble, however,
feels more natural in light of Dan's comment.

[Dan Beam, 2016-09-20 23:06:17]

On 2016/09/20 19:17:13, dschuyler wrote:
> On 2016/09/20 19:01:59, Dan Beam wrote:
> >
>
https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2358563002/diff/40001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/settings/site_settings_handler.cc:324:
> > CHECK(args->GetDouble(1, &storage_type));
> > On 2016/09/20 18:28:51, dschuyler wrote:
> > > On 2016/09/20 11:37:27, Finnur wrote:
> > > > This was the source of the crash. When I checked the code in 
> > > > initially, I was reading a double here but in a subsequent CL
> > > > (that converted the content settings enum to string) I accidentally 
> > > > changed this to GetString (mistakenly thinking this was a 
> > > > content settings type). But this is a storage type -- as should be 
> > > > obvious now.
> > > 
> > > Would it be ok to GetInteger here instead of GetDouble?
> > 
> > I don't think that will work
> 
> Ah, and that would be because JS only uses doubles? - rather than
> there being a limitation on the C++ side?

i don't really remember the exact details, but it's mainly that's it's really a
double from JS, yes

it may only matter for larger numbers?

[Finnur, 2016-09-21 10:06:54]

Friendly ping...

[dschuyler, 2016-09-21 17:11:09]

lgtm

[Finnur, 2016-09-21 18:23:55]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-09-21 18:24:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 19:25:00]

Committed patchset #1 (id:40001)

[commit-bot: I haz the power, 2016-09-21 19:27:18]

Description was changed from

==========
Site Settings Desktop: Fix crash when deleting storage for a site.

BUG=646801
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Fix crash when deleting storage for a site.

BUG=646801
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/8b38a4bfb8ceae1cd31616c146c699616fa26f94
Cr-Commit-Position: refs/heads/master@{#420133}
==========

[commit-bot: I haz the power, 2016-09-21 19:27:19]

Patchset 1 (id:??) landed as
https://crrev.com/8b38a4bfb8ceae1cd31616c146c699616fa26f94
Cr-Commit-Position: refs/heads/master@{#420133}