Add the navigation redirect-chain to Sync sessions proto for offline analysis.

Add a navigation redirect-chain to Sync sessions proto for offline analysis.

Google design doc at https://docs.google.com/a/google.com/document/d/1bh7aqvCMYzkobVA2MDuqWbQgymhdhdoqaOfvcLOaAYk

BUG=310373
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265515

[Donn Denman, 2014-01-15 22:35:25]

Reviewers, please take a look at the section that you're an owner of.  

Charlie, please look at navigation_controller_impl if you don't mind.  

Haitao, please review the entire CL.  

Thanks!

[Charlie Reis, 2014-01-17 23:21:31]

On 2014/01/15 22:35:25, Donn Denman wrote:
> Charlie, please look at navigation_controller_impl if you don't mind.  

I'm on parental leave, but Brett should be able to review anything in
NavigationController.

Thanks,
Charlie

[haitaol1, 2014-01-17 23:52:22]

https://codereview.chromium.org/101573003/diff/590001/components/sessions/ser...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/101573003/diff/590001/components/sessions/ser...
components/sessions/serialized_navigation_entry.cc:151: // The redirect chain
does not need to be synced down.
I think the comment should be on redirect_chain_ saying it's only filled for
local session.

https://codereview.chromium.org/101573003/diff/590001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/590001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:819:
GetMergedRedirectChain(params, active_entry->GetRedirectChain()));
I think previous version just overwrote entry's chain with params' chain and
seems working fine. I'm just curious in what case merging is needed. Is there a
risk that merging can lead to mixing of redirect chains of different
navigations.

https://codereview.chromium.org/101573003/diff/590001/sync/protocol/session_s...
File sync/protocol/session_specifics.proto (right):

https://codereview.chromium.org/101573003/diff/590001/sync/protocol/session_s...
sync/protocol/session_specifics.proto:145: optional string
last_navigation_redirect_url = 23;
Can you confirm that this could happen, i.e. virtual_url is different from last
url in redirect chain?

[Donn Denman, 2014-01-18 01:30:19]

https://codereview.chromium.org/101573003/diff/590001/components/sessions/ser...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/101573003/diff/590001/components/sessions/ser...
components/sessions/serialized_navigation_entry.cc:151: // The redirect chain
does not need to be synced down.
On 2014/01/17 23:52:23, haitaol1 wrote:
> I think the comment should be on redirect_chain_ saying it's only filled for
> local session.

Done.

https://codereview.chromium.org/101573003/diff/590001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/590001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:819:
GetMergedRedirectChain(params, active_entry->GetRedirectChain()));
On 2014/01/17 23:52:23, haitaol1 wrote:
> I think previous version just overwrote entry's chain with params' chain and
> seems working fine. I'm just curious in what case merging is needed. Is there
a
> risk that merging can lead to mixing of redirect chains of different
> navigations.

Actually this seems to happen quite frequently.  One example is case I put in
the design doc of voice.google.com.  I just tested this, and I see an active
entry of length 2 and params of length 2 that need to be merged to get the
correct 3-entry chain.

Frankly I don't know if there's a risk of mixing redirect chains from different
navigations, do you?

https://codereview.chromium.org/101573003/diff/590001/sync/protocol/session_s...
File sync/protocol/session_specifics.proto (right):

https://codereview.chromium.org/101573003/diff/590001/sync/protocol/session_s...
sync/protocol/session_specifics.proto:145: optional string
last_navigation_redirect_url = 23;
On 2014/01/17 23:52:23, haitaol1 wrote:
> Can you confirm that this could happen, i.e. virtual_url is different from
last
> url in redirect chain?
I have only seen this for urls like the new tab page.  I don't know if it would
every happen with redirection, but I was thinking it would be good to capture
the data in the case where it is different, so we would know.  Alternatively, we
could just remove this field.

[haitaol1, 2014-01-23 23:19:43]

sync LGTM. sorry for the delay

https://codereview.chromium.org/101573003/diff/590001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/590001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:819:
GetMergedRedirectChain(params, active_entry->GetRedirectChain()));
I don't know either. It would be better if you can confirm with Brett. But I
won't hold off you on this.

On 2014/01/18 01:30:19, Donn Denman wrote:
> On 2014/01/17 23:52:23, haitaol1 wrote:
> > I think previous version just overwrote entry's chain with params' chain and
> > seems working fine. I'm just curious in what case merging is needed. Is
there
> a
> > risk that merging can lead to mixing of redirect chains of different
> > navigations.
> 
> Actually this seems to happen quite frequently.  One example is case I put in
> the design doc of http://voice.google.com.  I just tested this, and I see an
active
> entry of length 2 and params of length 2 that need to be merged to get the
> correct 3-entry chain.
> 
> Frankly I don't know if there's a risk of mixing redirect chains from
different
> navigations, do you?

[sky, 2014-01-24 00:41:28]

Have you done any analysis of the effects of this on session and tab
state? I suspect this will seriously inflate the size needed to save
and restore tabs, thereby making both slower.

  -Scott

On Wed, Jan 15, 2014 at 2:35 PM,  <donnd@chromium.org> wrote:
> Reviewers: haitaol1, brettw, sky, Charlie Reis (OOO),
>
> Message:
> Reviewers, please take a look at the section that you're an owner of.
>
> Charlie, please look at navigation_controller_impl if you don't mind.
>
> Haitao, please review the entire CL.
>
> Thanks!
>
>
> Description:
> Add a navigation redirect-chain to Sync sessions proto for offline analysis.
>
> Google design doc at
>
https://docs.google.com/a/google.com/document/d/1bh7aqvCMYzkobVA2MDuqWbQgymhd...
>
> BUG=310373
>
> Please review this at https://codereview.chromium.org/101573003/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+332, -22 lines):
>   M android_webview/native/state_serializer.cc
>   M android_webview/native/state_serializer_unittests.cc
>   M components/sessions/serialized_navigation_entry.h
>   M components/sessions/serialized_navigation_entry.cc
>   M components/sessions/serialized_navigation_entry_unittest.cc
>   M content/browser/frame_host/navigation_controller_impl.h
>   M content/browser/frame_host/navigation_controller_impl.cc
>   M content/browser/frame_host/navigation_controller_impl_unittest.cc
>   M content/browser/frame_host/navigation_entry_impl.h
>   M content/browser/frame_host/navigation_entry_impl.cc
>   M content/browser/frame_host/navigator_impl.cc
>   M content/browser/web_contents/web_contents_impl.cc
>   M content/public/browser/navigation_entry.h
>   M content/public/common/page_transition_types.h
>   M content/public/common/page_transition_types.cc
>   M sync/protocol/proto_value_conversions.h
>   M sync/protocol/proto_value_conversions.cc
>   M sync/protocol/session_specifics.proto
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[donnd, 2014-01-24 01:34:39]

No, we have not done any analysis of the impact on storage or performance.
 Haitao, have you done any of this kind of analysis before?

FWIW, we've tried to minimize the size of the data being sent, so tabs for
non-redirecting URLs should be almost unchanged.  However, redirecting URLs
are fairly common and some redirect chains are long, so you may be right
about the impact.  Over time, having this data should help Chrome provide
the final destination of a redirect in Suggest, so overall performance
could improve, but that's all just my speculation.

I had hoped to avoid pickling/unpickling this data, which would have helped
with tab save/restore.  However we were unable to validate that this
wouldn't cause problems when recording to Chrome History when it overwrites
entries, so we added it to the pickle.


On Thu, Jan 23, 2014 at 4:41 PM, Scott Violet <sky@chromium.org> wrote:

> Have you done any analysis of the effects of this on session and tab
> state? I suspect this will seriously inflate the size needed to save
> and restore tabs, thereby making both slower.
>
>   -Scott
>
> On Wed, Jan 15, 2014 at 2:35 PM,  <donnd@chromium.org> wrote:
> > Reviewers: haitaol1, brettw, sky, Charlie Reis (OOO),
> >
> > Message:
> > Reviewers, please take a look at the section that you're an owner of.
> >
> > Charlie, please look at navigation_controller_impl if you don't mind.
> >
> > Haitao, please review the entire CL.
> >
> > Thanks!
> >
> >
> > Description:
> > Add a navigation redirect-chain to Sync sessions proto for offline
> analysis.
> >
> > Google design doc at
> >
>
https://docs.google.com/a/google.com/document/d/1bh7aqvCMYzkobVA2MDuqWbQgymhd...
> >
> > BUG=310373
> >
> > Please review this at https://codereview.chromium.org/101573003/
> >
> > SVN Base: https://chromium.googlesource.com/chromium/src.git@master
> >
> > Affected files (+332, -22 lines):
> >   M android_webview/native/state_serializer.cc
> >   M android_webview/native/state_serializer_unittests.cc
> >   M components/sessions/serialized_navigation_entry.h
> >   M components/sessions/serialized_navigation_entry.cc
> >   M components/sessions/serialized_navigation_entry_unittest.cc
> >   M content/browser/frame_host/navigation_controller_impl.h
> >   M content/browser/frame_host/navigation_controller_impl.cc
> >   M content/browser/frame_host/navigation_controller_impl_unittest.cc
> >   M content/browser/frame_host/navigation_entry_impl.h
> >   M content/browser/frame_host/navigation_entry_impl.cc
> >   M content/browser/frame_host/navigator_impl.cc
> >   M content/browser/web_contents/web_contents_impl.cc
> >   M content/public/browser/navigation_entry.h
> >   M content/public/common/page_transition_types.h
> >   M content/public/common/page_transition_types.cc
> >   M sync/protocol/proto_value_conversions.h
> >   M sync/protocol/proto_value_conversions.cc
> >   M sync/protocol/session_specifics.proto
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-01-24 16:32:22]

Before this lands some analysis needs to be done on potential impact.
I recommend tracking how the proposed changes would impact file size
in a histogram and then doing some analysis based on that. Also
remember that parts of this run on the android size where size changes
have a bigger impact on performance.

  -Scott

On Thu, Jan 23, 2014 at 5:34 PM, Donn Denman <donnd@google.com> wrote:
> No, we have not done any analysis of the impact on storage or performance.
> Haitao, have you done any of this kind of analysis before?
>
> FWIW, we've tried to minimize the size of the data being sent, so tabs for
> non-redirecting URLs should be almost unchanged.  However, redirecting URLs
> are fairly common and some redirect chains are long, so you may be right
> about the impact.  Over time, having this data should help Chrome provide
> the final destination of a redirect in Suggest, so overall performance could
> improve, but that's all just my speculation.
>
> I had hoped to avoid pickling/unpickling this data, which would have helped
> with tab save/restore.  However we were unable to validate that this
> wouldn't cause problems when recording to Chrome History when it overwrites
> entries, so we added it to the pickle.
>
>
> On Thu, Jan 23, 2014 at 4:41 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> Have you done any analysis of the effects of this on session and tab
>> state? I suspect this will seriously inflate the size needed to save
>> and restore tabs, thereby making both slower.
>>
>>   -Scott
>>
>> On Wed, Jan 15, 2014 at 2:35 PM,  <donnd@chromium.org> wrote:
>> > Reviewers: haitaol1, brettw, sky, Charlie Reis (OOO),
>> >
>> > Message:
>> > Reviewers, please take a look at the section that you're an owner of.
>> >
>> > Charlie, please look at navigation_controller_impl if you don't mind.
>> >
>> > Haitao, please review the entire CL.
>> >
>> > Thanks!
>> >
>> >
>> > Description:
>> > Add a navigation redirect-chain to Sync sessions proto for offline
>> > analysis.
>> >
>> > Google design doc at
>> >
>> >
https://docs.google.com/a/google.com/document/d/1bh7aqvCMYzkobVA2MDuqWbQgymhd...
>> >
>> > BUG=310373
>> >
>> > Please review this at https://codereview.chromium.org/101573003/
>> >
>> > SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>> >
>> > Affected files (+332, -22 lines):
>> >   M android_webview/native/state_serializer.cc
>> >   M android_webview/native/state_serializer_unittests.cc
>> >   M components/sessions/serialized_navigation_entry.h
>> >   M components/sessions/serialized_navigation_entry.cc
>> >   M components/sessions/serialized_navigation_entry_unittest.cc
>> >   M content/browser/frame_host/navigation_controller_impl.h
>> >   M content/browser/frame_host/navigation_controller_impl.cc
>> >   M content/browser/frame_host/navigation_controller_impl_unittest.cc
>> >   M content/browser/frame_host/navigation_entry_impl.h
>> >   M content/browser/frame_host/navigation_entry_impl.cc
>> >   M content/browser/frame_host/navigator_impl.cc
>> >   M content/browser/web_contents/web_contents_impl.cc
>> >   M content/public/browser/navigation_entry.h
>> >   M content/public/common/page_transition_types.h
>> >   M content/public/common/page_transition_types.cc
>> >   M sync/protocol/proto_value_conversions.h
>> >   M sync/protocol/proto_value_conversions.cc
>> >   M sync/protocol/session_specifics.proto
>> >
>> >
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Donn Denman, 2014-02-05 22:53:27]

Haitao and Scott, I removed the pickling, so hopefully that addresses the size
issue.  Please take another look.

[sky, 2014-02-06 17:49:15]

Can the sync side query the history db for the redirect chain when it needs it?

[sky, 2014-02-06 17:49:35]

I'm basically wondering if there is a way to avoid storing the redirect chain
for all navigations.

[Donn Denman, 2014-02-06 19:41:42]

On 2014/02/06 17:49:15, sky wrote:
> Can the sync side query the history db for the redirect chain when it needs
it?

I'm not sure if I understand your question, so maybe there's a misunderstanding
about what we're trying to do: build up a redirect chain to send to the history
db for offline analysis.  We don't plan to ever send redirect data back from the
sync server to the client.

[sky, 2014-02-06 22:32:08]

Then I am confused. Can't the history db build the redirect chain
itself. We do to figure out most visited.

  -Scott

On Thu, Feb 6, 2014 at 11:41 AM,  <donnd@chromium.org> wrote:
> On 2014/02/06 17:49:15, sky wrote:
>>
>> Can the sync side query the history db for the redirect chain when it
>> needs
>
> it?
>
> I'm not sure if I understand your question, so maybe there's a
> misunderstanding
> about what we're trying to do: build up a redirect chain to send to the
> history
> db for offline analysis.  We don't plan to ever send redirect data back from
> the
> sync server to the client.
>
> https://codereview.chromium.org/101573003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[donnd, 2014-02-07 01:56:30]

I don't think the history db has the intermediate redirects.  Maybe there
are some simple cases where it can infer a redirect from the referrer, or
in the case of client redirects.  I'm actually very new to chrome
navigation, so maybe someone with more experience can chime in, but here's
my understanding: For a single navigation in a tab only the referrer and
final URLs were captured so we needed to make changes to capture the
intermediates that redirect, and that chain can be quite long.  In the
buganizer issue <https://b.corp.google.com/issue?id=7327692> requesting
this data there are a lot of chrome history clients with use cases like
webspam<https://b.corp.google.com/issue?id=7327692&query=7327692&gsessionid=etI_VhaFIkQ#ISSUE_HistoryHeader90>that
are asking for the whole redirect chain, so it seems that the need is
legitimate.


On Thu, Feb 6, 2014 at 2:31 PM, Scott Violet <sky@chromium.org> wrote:

> Then I am confused. Can't the history db build the redirect chain
> itself. We do to figure out most visited.
>
>   -Scott
>
> On Thu, Feb 6, 2014 at 11:41 AM,  <donnd@chromium.org> wrote:
> > On 2014/02/06 17:49:15, sky wrote:
> >>
> >> Can the sync side query the history db for the redirect chain when it
> >> needs
> >
> > it?
> >
> > I'm not sure if I understand your question, so maybe there's a
> > misunderstanding
> > about what we're trying to do: build up a redirect chain to send to the
> > history
> > db for offline analysis.  We don't plan to ever send redirect data back
> from
> > the
> > sync server to the client.
> >
> > https://codereview.chromium.org/101573003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-02-07 16:43:02]

All the redirects should be added. The code that does this is here:
https://cs.corp.google.com/#aosp-ics/external/chromium/chrome/browser/history...
. If there is some case that it it missing let me know. Might the
issue be that sync isn't pulling all the visits, only a subset?

  -Scott

On Thu, Feb 6, 2014 at 5:56 PM, Donn Denman <donnd@google.com> wrote:
> I don't think the history db has the intermediate redirects.  Maybe there
> are some simple cases where it can infer a redirect from the referrer, or in
> the case of client redirects.  I'm actually very new to chrome navigation,
> so maybe someone with more experience can chime in, but here's my
> understanding: For a single navigation in a tab only the referrer and final
> URLs were captured so we needed to make changes to capture the intermediates
> that redirect, and that chain can be quite long.  In the buganizer issue
> requesting this data there are a lot of chrome history clients with use
> cases like webspam that are asking for the whole redirect chain, so it seems
> that the need is legitimate.
>
>
> On Thu, Feb 6, 2014 at 2:31 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> Then I am confused. Can't the history db build the redirect chain
>> itself. We do to figure out most visited.
>>
>>   -Scott
>>
>> On Thu, Feb 6, 2014 at 11:41 AM,  <donnd@chromium.org> wrote:
>> > On 2014/02/06 17:49:15, sky wrote:
>> >>
>> >> Can the sync side query the history db for the redirect chain when it
>> >> needs
>> >
>> > it?
>> >
>> > I'm not sure if I understand your question, so maybe there's a
>> > misunderstanding
>> > about what we're trying to do: build up a redirect chain to send to the
>> > history
>> > db for offline analysis.  We don't plan to ever send redirect data back
>> > from
>> > the
>> > sync server to the client.
>> >
>> > https://codereview.chromium.org/101573003/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[donnd, 2014-02-07 17:37:19]

Haitao, do you know anything about this?


On Fri, Feb 7, 2014 at 8:42 AM, Scott Violet <sky@chromium.org> wrote:

> All the redirects should be added. The code that does this is here:
>
>
https://cs.corp.google.com/#aosp-ics/external/chromium/chrome/browser/history...
> . If there is some case that it it missing let me know. Might the
> issue be that sync isn't pulling all the visits, only a subset?
>
>   -Scott
>
> On Thu, Feb 6, 2014 at 5:56 PM, Donn Denman <donnd@google.com> wrote:
> > I don't think the history db has the intermediate redirects.  Maybe there
> > are some simple cases where it can infer a redirect from the referrer,
> or in
> > the case of client redirects.  I'm actually very new to chrome
> navigation,
> > so maybe someone with more experience can chime in, but here's my
> > understanding: For a single navigation in a tab only the referrer and
> final
> > URLs were captured so we needed to make changes to capture the
> intermediates
> > that redirect, and that chain can be quite long.  In the buganizer issue
> > requesting this data there are a lot of chrome history clients with use
> > cases like webspam that are asking for the whole redirect chain, so it
> seems
> > that the need is legitimate.
> >
> >
> > On Thu, Feb 6, 2014 at 2:31 PM, Scott Violet <sky@chromium.org> wrote:
> >>
> >> Then I am confused. Can't the history db build the redirect chain
> >> itself. We do to figure out most visited.
> >>
> >>   -Scott
> >>
> >> On Thu, Feb 6, 2014 at 11:41 AM,  <donnd@chromium.org> wrote:
> >> > On 2014/02/06 17:49:15, sky wrote:
> >> >>
> >> >> Can the sync side query the history db for the redirect chain when it
> >> >> needs
> >> >
> >> > it?
> >> >
> >> > I'm not sure if I understand your question, so maybe there's a
> >> > misunderstanding
> >> > about what we're trying to do: build up a redirect chain to send to
> the
> >> > history
> >> > db for offline analysis.  We don't plan to ever send redirect data
> back
> >> > from
> >> > the
> >> > sync server to the client.
> >> >
> >> > https://codereview.chromium.org/101573003/
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haitaol1, 2014-02-07 19:10:08]

On 2014/02/07 17:37:19, donnd wrote:
> Haitao, do you know anything about this?
> 
> 
> On Fri, Feb 7, 2014 at 8:42 AM, Scott Violet <mailto:sky@chromium.org> wrote:
> 
> > All the redirects should be added. The code that does this is here:
> >
> >
>
https://cs.corp.google.com/#aosp-ics/external/chromium/chrome/browser/history...
> > . If there is some case that it it missing let me know. Might the
> > issue be that sync isn't pulling all the visits, only a subset?
> >
> >   -Scott
> >
> > On Thu, Feb 6, 2014 at 5:56 PM, Donn Denman <mailto:donnd@google.com> wrote:
> > > I don't think the history db has the intermediate redirects.  Maybe there
> > > are some simple cases where it can infer a redirect from the referrer,
> > or in
> > > the case of client redirects.  I'm actually very new to chrome
> > navigation,
> > > so maybe someone with more experience can chime in, but here's my
> > > understanding: For a single navigation in a tab only the referrer and
> > final
> > > URLs were captured so we needed to make changes to capture the
> > intermediates
> > > that redirect, and that chain can be quite long.  In the buganizer issue
> > > requesting this data there are a lot of chrome history clients with use
> > > cases like webspam that are asking for the whole redirect chain, so it
> > seems
> > > that the need is legitimate.
> > >
> > >
> > > On Thu, Feb 6, 2014 at 2:31 PM, Scott Violet <mailto:sky@chromium.org>
wrote:
> > >>
> > >> Then I am confused. Can't the history db build the redirect chain
> > >> itself. We do to figure out most visited.
> > >>
> > >>   -Scott
> > >>
> > >> On Thu, Feb 6, 2014 at 11:41 AM,  <mailto:donnd@chromium.org> wrote:
> > >> > On 2014/02/06 17:49:15, sky wrote:
> > >> >>
> > >> >> Can the sync side query the history db for the redirect chain when it
> > >> >> needs
> > >> >
> > >> > it?
> > >> >
> > >> > I'm not sure if I understand your question, so maybe there's a
> > >> > misunderstanding
> > >> > about what we're trying to do: build up a redirect chain to send to
> > the
> > >> > history
> > >> > db for offline analysis.  We don't plan to ever send redirect data
> > back
> > >> > from
> > >> > the
> > >> > sync server to the client.
> > >> >
> > >> > https://codereview.chromium.org/101573003/
> > >
> > >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

As session sync implemented today, it would be inefficient to query history db
for redirects because when a tab changes, we throw all previous data out and
recreate sync data for current active navigation and 6 navigations before/after
the active one. So that's 13 queries in total. Also history API only returns
redirects for the latest visit to a URL. So if there are duplicates in those 13
urls or urls on other tabs, result will be incomplete/incorrect.

[donnd, 2014-02-07 19:33:25]

Haitao, thanks for clarifying, I didn't know about this.  Scott, sorry
about my confusion regarding the history db!


On Fri, Feb 7, 2014 at 11:10 AM, <haitaol@chromium.org> wrote:

> On 2014/02/07 17:37:19, donnd wrote:
>
>> Haitao, do you know anything about this?
>>
>
>
>  On Fri, Feb 7, 2014 at 8:42 AM, Scott Violet <mailto:sky@chromium.org>
>> wrote:
>>
>
>  > All the redirects should be added. The code that does this is here:
>> >
>> >
>>
>
> https://cs.corp.google.com/#aosp-ics/external/chromium/
> chrome/browser/history/history_backend.cc&q=historybackend::addpage&l=367
>
>> > . If there is some case that it it missing let me know. Might the
>> > issue be that sync isn't pulling all the visits, only a subset?
>> >
>> >   -Scott
>> >
>> > On Thu, Feb 6, 2014 at 5:56 PM, Donn Denman <mailto:donnd@google.com>
>> wrote:
>> > > I don't think the history db has the intermediate redirects.  Maybe
>> there
>> > > are some simple cases where it can infer a redirect from the referrer,
>> > or in
>> > > the case of client redirects.  I'm actually very new to chrome
>> > navigation,
>> > > so maybe someone with more experience can chime in, but here's my
>> > > understanding: For a single navigation in a tab only the referrer and
>> > final
>> > > URLs were captured so we needed to make changes to capture the
>> > intermediates
>> > > that redirect, and that chain can be quite long.  In the buganizer
>> issue
>> > > requesting this data there are a lot of chrome history clients with
>> use
>> > > cases like webspam that are asking for the whole redirect chain, so it
>> > seems
>> > > that the need is legitimate.
>> > >
>> > >
>> > > On Thu, Feb 6, 2014 at 2:31 PM, Scott Violet <mailto:sky@chromium.org
>> >
>>
> wrote:
>
>> > >>
>> > >> Then I am confused. Can't the history db build the redirect chain
>> > >> itself. We do to figure out most visited.
>> > >>
>> > >>   -Scott
>> > >>
>> > >> On Thu, Feb 6, 2014 at 11:41 AM,  <mailto:donnd@chromium.org> wrote:
>> > >> > On 2014/02/06 17:49:15, sky wrote:
>> > >> >>
>> > >> >> Can the sync side query the history db for the redirect chain
>> when it
>> > >> >> needs
>> > >> >
>> > >> > it?
>> > >> >
>> > >> > I'm not sure if I understand your question, so maybe there's a
>> > >> > misunderstanding
>> > >> > about what we're trying to do: build up a redirect chain to send to
>> > the
>> > >> > history
>> > >> > db for offline analysis.  We don't plan to ever send redirect data
>> > back
>> > >> > from
>> > >> > the
>> > >> > sync server to the client.
>> > >> >
>> > >> > https://codereview.chromium.org/101573003/
>> > >
>> > >
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> As session sync implemented today, it would be inefficient to query
> history db
> for redirects because when a tab changes, we throw all previous data out
> and
> recreate sync data for current active navigation and 6 navigations
> before/after
> the active one. So that's 13 queries in total. Also history API only
> returns
> redirects for the latest visit to a URL. So if there are duplicates in
> those 13
> urls or urls on other tabs, result will be incomplete/incorrect.
>
> https://codereview.chromium.org/101573003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-02-07 21:25:41]

So, where does that leave the changes you've been doing?

  -Scott

On Fri, Feb 7, 2014 at 11:33 AM, Donn Denman <donnd@google.com> wrote:
> Haitao, thanks for clarifying, I didn't know about this.  Scott, sorry about
> my confusion regarding the history db!
>
>
> On Fri, Feb 7, 2014 at 11:10 AM, <haitaol@chromium.org> wrote:
>>
>> On 2014/02/07 17:37:19, donnd wrote:
>>>
>>> Haitao, do you know anything about this?
>>
>>
>>
>>> On Fri, Feb 7, 2014 at 8:42 AM, Scott Violet <mailto:sky@chromium.org>
>>> wrote:
>>
>>
>>> > All the redirects should be added. The code that does this is here:
>>> >
>>> >
>>
>>
>>
>>
https://cs.corp.google.com/#aosp-ics/external/chromium/chrome/browser/history...
>>>
>>> > . If there is some case that it it missing let me know. Might the
>>> > issue be that sync isn't pulling all the visits, only a subset?
>>> >
>>> >   -Scott
>>> >
>>> > On Thu, Feb 6, 2014 at 5:56 PM, Donn Denman <mailto:donnd@google.com>
>>> > wrote:
>>> > > I don't think the history db has the intermediate redirects.  Maybe
>>> > > there
>>> > > are some simple cases where it can infer a redirect from the
>>> > > referrer,
>>> > or in
>>> > > the case of client redirects.  I'm actually very new to chrome
>>> > navigation,
>>> > > so maybe someone with more experience can chime in, but here's my
>>> > > understanding: For a single navigation in a tab only the referrer and
>>> > final
>>> > > URLs were captured so we needed to make changes to capture the
>>> > intermediates
>>> > > that redirect, and that chain can be quite long.  In the buganizer
>>> > > issue
>>> > > requesting this data there are a lot of chrome history clients with
>>> > > use
>>> > > cases like webspam that are asking for the whole redirect chain, so
>>> > > it
>>> > seems
>>> > > that the need is legitimate.
>>> > >
>>> > >
>>> > > On Thu, Feb 6, 2014 at 2:31 PM, Scott Violet
>>> > > <mailto:sky@chromium.org>
>>
>> wrote:
>>>
>>> > >>
>>> > >> Then I am confused. Can't the history db build the redirect chain
>>> > >> itself. We do to figure out most visited.
>>> > >>
>>> > >>   -Scott
>>> > >>
>>> > >> On Thu, Feb 6, 2014 at 11:41 AM,  <mailto:donnd@chromium.org> wrote:
>>> > >> > On 2014/02/06 17:49:15, sky wrote:
>>> > >> >>
>>> > >> >> Can the sync side query the history db for the redirect chain
>>> > >> >> when it
>>> > >> >> needs
>>> > >> >
>>> > >> > it?
>>> > >> >
>>> > >> > I'm not sure if I understand your question, so maybe there's a
>>> > >> > misunderstanding
>>> > >> > about what we're trying to do: build up a redirect chain to send
>>> > >> > to
>>> > the
>>> > >> > history
>>> > >> > db for offline analysis.  We don't plan to ever send redirect data
>>> > back
>>> > >> > from
>>> > >> > the
>>> > >> > sync server to the client.
>>> > >> >
>>> > >> > https://codereview.chromium.org/101573003/
>>> > >
>>> > >
>>> >
>>
>>
>>> To unsubscribe from this group and stop receiving emails from it, send an
>>
>> email
>>>
>>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>>
>> As session sync implemented today, it would be inefficient to query
>> history db
>> for redirects because when a tab changes, we throw all previous data out
>> and
>> recreate sync data for current active navigation and 6 navigations
>> before/after
>> the active one. So that's 13 queries in total. Also history API only
>> returns
>> redirects for the latest visit to a URL. So if there are duplicates in
>> those 13
>> urls or urls on other tabs, result will be incomplete/incorrect.
>>
>> https://codereview.chromium.org/101573003/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[donnd, 2014-02-09 00:48:08]

Scott, Frankly when you mentioned the "history db" I thought you were
talking about the Chrome History Kansas bigtable.  Evidently the full
redirects are stored in an internal history db?  I'm sitting in your
building now so maybe we could easily have a quick meeting soon, and loop
in Haitao, to talk about whether some other architecture makes more sense?


On Fri, Feb 7, 2014 at 1:25 PM, Scott Violet <sky@chromium.org> wrote:

> So, where does that leave the changes you've been doing?
>
>   -Scott
>
> On Fri, Feb 7, 2014 at 11:33 AM, Donn Denman <donnd@google.com> wrote:
> > Haitao, thanks for clarifying, I didn't know about this.  Scott, sorry
> about
> > my confusion regarding the history db!
> >
> >
> > On Fri, Feb 7, 2014 at 11:10 AM, <haitaol@chromium.org> wrote:
> >>
> >> On 2014/02/07 17:37:19, donnd wrote:
> >>>
> >>> Haitao, do you know anything about this?
> >>
> >>
> >>
> >>> On Fri, Feb 7, 2014 at 8:42 AM, Scott Violet <mailto:sky@chromium.org>
> >>> wrote:
> >>
> >>
> >>> > All the redirects should be added. The code that does this is here:
> >>> >
> >>> >
> >>
> >>
> >>
> >>
>
https://cs.corp.google.com/#aosp-ics/external/chromium/chrome/browser/history...
> >>>
> >>> > . If there is some case that it it missing let me know. Might the
> >>> > issue be that sync isn't pulling all the visits, only a subset?
> >>> >
> >>> >   -Scott
> >>> >
> >>> > On Thu, Feb 6, 2014 at 5:56 PM, Donn Denman <mailto:donnd@google.com
> >
> >>> > wrote:
> >>> > > I don't think the history db has the intermediate redirects.  Maybe
> >>> > > there
> >>> > > are some simple cases where it can infer a redirect from the
> >>> > > referrer,
> >>> > or in
> >>> > > the case of client redirects.  I'm actually very new to chrome
> >>> > navigation,
> >>> > > so maybe someone with more experience can chime in, but here's my
> >>> > > understanding: For a single navigation in a tab only the referrer
> and
> >>> > final
> >>> > > URLs were captured so we needed to make changes to capture the
> >>> > intermediates
> >>> > > that redirect, and that chain can be quite long.  In the buganizer
> >>> > > issue
> >>> > > requesting this data there are a lot of chrome history clients with
> >>> > > use
> >>> > > cases like webspam that are asking for the whole redirect chain, so
> >>> > > it
> >>> > seems
> >>> > > that the need is legitimate.
> >>> > >
> >>> > >
> >>> > > On Thu, Feb 6, 2014 at 2:31 PM, Scott Violet
> >>> > > <mailto:sky@chromium.org>
> >>
> >> wrote:
> >>>
> >>> > >>
> >>> > >> Then I am confused. Can't the history db build the redirect chain
> >>> > >> itself. We do to figure out most visited.
> >>> > >>
> >>> > >>   -Scott
> >>> > >>
> >>> > >> On Thu, Feb 6, 2014 at 11:41 AM,  <mailto:donnd@chromium.org>
> wrote:
> >>> > >> > On 2014/02/06 17:49:15, sky wrote:
> >>> > >> >>
> >>> > >> >> Can the sync side query the history db for the redirect chain
> >>> > >> >> when it
> >>> > >> >> needs
> >>> > >> >
> >>> > >> > it?
> >>> > >> >
> >>> > >> > I'm not sure if I understand your question, so maybe there's a
> >>> > >> > misunderstanding
> >>> > >> > about what we're trying to do: build up a redirect chain to send
> >>> > >> > to
> >>> > the
> >>> > >> > history
> >>> > >> > db for offline analysis.  We don't plan to ever send redirect
> data
> >>> > back
> >>> > >> > from
> >>> > >> > the
> >>> > >> > sync server to the client.
> >>> > >> >
> >>> > >> > https://codereview.chromium.org/101573003/
> >>> > >
> >>> > >
> >>> >
> >>
> >>
> >>> To unsubscribe from this group and stop receiving emails from it, send
> an
> >>
> >> email
> >>>
> >>> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >>
> >> As session sync implemented today, it would be inefficient to query
> >> history db
> >> for redirects because when a tab changes, we throw all previous data out
> >> and
> >> recreate sync data for current active navigation and 6 navigations
> >> before/after
> >> the active one. So that's 13 queries in total. Also history API only
> >> returns
> >> redirects for the latest visit to a URL. So if there are duplicates in
> >> those 13
> >> urls or urls on other tabs, result will be incomplete/incorrect.
> >>
> >> https://codereview.chromium.org/101573003/
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Donn Denman, 2014-02-20 21:10:13]

Brett, Scott and Haitao, Please take another look at this change to add
Redirects to Sync.  The code is essentially unchanged since Scott last looked at
it.  Haitao has volunteered to add the histogram (and associated launch-blocking
bug) in a separate CL.

[sky, 2014-02-20 22:12:25]

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:482: // Copy all redirect
chain entries except the last URL.
Document why the last isn't copied.

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:483: for (size_t i = 0; i <
redirect_chain_.size(); i++) {
Why don't you have a loop for the first n-1 and then the last?

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:494:
sync_pb::NavigationRedirect *navigation_redirect =
NavigationRedirect*

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:496:
navigation_redirect->set_url(redirect_chain_.at(i).spec());
at->[]

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
File components/sessions/serialized_navigation_entry.h (right):

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.h:125: std::vector<GURL>
redirect_chain() const { return redirect_chain_; }
const std::vector<GURL>&

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
File components/sessions/serialized_navigation_entry_unittest.cc (right):

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry_unittest.cc:254: EXPECT_EQ(3U,
new_navigation_entry->GetRedirectChain().size());
This should be an ASSERT, since if it fails the following are likely to crash.
Same comment for other places you check array elements.

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry_unittest.cc:255:
EXPECT_EQ(kRedirectURL0, new_navigation_entry->GetRedirectChain().at(0));
at-> [] here and everywhere.

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:829:
active_entry->SetRedirectChain(
Seems like we should only need to do this for some types of navigations. For
example, auto-subframe doesn't seem like it should impact redirects.

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:873: } else if
(active_entry_redirects.size() == 0) {
no else after return (see style guide).

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:253: std::vector<GURL>
redirect_chain_;
This isn't really persisted via session restore like the rest of these fields,
so I would leave this field where it was with a comment.

[Donn Denman, 2014-02-21 08:18:55]

Scott, I think I have addressed all of your comments except the one about not
needing to call GetMergedRedirectChain for some types of navigations.  Any
suggestions you have are welcome.

Details: 
content/browser/frame_host/navigation_controller_impl.cc:829:
active_entry->SetRedirectChain(
On 2014/02/20 22:12:26, sky wrote:
> Seems like we should only need to do this for some types of navigations...

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:482: // Copy all redirect
chain entries except the last URL.
On 2014/02/20 22:12:26, sky wrote:
> Document why the last isn't copied.

Done.

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:483: for (size_t i = 0; i <
redirect_chain_.size(); i++) {
On 2014/02/20 22:12:26, sky wrote:
> Why don't you have a loop for the first n-1 and then the last?
Done.  I'm ashamed I didn't see that - it's much more readable now.

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:494:
sync_pb::NavigationRedirect *navigation_redirect =
On 2014/02/20 22:12:26, sky wrote:
> NavigationRedirect*

Done.

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:496:
navigation_redirect->set_url(redirect_chain_.at(i).spec());
On 2014/02/20 22:12:26, sky wrote:
> at->[]

Done.

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
File components/sessions/serialized_navigation_entry.h (right):

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry.h:125: std::vector<GURL>
redirect_chain() const { return redirect_chain_; }
On 2014/02/20 22:12:26, sky wrote:
> const std::vector<GURL>&

Done.

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
File components/sessions/serialized_navigation_entry_unittest.cc (right):

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry_unittest.cc:254: EXPECT_EQ(3U,
new_navigation_entry->GetRedirectChain().size());
On 2014/02/20 22:12:26, sky wrote:
> This should be an ASSERT, since if it fails the following are likely to crash.
> Same comment for other places you check array elements.

Done.

https://codereview.chromium.org/101573003/diff/1200001/components/sessions/se...
components/sessions/serialized_navigation_entry_unittest.cc:255:
EXPECT_EQ(kRedirectURL0, new_navigation_entry->GetRedirectChain().at(0));
On 2014/02/20 22:12:26, sky wrote:
> at-> [] here and everywhere.

Done.

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:829:
active_entry->SetRedirectChain(
On 2014/02/20 22:12:26, sky wrote:
> Seems like we should only need to do this for some types of navigations. For
> example, auto-subframe doesn't seem like it should impact redirects.
Agreed, but I don't feel I know how to enumerate those navigations, which is why
I flagged a TODO for myself below.  Maybe you or Brett could recommend someone
to review just this part of the CL so we can address this shortcoming?

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:873: } else if
(active_entry_redirects.size() == 0) {
On 2014/02/20 22:12:26, sky wrote:
> no else after return (see style guide).

Done, and also for the second "else" below.

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:253: std::vector<GURL>
redirect_chain_;
On 2014/02/20 22:12:26, sky wrote:
> This isn't really persisted via session restore like the rest of these fields,
> so I would leave this field where it was with a comment.

Done.

[sky, 2014-02-21 17:01:12]

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1200001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:829:
active_entry->SetRedirectChain(
On 2014/02/21 08:18:56, Donn Denman wrote:
> On 2014/02/20 22:12:26, sky wrote:
> > Seems like we should only need to do this for some types of navigations. For
> > example, auto-subframe doesn't seem like it should impact redirects.
> Agreed, but I don't feel I know how to enumerate those navigations, which is
why
> I flagged a TODO for myself below.  Maybe you or Brett could recommend someone
> to review just this part of the CL so we can address this shortcoming?

Brett knows this code better than I. Hopefully he can offer a suggestion.

https://codereview.chromium.org/101573003/diff/1320001/components/sessions/se...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/101573003/diff/1320001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:484: for (size_t i = 0; i <
redirect_chain_.size() - 1; i++) {
Be careful here. If the chain is empty you overflow in many places here.

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:857: // Produces a
redirect chain by merging the active entry redirects with the
Description belongs in header.

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:864: 
nit: no newline.

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:874: if
(active_entry_redirects.size() == 0) {
nit: no {}

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.h (right):

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.h:215: std::vector<GURL>
GetMergedRedirectChain(
Don't make this public.

[Donn Denman, 2014-02-25 19:23:13]

Scott, thanks for the catch!  PTAL.

https://codereview.chromium.org/101573003/diff/1320001/components/sessions/se...
File components/sessions/serialized_navigation_entry.cc (right):

https://codereview.chromium.org/101573003/diff/1320001/components/sessions/se...
components/sessions/serialized_navigation_entry.cc:484: for (size_t i = 0; i <
redirect_chain_.size() - 1; i++) {
On 2014/02/21 17:01:14, sky wrote:
> Be careful here. If the chain is empty you overflow in many places here.
Thanks for the warning.  PTAL.

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:857: // Produces a
redirect chain by merging the active entry redirects with the
On 2014/02/21 17:01:14, sky wrote:
> Description belongs in header.

Done.

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:864: 
On 2014/02/21 17:01:14, sky wrote:
> nit: no newline.

Done.

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:874: if
(active_entry_redirects.size() == 0) {
On 2014/02/21 17:01:14, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.h (right):

https://codereview.chromium.org/101573003/diff/1320001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.h:215: std::vector<GURL>
GetMergedRedirectChain(
On 2014/02/21 17:01:14, sky wrote:
> Don't make this public.

Done.

https://codereview.chromium.org/101573003/diff/1390001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/101573003/diff/1390001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:4031:
TEST_F(NavigationControllerTest, GetMergedRedirectChainNewPage) {
I noticed these test names were not following the convention so I renamed them.

[sky, 2014-02-25 22:42:14]

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:858: std::vector<GURL>
NavigationControllerImpl::GetMergedRedirectChain(
Seems like this should be only called in the switch statement when appropriate.
Brett will have to offer his advice on this though.

[haitaol1, 2014-02-27 22:30:21]

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:294: redirect_chain_ =
redirect_chain;
Donn: I found that even an empty GURL object takes almost 100 bytes because of
its internal parsed_url_ have multiple components each taking 8 bytes. Since you
only need URLs, can you just store a vector of URL strings?

[Donn Denman, 2014-03-03 19:42:11]

Brett, I think we're ready for you to do a quick first-pass on this CL now.  Do
you think you could take a quick look sometime this week?  I'm around and more
than happy to answer questions via IM, or otherwise help (e.g. rebase the CL,
etc).

It's really a very simple change, the only non-trivial part is the
navigation_controller_impl.cc change, which sky@ has some concerns about, and I
could use your help here.  Also, while you're looking, maybe you can give us
your opinion on Haitao's question about the footprint of GURL vs String for the
existing redirect_chain_ in navigation_entry_impl.cc.

Thanks!

[brettw, 2014-03-11 22:40:00]

https://codereview.chromium.org/101573003/diff/1410001/components/sessions/se...
File components/sessions/serialized_navigation_entry.h (right):

https://codereview.chromium.org/101573003/diff/1410001/components/sessions/se...
components/sessions/serialized_navigation_entry.h:160: std::vector<GURL>
redirect_chain_;  // Only filled for local sessions.
I don't know what a local session is in this context. Is this the same as "Not
persisted" on the previous line?

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:858: std::vector<GURL>
NavigationControllerImpl::GetMergedRedirectChain(
I'd prefer a function UpdateRedirectChain(params, active_entry) {
  if (PageTransitionCoreTypeIs(params.transition,
content::PAGE_TRANSITION_RELOAD))
    return;
  if (...FORWARDBACK))
     return;
  ...
}

In addition to that, I don't really understand the merging thing. It seems like
we'd always want to replace if we think we have a new list, and if we don't have
a new list, we always want to leave it as-is. If there is a need for this, there
should be a detailed comment here giving an example.

[Donn Denman, 2014-03-12 20:13:35]

Brett, thanks for the review!

PTAL

https://codereview.chromium.org/101573003/diff/1410001/components/sessions/se...
File components/sessions/serialized_navigation_entry.h (right):

https://codereview.chromium.org/101573003/diff/1410001/components/sessions/se...
components/sessions/serialized_navigation_entry.h:160: std::vector<GURL>
redirect_chain_;  // Only filled for local sessions.
On 2014/03/11 22:40:01, brettw wrote:
> I don't know what a local session is in this context. Is this the same as "Not
> persisted" on the previous line?
Yes, it means not persisted.  Done.

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:858: std::vector<GURL>
NavigationControllerImpl::GetMergedRedirectChain(
On 2014/03/11 22:40:01, brettw wrote:
> I'd prefer a function UpdateRedirectChain(params, active_entry) {
>   if (PageTransitionCoreTypeIs(params.transition,
> content::PAGE_TRANSITION_RELOAD))
>     return;
>   if (...FORWARDBACK))
>      return;
>   ...
> }
> 
> In addition to that, I don't really understand the merging thing. It seems
like
> we'd always want to replace if we think we have a new list, and if we don't
have
> a new list, we always want to leave it as-is. If there is a need for this,
there
> should be a detailed comment here giving an example.

Done.

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:858: std::vector<GURL>
NavigationControllerImpl::GetMergedRedirectChain(
On 2014/02/25 22:42:15, sky wrote:
> Seems like this should be only called in the switch statement when
appropriate.
> Brett will have to offer his advice on this though.
Applied Brett's suggestion.  Now that this function immediately returns when
appropriate, does that address your concern?

[Donn Denman, 2014-03-14 22:30:03]

Brett, PTAL at this simplified version.  Now we're just applying the redirect
chain from the frame load params, but it comes with a cost and a risk:
* Cost - now we don't splice client-side redirects to the current navigation (as
we discussed).
* Risk - we need to start with an empty redirect chain when we set up the frame
load params, instead of getting it from the Navigation Entry.  Please see my
comment on that line (below).

https://codereview.chromium.org/101573003/diff/1440001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1440001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:108: params->redirects =
std::vector<GURL>();
Please double-check that this makes sense: Before this CL the NavigationEntry
redirect chain was cleared in ResetForCommit(), so this vector was always empty.
 Now instead of trying to get one from the NavigationEntry we need to just start
with a fresh redirect chain.

[brettw, 2014-03-25 17:59:30]

lgtm

[Donn Denman, 2014-03-25 18:21:18]

Scott, PTAL at this simplified version.  The simplification should remove your
outstanding concern, but you may have other concerns regarding the
initialization of params->redirects done in navigator_impl.cc

Haitao, you may want to take another look too, since some things have changed
since you gave this LGTM.

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1410001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:294: redirect_chain_ =
redirect_chain;
On 2014/02/27 22:30:22, haitaol1 wrote:
> Donn: I found that even an empty GURL object takes almost 100 bytes because of
> its internal parsed_url_ have multiple components each taking 8 bytes. Since
you
> only need URLs, can you just store a vector of URL strings?
I don't think this is a problem, and doesn't seem worth changing.

[sky, 2014-03-25 19:36:07]

I'm good, LGTM

[Donn Denman, 2014-03-25 21:13:47]

The CQ bit was checked by donnd@chromium.org

[commit-bot: I haz the power, 2014-03-25 21:15:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/donnd@chromium.org/101573003/1460001

[commit-bot: I haz the power, 2014-03-25 21:33:09]

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

[commit-bot: I haz the power, 2014-03-25 21:33:10]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[Donn Denman, 2014-03-25 22:04:55]

The CQ bit was checked by donnd@chromium.org

[commit-bot: I haz the power, 2014-03-25 22:06:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/donnd@chromium.org/101573003/1670001

[commit-bot: I haz the power, 2014-03-25 22:58:38]

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

[commit-bot: I haz the power, 2014-03-25 22:58:39]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Donn Denman, 2014-03-25 23:04:24]

The CQ bit was checked by donnd@chromium.org

[commit-bot: I haz the power, 2014-03-25 23:04:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/donnd@chromium.org/101573003/1670001

[commit-bot: I haz the power, 2014-03-25 23:54:21]

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

[commit-bot: I haz the power, 2014-03-25 23:54:21]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[donnd, 2014-03-27 20:58:48]

Brett, 

Looks like a change to navigator_impl.cc fixed the problem.  Please take a look
at that change and let me know if you're still LGTM for me to commit this, or
have any questions.

BTW, Haitao is on vacation.

Thanks!

[brettw, 2014-03-27 22:16:21]

https://chromiumcodereview.appspot.com/101573003/diff/1850001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1850001/content/browser...
content/browser/frame_host/navigator_impl.cc:108: // We no longer clear
redirects in the navigation entry upon commit, so
This comment only makes sense in the context of this change. i.e. if somebody
reads this later the "we no longer" part makes no sense because they don't care
how it used to be. It should just say what happens now and why we need to do
this.

Also, the way you wrote "clear" them is confusing. You're not clearing anything
in this code, you're just not copying it in some case. I would expect from this
comment you're editing the navigation entry.

https://chromiumcodereview.appspot.com/101573003/diff/1850001/content/browser...
content/browser/frame_host/navigator_impl.cc:116: params->redirects =
std::vector<GURL>();
Actually, I'm curious why you need to not send the redirects? Were you just
trying to match the old behavior? If so, are we sure that was correct/best?

Its difficult for me to follow through what this is used for. It looks like the
renderer doesn't use this and just forwards it back to the browser in the
commit. And then it looks like maybe it just comes back to your new code in the
navigation controller where you do
active_entry->SetRedirectChain(params.redirects);

If that's true, it seems like we would want to preserve the redirects? I think
you should definitely trace through what happens with this.

[donnd, 2014-03-28 03:11:38]

Brett, PTAL.  Thanks!

https://chromiumcodereview.appspot.com/101573003/diff/1850001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1850001/content/browser...
content/browser/frame_host/navigator_impl.cc:108: // We no longer clear
redirects in the navigation entry upon commit, so
On 2014/03/27 22:16:22, brettw wrote:
> This comment only makes sense in the context of this change. i.e. if somebody
> reads this later the "we no longer" part makes no sense because they don't
care
> how it used to be. It should just say what happens now and why we need to do
> this.
> 
> Also, the way you wrote "clear" them is confusing. You're not clearing
anything
> in this code, you're just not copying it in some case. I would expect from
this
> comment you're editing the navigation entry.

I should have known better than to write a non-durable comment - shame on me,
and thanks for flagging that.

Does this look OK now?

https://chromiumcodereview.appspot.com/101573003/diff/1850001/content/browser...
content/browser/frame_host/navigator_impl.cc:116: params->redirects =
std::vector<GURL>();
On 2014/03/27 22:16:22, brettw wrote:
> Actually, I'm curious why you need to not send the redirects? Were you just
> trying to match the old behavior? If so, are we sure that was correct/best?
No, the old behavior was to preserve the redirects, and that's the default case
now too.

However, we need to prevent the chain from growing endlessly when returning to
previously committed navigations (e.g. when reloading the page).  The old
behavior cut off the chain at commit time, so that wasn't a problem that had to
be handled here.

> 
> Its difficult for me to follow through what this is used for. It looks like
the
> renderer doesn't use this and just forwards it back to the browser in the
> commit. And then it looks like maybe it just comes back to your new code in
the
> navigation controller where you do
> active_entry->SetRedirectChain(params.redirects);
> 
> If that's true, it seems like we would want to preserve the redirects? I think
> you should definitely trace through what happens with this.
That all sounds right to me, and we do preserve the redirects except in those
committed cases.

[brettw, 2014-03-28 21:15:33]

Sorry if I'm not 100% clear on this. Can you walk me through how the redirect
chain gets appended to if you do a back/forward? I thought we replaced it rather
than appended to it.

[donnd, 2014-03-28 23:53:03]

The problem with forward/back (and also with reload) is that the redirect
chain never gets reset from the original navigation.  So if you go to a
redirecting URL, go back, then forward again, that redirecting entry still
has all the redirect information.

My understanding is that whenever we navigate we append redirections to the
current navigation.  I think this is in WebKit, and I spent some time
trying to find exactly where -- maybe it's
here<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/fetch/Resource.cpp&l=346>.
 Although I'm not sure how the appending happens, it seems clear that a
forward/back or reload shouldn't have any redirects at all since we're just
navigating the final url.  This discarding of the redirect chain used to be
done in
ResetForCommit<https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/frame_host/navigation_entry_impl.cc&q=ResetForCommit%20redirect&sq=package:chromium&type=cs&l=338>but
that was always removing redirects, keeping them from being accessible
to Sync.  So these are the cases where we discard the previous redirect
chain now.

Does that help?

On Fri, Mar 28, 2014 at 2:15 PM, <brettw@chromium.org> wrote:

> Sorry if I'm not 100% clear on this. Can you walk me through how the
> redirect
> chain gets appended to if you do a back/forward? I thought we replaced it
> rather
> than appended to it.
>
> https://chromiumcodereview.appspot.com/101573003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Donn Denman, 2014-03-31 16:32:02]

Charlie, It would be great to get your review of this change, so PTAL.  It has a
long history so feel free to ping me if anything is unclear.  Thanks!

[Charlie Reis, 2014-03-31 22:06:07]

Besides exposing the redirect chain publicly, it seems like the biggest change
here is making it stick around after commit.  I don't see any immediate concerns
from that, though I guess there are some cases that arise (like the one you're
currently discussing with Brett).

No big objections here; just a few questions below.  Thanks for the design doc--
it helped bring me up to speed.

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1337:
EXPECT_EQ(0U, committed_entry->GetRedirectChain().size());
I thought you were preserving this after commit now?

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigation_entry_impl.h:311: // It is preserved in
|ResetForCommit| but should not be persisted.
nit: "is preserved after commit but should not be persisted."

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigator_impl.cc:112: bool
is_transition_forward_back = PageTransitionIsForwardBack(
Does PageTransitionIsRedirect not suffice here?

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/public/...
File content/public/common/page_transition_types.h (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/public/...
content/public/common/page_transition_types.h:42: CONTENT_EXPORT bool
PageTransitionIsForwardBack(PageTransition type);
Do we need this?  It seems like PageTransitionIsRedirect might be sufficient.

If not, something like PageTransitionIsNewNavigation might be semantically more
useful, rather than having all callers try to compute it using back/forward plus
reload.

[donnd, 2014-04-01 04:36:24]

Thanks for the review Charlie!

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1337:
EXPECT_EQ(0U, committed_entry->GetRedirectChain().size());
On 2014/03/31 22:06:08, Charlie Reis wrote:
> I thought you were preserving this after commit now?
This test includes both a ResetForCommit and also does a navigation.  Since we
now pick up the redirects exclusively from the navigation parameters those need
to be supplied for the data to show up at the end.  I added a test method to do
this, and another clause to this test.  

What do you think now?

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigation_entry_impl.h:311: // It is preserved in
|ResetForCommit| but should not be persisted.
On 2014/03/31 22:06:08, Charlie Reis wrote:
> nit: "is preserved after commit but should not be persisted."

Done.

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigator_impl.cc:112: bool
is_transition_forward_back = PageTransitionIsForwardBack(
On 2014/03/31 22:06:08, Charlie Reis wrote:
> Does PageTransitionIsRedirect not suffice here?
No, returning to any previously committed navigation will pick up those
redirects, but we don't want to send them to sync as if we just did the redirect
all over again.

[brettw, 2014-04-01 06:28:51]

not lgtm until we can figure out the redirect thing. Maybe Charlie has a
different opinion than mine about what the contents of the redirect chain should
be for back-forwarded entries?

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigator_impl.cc:112: bool
is_transition_forward_back = PageTransitionIsForwardBack(
On 2014/04/01 04:36:26, donnd wrote:
> On 2014/03/31 22:06:08, Charlie Reis wrote:
> > Does PageTransitionIsRedirect not suffice here?
> No, returning to any previously committed navigation will pick up those
> redirects, but we don't want to send them to sync as if we just did the
redirect
> all over again.

Ah, so this answer seems to be different than we were talking about on Friday. I
had asked you why you needed to clear here and you thought the list was getting
appended to, but weren't sure where.

In this comment, it seems like you're saying the redirects are preserved from
the original navigation "properly" when doing back-forward, but you don't want
them?

Is my summary correct?

If so, why don't we want these in sync? I don't think the contents of what's in
the navigation entry should be driven by what's desired by sync (although in
this case I question the desire) bur rather what makes sense to have in a
navigation entry. And I would expect the redirects to be there.

[donnd, 2014-04-01 18:31:09]

Brett, Hopefully this is a better explanation of what this change does.  Let me
know what you think.

Thanks!

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigator_impl.cc:112: bool
is_transition_forward_back = PageTransitionIsForwardBack(
On 2014/04/01 06:28:52, brettw wrote:
> On 2014/04/01 04:36:26, donnd wrote:
> > On 2014/03/31 22:06:08, Charlie Reis wrote:
> > > Does PageTransitionIsRedirect not suffice here?
> > No, returning to any previously committed navigation will pick up those
> > redirects, but we don't want to send them to sync as if we just did the
> redirect
> > all over again.
> 
> Ah, so this answer seems to be different than we were talking about on Friday.
I
> had asked you why you needed to clear here and you thought the list was
getting
> appended to, but weren't sure where.
> 
> In this comment, it seems like you're saying the redirects are preserved from
> the original navigation "properly" when doing back-forward, but you don't want
> them?
> 
> Is my summary correct?
No, they need to be preserved in the original navigation but should not be
passed on to the new navigation on back-forward.
> 
> If so, why don't we want these in sync? I don't think the contents of what's
in
> the navigation entry should be driven by what's desired by sync (although in
> this case I question the desire) bur rather what makes sense to have in a
> navigation entry. And I would expect the redirects to be there.

Sorry I have not been clear in communicating what happens in these cases.  When
I said "appended to" on Friday I meant that the previous redirects would be
preserved instead of cleared so subsequent navigations append to them.  

I think an example will help: When navigating to A, which redirects to B and
then C, we get a redirect chain ABC.  If the user does a reload or forward or
back, the redirect chain should just be C.  However, in this CL we're preserving
the redirect chain, so it will be appended to, and we'll see ABCC, which is
wrong.  Starting with an empty chain here fixes those cases.  

BTW, when I said we wouldn't want to send that to sync I just meant that as an
example of sending the wrong data to any part of the system.  We're just trying
to send a snapshot of the redirect chain to sync.

https://chromiumcodereview.appspot.com/101573003/diff/1890001/content/browser...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1890001/content/browser...
content/browser/frame_host/navigation_controller_impl.cc:708:
entry->SetRedirectChain(params.redirect_chain);
We set up the redirect chain here so it will be available to sync.  However we
don't want the Navigator to use this chain for new navigations or we'll end up
appending to it.

[brettw, 2014-04-01 20:28:21]

Then that gets me back to my previous question, who appends the extra redirect?
When you do the clear, is the result A->B->C or only C?

My suspicion is that we send the "previous redirects" to a renderer when we have
it navigate to preserve this across cross-site navigations, and when we're doing
a reload with a redirect chain we send the whole thing, and then renderer
appends the new navigation (I think we will tell it to load the last URL in the
chain) and it gets duplicated.

I think we should either verify that my guess is correct or find the real cause
of the extra append. It really doesn't seem that clearing the list when doing a
back-forward is correct. Your code only overwrites the redirect list.

[donnd, 2014-04-01 20:51:47]

On 2014/04/01 20:28:21, brettw wrote:
> Then that gets me back to my previous question, who appends the extra
redirect?
> When you do the clear, is the result A->B->C or only C?
Only C.

> 
> My suspicion is that we send the "previous redirects" to a renderer when we
have
> it navigate to preserve this across cross-site navigations, and when we're
doing
> a reload with a redirect chain we send the whole thing, and then renderer
> appends the new navigation (I think we will tell it to load the last URL in
the
> chain) and it gets duplicated.
That sounds right to me.

> 
> I think we should either verify that my guess is correct or find the real
cause
> of the extra append. It really doesn't seem that clearing the list when doing
a
> back-forward is correct. Your code only overwrites the redirect list.
I don't understand why back-forward would be different from reload.  In both
cases we've preserved the redirect chain, but we want to start out with a fresh
chain because we're just going to C. (no pun intended).  Isn't going back to a
navigation like reloading that navigation?

[brettw, 2014-04-01 21:12:25]

Okay, I talked to John and I think its OK for the redirect chain to be empty in
the back/forward/reload case and only show C.

But that still doesn't explain why you need to clear it. When we tell the
renderer to navigate, we'll send it C to load. Then it should just send us back
C with no redirects. Who is appending to the redirect list.

[donnd, 2014-04-01 21:33:41]

On 2014/04/01 21:12:25, brettw wrote:
> Okay, I talked to John and I think its OK for the redirect chain to be empty
in
> the back/forward/reload case and only show C.
> 
> But that still doesn't explain why you need to clear it. When we tell the
> renderer to navigate, we'll send it C to load. Then it should just send us
back
> C with no redirects. Who is appending to the redirect list.

It's a consequence of my change to NavigationControllerImpl where I added this
line:
active_entry->SetRedirectChain(params.redirects);
In HEAD the active_entry redirects were never set here.  MakeNavigateParams
initializes the param redirects from the pending entry's redirects, and in HEAD
that was fine because they were empty.  But in this CL it will *start* with the
previous chain (and append to it) if we don't prevent that.

Does that explain it?

[brettw, 2014-04-02 22:43:25]

Where does the redirect chain get appended to? This is what I was asking before.
It looks like you only overwrite it in your code. Can you explain how we end up
appending the same redirects again?

[donnd, 2014-04-03 01:43:52]

On 2014/04/02 22:43:25, brettw wrote:
> Where does the redirect chain get appended to? This is what I was asking
before.
> It looks like you only overwrite it in your code. Can you explain how we end
up
> appending the same redirects again?
Sure!  I'm really sorry this has been so tedious.  I certainly appreciate your
help.  Please let me know if there's any way I can make this easier for you!

Short answer: The Navigator appends to it when it does the navigation. 
It happens right here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
The entry->redirect_chain() is typically empty (I verified this by adding an
assert and doing a bunch of navigation).  The assert never fired.
However, in this CL we "preserve" the redirects in a committed entry, so they
are not empty.  Since we're starting a navigation with a whole existing
navigation chain of ABC, we'll end up with ABCC instead of just C.

Long answer: I don't think the appending is really the key, it's that redirects
are no longer being cleared.  If we do a navigation that redirects to ABC, when
we reload or return to it via back / forward we're not revisiting A and B, so
having them in the chain would just be wrong. The new navigation after
reload/back/forward should just have C in the redirect chain. 
In fact I do see appending, but I shouldn't have mentioned that because it's not
the root of the problem, and only confuses the situation.  Sorry about that.
The key to the problem is that we start a navigation by using an existing
redirect chain instead of an empty one.  That's due to these two points:
1) In this CL, any time we use a redirect chain from a NavigationEntry it may
have a long redirect chain instead of being empty after a commit.  That's
because of two changes in this CL that happen as we flow through
RendererDidNavigate:
A) We set the redirects from the navigation with this new line I added in
RendererDidNavigate:
  active_entry->SetRedirectChain(params.redirects);
which is added right after this SetPageState call:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
B) We do not clear them in ResetForCommit because I removed this line:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
So redirect chains are put into navigation entries and they hang around after a
commit instead of being empty.
2) MakeNavigateParams uses the supplied NavigationEntry's redirects.
That's done here (as mentioned above):
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
Now those redirects contain the whole chain from the committed entry instead of
being empty.  So we start out with [ABC] instead of [] for the redirects.  Then
the navigation logic appends to that whole chain, so we end up with [ABCC]. 
Even if it didn't append we'd have [ABC] which is wrong, and is not what the
code currently does.

Additional considerations: I thought it would be sufficient to just always start
with an empty chain in MakeNavigateParams, but as you know a browser test
failed, and I suspect that there are real cases with non-committed entries that
rely on building off of an existing redirect chain.  So I conditionalized the
chain in the latest patch.  I don't like the fact that the conditional has to be
there -- it would be nice if all the logic was in one place (e.g.
RendererDidNavigate), but I don't see any way to make that work and still have
the code be simple.

Another option would be to not change the redirect chain in the NavigationEntry
at all.  Instead we could add a separate redirect chain to the
LoadCommittedDetails, and sync could pick that up.  However, it seems odd to me
that the NavigationEntry wouldn't just have it's redirects in it.  Yet another
option would be to put a copy of the NavigationEntry into LoadCommittedDetails
and change its redirect chain, but that seems messy and inefficient too.

My gut feeling is that this change is the simplest way to get the redirects to
sync.  It's also unlikely to break anything except for parts of the system that
use redirects.  I do think it's possible that it could break the MostLikely /
History stuff since that code uses redirects.  If you're worried about this I'm
happy to work to verify that it doesn't.  I also think that getting this CL
landed in this simple form will allow us to revise it with simpler changes later
if needed.  There are two or three possible revisions that I anticipate: 1) we
need to fix something we broke, 2) the redirects we supply don't connect client
redirects into a single chain and our WebHistory clients might have troubles
with that, 3) we'd like to add additional information from Blink giving details
on each redirect, which could be very useful.

I hope this helps.

[Charlie Reis, 2014-04-05 00:09:09]

I'm having trouble keeping up with the discussion about redirects being cleared,
so I'll mostly defer to Brett on that.  It sounds vaguely like there's a
conflict between two cases, though:
1) Cross-process redirects, which want the in-progress redirect chain added to
the params sent to the renderer.
2) Back/forward/reload, which don't want the old redirect chain added to the
params sent to the renderer.

Maybe there's a way to distinguish those cases.

A few unrelated comments below.

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1337:
EXPECT_EQ(0U, committed_entry->GetRedirectChain().size());
On 2014/04/01 04:36:26, Donn wrote:
> On 2014/03/31 22:06:08, Charlie Reis wrote:
> > I thought you were preserving this after commit now?
> This test includes both a ResetForCommit and also does a navigation.  Since we
> now pick up the redirects exclusively from the navigation parameters those
need
> to be supplied for the data to show up at the end.  I added a test method to
do
> this, and another clause to this test.  
> 
> What do you think now?

1) I don't think a check for GetRedirectChain belongs in this particular test
anymore, since this test is about what's cleared by ResetForCommit.

2) I'm still a bit puzzled how the redirect chain can be correct (non-empty)
while pending but then we completely forget it when the commit comes in.  I
guess we just let the commit message trump anything we've seen in the interim? 
(I guess that's just existing behavior, though, so I won't ask for it to be
changed here.)

https://chromiumcodereview.appspot.com/101573003/diff/1890001/content/browser...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1890001/content/browser...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1342:
main_test_rfh()->SendNavigateWithRedirects(0, url1, redirects);
This sounds like it should be in a test of its own, since it's the opposite of
what the rest of the test is doing.

https://chromiumcodereview.appspot.com/101573003/diff/1890001/content/public/...
File content/public/common/page_transition_types.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1890001/content/public/...
content/public/common/page_transition_types.cc:51: return (type &
PAGE_TRANSITION_FORWARD_BACK) != 0;
We generally don't expose new public functions until they're needed.  Let's just
put this clause inside PageTransitionIsNewNavigation and not add
PageTransitionIsForwardBack.

https://chromiumcodereview.appspot.com/101573003/diff/1890001/content/test/te...
File content/test/test_render_frame_host.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1890001/content/test/te...
content/test/test_render_frame_host.cc:67: url_copy,
Style nit: There's no need to put each function parameter on its own line if
they don't all fit on the same line.  It's fine in cases that improve
readability, but doing one-per-line is only required for function signatures,
not for call sites.

[donnd, 2014-04-08 21:38:25]

Charlie, PTAL, and thanks for the review.

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/101573003/diff/1870001/content/browser...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1337:
EXPECT_EQ(0U, committed_entry->GetRedirectChain().size());
On 2014/04/05 00:09:10, Charlie Reis wrote:
> On 2014/04/01 04:36:26, Donn wrote:
> > On 2014/03/31 22:06:08, Charlie Reis wrote:
> > > I thought you were preserving this after commit now?
> > This test includes both a ResetForCommit and also does a navigation.  Since
we
> > now pick up the redirects exclusively from the navigation parameters those
> need
> > to be supplied for the data to show up at the end.  I added a test method to
> do
> > this, and another clause to this test.  
> > 
> > What do you think now?
> 
> 1) I don't think a check for GetRedirectChain belongs in this particular test
> anymore, since this test is about what's cleared by ResetForCommit.
> 
> 2) I'm still a bit puzzled how the redirect chain can be correct (non-empty)
> while pending but then we completely forget it when the commit comes in.  I
> guess we just let the commit message trump anything we've seen in the interim?

> (I guess that's just existing behavior, though, so I won't ask for it to be
> changed here.)

Made a separate test that the redirect chain is preserved.

Yes, I think the existing behavior to clear the redirects on commit was just the
simplest way to prevent it from being used in subsequent navigations.  Since we
need to preserve the chain for sync, we've moved that logic into
MakeNavigateParams.

[Donn Denman, 2014-04-09 21:09:20]

Charlie, for some reason my comments didn't get included in my last message --
sorry about the confusion there.

https://codereview.chromium.org/101573003/diff/1890001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/101573003/diff/1890001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1342:
main_test_rfh()->SendNavigateWithRedirects(0, url1, redirects);
On 2014/04/05 00:09:10, Charlie Reis wrote:
> This sounds like it should be in a test of its own, since it's the opposite of
> what the rest of the test is doing.

Done.

https://codereview.chromium.org/101573003/diff/1890001/content/public/common/...
File content/public/common/page_transition_types.cc (right):

https://codereview.chromium.org/101573003/diff/1890001/content/public/common/...
content/public/common/page_transition_types.cc:51: return (type &
PAGE_TRANSITION_FORWARD_BACK) != 0;
On 2014/04/05 00:09:10, Charlie Reis wrote:
> We generally don't expose new public functions until they're needed.  Let's
just
> put this clause inside PageTransitionIsNewNavigation and not add
> PageTransitionIsForwardBack.

Done.

https://codereview.chromium.org/101573003/diff/1890001/content/test/test_rend...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/101573003/diff/1890001/content/test/test_rend...
content/test/test_render_frame_host.cc:67: url_copy,
On 2014/04/05 00:09:10, Charlie Reis wrote:
> Style nit: There's no need to put each function parameter on its own line if
> they don't all fit on the same line.  It's fine in cases that improve
> readability, but doing one-per-line is only required for function signatures,
> not for call sites.

Done.

[Charlie Reis, 2014-04-09 22:15:11]

Ok, I think that takes care of my feedback, apart from a few small comments
below.  Please make sure you and Brett resolve the redirect clear/append
questions.  Otherwise LGTM.

(Also, I'm not a sync reviewer, but are there ways to test your proto changes? 
Looks like there's a proto_value_conversions_unittest.cc for that.)

https://codereview.chromium.org/101573003/diff/1910001/content/public/common/...
File content/public/common/page_transition_types.cc (right):

https://codereview.chromium.org/101573003/diff/1910001/content/public/common/...
content/public/common/page_transition_types.cc:51: return !((type &
PAGE_TRANSITION_FORWARD_BACK) != 0 ||
nit: We can make this easier to read by distributing the not:
return (...) == 0 && !PageTransitionCoreTypeIs(...);

(i.e., "it's not a back/forward and it's not a reload")

https://codereview.chromium.org/101573003/diff/1910001/content/public/common/...
File content/public/common/page_transition_types.h (right):

https://codereview.chromium.org/101573003/diff/1910001/content/public/common/...
content/public/common/page_transition_types.h:42: CONTENT_EXPORT bool
PageTransitionIsForwardBack(PageTransition type);
This still needs to be removed.

[Donn Denman, 2014-04-10 06:46:17]

On 2014/04/09 22:15:11, Charlie Reis wrote:
> Ok, I think that takes care of my feedback, apart from a few small comments
> below.  

Thanks Charlie!

>Please make sure you and Brett resolve the redirect clear/append
> questions. 
Sure thing!

> (Also, I'm not a sync reviewer, but are there ways to test your proto changes?

> Looks like there's a proto_value_conversions_unittest.cc for that.)
Nice catch, added a test there.

https://codereview.chromium.org/101573003/diff/1910001/content/public/common/...
File content/public/common/page_transition_types.cc (right):

https://codereview.chromium.org/101573003/diff/1910001/content/public/common/...
content/public/common/page_transition_types.cc:51: return !((type &
PAGE_TRANSITION_FORWARD_BACK) != 0 ||
On 2014/04/09 22:15:12, Charlie Reis wrote:
> nit: We can make this easier to read by distributing the not:
> return (...) == 0 && !PageTransitionCoreTypeIs(...);
> 
> (i.e., "it's not a back/forward and it's not a reload")

Done.

https://codereview.chromium.org/101573003/diff/1910001/content/public/common/...
File content/public/common/page_transition_types.h (right):

https://codereview.chromium.org/101573003/diff/1910001/content/public/common/...
content/public/common/page_transition_types.h:42: CONTENT_EXPORT bool
PageTransitionIsForwardBack(PageTransition type);
On 2014/04/09 22:15:12, Charlie Reis wrote:
> This still needs to be removed.

Done.

[brettw, 2014-04-10 19:51:01]

LGTM. Thanks for the thorough explanation. The thing I wasn't realizing is that
the browser sends the redirects to the renderer, which appends to them, and then
sends them back.

https://codereview.chromium.org/101573003/diff/1930001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1930001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:113: params->redirects =
std::vector<GURL>();
Just do params->redirects.clear() instead, its faster and the code is shorter.

[Donn Denman, 2014-04-10 23:45:22]

Thanks Brett!

https://codereview.chromium.org/101573003/diff/1930001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/101573003/diff/1930001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:113: params->redirects =
std::vector<GURL>();
On 2014/04/10 19:51:02, brettw wrote:
> Just do params->redirects.clear() instead, its faster and the code is shorter.

Done.

[Donn Denman, 2014-04-22 20:28:18]

The CQ bit was checked by donnd@chromium.org

[commit-bot: I haz the power, 2014-04-22 20:29:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/donnd@chromium.org/101573003/1950001

[commit-bot: I haz the power, 2014-04-23 03:37:23]

Change committed as 265515