Switch database/file_identifier to std::string, remove createFromDatabaseIdentifier calls

Switch database/file_identifier to std::string, remove createFromDatabaseIdentifier calls

Origin identifiers are always ASCII and can be stored as strings. Database
identifiers are the same thing as origin identifiers. File identifiers are
origin identifiers with an ASCII suffix. This keeps all of these ASCII,
stored in std::string, and names origin identifiers "origin_identifier" more
consistently. Origin identifiers are coded as UTF-16, so the leveldb coding
code inflates/deflates to 16-bit strings.

This also removes some remaining calls to WebSecurityOrigin::createFromDatabaseIdentifier
in favor of calls to webkit/common/database/database_identifier to remove
dependencies on Blink and unnecessary 8bit -> 16bit -> 8bit string conversions.

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

[jamesr, 2013-06-20 05:31:45]

This patch is on top of https://codereview.chromium.org/17003010/.  My true goal
is to cut down on WebSecurityOrigin calls from the browser process, but the rest
of this seems nice as well.

[jsbell, 2013-06-20 16:38:39]

lgtm with a few nits (and notes to myself for future work)

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_backing_store.h:54: const std::string&
file_identifier);
file_identifier for OpenInMemory is a little odd; we should probably rename it
everywhere or put a comment somewhere that it's the identifier used *within* the
backing store data for historical reasons. That doesn't have to be this CL,
though.

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_factory.cc (right):

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_factory.cc:34: static string16
ComputeUniqueIdentifier(const string16& name,
There's actually no reason this needs to be a string16 either, but that can be
another CL. (ISTR it's only used for some indirect reference counting scheme, so
should probably just be ripped out. *sigh*)

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:13: #include
"base/strings/string_piece.h"
Explicitly included by the .h for this .cc... is this intentional? (Maybe I'm
missing something.)

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1286:
EncodeStringWithLength(base::ASCIIToUTF16(origin_identifier), &ret);
I haven't see anyone bother to tack "base::" on to ASCIIToUTF16 elsewhere - are
we planning to do that broadly?

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/we...
File content/browser/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/we...
content/browser/indexed_db/webidbfactory_impl.cc:29: database_identifier.utf8(),
Despite the name, these WebIDB*Impl files no longer implement WebIDB interfaces
- they're non-refcounted wrappers around refcounted objects. We'll refactor them
away soon.

So the signature on these can change, and they don't need to take WebStrings in.
We can push the conversion one level higher up into the caller
(IndexedDBDispatcherHost) if desired.

[jamesr, 2013-06-20 16:54:32]

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_factory.cc (right):

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_factory.cc:34: static string16
ComputeUniqueIdentifier(const string16& name,
On 2013/06/20 16:38:40, jsbell wrote:
> There's actually no reason this needs to be a string16 either, but that can be
> another CL. (ISTR it's only used for some indirect reference counting scheme,
so
> should probably just be ripped out. *sigh*)

Ah, I had assumed that 'name' was the same as the 'database_name' exposed to
script. Good to know that it's something else, although I'd rather leave it
alone in this patch.

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:13: #include
"base/strings/string_piece.h"
On 2013/06/20 16:38:40, jsbell wrote:
> Explicitly included by the .h for this .cc... is this intentional? (Maybe I'm
> missing something.)

I was having trouble seeing the std::string -> base::StringPiece implicit
conversion required by ASCIIToUTF16 due to getting some other #includes wrong,
but this isn't really necessary. will remove

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:1286:
EncodeStringWithLength(base::ASCIIToUTF16(origin_identifier), &ret);
On 2013/06/20 16:38:40, jsbell wrote:
> I haven't see anyone bother to tack "base::" on to ASCIIToUTF16 elsewhere -
are
> we planning to do that broadly?

https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/utf_s...
says that we are (although I'm not consistent even within this CL, tsk).

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/we...
File content/browser/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/we...
content/browser/indexed_db/webidbfactory_impl.cc:29: database_identifier.utf8(),
On 2013/06/20 16:38:40, jsbell wrote:
> Despite the name, these WebIDB*Impl files no longer implement WebIDB
interfaces
> - they're non-refcounted wrappers around refcounted objects. We'll refactor
them
> away soon.
> 
> So the signature on these can change, and they don't need to take WebStrings
in.
> We can push the conversion one level higher up into the caller
> (IndexedDBDispatcherHost) if desired.

In fact, it looks like the caller is taking a std::string and calling
WebString::fromUTF8() to generate this value.  Will nuke!

[jsbell, 2013-07-02 22:27:12]

On 2013/06/20 16:54:32, jamesr wrote:
>
https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
> File content/browser/indexed_db/indexed_db_factory.cc (right):
> 
>
https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
> content/browser/indexed_db/indexed_db_factory.cc:34: static string16
> ComputeUniqueIdentifier(const string16& name,
> On 2013/06/20 16:38:40, jsbell wrote:
> > There's actually no reason this needs to be a string16 either, but that can
be
> > another CL. (ISTR it's only used for some indirect reference counting
scheme,
> so
> > should probably just be ripped out. *sigh*)
> 
> Ah, I had assumed that 'name' was the same as the 'database_name' exposed to
> script. Good to know that it's something else, although I'd rather leave it
> alone in this patch.
> 
>
https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
> File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):
> 
>
https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
> content/browser/indexed_db/indexed_db_leveldb_coding.cc:13: #include
> "base/strings/string_piece.h"
> On 2013/06/20 16:38:40, jsbell wrote:
> > Explicitly included by the .h for this .cc... is this intentional? (Maybe
I'm
> > missing something.)
> 
> I was having trouble seeing the std::string -> base::StringPiece implicit
> conversion required by ASCIIToUTF16 due to getting some other #includes wrong,
> but this isn't really necessary. will remove
> 
>
https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/in...
> content/browser/indexed_db/indexed_db_leveldb_coding.cc:1286:
> EncodeStringWithLength(base::ASCIIToUTF16(origin_identifier), &ret);
> On 2013/06/20 16:38:40, jsbell wrote:
> > I haven't see anyone bother to tack "base::" on to ASCIIToUTF16 elsewhere -
> are
> > we planning to do that broadly?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/utf_s...
> says that we are (although I'm not consistent even within this CL, tsk).
> 
>
https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/we...
> File content/browser/indexed_db/webidbfactory_impl.cc (right):
> 
>
https://codereview.chromium.org/16870007/diff/1/content/browser/indexed_db/we...
> content/browser/indexed_db/webidbfactory_impl.cc:29:
database_identifier.utf8(),
> On 2013/06/20 16:38:40, jsbell wrote:
> > Despite the name, these WebIDB*Impl files no longer implement WebIDB
> interfaces
> > - they're non-refcounted wrappers around refcounted objects. We'll refactor
> them
> > away soon.
> > 
> > So the signature on these can change, and they don't need to take WebStrings
> in.
> > We can push the conversion one level higher up into the caller
> > (IndexedDBDispatcherHost) if desired.
> 
> In fact, it looks like the caller is taking a std::string and calling
> WebString::fromUTF8() to generate this value.  Will nuke!

Huh, forgot about this. I probably took care of a bunch of this, but the unit
tests are still using WebSecurityOrigin

Is it worth trying to rebase this patch or should we start over?

[jsbell, 2013-07-02 23:49:25]

Rebased. Following https://codereview.chromium.org/18221003/ this may get even
simpler.

[jsbell, 2013-07-03 16:43:57]

Rebased again. Should be good to go now.

[commit-bot: I haz the power, 2013-07-03 16:44:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/16870007/14001

[commit-bot: I haz the power, 2013-07-03 16:51:59]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-07-03 16:55:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/16870007/14001

[commit-bot: I haz the power, 2013-07-03 20:15:56]

Change committed as 210033

[jamesr, 2013-07-03 22:13:08]

Poo - sorry about dropping the ball on this!  For some reason I thought it had
landed.  Thanks for following up!