Define DATA_TYPE_SITE_DATA in terms of DATA_TYPE_DOM_STORAGE.

Define DATA_TYPE_SITE_DATA in terms of DATA_TYPE_DOM_STORAGE.

DATA_TYPE_SITE_DATA, i.e. "Cookies and site data" as presented to the
user in the Clear Browsing Data UI, is a superset of DATA_TYPE_DOM_STORAGE,
which represents the "storage" datatype in Clear-Site-Data.

Define it as such to avoid duplication.

Furthermore, this CL fixes a bug where DATA_TYPE_EMBEDDER_DOM_STORAGE,
one of the components of DATA_TYPE_DOM_STORAGE, was not a part of
DATA_TYPE_SITE_DATA. While this had no effect on DATA_TYPE_SITE_DATA,
it meant that DATA_TYPE_EMBEDDER_DOM_STORAGE was not part of
FILTERABLE_DATATYPES, making Clear-Site-Data for "storage" DCHECK().

BUG=668114
Review-Url: https://codereview.chromium.org/2910923002
Cr-Commit-Position: refs/heads/master@{#475370}
Committed: https://chromium.googlesource.com/chromium/src/+/168dc2f3cb966091dcca1abb798b5c4ebd7c5437

[msramek, 2017-05-29 07:19:14]

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

[commit-bot: I haz the power, 2017-05-29 07:19:30]

Dry run: CQ is trying da patch.

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

[msramek, 2017-05-29 07:20:55]

msramek@chromium.org changed reviewers:
+ mkwst@chromium.org

[msramek, 2017-05-29 07:20:56]

Hi Mike,

Please have a look at this one as well! I discovered this bug while testing the
WPT that I just sent you. This bug was not uncovered by browsertests in
content/, as it's in ChromeBrowsingDataRemoverDelegate, a layer above them.

Thanks,
Martin

[commit-bot: I haz the power, 2017-05-29 07:47:28]

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

[commit-bot: I haz the power, 2017-05-29 07:47:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2017-05-29 08:04:42]

The code change looks reasonable, but several `ExtensionBrowsingDataTest.*`
tests are failing.

[Mike West, 2017-05-29 08:05:57]

On 2017/05/29 at 08:04:42, Mike West wrote:
> The code change looks reasonable, but several `ExtensionBrowsingDataTest.*`
tests are failing.

Also, why don't you bring the WPT change into this CL so you can make sure
things are working on the bots before landing? We'll automatically export new
tests under `LayoutTests/external/wpt` to the upstream repository. Should be
even simpler than doing the review on GitHub.

[msramek, 2017-05-29 10:59:38]

Oh yeah, these tests always goes red when there is a change to SITE_DATA. Will
fix that.

Just wanted to respond to your suggestion in the meantime: It doesn't really
make sense to join this CL with the one that adds the WPT for cache. The bug I'm
fixing here is related to storage, not cache, and it's reproducible on the
existing WPTs for about a month. I just happened to notice it when I was adding
the new one for cache. And adding a layout test through this CL won't help prove
that it's fixed, because the bug is in the embedder, so the layout tests don't
trigger it.

But for a more general question - do you prefer that I send you new WPTs through
Blink rather than Github?

[Mike West, 2017-05-29 12:03:50]

On 2017/05/29 at 10:59:38, msramek wrote:
> Oh yeah, these tests always goes red when there is a change to SITE_DATA. Will
fix that.
> 
> Just wanted to respond to your suggestion in the meantime: It doesn't really
make sense to join this CL with the one that adds the WPT for cache. The bug I'm
fixing here is related to storage, not cache, and it's reproducible on the
existing WPTs for about a month. I just happened to notice it when I was adding
the new one for cache. And adding a layout test through this CL won't help prove
that it's fixed, because the bug is in the embedder, so the layout tests don't
trigger it.

I see. I misunderstood the note you wrote. :( In that case, I don't understand
the effect of the change. Is this just fixing an incorrect DCHECK, or is there a
behavioral change either to `Clear-Site-Data` or the clear browsing data UI?

> But for a more general question - do you prefer that I send you new WPTs
through Blink rather than Github?

Yes. Proving that we pass the tests via our bots is a hugely helpful when
reviewing. Also, GitHub's review tools are miserable. :)

[msramek, 2017-05-29 15:51:41]

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

[commit-bot: I haz the power, 2017-05-29 15:51:54]

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, 2017-05-29 16:39:02]

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

[commit-bot: I haz the power, 2017-05-29 16:39:03]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-05-29 16:46:24]

On 2017/05/29 12:03:50, Mike West wrote:
> On 2017/05/29 at 10:59:38, msramek wrote:
> > Oh yeah, these tests always goes red when there is a change to SITE_DATA.
Will
> fix that.

I fixed the tests (once and for all!) by rewriting the offended masks to be
additive rather than subtractive, so that next time an entry is added to
SITE_DATA,
it will not require updating the extension tests as well.

> > 
> > Just wanted to respond to your suggestion in the meantime: It doesn't really
> make sense to join this CL with the one that adds the WPT for cache. The bug
I'm
> fixing here is related to storage, not cache, and it's reproducible on the
> existing WPTs for about a month. I just happened to notice it when I was
adding
> the new one for cache. And adding a layout test through this CL won't help
prove
> that it's fixed, because the bug is in the embedder, so the layout tests don't
> trigger it.
> 
> I see. I misunderstood the note you wrote. :( In that case, I don't understand
> the effect of the change. Is this just fixing an incorrect DCHECK, or is there
a
> behavioral change either to `Clear-Site-Data` or the clear browsing data UI?

Exactly. No functional change to either Clear Browsing Data dialog or
Clear-Site-Data, it's just that I forgot to update FILTERABLE_DATATYPES
(or SITE_DATA) with EMBEDDER_DOM_STORAGE, and it DCHECK()ed.

> 
> > But for a more general question - do you prefer that I send you new WPTs
> through Blink rather than Github?
> 
> Yes. Proving that we pass the tests via our bots is a hugely helpful when
> reviewing. Also, GitHub's review tools are miserable. :)

Alright. I ported the CL here https://codereview.chromium.org/2913553004/, but
I have some questions I'll ask you tomorrow :)

[Mike West, 2017-05-29 18:36:15]

Ok. LGTM, thanks for the explanation.

[msramek, 2017-05-29 18:57:10]

Thanks!

[msramek, 2017-05-29 18:57:46]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2017-05-29 18:57:57]

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, 2017-05-29 19:01:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496084266563990,
"parent_rev": "b6f69d27ef4a1d09d397dab0c55e692c47bb8284", "commit_rev":
"168dc2f3cb966091dcca1abb798b5c4ebd7c5437"}

[commit-bot: I haz the power, 2017-05-29 19:01:41]

Description was changed from

==========
Define DATA_TYPE_SITE_DATA in terms of DATA_TYPE_DOM_STORAGE.

DATA_TYPE_SITE_DATA, i.e. "Cookies and site data" as presented to the
user in the Clear Browsing Data UI, is a superset of DATA_TYPE_DOM_STORAGE,
which represents the "storage" datatype in Clear-Site-Data.

Define it as such to avoid duplication.

Furthermore, this CL fixes a bug where DATA_TYPE_EMBEDDER_DOM_STORAGE,
one of the components of DATA_TYPE_DOM_STORAGE, was not a part of
DATA_TYPE_SITE_DATA. While this had no effect on DATA_TYPE_SITE_DATA,
it meant that DATA_TYPE_EMBEDDER_DOM_STORAGE was not part of
FILTERABLE_DATATYPES, making Clear-Site-Data for "storage" DCHECK().

BUG=668114
==========

to

==========
Define DATA_TYPE_SITE_DATA in terms of DATA_TYPE_DOM_STORAGE.

DATA_TYPE_SITE_DATA, i.e. "Cookies and site data" as presented to the
user in the Clear Browsing Data UI, is a superset of DATA_TYPE_DOM_STORAGE,
which represents the "storage" datatype in Clear-Site-Data.

Define it as such to avoid duplication.

Furthermore, this CL fixes a bug where DATA_TYPE_EMBEDDER_DOM_STORAGE,
one of the components of DATA_TYPE_DOM_STORAGE, was not a part of
DATA_TYPE_SITE_DATA. While this had no effect on DATA_TYPE_SITE_DATA,
it meant that DATA_TYPE_EMBEDDER_DOM_STORAGE was not part of
FILTERABLE_DATATYPES, making Clear-Site-Data for "storage" DCHECK().

BUG=668114

Review-Url: https://codereview.chromium.org/2910923002
Cr-Commit-Position: refs/heads/master@{#475370}
Committed:
https://chromium.googlesource.com/chromium/src/+/168dc2f3cb966091dcca1abb798b...
==========

[commit-bot: I haz the power, 2017-05-29 19:01:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/168dc2f3cb966091dcca1abb798b...