Reduce use of DatabaseIdentifier in Indexed DB entry points.

Reduce use of DatabaseIdentifier in Indexed DB entry points.

A DatabaseIdentifier is a way to serialize an origin URL, but it is
only needed when creating filenames. Stop using it where an origin URL
is perfectly appropriate, and remove duplicate conversion functions
when it is used.

Also, remove a few unused includes of database identifier headers.

BUG=591240
R=michaeln@chromium.org

Committed: https://crrev.com/b8f7a9160398c23a44807ff474e7e5288e223416
Cr-Commit-Position: refs/heads/master@{#380438}

[jsbell, 2016-03-02 17:46:03]

Description was changed from

==========
Reduce use of DatabaseIdentifier in Indexed DB entry points.

A DatabaseIdentifier is a way to serialize an origin URL, but it is
only needed when creating filenames. Stop using it where an origin URL
is perfectly appropriate, and remove duplicate conversion functions
when it is used.

BUG=591240
R=michaeln@chromium.org
==========

to

==========
Reduce use of DatabaseIdentifier in Indexed DB entry points.

A DatabaseIdentifier is a way to serialize an origin URL, but it is
only needed when creating filenames. Stop using it where an origin URL
is perfectly appropriate, and remove duplicate conversion functions
when it is used.

Also, remove a few unused includes of database identifier headers.

BUG=591240
R=michaeln@chromium.org
==========

[jsbell, 2016-03-02 17:52:56]

michaeln@ - please take a look?

(I'll need OWNERS review for content/public and chrome/browser/extensions tho)

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_context_impl.h:132: base::FilePath
GetBlobStorePath(const GURL& origin_url) const;
Added "Store" here just to make it clear this wasn't the name/path of a specific
blob, since there are very similarly named functions for that nearby.

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_internals_ui.cc (right):

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_internals_ui.cc:236:
temp_path.Append(IndexedDBContextImpl::GetLevelDBFileName(origin_url))
Using GetIdentifierFromOrigin here would be totally legitimate, since it's doing
an origin->filename creation that's not consumed elsewhere. But I decided to
switch it to use the utility just to reduce the use of DatabaseIdentifier
generally. I can switch back if desired.

[michaeln, 2016-03-02 23:51:33]

qq about url::Origin?

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:89: return
storage::GetIdentifierFromOrigin(origin_url) + "@1";
A little OT but curious, what's the purpose of "@1"?

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_context_impl.h:77: base::FilePath
GetFilePathForTesting(const GURL& origin_url) const override;
Are you familiar with the url::Origin class? Wdyt about using that for this
purpose in new code?

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_context_impl.h:132: base::FilePath
GetBlobStorePath(const GURL& origin_url) const;
On 2016/03/02 17:52:56, jsbell wrote:
> Added "Store" here just to make it clear this wasn't the name/path of a
specific
> blob, since there are very similarly named functions for that nearby.

sgtm

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_internals_ui.cc (right):

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_internals_ui.cc:236:
temp_path.Append(IndexedDBContextImpl::GetLevelDBFileName(origin_url))
On 2016/03/02 17:52:56, jsbell wrote:
> Using GetIdentifierFromOrigin here would be totally legitimate, since it's
doing
> an origin->filename creation that's not consumed elsewhere. But I decided to
> switch it to use the utility just to reduce the use of DatabaseIdentifier
> generally. I can switch back if desired.

I don't have a strong opinion. Is this filename seen by the end user producing
the zip file? If so you might want to switch this usage back. If we ever switch
away from using db identifier strings for the filenames in the users data
directory, the origin identifier would probably be a more user friendly name.

[jsbell, 2016-03-03 01:11:25]

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:89: return
storage::GetIdentifierFromOrigin(origin_url) + "@1";
On 2016/03/02 23:51:33, michaeln wrote:
> A little OT but curious, what's the purpose of "@1"?

The leveldb schema was designed to support multiple origins in one database
(even though that's not done), and this identifier is used in keys.

The "@1" I assume is versioning related to that and/or just being forward
thinking.

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_context_impl.h:77: base::FilePath
GetFilePathForTesting(const GURL& origin_url) const override;
On 2016/03/02 23:51:33, michaeln wrote:
> Are you familiar with the url::Origin class? Wdyt about using that for this
> purpose in new code?

I was chatting about that w/ mkwst@ in
https://codereview.chromium.org/1760673003/ and filed https://crbug.com/591482
to to track converting everything over. I think we should do it especially for
the IPC layer, but you want to plumb it all the way from Blink > Renderer > IPC
> Browser which is more work than I wanted to do here.

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_internals_ui.cc (right):

https://codereview.chromium.org/1757693002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_internals_ui.cc:236:
temp_path.Append(IndexedDBContextImpl::GetLevelDBFileName(origin_url))
On 2016/03/02 23:51:33, michaeln wrote:
> On 2016/03/02 17:52:56, jsbell wrote:
> > Using GetIdentifierFromOrigin here would be totally legitimate, since it's
> doing
> > an origin->filename creation that's not consumed elsewhere. But I decided to
> > switch it to use the utility just to reduce the use of DatabaseIdentifier
> > generally. I can switch back if desired.
> 
> I don't have a strong opinion. Is this filename seen by the end user producing
> the zip file? If so you might want to switch this usage back. If we ever
switch
> away from using db identifier strings for the filenames in the users data
> directory, the origin identifier would probably be a more user friendly name.

Yes, seen by the user, it becomes a file in "Downloads". And good point - I'll
switch it back.

[jsbell, 2016-03-03 01:12:40]

Also, https://crrev.com/1755343002 is in progress too. (I split the CLs in to
backend-centric and Blink/IPC-centric. If we plumbed url::Origin through it
should go all the way.)

[michaeln, 2016-03-03 01:22:40]

lgtm!

[Marijn Kruisselbrink, 2016-03-03 01:24:45]

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

[Marijn Kruisselbrink, 2016-03-03 01:24:46]

chrome/browser/extensions drive-by LGTM

[jsbell, 2016-03-08 22:26:24]

jsbell@chromium.org changed reviewers:
+ jam@chromium.org

[jsbell, 2016-03-08 22:26:24]

jam@ - OWNERS review for content/public change?

(and thanks mek@ !)

[jsbell, 2016-03-09 20:34:15]

jsbell@chromium.org changed reviewers:
+ jochen@chromium.org
- jam@chromium.org

[jsbell, 2016-03-09 20:34:16]

Oops, jam@ is away.

jochen@ - can you OWNERS review the content/public change?

[kinuko, 2016-03-10 01:27:24]

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

[kinuko, 2016-03-10 01:27:25]

drive-by lgtm for content/public in case it helps

[jochen (gone - plz use gerrit), 2016-03-10 15:46:52]

lgtm either way

https://codereview.chromium.org/1757693002/diff/20001/content/public/browser/...
File content/public/browser/indexed_db_context.h (right):

https://codereview.chromium.org/1757693002/diff/20001/content/public/browser/...
content/public/browser/indexed_db_context.h:45: const GURL& origin_url) const =
0;
should this be Origin?

[jsbell, 2016-03-10 17:27:55]

https://codereview.chromium.org/1757693002/diff/20001/content/public/browser/...
File content/public/browser/indexed_db_context.h (right):

https://codereview.chromium.org/1757693002/diff/20001/content/public/browser/...
content/public/browser/indexed_db_context.h:45: const GURL& origin_url) const =
0;
On 2016/03/10 15:46:51, jochen wrote:
> should this be Origin?

In general, yes we should switch all of these uses from GURL to Origin.

All the storage backends use GURL internally for handling origins, so I'm first
minimizing use of DatabaseIdentifier for entry points, then will tackle GURL to
Origin later - tracked as https://crbug.com/591482

[jsbell, 2016-03-10 17:28:06]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-03-10 17:28:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1757693002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1757693002/20001

[commit-bot: I haz the power, 2016-03-10 18:59:58]

Description was changed from

==========
Reduce use of DatabaseIdentifier in Indexed DB entry points.

A DatabaseIdentifier is a way to serialize an origin URL, but it is
only needed when creating filenames. Stop using it where an origin URL
is perfectly appropriate, and remove duplicate conversion functions
when it is used.

Also, remove a few unused includes of database identifier headers.

BUG=591240
R=michaeln@chromium.org
==========

to

==========
Reduce use of DatabaseIdentifier in Indexed DB entry points.

A DatabaseIdentifier is a way to serialize an origin URL, but it is
only needed when creating filenames. Stop using it where an origin URL
is perfectly appropriate, and remove duplicate conversion functions
when it is used.

Also, remove a few unused includes of database identifier headers.

BUG=591240
R=michaeln@chromium.org
==========

[commit-bot: I haz the power, 2016-03-10 18:59:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-10 19:01:25]

Description was changed from

==========
Reduce use of DatabaseIdentifier in Indexed DB entry points.

A DatabaseIdentifier is a way to serialize an origin URL, but it is
only needed when creating filenames. Stop using it where an origin URL
is perfectly appropriate, and remove duplicate conversion functions
when it is used.

Also, remove a few unused includes of database identifier headers.

BUG=591240
R=michaeln@chromium.org
==========

to

==========
Reduce use of DatabaseIdentifier in Indexed DB entry points.

A DatabaseIdentifier is a way to serialize an origin URL, but it is
only needed when creating filenames. Stop using it where an origin URL
is perfectly appropriate, and remove duplicate conversion functions
when it is used.

Also, remove a few unused includes of database identifier headers.

BUG=591240
R=michaeln@chromium.org

Committed: https://crrev.com/b8f7a9160398c23a44807ff474e7e5288e223416
Cr-Commit-Position: refs/heads/master@{#380438}
==========

[commit-bot: I haz the power, 2016-03-10 19:01:27]

Patchset 2 (id:??) landed as
https://crrev.com/b8f7a9160398c23a44807ff474e7e5288e223416
Cr-Commit-Position: refs/heads/master@{#380438}