Expose StoragePartition clear methods for Android WebView

Add StoragePartition::StorageMask for ClearData methods

Android WebView has some storage clear methods are based on different
conditions than what is exposed already in StoragePartition. But all
the underlying logic is already implemented. So just add a mask
parameter to the existing methods to for this flexibility.

BUG=

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

[boliu, 2013-02-22 03:30:18]

jam: Could you take a look at the content/ part. This adds public methods to
StoragePartition.

This will depend on https://codereview.chromium.org/12253057/ But maybe they can
be merged depending on how fast they get reviewed.

[jam, 2013-02-22 17:45:42]

https://codereview.chromium.org/12317062/diff/1/content/public/browser/storag...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/12317062/diff/1/content/public/browser/storag...
content/public/browser/storage_partition.h:30: enum StorageType;
you can't forward declare enums. this is included transitivity
(quota_manager.h->quota_client.h->quota_types.h).

https://codereview.chromium.org/12317062/diff/1/content/public/browser/storag...
content/public/browser/storage_partition.h:89: virtual void
AsyncClearAllDomStorage() = 0;
it's confusing that there are these methods + the existing
ClearData/ClearAllData.

why shouldn't one set of methods do both?

[boliu, 2013-02-22 21:55:59]

Went beyond merging the new methods by introducing a bit mask for all the
different types of storage that can be cleared to the existing methods. Also
possibly found a bug

https://codereview.chromium.org/12317062/diff/2002/content/browser/storage_pa...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/12317062/diff/2002/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:270:
GetDOMStorageContext()->DeleteLocalStorage(storage_origin);
Dom session storage is not explicitly cleared here (compare to AsyncClearALlData
below). I'm not sure if it's a bug or not. On one hand I'm guessing session
storage is cleared on exit which maybe be good enough? On the other hand all the
documentation indicates everything is cleared. Do you think this is a bug? If so
I can prepare a fix in a follow up patch.

[joth, 2013-02-22 21:59:38]

new iteration looks much cleaner to me, fwiw

https://codereview.chromium.org/12317062/diff/2002/content/public/browser/sto...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/12317062/diff/2002/content/public/browser/sto...
content/public/browser/storage_partition.h:85: int storage_mask,
I think we use unsigned or uint32  for bit masks. (I maybe mis-remembering from
a previous project; whatever it's my personal preference anyway),

[joth, 2013-02-22 22:01:08]

https://codereview.chromium.org/12317062/diff/2002/android_webview/native/aw_...
File android_webview/native/aw_quota_manager_bridge_impl.cc (right):

https://codereview.chromium.org/12317062/diff/2002/android_webview/native/aw_...
android_webview/native/aw_quota_manager_bridge_impl.cc:199:
GetStoragePartition()->AsyncClearAllData(
nit: the "All" in the function name maybe misleading now.
AsyncClearData() (could append ByType if it helps?)

[boliu, 2013-02-25 17:39:20]

Joth's comments addressed. PTAL

https://codereview.chromium.org/12317062/diff/2002/android_webview/native/aw_...
File android_webview/native/aw_quota_manager_bridge_impl.cc (right):

https://codereview.chromium.org/12317062/diff/2002/android_webview/native/aw_...
android_webview/native/aw_quota_manager_bridge_impl.cc:199:
GetStoragePartition()->AsyncClearAllData(
On 2013/02/22 22:01:08, joth wrote:
> nit: the "All" in the function name maybe misleading now.
> AsyncClearData() (could append ByType if it helps?)

Renamed to AsyncClearData

https://codereview.chromium.org/12317062/diff/2002/content/public/browser/sto...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/12317062/diff/2002/content/public/browser/sto...
content/public/browser/storage_partition.h:85: int storage_mask,
On 2013/02/22 21:59:39, joth wrote:
> I think we use unsigned or uint32  for bit masks. (I maybe mis-remembering
from
> a previous project; whatever it's my personal preference anyway),

Done.

[boliu, 2013-02-27 16:26:32]

jam: Ping!

[boliu, 2013-03-04 19:59:04]

On 2013/02/27 16:26:32, boliu wrote:
> jam: Ping!

Ping x2!

[jam, 2013-03-04 23:32:18]

lgtm, sorry I missed the ping (feel free to IM me next time if I don't respond
within a few hours)

+ajwong for his ok on StorageParitition

[boliu, 2013-03-04 23:39:27]

Sorry, I had a question from a previous patch set that was missed. Copied over
again to the latest one

https://codereview.chromium.org/12317062/diff/15001/content/browser/storage_p...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/12317062/diff/15001/content/browser/storage_p...
content/browser/storage_partition_impl.cc:270:
GetDOMStorageContext()->DeleteLocalStorage(storage_origin);
Dom session storage is not explicitly cleared here (compare to AsyncClearALlData
below). I'm not sure if it's a bug or not. On one hand I'm guessing session
storage is cleared on exit which maybe be good enough? On the other hand all the
documentation indicates everything is cleared. Do you think this is a bug? If so
I can prepare a fix in a follow up patch.

[awong, 2013-03-04 23:44:27]

I'm slightly worried here because there seems to be some amount of overlap in
functionality between this and the BrowsingDataRemover.  Even before this patch,
there was some duplicated logic, but it felt okayish because the
StoragePartition APIs are very simple (whack everything).

Have you looked at BrowsingDataRemover?

https://codereview.chromium.org/12317062/diff/15001/android_webview/native/aw...
File android_webview/native/aw_quota_manager_bridge_impl.cc (right):

https://codereview.chromium.org/12317062/diff/15001/android_webview/native/aw...
android_webview/native/aw_quota_manager_bridge_impl.cc:181:
content::BrowserContext::GetStoragePartitionForSite(
Please prefer BrowserContext::GetDefaultStoragePartition() to manually passing a
GURL().  I know there's a scary comment on that API...but things have changed
and calling GetDefaultStoragePartition() is the right thing here. I'll be
updating that comment soon.

https://codereview.chromium.org/12317062/diff/15001/content/browser/storage_p...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/12317062/diff/15001/content/browser/storage_p...
content/browser/storage_partition_impl.cc:270:
GetDOMStorageContext()->DeleteLocalStorage(storage_origin);
On 2013/03/04 23:39:27, boliu wrote:
> Dom session storage is not explicitly cleared here (compare to
AsyncClearALlData
> below). I'm not sure if it's a bug or not. On one hand I'm guessing session
> storage is cleared on exit which maybe be good enough? On the other hand all
the
> documentation indicates everything is cleared. Do you think this is a bug? If
so
> I can prepare a fix in a follow up patch.

Good catch. That is indeed a mistake.

[jam, 2013-03-04 23:50:56]

On 2013/03/04 23:44:27, awong wrote:
> I'm slightly worried here because there seems to be some amount of overlap in
> functionality between this and the BrowsingDataRemover.  Even before this
patch,
> there was some duplicated logic, but it felt okayish because the
> StoragePartition APIs are very simple (whack everything).
> 
> Have you looked at BrowsingDataRemover?

note that code is in src\chroem, which src\android_webview is independent from
(it's another embedder of content)

[boliu, 2013-03-05 00:00:00]

Right, android_webview is a separate content embedder and does not include code
under chrome/. I did look at it before and "drew inspiration" from it though.

So looks like some BrowserDataRemover code can be removed and just call this
instead? More follow up...

https://codereview.chromium.org/12317062/diff/15001/android_webview/native/aw...
File android_webview/native/aw_quota_manager_bridge_impl.cc (right):

https://codereview.chromium.org/12317062/diff/15001/android_webview/native/aw...
android_webview/native/aw_quota_manager_bridge_impl.cc:181:
content::BrowserContext::GetStoragePartitionForSite(
On 2013/03/04 23:44:28, awong wrote:
> Please prefer BrowserContext::GetDefaultStoragePartition() to manually passing
a
> GURL().  I know there's a scary comment on that API...but things have changed
> and calling GetDefaultStoragePartition() is the right thing here. I'll be
> updating that comment soon.

Done.

https://codereview.chromium.org/12317062/diff/15001/content/browser/storage_p...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/12317062/diff/15001/content/browser/storage_p...
content/browser/storage_partition_impl.cc:270:
GetDOMStorageContext()->DeleteLocalStorage(storage_origin);
On 2013/03/04 23:44:28, awong wrote:
> On 2013/03/04 23:39:27, boliu wrote:
> > Dom session storage is not explicitly cleared here (compare to
> AsyncClearALlData
> > below). I'm not sure if it's a bug or not. On one hand I'm guessing session
> > storage is cleared on exit which maybe be good enough? On the other hand all
> the
> > documentation indicates everything is cleared. Do you think this is a bug?
If
> so
> > I can prepare a fix in a follow up patch.
> 
> Good catch. That is indeed a mistake.

And I promise to send a follow up patch...

[awong, 2013-03-05 00:05:47]

LGTM w/ the previous 2 comments.

Yeah, I think we need to clean up BrowsingDataRemover then. Bo, you willing
to do that refactor?


On Mon, Mar 4, 2013 at 4:00 PM, <boliu@chromium.org> wrote:

> Right, android_webview is a separate content embedder and does not include
> code
> under chrome/. I did look at it before and "drew inspiration" from it
> though.
>
> So looks like some BrowserDataRemover code can be removed and just call
> this
> instead? More follow up...
>
>
>
> https://codereview.chromium.**org/12317062/diff/15001/**
>
android_webview/native/aw_**quota_manager_bridge_impl.cc<https://codereview.chromium.org/12317062/diff/15001/android_webview/native/aw_quota_manager_bridge_impl.cc>
> File android_webview/native/aw_**quota_manager_bridge_impl.cc (right):
>
> https://codereview.chromium.**org/12317062/diff/15001/**
>
android_webview/native/aw_**quota_manager_bridge_impl.cc#**newcode181<https://codereview.chromium.org/12317062/diff/15001/android_webview/native/aw_quota_manager_bridge_impl.cc#newcode181>
> android_webview/native/aw_**quota_manager_bridge_impl.cc:**181:
> content::BrowserContext::**GetStoragePartitionForSite(
> On 2013/03/04 23:44:28, awong wrote:
>
>> Please prefer BrowserContext::**GetDefaultStoragePartition() to manually
>>
> passing a
>
>> GURL().  I know there's a scary comment on that API...but things have
>>
> changed
>
>> and calling GetDefaultStoragePartition() is the right thing here. I'll
>>
> be
>
>> updating that comment soon.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/12317062/diff/15001/**
>
content/browser/storage_**partition_impl.cc<https://codereview.chromium.org/12317062/diff/15001/content/browser/storage_partition_impl.cc>
> File content/browser/storage_**partition_impl.cc (right):
>
> https://codereview.chromium.**org/12317062/diff/15001/**
>
content/browser/storage_**partition_impl.cc#newcode270<https://codereview.chromium.org/12317062/diff/15001/content/browser/storage_partition_impl.cc#newcode270>
> content/browser/storage_**partition_impl.cc:270:
> GetDOMStorageContext()->**DeleteLocalStorage(storage_**origin);
> On 2013/03/04 23:44:28, awong wrote:
>
>> On 2013/03/04 23:39:27, boliu wrote:
>> > Dom session storage is not explicitly cleared here (compare to
>> AsyncClearALlData
>> > below). I'm not sure if it's a bug or not. On one hand I'm guessing
>>
> session
>
>> > storage is cleared on exit which maybe be good enough? On the other
>>
> hand all
>
>> the
>> > documentation indicates everything is cleared. Do you think this is
>>
> a bug? If
>
>> so
>> > I can prepare a fix in a follow up patch.
>>
>
>  Good catch. That is indeed a mistake.
>>
>
> And I promise to send a follow up patch...
>
>
https://codereview.chromium.**org/12317062/<https://codereview.chromium.org/1...
>

[boliu, 2013-03-05 00:08:37]

On 2013/03/05 00:05:47, awong wrote:
> LGTM w/ the previous 2 comments.
> 
> Yeah, I think we need to clean up BrowsingDataRemover then. Bo, you willing
> to do that refactor?
> 
Sure (assuming it's not urgent. Tied up in some android webview graphics stuff
right now)

[awong, 2013-03-05 00:10:10]

On Mon, Mar 4, 2013 at 4:08 PM, <boliu@chromium.org> wrote:

> On 2013/03/05 00:05:47, awong wrote:
>
>> LGTM w/ the previous 2 comments.
>>
>
>  Yeah, I think we need to clean up BrowsingDataRemover then. Bo, you
>> willing
>> to do that refactor?
>>
>
> Sure (assuming it's not urgent. Tied up in some android webview graphics
> stuff
> right now)
>

Totally not urgent.  Feel free to send reviews to me.

-Albert


>
>
https://codereview.chromium.**org/12317062/<https://codereview.chromium.org/1...
>

[joth, 2013-03-05 00:10:43]

aw/ lgtm

[commit-bot: I haz the power, 2013-03-05 00:13:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/12317062/16008

[commit-bot: I haz the power, 2013-03-05 03:52:03]

Change committed as 186080

[jam, 2013-03-25 19:54:49]

btw, just curious what the status of the refactoring is?


On Mon, Mar 4, 2013 at 4:09 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> On Mon, Mar 4, 2013 at 4:08 PM, <boliu@chromium.org> wrote:
>
>> On 2013/03/05 00:05:47, awong wrote:
>>
>>> LGTM w/ the previous 2 comments.
>>>
>>
>>  Yeah, I think we need to clean up BrowsingDataRemover then. Bo, you
>>> willing
>>> to do that refactor?
>>>
>>
>> Sure (assuming it's not urgent. Tied up in some android webview graphics
>> stuff
>> right now)
>>
>
> Totally not urgent.  Feel free to send reviews to me.
>
> -Albert
>
>
>>
>>
https://codereview.chromium.**org/12317062/<https://codereview.chromium.org/1...
>>
>
>

[boliu, 2013-03-25 20:00:04]

On 2013/03/25 19:54:49, jam wrote:
> btw, just curious what the status of the refactoring is?
> 

Have not started. I'm still tied up with webview graphics work for awhile. I did
file crbug.com/180118 so I don't forget.