Use string16 for cache_name in the back end of CacheStorage.

Use string16 for cache_name in the back end of CacheStorage.

BUG=432746

[horo, 2016-03-01 09:17:22]

Description was changed from

==========
Use string16 for cache_name in the back end of CacheStorage.

BUG=432746
==========

to

==========
Use string16 for cache_name in the back end of CacheStorage.

BUG=432746
==========

[horo, 2016-03-02 03:45:13]

Patchset #2 (id:20001) has been deleted

[horo, 2016-03-02 03:45:22]

Patchset #1 (id:1) has been deleted

[horo, 2016-03-02 03:47:09]

horo@chromium.org changed reviewers:
+ jsbell@chromium.org, nhiroki@chromium.org

[horo, 2016-03-02 03:47:10]

nhiroki@
Could you please review this?

[nhiroki, 2016-03-02 05:43:13]

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:289: // TODO(horo): Depricate
|name| when M51 become stable.
s/become/becomes/

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:359: if (cache.has_name_utf16())
{
Should we have a UMA to track the number of attempts to update the protobuf
schema so that we can know when to drop these conversion code?

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:362: cache.name_utf16().size() /
sizeof(base::char16));
We may need to store the index if there is a cache that doesn't have a valid
|name_utf16| field because WriteIndex() would be called only when a Cache is
newly created or deleted, that is, the index is never updated until those
operations are issued.

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_manager_unittest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Can we have a test for the case where the protobuf schema is updated?

[horo, 2016-03-02 08:34:12]

Patchset #2 (id:60001) has been deleted

[horo, 2016-03-02 08:35:27]

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:289: // TODO(horo): Depricate
|name| when M51 become stable.
On 2016/03/02 05:43:13, nhiroki wrote:
> s/become/becomes/

Done.

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:359: if (cache.has_name_utf16())
{
On 2016/03/02 05:43:13, nhiroki wrote:
> Should we have a UMA to track the number of attempts to update the protobuf
> schema so that we can know when to drop these conversion code?

We can't say when users return back to the sites which created the cache storage
in old Chrome.
So it is not easy to drop these conversion code.

Of course we can add codes which migrate the all cache storage indexes for all
origins.
But I prefer to keep this simple conversion code rather than to add complex
migration codes.

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_manager_unittest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2016/03/02 05:43:13, nhiroki wrote:
> Can we have a test for the case where the protobuf schema is updated?

Added in cache_storage_unittest.cc

[nhiroki, 2016-03-02 09:22:58]

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:359: if (cache.has_name_utf16())
{
On 2016/03/02 08:35:27, horo wrote:
> On 2016/03/02 05:43:13, nhiroki wrote:
> > Should we have a UMA to track the number of attempts to update the protobuf
> > schema so that we can know when to drop these conversion code?
> 
> We can't say when users return back to the sites which created the cache
storage
> in old Chrome.
> So it is not easy to drop these conversion code.
> 
> Of course we can add codes which migrate the all cache storage indexes for all
> origins.
> But I prefer to keep this simple conversion code rather than to add complex
> migration codes.

Hmm... I'm a bit confused. According to the TODO comment at line 289, you will
actually remove/deprecate the name field (and the conversion code), right? How
do you decide the timing to deprecate it w/o such the UMA and/or migration code?
If we don't have either the UMA or the migration code, I think we couldn't
estimate the number of users who may lose access to their contents...

[jkarlin, 2016-03-02 14:32:01]

jkarlin@chromium.org changed reviewers:
+ jkarlin@chromium.org

[jkarlin, 2016-03-02 14:32:01]

Drive by comment:

The browser normally uses UTF8. How about we keep the UTF8 in the browser and
perform the conversion via WebString::utf8() (which is lenient and does the
correct thing with unpaired surrogates) in CacheStorageDispatcher which defaults
to lenient conversion of unpaired surrogates?

[jsbell, 2016-03-02 17:43:35]

On 2016/03/02 14:32:01, jkarlin wrote:
> Drive by comment:
> 
> The browser normally uses UTF8. How about we keep the UTF8 in the browser and
> perform the conversion via WebString::utf8() (which is lenient and does the
> correct thing with unpaired surrogates) in CacheStorageDispatcher which
defaults
> to lenient conversion of unpaired surrogates?

+1 to this. 

I used this approach by accident in another work-in-progress CL, and was
surprised when my unpaired-surrogate tests passed - I was expecting to need to
string16ify anything, but doing the conversion in WebString "just works".

[jkarlin, 2016-03-02 21:07:08]

On 2016/03/02 14:32:01, jkarlin wrote:
> Drive by comment:
> 
> The browser normally uses UTF8. How about we keep the UTF8 in the browser and
> perform the conversion via WebString::utf8() (which is lenient and does the
> correct thing with unpaired surrogates) in CacheStorageDispatcher which
defaults
> to lenient conversion of unpaired surrogates?

Er, that was supposed to be: 

The browser normally uses UTF8. How about we keep the UTF8 in the browser and
perform the conversion via WebString::utf8() (which is lenient and does the
correct thing with unpaired surrogates) in CacheStorageDispatcher?

Glad it made sense to you jsbell :)

[horo, 2016-03-03 04:53:45]

On 2016/03/02 21:07:08, jkarlin wrote:
> On 2016/03/02 14:32:01, jkarlin wrote:
> > Drive by comment:
> > 
> > The browser normally uses UTF8. How about we keep the UTF8 in the browser
and
> > perform the conversion via WebString::utf8() (which is lenient and does the
> > correct thing with unpaired surrogates) in CacheStorageDispatcher which
> defaults
> > to lenient conversion of unpaired surrogates?
> 
> Er, that was supposed to be: 
> 
> The browser normally uses UTF8. How about we keep the UTF8 in the browser and
> perform the conversion via WebString::utf8() (which is lenient and does the
> correct thing with unpaired surrogates) in CacheStorageDispatcher?
> 
> Glad it made sense to you jsbell :)

How can I get the WebString from the leniently converted std::string in
CacheStorageDispatcher::OnCacheStorageKeysSuccess()?
String::fromUTF8() returns a null string if the input data contains invalid
UTF-8 sequences.

[jkarlin, 2016-03-03 13:10:24]

On 2016/03/03 04:53:45, horo wrote:
> On 2016/03/02 21:07:08, jkarlin wrote:
> > On 2016/03/02 14:32:01, jkarlin wrote:
> > > Drive by comment:
> > > 
> > > The browser normally uses UTF8. How about we keep the UTF8 in the browser
> and
> > > perform the conversion via WebString::utf8() (which is lenient and does
the
> > > correct thing with unpaired surrogates) in CacheStorageDispatcher which
> > defaults
> > > to lenient conversion of unpaired surrogates?
> > 
> > Er, that was supposed to be: 
> > 
> > The browser normally uses UTF8. How about we keep the UTF8 in the browser
and
> > perform the conversion via WebString::utf8() (which is lenient and does the
> > correct thing with unpaired surrogates) in CacheStorageDispatcher?
> > 
> > Glad it made sense to you jsbell :)
> 
> How can I get the WebString from the leniently converted std::string in
> CacheStorageDispatcher::OnCacheStorageKeysSuccess()?
> String::fromUTF8() returns a null string if the input data contains invalid
> UTF-8 sequences.

Ah. I assumed we could convert back. So either we alter String::fromUTF8 to take
a mode and ignore conversion errors if the mode is lenient, or we use string16
here. I suppose string16 here is the simplest route.

[horo, 2016-03-04 03:38:24]

On 2016/03/03 13:10:24, jkarlin wrote:
> On 2016/03/03 04:53:45, horo wrote:
> > On 2016/03/02 21:07:08, jkarlin wrote:
> > > On 2016/03/02 14:32:01, jkarlin wrote:
> > > > Drive by comment:
> > > > 
> > > > The browser normally uses UTF8. How about we keep the UTF8 in the
browser
> > and
> > > > perform the conversion via WebString::utf8() (which is lenient and does
> the
> > > > correct thing with unpaired surrogates) in CacheStorageDispatcher which
> > > defaults
> > > > to lenient conversion of unpaired surrogates?
> > > 
> > > Er, that was supposed to be: 
> > > 
> > > The browser normally uses UTF8. How about we keep the UTF8 in the browser
> and
> > > perform the conversion via WebString::utf8() (which is lenient and does
the
> > > correct thing with unpaired surrogates) in CacheStorageDispatcher?
> > > 
> > > Glad it made sense to you jsbell :)
> > 
> > How can I get the WebString from the leniently converted std::string in
> > CacheStorageDispatcher::OnCacheStorageKeysSuccess()?
> > String::fromUTF8() returns a null string if the input data contains invalid
> > UTF-8 sequences.
> 
> Ah. I assumed we could convert back. So either we alter String::fromUTF8 to
take
> a mode and ignore conversion errors if the mode is lenient, or we use string16
> here. I suppose string16 here is the simplest route.

OK then I will use string16.

[horo, 2016-03-04 03:49:14]

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/1751833002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:359: if (cache.has_name_utf16())
{
On 2016/03/02 09:22:58, nhiroki (ooo) wrote:
> On 2016/03/02 08:35:27, horo wrote:
> > On 2016/03/02 05:43:13, nhiroki wrote:
> > > Should we have a UMA to track the number of attempts to update the
protobuf
> > > schema so that we can know when to drop these conversion code?
> > 
> > We can't say when users return back to the sites which created the cache
> storage
> > in old Chrome.
> > So it is not easy to drop these conversion code.
> > 
> > Of course we can add codes which migrate the all cache storage indexes for
all
> > origins.
> > But I prefer to keep this simple conversion code rather than to add complex
> > migration codes.
> 
> Hmm... I'm a bit confused. According to the TODO comment at line 289, you will
> actually remove/deprecate the name field (and the conversion code), right? How
> do you decide the timing to deprecate it w/o such the UMA and/or migration
code?
> If we don't have either the UMA or the migration code, I think we couldn't
> estimate the number of users who may lose access to their contents...

Line 290 is writing the name for FORWARD compatibility so that Chrome 50 and
older (or 51 if this patch will be reverted unfortunately) can read the
CacheStorage index which was written by Chrome 51 (with this patch).
We can stop writing the name when M51 becomes stable.

Line 358 is reading the name for BACKWARD compatibility so that Chrome with this
patch will be able to read the old CacheStorage index which was written by
Chrome 50 and older.
I think we should keep this line.

[jkarlin, 2016-03-04 16:14:42]

Hmm, BackgroundSync has the same issue (tag is a DOMString) and we're currently
using utf8 on the browser side. Perhaps we actually should make a
String::fromUTF8Lenient. WDYT?

[horo, 2016-03-07 08:57:14]

On 2016/03/04 16:14:42, jkarlin wrote:
> Hmm, BackgroundSync has the same issue (tag is a DOMString) and we're
currently
> using utf8 on the browser side. Perhaps we actually should make a
> String::fromUTF8Lenient. WDYT?

Created https://codereview.chromium.org/1768063002/

[jkarlin, 2016-03-07 14:36:35]

Code looks good, thanks for doing this.

We're going to lose existing caches that contain \uFFFD characters. Do we have a
sense for how many of those there are?

When trying to open a cache we could check if converting the utf8 cache name to
16 and strict-mode back to 8 results in an already existing cache name. If so we
could migrate that cache to the lenient utf8 name though there is risk of error.

[jkarlin, 2016-03-07 14:37:15]

On 2016/03/07 14:36:35, jkarlin wrote:
> Code looks good, thanks for doing this.
> 
> We're going to lose existing caches that contain \uFFFD characters. Do we have
a
> sense for how many of those there are?
> 
> When trying to open a cache we could check if converting the utf8 cache name
to
> 16 and strict-mode back to 8 results in an already existing cache name. If so
we
> could migrate that cache to the lenient utf8 name though there is risk of
error.

Sorry, posted this on the wrong CL. Meant for
https://codereview.chromium.org/1768063002/.