Convert 'CookieOptions::first_party_url' to a 'url::Origin'.

Convert 'CookieOptions::first_party_url' to a 'url::Origin'.

We don't need a full URL for any check we're doing inside //net, so let's
throw away the bits we don't need, and make the origin check explicit
(rather than matching the URLs that result from 'GURL::GetOrigin()').

BUG=459154
Committed: https://crrev.com/8241a123220e71b3733df642e2e56e1962be99a9
Cr-Commit-Position: refs/heads/master@{#354999}

[Mike West, 2015-10-16 13:34:19]

mkwst@chromium.org changed reviewers:
+ mmenke@chromium.org

[Mike West, 2015-10-16 13:34:20]

Matt, mind taking a look at this, since you're looking at cookie things anyway?
Should be no behavioral change, just flipping one type out for another, more
explicit type.

[Mike West, 2015-10-16 13:34:27]

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

[commit-bot: I haz the power, 2015-10-16 13:34:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413623002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413623002/1

[commit-bot: I haz the power, 2015-10-16 14:45:59]

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

[commit-bot: I haz the power, 2015-10-16 14:45:59]

Dry run: This issue passed the CQ dry run.

[mmenke, 2015-10-16 17:29:26]

https://codereview.chromium.org/1413623002/diff/1/net/cookies/canonical_cooki...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1413623002/diff/1/net/cookies/canonical_cooki...
net/cookies/canonical_cookie.cc:398: if (IsFirstPartyOnly() &&
!options.include_first_party_only() &&
Wait....We're more restrictive when options.include_first_party_only is not set
than when it is set?

https://codereview.chromium.org/1413623002/diff/1/net/cookies/canonical_cooki...
net/cookies/canonical_cookie.cc:399:
!options.first_party().IsSameOriginWith(url::Origin(url.GetOrigin()))) {
I don't think GetOrigin() is needed here?  It's not used in
options.set_first_party.

https://codereview.chromium.org/1413623002/diff/1/net/cookies/cookie_options.h
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1413623002/diff/1/net/cookies/cookie_options....
net/cookies/cookie_options.h:37: url::Origin first_party() const { return
first_party_; }
while you're here...const url::Origin&?

[Mike West, 2015-10-16 18:57:08]

Thanks for taking a look!

Would you be alright stamping it on the assumption that I'll make those changes
Monday morning when I'm back at the office? Or would you like to see the result?

https://codereview.chromium.org/1413623002/diff/1/net/cookies/canonical_cooki...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1413623002/diff/1/net/cookies/canonical_cooki...
net/cookies/canonical_cookie.cc:398: if (IsFirstPartyOnly() &&
!options.include_first_party_only() &&
On 2015/10/16 at 17:29:26, mmenke wrote:
> Wait....We're more restrictive when options.include_first_party_only is not
set than when it is set?

No? If `include_first_party_only` is set, we skip out of this `if`, which means
we don't return `false`, which means the cookie is included. Too many negations,
I know... :)

https://codereview.chromium.org/1413623002/diff/1/net/cookies/canonical_cooki...
net/cookies/canonical_cookie.cc:399:
!options.first_party().IsSameOriginWith(url::Origin(url.GetOrigin()))) {
On 2015/10/16 at 17:29:26, mmenke wrote:
> I don't think GetOrigin() is needed here?  It's not used in
options.set_first_party.

Yeah. I should have dropped it.

https://codereview.chromium.org/1413623002/diff/1/net/cookies/cookie_options.h
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1413623002/diff/1/net/cookies/cookie_options....
net/cookies/cookie_options.h:37: url::Origin first_party() const { return
first_party_; }
On 2015/10/16 at 17:29:26, mmenke wrote:
> while you're here...const url::Origin&?

Makes sense.

[mmenke, 2015-10-16 19:02:09]

Think change LGTM (With the two changes you said you'd make), but the existing
code around include_first_party_only confuses me.

https://codereview.chromium.org/1413623002/diff/1/net/cookies/canonical_cooki...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1413623002/diff/1/net/cookies/canonical_cooki...
net/cookies/canonical_cookie.cc:398: if (IsFirstPartyOnly() &&
!options.include_first_party_only() &&
On 2015/10/16 18:57:08, Mike West (slow) wrote:
> On 2015/10/16 at 17:29:26, mmenke wrote:
> > Wait....We're more restrictive when options.include_first_party_only is not
> set than when it is set?
> 
> No? If `include_first_party_only` is set, we skip out of this `if`, which
means
> we don't return `false`, which means the cookie is included. Too many
negations,
> I know... :)

Right...So "include_first_party_only()" is set, we return more in another
case...  So we're less restrictive when it's true, which seems counter
intuitive.

[Mike West, 2015-10-19 07:17:12]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-10-19 07:17:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1413623002/#ps20001
(title: "mmenke.")

[commit-bot: I haz the power, 2015-10-19 07:17:21]

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

[commit-bot: I haz the power, 2015-10-19 07:18:50]

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

[commit-bot: I haz the power, 2015-10-19 07:18:50]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mike West, 2015-10-19 07:21:21]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-10-19 07:21:22]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1413623002/#ps40001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-10-19 07:21:30]

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

[commit-bot: I haz the power, 2015-10-19 08:50:21]

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

[commit-bot: I haz the power, 2015-10-19 08:50:22]

Failed to apply patch for net/cookies/cookie_options.h:
While running git apply --index -3 -p1;
  error: patch failed: net/cookies/cookie_options.h:32
  Falling back to three-way merge...
  Applied patch to 'net/cookies/cookie_options.h' with conflicts.
  U net/cookies/cookie_options.h

Patch:       net/cookies/cookie_options.h
Index: net/cookies/cookie_options.h
diff --git a/net/cookies/cookie_options.h b/net/cookies/cookie_options.h
index
9b408861c6e8ee81d7462514a21619a990598fb4..1162c96f1d2d036a65e9e6f548ea1697e345e15d
100644
--- a/net/cookies/cookie_options.h
+++ b/net/cookies/cookie_options.h
@@ -10,6 +10,7 @@
 #include "base/time/time.h"
 #include "net/base/net_export.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 
 namespace net {
 
@@ -32,8 +33,8 @@ class NET_EXPORT CookieOptions {
   void set_include_first_party_only() { include_first_party_only_ = true; }
   bool include_first_party_only() const { return include_first_party_only_; }
 
-  void set_first_party_url(const GURL& url) { first_party_url_ = url; }
-  GURL first_party_url() const { return first_party_url_; }
+  void set_first_party(const url::Origin& origin) { first_party_ = origin; }
+  const url::Origin& first_party() const { return first_party_; }
 
   // TODO(estark): Remove once we decide whether to ship cookie
   // prefixes. https://crbug.com/541511
@@ -52,7 +53,7 @@ class NET_EXPORT CookieOptions {
  private:
   bool exclude_httponly_;
   bool include_first_party_only_;
-  GURL first_party_url_;
+  url::Origin first_party_;
   bool enforce_prefixes_;
   base::Time server_time_;
 };

[commit-bot: I haz the power, 2015-10-19 08:50:53]

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

[Mike West, 2015-10-20 07:08:57]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-10-20 07:09:55]

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

[commit-bot: I haz the power, 2015-10-20 07:15:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-20 07:16:26]

Patchset 3 (id:??) landed as
https://crrev.com/8241a123220e71b3733df642e2e56e1962be99a9
Cr-Commit-Position: refs/heads/master@{#354999}