WebSQL: Use url::Origin rather than database identifiers for IPC

WebSQL: Use url::Origin rather than database identifiers for IPC

Replace WebSecurityOrigin -> GURL -> string -> IPC -> string -> GURL with
simpler WebSecurityOrigin -> IPC -> Origin -> GURL. Consumers of the
messages still consume GURLs (Quota) or DatabaseIdentifiers (WebSQL) so
there's more tidying that can be done in follow-ups.

R=michaeln@chromium.org,mkwst@chromium.org
BUG=591482
Committed: https://crrev.com/984e21a28bb2e43ba6df6e28c8f0738ee6e23018
Cr-Commit-Position: refs/heads/master@{#400996}

[jsbell, 2016-03-23 20:33:05]

mkwst@, michaeln@ - please take a look?

https://codereview.chromium.org/1832473002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/database_message_filter.cc (right):

https://codereview.chromium.org/1832473002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/database_message_filter.cc:272:
GURL(origin.Serialize()),
Any use of GURL(url::Origin::Serialize()) indicates some subsystem that should
switch from GURL to Origin. I didn't bother with TODOs as this is widespread.

https://codereview.chromium.org/1832473002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/database_message_filter.cc:365: const std::string&
origin_identifier,
Note that this is not coming from IPC here and below, but an Observer interface.

[Mike West, 2016-03-24 15:06:08]

https://codereview.chromium.org/1832473002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/database_message_filter.cc (right):

https://codereview.chromium.org/1832473002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/database_message_filter.cc:272:
GURL(origin.Serialize()),
On 2016/03/23 at 20:33:05, jsbell wrote:
> Any use of GURL(url::Origin::Serialize()) indicates some subsystem that should
switch from GURL to Origin. I didn't bother with TODOs as this is widespread.

This LGTM, with the same caveat as the other CL. I, personally, would try to
avoid this kind of thing by piping through one callsite at a time. But this is
pretty much your code, so I'm fine with you landing this if your aesthetic sense
differs from mine. :)

[jsbell, 2016-03-30 23:26:40]

michaeln@ - can you also take a look?

[michaeln, 2016-04-06 20:55:14]

lgtm

https://codereview.chromium.org/1832473002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/database_message_filter.cc (left):

https://codereview.chromium.org/1832473002/diff/20001/content/browser/rendere...
content/browser/renderer_host/database_message_filter.cc:310:
bad_message::DBMF_INVALID_ORIGIN_ON_OPEN);
is there a way to mark DBMF_INVALID_ORIGIN_ON_OPEN as obsolete in the .xml file?

[jsbell, 2016-04-06 23:28:21]

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

[jsbell, 2016-04-06 23:28:22]

mpearson@ - histograms review?

Also, any feedback on the specificity of bad_message enum/histograms? This one
already had 2 separate entries. I'm adding 3 more, but if I was doing the API
from scratch I probably would only have used a single entry for all of them, but
maybe we really do want one per method.

https://codereview.chromium.org/1832473002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/database_message_filter.cc (left):

https://codereview.chromium.org/1832473002/diff/20001/content/browser/rendere...
content/browser/renderer_host/database_message_filter.cc:310:
bad_message::DBMF_INVALID_ORIGIN_ON_OPEN);
On 2016/04/06 20:55:14, michaeln wrote:
> is there a way to mark DBMF_INVALID_ORIGIN_ON_OPEN as obsolete in the .xml
file?

I went the other direction and reverted to emitting DBMF_INVALID_ORIGIN_ON_OPEN
here and adding distinct DBMFs everywhere.

Which is a question for the histograms reviewer - how much benefit do we get out
of these being distinct?

[Mark P, 2016-04-07 21:40:41]

histograms.xml lgtm as is
(though I'd likely also be okay if you decide it's better to have fewer enums
based on my comment below.  Personally though I think there's little harm in
specificity.)

https://codereview.chromium.org/1832473002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/database_message_filter.cc (left):

https://codereview.chromium.org/1832473002/diff/20001/content/browser/rendere...
content/browser/renderer_host/database_message_filter.cc:310:
bad_message::DBMF_INVALID_ORIGIN_ON_OPEN);
On 2016/04/06 23:28:22, jsbell wrote:
> On 2016/04/06 20:55:14, michaeln wrote:
> > is there a way to mark DBMF_INVALID_ORIGIN_ON_OPEN as obsolete in the .xml
> file?
> 
> I went the other direction and reverted to emitting
DBMF_INVALID_ORIGIN_ON_OPEN
> here and adding distinct DBMFs everywhere.
> 
> Which is a question for the histograms reviewer - how much benefit do we get
out
> of these being distinct?

Well, I think the benefit only comes when there are problems.  Suppose you see a
spike in some Chrome release of the bundle DBMF_INVALID_ORIGIN?  Would you wish
that you had the distinct sub-categories as entries in order to help you
investigate?

[jsbell, 2016-04-07 21:53:15]

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

[jsbell, 2016-04-07 21:53:16]

One more OWNER review needed for content/ stuff - jam@ ?

For the future I added myself and michaeln as per-file OWNERS for some of the
websql files (that doesn't cover things like render_*_impl of course)

[jsbell, 2016-04-18 18:34:32]

jam@ - ping?

[jsbell, 2016-06-20 22:09:55]

jam@ - can you take a look?

(still not important, but it's been lingering for a few months now)

[jam, 2016-06-21 15:21:28]

lgtm, sorry I completely missed this cl (feel free to IM me if I don't respond
in a few hours)

[jsbell, 2016-06-21 15:22:12]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-06-21 15:22:13]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
michaeln@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1832473002/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-06-21 15:22:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1832473002/100001

[jsbell, 2016-06-21 15:22:55]

On 2016/06/21 15:21:28, jam wrote:
> sorry I completely missed this cl (feel free to IM me if I don't respond
> in a few hours)

No worries! I think you were OOO when I first uploaded. :)

[commit-bot: I haz the power, 2016-06-21 15:27:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-21 15:29:07]

Description was changed from

==========
WebSQL: Use url::Origin rather than database identifiers for IPC

Replace WebSecurityOrigin -> GURL -> string -> IPC -> string -> GURL with
simpler WebSecurityOrigin -> IPC -> Origin -> GURL. Consumers of the
messages still consume GURLs (Quota) or DatabaseIdentifiers (WebSQL) so
there's more tidying that can be done in follow-ups.

R=michaeln@chromium.org,mkwst@chromium.org
BUG=591482
==========

to

==========
WebSQL: Use url::Origin rather than database identifiers for IPC

Replace WebSecurityOrigin -> GURL -> string -> IPC -> string -> GURL with
simpler WebSecurityOrigin -> IPC -> Origin -> GURL. Consumers of the
messages still consume GURLs (Quota) or DatabaseIdentifiers (WebSQL) so
there's more tidying that can be done in follow-ups.

R=michaeln@chromium.org,mkwst@chromium.org
BUG=591482

Committed: https://crrev.com/984e21a28bb2e43ba6df6e28c8f0738ee6e23018
Cr-Commit-Position: refs/heads/master@{#400996}
==========

[commit-bot: I haz the power, 2016-06-21 15:29:09]

Patchset 6 (id:??) landed as
https://crrev.com/984e21a28bb2e43ba6df6e28c8f0738ee6e23018
Cr-Commit-Position: refs/heads/master@{#400996}