Initial implementation of Cookie service.

Initial implementation of Cookie service.

BUG=721395

[Randy Smith (Not in Mondays), 2017-06-17 19:48:45]

rdsmith@chromium.org changed reviewers:
+ yzshen@chromium.org

[Randy Smith (Not in Mondays), 2017-06-17 19:48:46]

Yuzhu: Would you be willing to take a look at this CL and give me initial
feedback?  I suspect I've got a lot of things wrong (around naming conventions
and location in the tree) and I figure getting feedback on those early is better
than doing a lot of work and then having to go larger global replaces.

Remember that my intent is for this to not be part of the network service until
we're actually using the network service in shipping code--instead, I'm going to
put a cookie interface pointer in a central location (probably StoragePartition)
and then convert the consumers over to it incrementally.

Thanks much in advance!

[yzshen1, 2017-06-20 18:33:17]

In general I think naming and file locations look good.

I only have some nits. Thanks!

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookie_s...
File content/common/cookie_service_impl.cc (right):

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookie_s...
content/common/cookie_service_impl.cc:19:
PredicateWrapper(content::mojom::CookieDeletionFilterPtr filter)
explicit?

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookie_s...
File content/common/cookie_service_impl.h (right):

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookie_s...
content/common/cookie_service_impl.h:5: #ifndef CONTENT_COMMON_COOKIE_SERVICE_H_
Please match the file path exactly.

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookie_s...
content/common/cookie_service_impl.h:33: // Associated a cookie request pointer
with this object.  Mojo messages
It would be nice to use "bind...to" instead of "associate...with" because
"associated interface" has special meaning.

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies....
File content/common/cookies.mojom (right):

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies....
content/common/cookies.mojom:33: // TOOD(rdsmith): Defaults from constructor or
wrapping function?
The default values are set by the constructor.
(Please note that this only affects the standard generated type. It has no
control over custom types if you do typemapping.)

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies....
content/common/cookies.mojom:81: // If all booleans are false (i.e there is no
filter) then
This comment may need to be updated, it doesn't have booleans, or
|delete_created|after|, |delete_blacklist|.

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies....
content/common/cookies.mojom:95: array<string>? domain_whitelist;
I am a little confused: do you mean *not* deleting cookies whose domains are in
this list? Otherwise it is the same as the domain_blacklist, right?

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies....
content/common/cookies.mojom:126: DeleteCookies(CookieDeletionFilter filter) =>
(int32 num_deleted);
nit: the result could be an unsigned number, say, uint32.

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies....
content/common/cookies.mojom:136: CookieChangeNotification&
notification_interface) => ();
nit: please comment on why an empty callback is necessary. (I know you have
explained the reason to me, but it is not obvious and could be useful for future
readers.)

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies_...
File content/common/cookies_traits.cc (right):

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies_...
content/common/cookies_traits.cc:29: *output =
net::CookiePriority::COOKIE_PRIORITY_LOW;
We need to return true for each case?

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies_...
File content/common/cookies_traits.h (right):

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies_...
content/common/cookies_traits.h:55: static content::mojom::CookieSameSiteFilter
cookie_same_site_filter(
You could directly return net::CookieOptions::SameSiteCookieMode, the
serialization code will automatically use the EnumTraits it can find to do the
conversion. (Here and some other enum getters below)

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies_...
content/common/cookies_traits.h:66: return base::Optional<base::Time>();
nit: you could use base::nullopt

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies_...
content/common/cookies_traits.h:88: static std::string name(const
net::CanonicalCookie& c) { return c.Name(); }
It is more efficient to return const& in this case (here and below).

Getters for structs/unions/strings/arrays/maps are called twice during
serialization.

Returning a const ref will remove two copies of the string.

https://codereview.chromium.org/2908443002/diff/20001/content/common/cookies_...
content/common/cookies_traits.h:94: static base::Time creation(const
net::CanonicalCookie& c) {
Ditto

[Randy Smith (Not in Mondays), 2017-06-27 17:11:04]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 17:11:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 17:21:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 17:21:30]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Randy Smith (Not in Mondays), 2017-06-27 17:41:40]

Yuzhu: Thanks very much for the comments so far!  

Before integrating them, I'm trying to merge this CL up to the top of tree, and
in doing some I'm getting some compilation problems I wanted to consult with you
on.  Does the below make sense to you, and is the right solution switching over
to initializing fields in the .mojom file with the C++ types mapped to?  If so,
how will that work for compilations of those .mojom files into, e.g.,
JavaScript?

https://codereview.chromium.org/2908443002/diff/40001/content/common/cookies....
File content/common/cookies.mojom (right):

https://codereview.chromium.org/2908443002/diff/40001/content/common/cookies....
content/common/cookies.mojom:35: CookieSameSiteFilter cookie_same_site_filter =
DO_NOT_INCLUDE;
I'm getting a compilation error in the generated file at the code corresponding
to this site:

gen/content/common/cookies.mojom.cc:35:7: error: cannot initialize a member
subobject of type 'net::CookieOptions::SameSiteCookieMode' with an rvalue of
type 'content::mojom::CookieSameSiteFilter'
      cookie_same_site_filter(CookieSameSiteFilter::DO_NOT_INCLUDE),
      ^                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I assume this is based on the typemap I setup between CookieSameSiteFilter and
net::CookieOptions::SameSiteCookieMode.  However, it still seems a bit weird to
me, since I'd think the .mojom file would be self-consistent, and the typemap
would control how values were mapped to the outside world.   So before I went
through and converted all the default values over to the values they're
typemapped from I wanted to confirm that that's intended best practice rather
than a .mojom -> .cc compilation glitch?

[yzshen1, 2017-06-28 07:00:37]

https://codereview.chromium.org/2908443002/diff/40001/content/common/cookies....
File content/common/cookies.mojom (right):

https://codereview.chromium.org/2908443002/diff/40001/content/common/cookies....
content/common/cookies.mojom:35: CookieSameSiteFilter cookie_same_site_filter =
DO_NOT_INCLUDE;
On 2017/06/27 17:41:40, Randy Smith (Not in Mondays) wrote:
> I'm getting a compilation error in the generated file at the code
corresponding
> to this site:
> 
> gen/content/common/cookies.mojom.cc:35:7: error: cannot initialize a member
> subobject of type 'net::CookieOptions::SameSiteCookieMode' with an rvalue of
> type 'content::mojom::CookieSameSiteFilter'
>       cookie_same_site_filter(CookieSameSiteFilter::DO_NOT_INCLUDE),
>       ^                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I assume this is based on the typemap I setup between CookieSameSiteFilter and
> net::CookieOptions::SameSiteCookieMode.  However, it still seems a bit weird
to
> me, since I'd think the .mojom file would be self-consistent, and the typemap
> would control how values were mapped to the outside world.   So before I went
> through and converted all the default values over to the values they're
> typemapped from I wanted to confirm that that's intended best practice rather
> than a .mojom -> .cc compilation glitch?

Sorry about this. I don't think we have carefully looked at the default value
initialization logic to make sure it works with mapped types. In this case we
should be able to use EnumTraits to map the value.

I could fix that when I get back to the office on Thursday. WDYT?

[Randy Smith (Not in Mondays), 2017-06-28 12:42:44]

https://codereview.chromium.org/2908443002/diff/40001/content/common/cookies....
File content/common/cookies.mojom (right):

https://codereview.chromium.org/2908443002/diff/40001/content/common/cookies....
content/common/cookies.mojom:35: CookieSameSiteFilter cookie_same_site_filter =
DO_NOT_INCLUDE;
On 2017/06/28 07:00:37, yzshen1 wrote:
> On 2017/06/27 17:41:40, Randy Smith (Not in Mondays) wrote:
> > I'm getting a compilation error in the generated file at the code
> corresponding
> > to this site:
> > 
> > gen/content/common/cookies.mojom.cc:35:7: error: cannot initialize a member
> > subobject of type 'net::CookieOptions::SameSiteCookieMode' with an rvalue of
> > type 'content::mojom::CookieSameSiteFilter'
> >       cookie_same_site_filter(CookieSameSiteFilter::DO_NOT_INCLUDE),
> >       ^                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > I assume this is based on the typemap I setup between CookieSameSiteFilter
and
> > net::CookieOptions::SameSiteCookieMode.  However, it still seems a bit weird
> to
> > me, since I'd think the .mojom file would be self-consistent, and the
typemap
> > would control how values were mapped to the outside world.   So before I
went
> > through and converted all the default values over to the values they're
> > typemapped from I wanted to confirm that that's intended best practice
rather
> > than a .mojom -> .cc compilation glitch?
> 
> Sorry about this. I don't think we have carefully looked at the default value
> initialization logic to make sure it works with mapped types. In this case we
> should be able to use EnumTraits to map the value.
> 
> I could fix that when I get back to the office on Thursday. WDYT?

Sure; I can find a workaround, or just work on my next CL until then.  Thanks!

[Randy Smith (Not in Mondays), 2017-07-02 01:49:01]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-02 01:49:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-02 01:59:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-02 01:59:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Randy Smith (Not in Mondays), 2017-07-04 02:01:39]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 02:01:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...