Move referrer policy to a different field when serializing.

Move referrer policy to a different field when serializing.

Also try to fix old entries when possible and play nice with older
versions

BUG=450589
R=mkwst@chromium.org,marja@chromium.org,atwilson@chromium.org

Committed: https://crrev.com/31d74bdc6a4b3c9e766ae8fc9297f1db8fedff49
Cr-Commit-Position: refs/heads/master@{#313742}

[jochen (gone - plz use gerrit), 2015-01-29 10:31:51]

ptal

Mike, overall
Marja, sessions owner
Drew, sync owner

[marja, 2015-01-29 10:54:20]

https://codereview.chromium.org/869613006/diff/1/components/sessions/ios/ios_...
File components/sessions/ios/ios_serialized_navigation_driver.cc (right):

https://codereview.chromium.org/869613006/diff/1/components/sessions/ios/ios_...
components/sessions/ios/ios_serialized_navigation_driver.cc:35: bool
ContentSerializedNavigationDriver::MapReferrerPolicyToOldValues(
Why does this code have to be in both drivers - is it not possible to have it as
a helper func in one place? Afaics, it's a virtual func with 2 implementations
which are exactly the same.

https://codereview.chromium.org/869613006/diff/1/components/sessions/ios/ios_...
components/sessions/ios/ios_serialized_navigation_driver.cc:108: case
web::ReferrerPolicyOriginWhenCrossOrigin:
Why no default case anymore?

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
components/sessions/serialized_navigation_entry.cc:290: referrer_url_ = GURL();
Why do we want to do this also in the case where we do have a correct referrer
policy too?

[jochen (gone - plz use gerrit), 2015-01-29 10:59:31]

https://codereview.chromium.org/869613006/diff/1/components/sessions/ios/ios_...
File components/sessions/ios/ios_serialized_navigation_driver.cc (right):

https://codereview.chromium.org/869613006/diff/1/components/sessions/ios/ios_...
components/sessions/ios/ios_serialized_navigation_driver.cc:35: bool
ContentSerializedNavigationDriver::MapReferrerPolicyToOldValues(
On 2015/01/29 at 10:54:20, marja wrote:
> Why does this code have to be in both drivers - is it not possible to have it
as a helper func in one place? Afaics, it's a virtual func with 2
implementations which are exactly the same.

it's not!

One uses web::ReferrerPolicy*, the other uses blink::WebReferrerPolicy*

https://codereview.chromium.org/869613006/diff/1/components/sessions/ios/ios_...
components/sessions/ios/ios_serialized_navigation_driver.cc:108: case
web::ReferrerPolicyOriginWhenCrossOrigin:
On 2015/01/29 at 10:54:20, marja wrote:
> Why no default case anymore?

so the compiler catches the case when an enum value is missing. I added the
NOTREACHED in lines 86-89 back

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
components/sessions/serialized_navigation_entry.cc:290: referrer_url_ = GURL();
On 2015/01/29 at 10:54:20, marja wrote:
> Why do we want to do this also in the case where we do have a correct referrer
policy too?

good point. moved this code block down.

[Andrew T Wilson (Slow), 2015-01-29 11:02:13]

https://codereview.chromium.org/869613006/diff/20001/components/sessions/cont...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/869613006/diff/20001/components/sessions/cont...
components/sessions/content/content_serialized_navigation_driver.cc:40: if
(referrer_policy == blink::WebReferrerPolicyAlways ||
This is fine, but I wonder if a case statement would've been easier to read/more
succinct.

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
components/sessions/serialized_navigation_entry.cc:39: if
(sync_data.has_correct_referrer_policy()) {
Does this mean that if you have an old client and you modify the referrer
policy, the change won't take effect on newer clients because they are only
looking at "correct_referrer_policy" now?

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
components/sessions/serialized_navigation_entry.cc:227: referrer_policy_,
&mapped_referrer_policy)) {
Fine as-is, but I wonder if the following is easier to read:

if (SerializedNavigationDriver::Get()->Map...() && referrer_url_.is_valid()) {
  WriteStringToPickle(pickle, &bytes_written, max_size, referrer_url_.spec());
} else {
  WriteStringToPickle(pickle, &bytes_written, max_size, std::string());
}

https://codereview.chromium.org/869613006/diff/20001/sync/protocol/session_sp...
File sync/protocol/session_specifics.proto (right):

https://codereview.chromium.org/869613006/diff/20001/sync/protocol/session_sp...
sync/protocol/session_specifics.proto:139: optional int32 referrer_policy = 21
[default = 1];
Should this be named obsolete_referrer_policy?

[Mike West, 2015-01-29 11:53:32]

This LGTM (and I'd agree with atwilson@'s "obsolete_" suggestion), thanks for
poking at it (and sorry for breaking it in the first place).

https://codereview.chromium.org/869613006/diff/40001/components/sessions/cont...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/869613006/diff/40001/components/sessions/cont...
components/sessions/content/content_serialized_navigation_driver.cc:48:
*mapped_referrer_policy = 3;
Nit: Magic numbers => :(

[jochen (gone - plz use gerrit), 2015-01-29 12:04:44]

ptal

all comments addressed

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/869613006/diff/20001/components/sessions/seri...
components/sessions/serialized_navigation_entry.cc:39: if
(sync_data.has_correct_referrer_policy()) {
The entries aren't modified by clients.

[Mike West, 2015-01-29 12:06:35]

Still LGTM.

https://codereview.chromium.org/869613006/diff/60001/components/sessions/cont...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/869613006/diff/60001/components/sessions/cont...
components/sessions/content/content_serialized_navigation_driver.cc:18: const
int kObsoleteReferrerPolicyOrigin = 3;
\o/

[marja, 2015-01-29 12:09:28]

components/sessions lgtm

[Andrew T Wilson (Slow), 2015-01-29 17:36:14]

atwilson@chromium.org changed reviewers:
+ zea@google.com

[Andrew T Wilson (Slow), 2015-01-29 17:36:15]

LGTM

+zea FYI

[jochen (gone - plz use gerrit), 2015-01-29 17:49:33]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2015-01-29 17:50:22]

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

[commit-bot: I haz the power, 2015-01-29 18:33:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-29 18:35:46]

Patchset 5 (id:??) landed as
https://crrev.com/31d74bdc6a4b3c9e766ae8fc9297f1db8fedff49
Cr-Commit-Position: refs/heads/master@{#313742}