Cleanup referrer_charset

Non-functional change to simply have Downloads query Prefs for the charset when we need it rather than have ChromeURLRequestContext track it on the network thread and pass it through three different classes.

BUG=146596
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163652

[pauljensen, 2012-10-12 15:23:15]

Hey downloads team!  What are your thoughts on this change?  It facilitates some
refactoring I'm trying to land (http://crrev.com/10918279).  While it's
non-functional and does remove a number of bits of code, I'd be remiss without
saying I feel like its yanking a bunch of code that never really got
finished...I say this because we're using the user's preferred charset in place
of the referrer's charset (which looks like its set but never used, see `git
grep referrer_encoding`).

[erikwright (departed), 2012-10-12 15:30:28]

Another thing worth noting is that it seems like a poor idea to pass the
"referrer charset" around in the context anyway.

A context is used by an arbitrary number of requests at a time, and presumably
they could have different referrer charsets.

It would be more appropriate (not necessarily "right") to make this an attribute
of an URLRequest.

[Randy Smith (Not in Mondays), 2012-10-12 16:39:41]

Quick note that I won't be able to review this before Monday.  It sounds good,
but I'd like to make sure there aren't any corner-case implications to it.

(You also probably want to pick a primary reviewer--in general asking multiple
people to review the same code provokes a "Cool, I can relax; someone else will
do it" reaction :-}.)

[asanka, 2012-10-12 16:43:10]

There was also this CL by abarth: https://codereview.chromium.org/9317018/

The plumbing for referrer_charset (though not quite right as erikwright points
out) was removed in
http://codereview.chromium.org/6995013/diff/13005/chrome/browser/download/dow...

[benjhayden, 2012-10-12 17:01:26]

Jochen, I recall you did content::Referrer. Do you have any insight into the
intentions behind referrer_charset? Do you know anybody else who might care
about this change?

[pauljensen, 2012-10-12 17:31:06]

In the interest of doing the Right Thing, should I instead re-plumb this back
through?  Not from URLRequestContext but instead from
DownloadUrlParameters::referrer_encoding_ into say DownloadSaveInfo then in
DownloadResourceHandler::OnResponseStarted() take it from there instead of the
URLRequestContext.

[pauljensen, 2012-10-12 17:32:33]

And in ChromeDownloadMangerDelegate if referrer_charset is empty fall back on
the Prefs value.

[jochen (gone - plz use gerrit), 2012-10-12 17:48:15]

On 2012/10/12 17:01:26, benjhayden_chromium wrote:
> Jochen, I recall you did content::Referrer. Do you have any insight into the
> intentions behind referrer_charset? Do you know anybody else who might care
> about this change?

I have no idea what the referrer_charset is, sorry

[Randy Smith (Not in Mondays), 2012-10-15 01:29:08]

[cc: abarth@ because of https://codereview.chromium.org/9317018/.]

Wow, this is a mess (the referrer_charset stuff, not your CL, Paul :-}).  If I'm
reading the various different tea-leaves right:

* Most of the time referrer_charset is a lie (above CL).  It's really the user's
default charset from their profile, and shouldn't be in anything URLRequest
related at all, but instead pulled from the user's profile (modulo difficulties
of getting stuff from the profile on the IO thread--I think that means it should
be in the ProfileIOData).  I'll note that the global URLRequestContexts are also
hung off of the ProfileIOData; this may be where the storage of the default
charset on the URLRequestContext came from.

* There are some times when it really is the referring charset from the web page
from which the link that is being clicked.  The obvious case is the one Asanka
mentions in the referred CL.  Conceptually, a download initiated by navigation
should have the same context--we might want to trace out where that should be
and see if it is there, and if not, plumb it in.

To me this means that we should actually have two different values.  One's the
default_charset, which may as well stay on the URLRequestContext, and should not
be modified after construction, and the other is the actually referring charset,
which should be plumbed into the URLRequest.  Does this make sense to everyone? 
(Specifically Erik and Adam, who seem like the closest things we have to
experts.)  Erik, I'm specifically interested in the ways in which you think
plumbing the (actual) referrer_charset into the URLRequest wouldn't be the idea
solution.

[Randy Smith (Not in Mondays), 2012-10-15 01:29:39]

On 2012/10/15 01:29:08, rdsmith wrote:
> [cc: abarth@ because of https://codereview.chromium.org/9317018/.%5D
> 
> Wow, this is a mess (the referrer_charset stuff, not your CL, Paul :-}).  If
I'm
> reading the various different tea-leaves right:
> 
> * Most of the time referrer_charset is a lie (above CL).  It's really the
user's
> default charset from their profile, and shouldn't be in anything URLRequest
> related at all, but instead pulled from the user's profile (modulo
difficulties
> of getting stuff from the profile on the IO thread--I think that means it
should
> be in the ProfileIOData).  I'll note that the global URLRequestContexts are
also
> hung off of the ProfileIOData; this may be where the storage of the default
> charset on the URLRequestContext came from.
> 
> * There are some times when it really is the referring charset from the web
page
> from which the link that is being clicked.  The obvious case is the one Asanka
> mentions in the referred CL.  Conceptually, a download initiated by navigation
> should have the same context--we might want to trace out where that should be
> and see if it is there, and if not, plumb it in.
> 
> To me this means that we should actually have two different values.  One's the
> default_charset, which may as well stay on the URLRequestContext, and should
not
> be modified after construction, and the other is the actually referring
charset,
> which should be plumbed into the URLRequest.  Does this make sense to
everyone? 
> (Specifically Erik and Adam, who seem like the closest things we have to
> experts.)  Erik, I'm specifically interested in the ways in which you think
> plumbing the (actual) referrer_charset into the URLRequest wouldn't be the
idea
> solution.

*ideal

[erikwright (departed), 2012-10-15 01:53:00]

On 2012/10/15 01:29:39, rdsmith wrote:
> On 2012/10/15 01:29:08, rdsmith wrote:
> > [cc: abarth@ because of https://codereview.chromium.org/9317018/.%255D
> > 
> > Wow, this is a mess (the referrer_charset stuff, not your CL, Paul :-}).  If
> I'm
> > reading the various different tea-leaves right:
> > 
> > * Most of the time referrer_charset is a lie (above CL).  It's really the
> user's
> > default charset from their profile, and shouldn't be in anything URLRequest
> > related at all, but instead pulled from the user's profile (modulo
> difficulties
> > of getting stuff from the profile on the IO thread--I think that means it
> should
> > be in the ProfileIOData).  I'll note that the global URLRequestContexts are
> also
> > hung off of the ProfileIOData; this may be where the storage of the default
> > charset on the URLRequestContext came from.
> > 
> > * There are some times when it really is the referring charset from the web
> page
> > from which the link that is being clicked.  The obvious case is the one
Asanka
> > mentions in the referred CL.  Conceptually, a download initiated by
navigation
> > should have the same context--we might want to trace out where that should
be
> > and see if it is there, and if not, plumb it in.
> > 
> > To me this means that we should actually have two different values.  One's
the
> > default_charset, which may as well stay on the URLRequestContext, and should
> not
> > be modified after construction, and the other is the actually referring
> charset,
> > which should be plumbed into the URLRequest.  Does this make sense to
> everyone? 
> > (Specifically Erik and Adam, who seem like the closest things we have to
> > experts.)  Erik, I'm specifically interested in the ways in which you think
> > plumbing the (actual) referrer_charset into the URLRequest wouldn't be the
> idea
> > solution.
> 
> *ideal

I didn't see an actual case where it is currently right, despite following the
referred CLs and code-searching. But even if it were, the current mechanism
seems self-evidently wrong.

At a minimum, the referrer charset should not be attached to a
URLRequestContext, for the simple reason that there may be multiple simultaneous
requests associated with a context, each having different referrers.

Is it "correct" to attach it to a URLRequest? Personally, I say "no". An URL
Request should not be a place where we define every arbitrary attribute that any
potential client of the network stack might want to have available.

I haven't looked into the details of the download architecture to be able to
make a precise recommendation, but I would say that, in general, the network
stack client should supply an URLRequest::Delegate that knows (directly or
indirectly, eventually, etc.) all necessary state required to handle the
response when it is received.

In any case, I think that the current CL is right, as it removes something that
was both wrong and ineffective. Assuming everyone agrees with that, I suggest a
separate email thread or CL for a right and effective solution, upon which I'm
very happy to lend an opinion (heck, I give them away for free!).

[Randy Smith (Not in Mondays), 2012-10-15 02:01:57]

On 2012/10/15 01:53:00, erikwright wrote:
> On 2012/10/15 01:29:39, rdsmith wrote:
> > On 2012/10/15 01:29:08, rdsmith wrote:
> > > [cc: abarth@ because of https://codereview.chromium.org/9317018/.%25255D
> > > 
> > > Wow, this is a mess (the referrer_charset stuff, not your CL, Paul :-}). 
If
> > I'm
> > > reading the various different tea-leaves right:
> > > 
> > > * Most of the time referrer_charset is a lie (above CL).  It's really the
> > user's
> > > default charset from their profile, and shouldn't be in anything
URLRequest
> > > related at all, but instead pulled from the user's profile (modulo
> > difficulties
> > > of getting stuff from the profile on the IO thread--I think that means it
> > should
> > > be in the ProfileIOData).  I'll note that the global URLRequestContexts
are
> > also
> > > hung off of the ProfileIOData; this may be where the storage of the
default
> > > charset on the URLRequestContext came from.
> > > 
> > > * There are some times when it really is the referring charset from the
web
> > page
> > > from which the link that is being clicked.  The obvious case is the one
> Asanka
> > > mentions in the referred CL.  Conceptually, a download initiated by
> navigation
> > > should have the same context--we might want to trace out where that should
> be
> > > and see if it is there, and if not, plumb it in.
> > > 
> > > To me this means that we should actually have two different values.  One's
> the
> > > default_charset, which may as well stay on the URLRequestContext, and
should
> > not
> > > be modified after construction, and the other is the actually referring
> > charset,
> > > which should be plumbed into the URLRequest.  Does this make sense to
> > everyone? 
> > > (Specifically Erik and Adam, who seem like the closest things we have to
> > > experts.)  Erik, I'm specifically interested in the ways in which you
think
> > > plumbing the (actual) referrer_charset into the URLRequest wouldn't be the
> > idea
> > > solution.
> > 
> > *ideal
> 
> I didn't see an actual case where it is currently right, despite following the
> referred CLs and code-searching. But even if it were, the current mechanism
> seems self-evidently wrong.
> 
> At a minimum, the referrer charset should not be attached to a
> URLRequestContext, for the simple reason that there may be multiple
simultaneous
> requests associated with a context, each having different referrers.

I'm not sure we're communicating.  I think the thing that should be attached to
the URLRequestContext is the default_charset, derived from the Profile and
unchangeable.  For those cases where we need an actual referrer_charset, it
should be somewhere else.

> Is it "correct" to attach it to a URLRequest? Personally, I say "no". An URL
> Request should not be a place where we define every arbitrary attribute that
any
> potential client of the network stack might want to have available.
> 
> I haven't looked into the details of the download architecture to be able to
> make a precise recommendation, but I would say that, in general, the network
> stack client should supply an URLRequest::Delegate that knows (directly or
> indirectly, eventually, etc.) all necessary state required to handle the
> response when it is received.

I think I'm limited by my lack of knowledge of the network stack architecture
(unfortunate symmetry, that :-}).  Who would the network stack client be in this
case?  If it's the downloads system, we lose, because the downloads system
doesn't have any knowledge of what the referring page was, and thus what charset
it would suggest. 

>> In any case, I think that the current CL is right, as it removes something
that
> was both wrong and ineffective. Assuming everyone agrees with that, I suggest
a
> separate email thread or CL for a right and effective solution, upon which I'm
> very happy to lend an opinion (heck, I give them away for free!).

My general preference is to figure out where we want to get to long-term, and
then figure out what the incremental steps along the way are.  In this specific
case, I'm concerned that John's CL moved us away from proper functionality
(implemented in a broken fashion by modifying the URLRequestContext, but the
functionality was right in some fashion) and that this CL might move us further
away.  Having said that, if this CL doesn't change behavior I have no objection
to putting it in; it sounds like it will make the code cleaner.  If we decide
later that our final code state is closer to the current code than the result of
this CL, modifying it back won't be that much code.

[erikwright (departed), 2012-10-15 02:14:38]

On 2012/10/15 02:01:57, rdsmith wrote:
> On 2012/10/15 01:53:00, erikwright wrote:
> > On 2012/10/15 01:29:39, rdsmith wrote:
> > > On 2012/10/15 01:29:08, rdsmith wrote:
> > > > [cc: abarth@ because of
https://codereview.chromium.org/9317018/.%2525255D
> > > > 
> > > > Wow, this is a mess (the referrer_charset stuff, not your CL, Paul :-}).

> If
> > > I'm
> > > > reading the various different tea-leaves right:
> > > > 
> > > > * Most of the time referrer_charset is a lie (above CL).  It's really
the
> > > user's
> > > > default charset from their profile, and shouldn't be in anything
> URLRequest
> > > > related at all, but instead pulled from the user's profile (modulo
> > > difficulties
> > > > of getting stuff from the profile on the IO thread--I think that means
it
> > > should
> > > > be in the ProfileIOData).  I'll note that the global URLRequestContexts
> are
> > > also
> > > > hung off of the ProfileIOData; this may be where the storage of the
> default
> > > > charset on the URLRequestContext came from.
> > > > 
> > > > * There are some times when it really is the referring charset from the
> web
> > > page
> > > > from which the link that is being clicked.  The obvious case is the one
> > Asanka
> > > > mentions in the referred CL.  Conceptually, a download initiated by
> > navigation
> > > > should have the same context--we might want to trace out where that
should
> > be
> > > > and see if it is there, and if not, plumb it in.
> > > > 
> > > > To me this means that we should actually have two different values. 
One's
> > the
> > > > default_charset, which may as well stay on the URLRequestContext, and
> should
> > > not
> > > > be modified after construction, and the other is the actually referring
> > > charset,
> > > > which should be plumbed into the URLRequest.  Does this make sense to
> > > everyone? 
> > > > (Specifically Erik and Adam, who seem like the closest things we have to
> > > > experts.)  Erik, I'm specifically interested in the ways in which you
> think
> > > > plumbing the (actual) referrer_charset into the URLRequest wouldn't be
the
> > > idea
> > > > solution.
> > > 
> > > *ideal
> > 
> > I didn't see an actual case where it is currently right, despite following
the
> > referred CLs and code-searching. But even if it were, the current mechanism
> > seems self-evidently wrong.
> > 
> > At a minimum, the referrer charset should not be attached to a
> > URLRequestContext, for the simple reason that there may be multiple
> simultaneous
> > requests associated with a context, each having different referrers.
> 
> I'm not sure we're communicating.  I think the thing that should be attached
to
> the URLRequestContext is the default_charset, derived from the Profile and
> unchangeable.  For those cases where we need an actual referrer_charset, it
> should be somewhere else.

I was simply agreeing with you about why referrer charset clearly shouldn't be
there.

As to the default charset, that currently _is_ plumbed into the context (well,
in the form of accept_charset) but we are gradually taking it out and binding it
into the protocol handlers.

Downloads, for example (after some brief research), have no problem accessing
this because there is a download manager delegate that is per-profile and is
ultimately the recipient of the data that is currently being plumbed through the
context. So instead of pulling it out of the DownloadItemData (or whatever),
which is populated from the context, it could simply access it directly.

> 
> > Is it "correct" to attach it to a URLRequest? Personally, I say "no". An URL
> > Request should not be a place where we define every arbitrary attribute that
> any
> > potential client of the network stack might want to have available.
> > 
> > I haven't looked into the details of the download architecture to be able to
> > make a precise recommendation, but I would say that, in general, the network
> > stack client should supply an URLRequest::Delegate that knows (directly or
> > indirectly, eventually, etc.) all necessary state required to handle the
> > response when it is received.
> 
> I think I'm limited by my lack of knowledge of the network stack architecture
> (unfortunate symmetry, that :-}).  Who would the network stack client be in
this
> case?  If it's the downloads system, we lose, because the downloads system
> doesn't have any knowledge of what the referring page was, and thus what
charset
> it would suggest. 

No, the client is whoever creates the URLRequest object, passing it a
URLRequestContext, and calls URLRequest::Start. That client _should_ be somehow
aware when that request is going to end up as a download and should be able to
intervene at the appropriate time.

> >> In any case, I think that the current CL is right, as it removes something
> that
> > was both wrong and ineffective. Assuming everyone agrees with that, I
suggest
> a
> > separate email thread or CL for a right and effective solution, upon which
I'm
> > very happy to lend an opinion (heck, I give them away for free!).
> 
> My general preference is to figure out where we want to get to long-term, and
> then figure out what the incremental steps along the way are.  In this
specific
> case, I'm concerned that John's CL moved us away from proper functionality
> (implemented in a broken fashion by modifying the URLRequestContext, but the
> functionality was right in some fashion) and that this CL might move us
further
> away.  Having said that, if this CL doesn't change behavior I have no
objection
> to putting it in; it sounds like it will make the code cleaner.  If we decide
> later that our final code state is closer to the current code than the result
of
> this CL, modifying it back won't be that much code.

Well, Paul's next CL will move us further away:
http://codereview.chromium.org/10918279/

I will give a concrete suggestion for how I think this should work on Monday
morning.

[Randy Smith (Not in Mondays), 2012-10-15 13:02:22]

On 2012/10/15 02:14:38, erikwright wrote:
> On 2012/10/15 02:01:57, rdsmith wrote:
> > On 2012/10/15 01:53:00, erikwright wrote:
> > > On 2012/10/15 01:29:39, rdsmith wrote:
> > > > On 2012/10/15 01:29:08, rdsmith wrote:
> I was simply agreeing with you about why referrer charset clearly shouldn't be
> there.
> 
> As to the default charset, that currently _is_ plumbed into the context (well,
> in the form of accept_charset) but we are gradually taking it out and binding
it
> into the protocol handlers.
> 
> Downloads, for example (after some brief research), have no problem accessing
> this because there is a download manager delegate that is per-profile and is
> ultimately the recipient of the data that is currently being plumbed through
the
> context. So instead of pulling it out of the DownloadItemData (or whatever),
> which is populated from the context, it could simply access it directly.

Yes, this will work (I was worried that we needed it on the IO thread, but we do
the transformation on the UI thread, so this is fine.)

> > > Is it "correct" to attach it to a URLRequest? Personally, I say "no". An
URL
> > > Request should not be a place where we define every arbitrary attribute
that
> > any
> > > potential client of the network stack might want to have available.
> > > 
> > > I haven't looked into the details of the download architecture to be able
to
> > > make a precise recommendation, but I would say that, in general, the
network
> > > stack client should supply an URLRequest::Delegate that knows (directly or
> > > indirectly, eventually, etc.) all necessary state required to handle the
> > > response when it is received.
> > 
> > I think I'm limited by my lack of knowledge of the network stack
architecture
> > (unfortunate symmetry, that :-}).  Who would the network stack client be in
> this
> > case?  If it's the downloads system, we lose, because the downloads system
> > doesn't have any knowledge of what the referring page was, and thus what
> charset
> > it would suggest. 
> 
> No, the client is whoever creates the URLRequest object, passing it a
> URLRequestContext, and calls URLRequest::Start. That client _should_ be
somehow
> aware when that request is going to end up as a download and should be able to
> intervene at the appropriate time.

The client is in general _not_ aware that the request is going to end up as a
download.  If there's a Content-disposition field, it would be, and if Adam's
right about what the charset is used for and that's the only use we have for the
referrer_charset, conditionalizing saving the refererr_charset on the
Content-disposition field might work.  But it feels fragile to me.  In general,
we download a URL when we can't figure out what else to do with it, and that
decision is made well well after the URLRequest is created (in
BufferedResourcehandler).

Do we have access to the referrer until ResourceHandler::OnResponseStarted()? 
If so, we can grab this information in
DownloadResourceHandler::OnResponseStarted().  But if we're going to grab it at
URLRequest creation time, I feel like we might as well put it in the URLRequest,
as I don't think in general we're going to know whether we need it.

> > >> In any case, I think that the current CL is right, as it removes
something
> > that
> > > was both wrong and ineffective. Assuming everyone agrees with that, I
> suggest
> > a
> > > separate email thread or CL for a right and effective solution, upon which
> I'm
> > > very happy to lend an opinion (heck, I give them away for free!).
> > 
> > My general preference is to figure out where we want to get to long-term,
and
> > then figure out what the incremental steps along the way are.  In this
> specific
> > case, I'm concerned that John's CL moved us away from proper functionality
> > (implemented in a broken fashion by modifying the URLRequestContext, but the
> > functionality was right in some fashion) and that this CL might move us
> further
> > away.  Having said that, if this CL doesn't change behavior I have no
> objection
> > to putting it in; it sounds like it will make the code cleaner.  If we
decide
> > later that our final code state is closer to the current code than the
result
> of
> > this CL, modifying it back won't be that much code.
> 
> Well, Paul's next CL will move us further away:
> http://codereview.chromium.org/10918279/
> 
> I will give a concrete suggestion for how I think this should work on Monday
> morning.

Cool.  I don't so much want to do the Right Thing Right Now as figure out where
we want to eventually get to so that we don't head off in the completely wrong
direction right now.

[erikwright (departed), 2012-10-15 14:51:57]

I did a fair bit of looking into this.

The direct "client" in this case is ResourceDispatcherHostImpl::BeginRequest,
which creates the Request and also sets up all the resource handlers.

If BeginRequest _actually_knew_ the referrer charset, we would be golden. I
would just plug it into the BufferedResourceHandler that is created for the
request.

Unfortunately, BeginRequest does _not_ receive this info in
ResourceHostMsg_Request.

I think a pretty reasonable (perhaps even "right") alternative is for the
ChromeDownloadManagerDelegate to know the charsets for recently received
documents. A FIFO cache mapping URLs to charsets would be more than sufficient.

It could have that information via ChromeNetworkDelegate::OnHeadersReceived.

So I would do this:

1) Define a class ReferrerCharsetCache. It would be owned by the profile.

2) Pass a pointer to (1) to ChromeNetworkDelegate. It should notify the
ReferrerCharsetCache in OnHeadersReceived.

3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It should query for a
cached referrer charset in CheckVisitedReferrerBeforeDone

[Randy Smith (Not in Mondays), 2012-10-15 16:06:20]

On 2012/10/15 14:51:57, erikwright wrote:
> I did a fair bit of looking into this.
> 
> The direct "client" in this case is ResourceDispatcherHostImpl::BeginRequest,
> which creates the Request and also sets up all the resource handlers.
> 
> If BeginRequest _actually_knew_ the referrer charset, we would be golden. I
> would just plug it into the BufferedResourceHandler that is created for the
> request.
> 
> Unfortunately, BeginRequest does _not_ receive this info in
> ResourceHostMsg_Request.
> 
> I think a pretty reasonable (perhaps even "right") alternative is for the
> ChromeDownloadManagerDelegate to know the charsets for recently received
> documents. A FIFO cache mapping URLs to charsets would be more than
sufficient.
> 
> It could have that information via ChromeNetworkDelegate::OnHeadersReceived.
> 
> So I would do this:
> 
> 1) Define a class ReferrerCharsetCache. It would be owned by the profile.
> 
> 2) Pass a pointer to (1) to ChromeNetworkDelegate. It should notify the
> ReferrerCharsetCache in OnHeadersReceived.
> 
> 3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It should query for
a
> cached referrer charset in CheckVisitedReferrerBeforeDone

Well, that would certainly work, but I wince at doing it that way--we're
creating a side channel communications medium (at least a little unreliable,
because of the cache aspect) rather than directly passing the information from
the place that has it to the place that wants it.  Did you look at how hard it
would be to plumb the information into ResourceHostMsg_Request?

[erikwright (departed), 2012-10-15 20:39:33]

On 2012/10/15 16:06:20, rdsmith wrote:
> On 2012/10/15 14:51:57, erikwright wrote:
> > I did a fair bit of looking into this.
> > 
> > The direct "client" in this case is
ResourceDispatcherHostImpl::BeginRequest,
> > which creates the Request and also sets up all the resource handlers.
> > 
> > If BeginRequest _actually_knew_ the referrer charset, we would be golden. I
> > would just plug it into the BufferedResourceHandler that is created for the
> > request.
> > 
> > Unfortunately, BeginRequest does _not_ receive this info in
> > ResourceHostMsg_Request.
> > 
> > I think a pretty reasonable (perhaps even "right") alternative is for the
> > ChromeDownloadManagerDelegate to know the charsets for recently received
> > documents. A FIFO cache mapping URLs to charsets would be more than
> sufficient.
> > 
> > It could have that information via ChromeNetworkDelegate::OnHeadersReceived.
> > 
> > So I would do this:
> > 
> > 1) Define a class ReferrerCharsetCache. It would be owned by the profile.
> > 
> > 2) Pass a pointer to (1) to ChromeNetworkDelegate. It should notify the
> > ReferrerCharsetCache in OnHeadersReceived.
> > 
> > 3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It should query
for
> a
> > cached referrer charset in CheckVisitedReferrerBeforeDone
> 
> Well, that would certainly work, but I wince at doing it that way--we're
> creating a side channel communications medium (at least a little unreliable,
> because of the cache aspect) rather than directly passing the information from
> the place that has it to the place that wants it.  Did you look at how hard it
> would be to plumb the information into ResourceHostMsg_Request?

Personally, I don't think that the "side channel" aspect is wrong. From an
encapsulation point of view, it seems more wrong to me that so many components
between the renderer, the content layer, the downloads subsystem, and
chrome/browser should need to be modified to implement this feature.

To plumb it into ResourceHostMsg_Request I believe you will need WebKit changes.
See the original commit message from 2009 for this feature:

http://codereview.chromium.org/83002

Is this still true? The request is originally received in
WebURLLoaderImpl::Context::Start, and in fact we only even see the referrer URL
because WebKit supplies it as an HTTP header on the WebURLRequest object.

But perhaps it can be had in RenderViewImpl::willSendRequest. This function
appears to be called by webkit and have access to the frame that is generating a
request. The WebFrame has a WebDocument, has an |encoding()|. Presumably that's
what you want. You could add it to the RequestExtraData
(content/common/request_extra_data.h) and unpack it in IPCResourceLoaderBridge.
From there eventually you can get it to ResourceHostMsg_Request.

[Randy Smith (Not in Mondays), 2012-10-17 21:25:47]

On 2012/10/15 20:39:33, erikwright wrote:
> On 2012/10/15 16:06:20, rdsmith wrote:
> > On 2012/10/15 14:51:57, erikwright wrote:
> > > I did a fair bit of looking into this.
> > > 
> > > The direct "client" in this case is
> ResourceDispatcherHostImpl::BeginRequest,
> > > which creates the Request and also sets up all the resource handlers.
> > > 
> > > If BeginRequest _actually_knew_ the referrer charset, we would be golden.
I
> > > would just plug it into the BufferedResourceHandler that is created for
the
> > > request.
> > > 
> > > Unfortunately, BeginRequest does _not_ receive this info in
> > > ResourceHostMsg_Request.
> > > 
> > > I think a pretty reasonable (perhaps even "right") alternative is for the
> > > ChromeDownloadManagerDelegate to know the charsets for recently received
> > > documents. A FIFO cache mapping URLs to charsets would be more than
> > sufficient.
> > > 
> > > It could have that information via
ChromeNetworkDelegate::OnHeadersReceived.
> > > 
> > > So I would do this:
> > > 
> > > 1) Define a class ReferrerCharsetCache. It would be owned by the profile.
> > > 
> > > 2) Pass a pointer to (1) to ChromeNetworkDelegate. It should notify the
> > > ReferrerCharsetCache in OnHeadersReceived.
> > > 
> > > 3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It should query
> for
> > a
> > > cached referrer charset in CheckVisitedReferrerBeforeDone
> > 
> > Well, that would certainly work, but I wince at doing it that way--we're
> > creating a side channel communications medium (at least a little unreliable,
> > because of the cache aspect) rather than directly passing the information
from
> > the place that has it to the place that wants it.  Did you look at how hard
it
> > would be to plumb the information into ResourceHostMsg_Request?
> 
> Personally, I don't think that the "side channel" aspect is wrong. From an
> encapsulation point of view, it seems more wrong to me that so many components
> between the renderer, the content layer, the downloads subsystem, and
> chrome/browser should need to be modified to implement this feature.

One point that came up while Paul and I were chatting locally is that it isn't
clear how long we'd have to keep entries in this cache around.  If I bring up a
couple of tabs when I start the browser, work in one of them for a couple of
hours, then go back and do a download in another, doesn't that run some risk of
blowing out the cache so that we can't get the information anymore?

> 
> To plumb it into ResourceHostMsg_Request I believe you will need WebKit
changes.
> See the original commit message from 2009 for this feature:
> 
> http://codereview.chromium.org/83002
> 
> Is this still true? The request is originally received in
> WebURLLoaderImpl::Context::Start, and in fact we only even see the referrer
URL
> because WebKit supplies it as an HTTP header on the WebURLRequest object.
> 
> But perhaps it can be had in RenderViewImpl::willSendRequest. This function
> appears to be called by webkit and have access to the frame that is generating
a
> request. The WebFrame has a WebDocument, has an |encoding()|. Presumably
that's
> what you want. You could add it to the RequestExtraData
> (content/common/request_extra_data.h) and unpack it in
IPCResourceLoaderBridge.
> From there eventually you can get it to ResourceHostMsg_Request.

[Caveat: my knowledge of the renderer and webkit is low; I asked a colleague for
guidance in navigating this code to understand your suggestion.  ]

This sounds like it would work--not dirt simple, but not involving WebKit.  We'd
have to modify the RequestExtraData, ResourceLoaderBridge::RequestInfo,
ResourceHostMessage_Request, and URLRequest, as well as the glue code between
each of those.  But I don't see any reason why it wouldn't work.

Would you be ok with that plan?  I don't see any place to put the information
once the request has been started other than the URLRequest, which I think you
objected to?

(Not saying this has to happen tomorrow, or any time in particular--as noted
above, I'm just trying to figure out what the right long term plan is so that we
can do short term planning that advances it instead of retarding it.)

[erikwright (departed), 2012-10-17 23:04:07]

On 2012/10/17 21:25:47, rdsmith wrote:
> On 2012/10/15 20:39:33, erikwright wrote:
> > On 2012/10/15 16:06:20, rdsmith wrote:
> > > On 2012/10/15 14:51:57, erikwright wrote:
> > > > I did a fair bit of looking into this.
> > > > 
> > > > The direct "client" in this case is
> > ResourceDispatcherHostImpl::BeginRequest,
> > > > which creates the Request and also sets up all the resource handlers.
> > > > 
> > > > If BeginRequest _actually_knew_ the referrer charset, we would be
golden.
> I
> > > > would just plug it into the BufferedResourceHandler that is created for
> the
> > > > request.
> > > > 
> > > > Unfortunately, BeginRequest does _not_ receive this info in
> > > > ResourceHostMsg_Request.
> > > > 
> > > > I think a pretty reasonable (perhaps even "right") alternative is for
the
> > > > ChromeDownloadManagerDelegate to know the charsets for recently received
> > > > documents. A FIFO cache mapping URLs to charsets would be more than
> > > sufficient.
> > > > 
> > > > It could have that information via
> ChromeNetworkDelegate::OnHeadersReceived.
> > > > 
> > > > So I would do this:
> > > > 
> > > > 1) Define a class ReferrerCharsetCache. It would be owned by the
profile.
> > > > 
> > > > 2) Pass a pointer to (1) to ChromeNetworkDelegate. It should notify the
> > > > ReferrerCharsetCache in OnHeadersReceived.
> > > > 
> > > > 3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It should
query
> > for
> > > a
> > > > cached referrer charset in CheckVisitedReferrerBeforeDone
> > > 
> > > Well, that would certainly work, but I wince at doing it that way--we're
> > > creating a side channel communications medium (at least a little
unreliable,
> > > because of the cache aspect) rather than directly passing the information
> from
> > > the place that has it to the place that wants it.  Did you look at how
hard
> it
> > > would be to plumb the information into ResourceHostMsg_Request?
> > 
> > Personally, I don't think that the "side channel" aspect is wrong. From an
> > encapsulation point of view, it seems more wrong to me that so many
components
> > between the renderer, the content layer, the downloads subsystem, and
> > chrome/browser should need to be modified to implement this feature.
> 
> One point that came up while Paul and I were chatting locally is that it isn't
> clear how long we'd have to keep entries in this cache around.  If I bring up
a
> couple of tabs when I start the browser, work in one of them for a couple of
> hours, then go back and do a download in another, doesn't that run some risk
of
> blowing out the cache so that we can't get the information anymore?
> 
> > 
> > To plumb it into ResourceHostMsg_Request I believe you will need WebKit
> changes.
> > See the original commit message from 2009 for this feature:
> > 
> > http://codereview.chromium.org/83002
> > 
> > Is this still true? The request is originally received in
> > WebURLLoaderImpl::Context::Start, and in fact we only even see the referrer
> URL
> > because WebKit supplies it as an HTTP header on the WebURLRequest object.
> > 
> > But perhaps it can be had in RenderViewImpl::willSendRequest. This function
> > appears to be called by webkit and have access to the frame that is
generating
> a
> > request. The WebFrame has a WebDocument, has an |encoding()|. Presumably
> that's
> > what you want. You could add it to the RequestExtraData
> > (content/common/request_extra_data.h) and unpack it in
> IPCResourceLoaderBridge.
> > From there eventually you can get it to ResourceHostMsg_Request.
> 
> [Caveat: my knowledge of the renderer and webkit is low; I asked a colleague
for
> guidance in navigating this code to understand your suggestion.  ]
> 
> This sounds like it would work--not dirt simple, but not involving WebKit. 
We'd
> have to modify the RequestExtraData, ResourceLoaderBridge::RequestInfo,
> ResourceHostMessage_Request, and URLRequest, as well as the glue code between
> each of those.  But I don't see any reason why it wouldn't work.
> 
> Would you be ok with that plan?  I don't see any place to put the information
> once the request has been started other than the URLRequest, which I think you
> objected to?
> 
> (Not saying this has to happen tomorrow, or any time in particular--as noted
> above, I'm just trying to figure out what the right long term plan is so that
we
> can do short term planning that advances it instead of retarding it.)

There shouldn't be any need to put it in URLRequest. In
ResourceDispatcherHostImpl::BeginRequest a bunch of ResourceHandlers are created
and associated with the URLRequest. One or more of them are presumably
responsible for detecting when a download should begin, and they should be able
to hold the referer charset for that eventuality.

Or, you could probably put it in and retrieve it from the
ResourceRequestInfoImpl.

[Randy Smith (Not in Mondays), 2012-10-17 23:06:23]

On 2012/10/17 23:04:07, erikwright wrote:
> On 2012/10/17 21:25:47, rdsmith wrote:
> > On 2012/10/15 20:39:33, erikwright wrote:
> > > On 2012/10/15 16:06:20, rdsmith wrote:
> > > > On 2012/10/15 14:51:57, erikwright wrote:
> > > > > I did a fair bit of looking into this.
> > > > > 
> > > > > The direct "client" in this case is
> > > ResourceDispatcherHostImpl::BeginRequest,
> > > > > which creates the Request and also sets up all the resource handlers.
> > > > > 
> > > > > If BeginRequest _actually_knew_ the referrer charset, we would be
> golden.
> > I
> > > > > would just plug it into the BufferedResourceHandler that is created
for
> > the
> > > > > request.
> > > > > 
> > > > > Unfortunately, BeginRequest does _not_ receive this info in
> > > > > ResourceHostMsg_Request.
> > > > > 
> > > > > I think a pretty reasonable (perhaps even "right") alternative is for
> the
> > > > > ChromeDownloadManagerDelegate to know the charsets for recently
received
> > > > > documents. A FIFO cache mapping URLs to charsets would be more than
> > > > sufficient.
> > > > > 
> > > > > It could have that information via
> > ChromeNetworkDelegate::OnHeadersReceived.
> > > > > 
> > > > > So I would do this:
> > > > > 
> > > > > 1) Define a class ReferrerCharsetCache. It would be owned by the
> profile.
> > > > > 
> > > > > 2) Pass a pointer to (1) to ChromeNetworkDelegate. It should notify
the
> > > > > ReferrerCharsetCache in OnHeadersReceived.
> > > > > 
> > > > > 3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It should
> query
> > > for
> > > > a
> > > > > cached referrer charset in CheckVisitedReferrerBeforeDone
> > > > 
> > > > Well, that would certainly work, but I wince at doing it that way--we're
> > > > creating a side channel communications medium (at least a little
> unreliable,
> > > > because of the cache aspect) rather than directly passing the
information
> > from
> > > > the place that has it to the place that wants it.  Did you look at how
> hard
> > it
> > > > would be to plumb the information into ResourceHostMsg_Request?
> > > 
> > > Personally, I don't think that the "side channel" aspect is wrong. From an
> > > encapsulation point of view, it seems more wrong to me that so many
> components
> > > between the renderer, the content layer, the downloads subsystem, and
> > > chrome/browser should need to be modified to implement this feature.
> > 
> > One point that came up while Paul and I were chatting locally is that it
isn't
> > clear how long we'd have to keep entries in this cache around.  If I bring
up
> a
> > couple of tabs when I start the browser, work in one of them for a couple of
> > hours, then go back and do a download in another, doesn't that run some risk
> of
> > blowing out the cache so that we can't get the information anymore?
> > 
> > > 
> > > To plumb it into ResourceHostMsg_Request I believe you will need WebKit
> > changes.
> > > See the original commit message from 2009 for this feature:
> > > 
> > > http://codereview.chromium.org/83002
> > > 
> > > Is this still true? The request is originally received in
> > > WebURLLoaderImpl::Context::Start, and in fact we only even see the
referrer
> > URL
> > > because WebKit supplies it as an HTTP header on the WebURLRequest object.
> > > 
> > > But perhaps it can be had in RenderViewImpl::willSendRequest. This
function
> > > appears to be called by webkit and have access to the frame that is
> generating
> > a
> > > request. The WebFrame has a WebDocument, has an |encoding()|. Presumably
> > that's
> > > what you want. You could add it to the RequestExtraData
> > > (content/common/request_extra_data.h) and unpack it in
> > IPCResourceLoaderBridge.
> > > From there eventually you can get it to ResourceHostMsg_Request.
> > 
> > [Caveat: my knowledge of the renderer and webkit is low; I asked a colleague
> for
> > guidance in navigating this code to understand your suggestion.  ]
> > 
> > This sounds like it would work--not dirt simple, but not involving WebKit. 
> We'd
> > have to modify the RequestExtraData, ResourceLoaderBridge::RequestInfo,
> > ResourceHostMessage_Request, and URLRequest, as well as the glue code
between
> > each of those.  But I don't see any reason why it wouldn't work.
> > 
> > Would you be ok with that plan?  I don't see any place to put the
information
> > once the request has been started other than the URLRequest, which I think
you
> > objected to?
> > 
> > (Not saying this has to happen tomorrow, or any time in particular--as noted
> > above, I'm just trying to figure out what the right long term plan is so
that
> we
> > can do short term planning that advances it instead of retarding it.)
> 
> There shouldn't be any need to put it in URLRequest. In
> ResourceDispatcherHostImpl::BeginRequest a bunch of ResourceHandlers are
created
> and associated with the URLRequest. One or more of them are presumably
> responsible for detecting when a download should begin, and they should be
able
> to hold the referer charset for that eventuality.
> 
> Or, you could probably put it in and retrieve it from the
> ResourceRequestInfoImpl.

Good point.  I'd be good with either of those options.

[Randy Smith (Not in Mondays), 2012-10-21 00:12:47]

On 2012/10/17 23:06:23, rdsmith wrote:
> On 2012/10/17 23:04:07, erikwright wrote:
> > On 2012/10/17 21:25:47, rdsmith wrote:
> > > On 2012/10/15 20:39:33, erikwright wrote:
> > > > On 2012/10/15 16:06:20, rdsmith wrote:
> > > > > On 2012/10/15 14:51:57, erikwright wrote:
> > > > > > I did a fair bit of looking into this.
> > > > > > 
> > > > > > The direct "client" in this case is
> > > > ResourceDispatcherHostImpl::BeginRequest,
> > > > > > which creates the Request and also sets up all the resource
handlers.
> > > > > > 
> > > > > > If BeginRequest _actually_knew_ the referrer charset, we would be
> > golden.
> > > I
> > > > > > would just plug it into the BufferedResourceHandler that is created
> for
> > > the
> > > > > > request.
> > > > > > 
> > > > > > Unfortunately, BeginRequest does _not_ receive this info in
> > > > > > ResourceHostMsg_Request.
> > > > > > 
> > > > > > I think a pretty reasonable (perhaps even "right") alternative is
for
> > the
> > > > > > ChromeDownloadManagerDelegate to know the charsets for recently
> received
> > > > > > documents. A FIFO cache mapping URLs to charsets would be more than
> > > > > sufficient.
> > > > > > 
> > > > > > It could have that information via
> > > ChromeNetworkDelegate::OnHeadersReceived.
> > > > > > 
> > > > > > So I would do this:
> > > > > > 
> > > > > > 1) Define a class ReferrerCharsetCache. It would be owned by the
> > profile.
> > > > > > 
> > > > > > 2) Pass a pointer to (1) to ChromeNetworkDelegate. It should notify
> the
> > > > > > ReferrerCharsetCache in OnHeadersReceived.
> > > > > > 
> > > > > > 3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It should
> > query
> > > > for
> > > > > a
> > > > > > cached referrer charset in CheckVisitedReferrerBeforeDone
> > > > > 
> > > > > Well, that would certainly work, but I wince at doing it that
way--we're
> > > > > creating a side channel communications medium (at least a little
> > unreliable,
> > > > > because of the cache aspect) rather than directly passing the
> information
> > > from
> > > > > the place that has it to the place that wants it.  Did you look at how
> > hard
> > > it
> > > > > would be to plumb the information into ResourceHostMsg_Request?
> > > > 
> > > > Personally, I don't think that the "side channel" aspect is wrong. From
an
> > > > encapsulation point of view, it seems more wrong to me that so many
> > components
> > > > between the renderer, the content layer, the downloads subsystem, and
> > > > chrome/browser should need to be modified to implement this feature.
> > > 
> > > One point that came up while Paul and I were chatting locally is that it
> isn't
> > > clear how long we'd have to keep entries in this cache around.  If I bring
> up
> > a
> > > couple of tabs when I start the browser, work in one of them for a couple
of
> > > hours, then go back and do a download in another, doesn't that run some
risk
> > of
> > > blowing out the cache so that we can't get the information anymore?
> > > 
> > > > 
> > > > To plumb it into ResourceHostMsg_Request I believe you will need WebKit
> > > changes.
> > > > See the original commit message from 2009 for this feature:
> > > > 
> > > > http://codereview.chromium.org/83002
> > > > 
> > > > Is this still true? The request is originally received in
> > > > WebURLLoaderImpl::Context::Start, and in fact we only even see the
> referrer
> > > URL
> > > > because WebKit supplies it as an HTTP header on the WebURLRequest
object.
> > > > 
> > > > But perhaps it can be had in RenderViewImpl::willSendRequest. This
> function
> > > > appears to be called by webkit and have access to the frame that is
> > generating
> > > a
> > > > request. The WebFrame has a WebDocument, has an |encoding()|. Presumably
> > > that's
> > > > what you want. You could add it to the RequestExtraData
> > > > (content/common/request_extra_data.h) and unpack it in
> > > IPCResourceLoaderBridge.
> > > > From there eventually you can get it to ResourceHostMsg_Request.
> > > 
> > > [Caveat: my knowledge of the renderer and webkit is low; I asked a
colleague
> > for
> > > guidance in navigating this code to understand your suggestion.  ]
> > > 
> > > This sounds like it would work--not dirt simple, but not involving WebKit.

> > We'd
> > > have to modify the RequestExtraData, ResourceLoaderBridge::RequestInfo,
> > > ResourceHostMessage_Request, and URLRequest, as well as the glue code
> between
> > > each of those.  But I don't see any reason why it wouldn't work.
> > > 
> > > Would you be ok with that plan?  I don't see any place to put the
> information
> > > once the request has been started other than the URLRequest, which I think
> you
> > > objected to?
> > > 
> > > (Not saying this has to happen tomorrow, or any time in particular--as
noted
> > > above, I'm just trying to figure out what the right long term plan is so
> that
> > we
> > > can do short term planning that advances it instead of retarding it.)
> > 
> > There shouldn't be any need to put it in URLRequest. In
> > ResourceDispatcherHostImpl::BeginRequest a bunch of ResourceHandlers are
> created
> > and associated with the URLRequest. One or more of them are presumably
> > responsible for detecting when a download should begin, and they should be
> able
> > to hold the referer charset for that eventuality.
> > 
> > Or, you could probably put it in and retrieve it from the
> > ResourceRequestInfoImpl.
> 
> Good point.  I'd be good with either of those options.

To be clear: Now that I think we have a vision of where we want to go long-term,
I'm cool with the original CL.  Getting things out of the URLRequestContext
sounds fine to me.  

Paul, if you feel like taking on the plumbing that Erik's sketched out above, I
certainly wouldn't object--I don't consider it a high priority, just a nice to
have at some point.  And I'd certainly understand if you don't want to do it. 
If you aren't planning to do it, I'll file a bug on it and prioritize it in with
the other downloads bugs.

[erikwright (departed), 2012-10-21 01:02:11]

On 2012/10/21 00:12:47, rdsmith wrote:
> On 2012/10/17 23:06:23, rdsmith wrote:
> > On 2012/10/17 23:04:07, erikwright wrote:
> > > On 2012/10/17 21:25:47, rdsmith wrote:
> > > > On 2012/10/15 20:39:33, erikwright wrote:
> > > > > On 2012/10/15 16:06:20, rdsmith wrote:
> > > > > > On 2012/10/15 14:51:57, erikwright wrote:
> > > > > > > I did a fair bit of looking into this.
> > > > > > > 
> > > > > > > The direct "client" in this case is
> > > > > ResourceDispatcherHostImpl::BeginRequest,
> > > > > > > which creates the Request and also sets up all the resource
> handlers.
> > > > > > > 
> > > > > > > If BeginRequest _actually_knew_ the referrer charset, we would be
> > > golden.
> > > > I
> > > > > > > would just plug it into the BufferedResourceHandler that is
created
> > for
> > > > the
> > > > > > > request.
> > > > > > > 
> > > > > > > Unfortunately, BeginRequest does _not_ receive this info in
> > > > > > > ResourceHostMsg_Request.
> > > > > > > 
> > > > > > > I think a pretty reasonable (perhaps even "right") alternative is
> for
> > > the
> > > > > > > ChromeDownloadManagerDelegate to know the charsets for recently
> > received
> > > > > > > documents. A FIFO cache mapping URLs to charsets would be more
than
> > > > > > sufficient.
> > > > > > > 
> > > > > > > It could have that information via
> > > > ChromeNetworkDelegate::OnHeadersReceived.
> > > > > > > 
> > > > > > > So I would do this:
> > > > > > > 
> > > > > > > 1) Define a class ReferrerCharsetCache. It would be owned by the
> > > profile.
> > > > > > > 
> > > > > > > 2) Pass a pointer to (1) to ChromeNetworkDelegate. It should
notify
> > the
> > > > > > > ReferrerCharsetCache in OnHeadersReceived.
> > > > > > > 
> > > > > > > 3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It
should
> > > query
> > > > > for
> > > > > > a
> > > > > > > cached referrer charset in CheckVisitedReferrerBeforeDone
> > > > > > 
> > > > > > Well, that would certainly work, but I wince at doing it that
> way--we're
> > > > > > creating a side channel communications medium (at least a little
> > > unreliable,
> > > > > > because of the cache aspect) rather than directly passing the
> > information
> > > > from
> > > > > > the place that has it to the place that wants it.  Did you look at
how
> > > hard
> > > > it
> > > > > > would be to plumb the information into ResourceHostMsg_Request?
> > > > > 
> > > > > Personally, I don't think that the "side channel" aspect is wrong.
From
> an
> > > > > encapsulation point of view, it seems more wrong to me that so many
> > > components
> > > > > between the renderer, the content layer, the downloads subsystem, and
> > > > > chrome/browser should need to be modified to implement this feature.
> > > > 
> > > > One point that came up while Paul and I were chatting locally is that it
> > isn't
> > > > clear how long we'd have to keep entries in this cache around.  If I
bring
> > up
> > > a
> > > > couple of tabs when I start the browser, work in one of them for a
couple
> of
> > > > hours, then go back and do a download in another, doesn't that run some
> risk
> > > of
> > > > blowing out the cache so that we can't get the information anymore?
> > > > 
> > > > > 
> > > > > To plumb it into ResourceHostMsg_Request I believe you will need
WebKit
> > > > changes.
> > > > > See the original commit message from 2009 for this feature:
> > > > > 
> > > > > http://codereview.chromium.org/83002
> > > > > 
> > > > > Is this still true? The request is originally received in
> > > > > WebURLLoaderImpl::Context::Start, and in fact we only even see the
> > referrer
> > > > URL
> > > > > because WebKit supplies it as an HTTP header on the WebURLRequest
> object.
> > > > > 
> > > > > But perhaps it can be had in RenderViewImpl::willSendRequest. This
> > function
> > > > > appears to be called by webkit and have access to the frame that is
> > > generating
> > > > a
> > > > > request. The WebFrame has a WebDocument, has an |encoding()|.
Presumably
> > > > that's
> > > > > what you want. You could add it to the RequestExtraData
> > > > > (content/common/request_extra_data.h) and unpack it in
> > > > IPCResourceLoaderBridge.
> > > > > From there eventually you can get it to ResourceHostMsg_Request.
> > > > 
> > > > [Caveat: my knowledge of the renderer and webkit is low; I asked a
> colleague
> > > for
> > > > guidance in navigating this code to understand your suggestion.  ]
> > > > 
> > > > This sounds like it would work--not dirt simple, but not involving
WebKit.
> 
> > > We'd
> > > > have to modify the RequestExtraData, ResourceLoaderBridge::RequestInfo,
> > > > ResourceHostMessage_Request, and URLRequest, as well as the glue code
> > between
> > > > each of those.  But I don't see any reason why it wouldn't work.
> > > > 
> > > > Would you be ok with that plan?  I don't see any place to put the
> > information
> > > > once the request has been started other than the URLRequest, which I
think
> > you
> > > > objected to?
> > > > 
> > > > (Not saying this has to happen tomorrow, or any time in particular--as
> noted
> > > > above, I'm just trying to figure out what the right long term plan is so
> > that
> > > we
> > > > can do short term planning that advances it instead of retarding it.)
> > > 
> > > There shouldn't be any need to put it in URLRequest. In
> > > ResourceDispatcherHostImpl::BeginRequest a bunch of ResourceHandlers are
> > created
> > > and associated with the URLRequest. One or more of them are presumably
> > > responsible for detecting when a download should begin, and they should be
> > able
> > > to hold the referer charset for that eventuality.
> > > 
> > > Or, you could probably put it in and retrieve it from the
> > > ResourceRequestInfoImpl.
> > 
> > Good point.  I'd be good with either of those options.
> 
> To be clear: Now that I think we have a vision of where we want to go
long-term,
> I'm cool with the original CL.  Getting things out of the URLRequestContext
> sounds fine to me.

I don't think Rietveld formally recognizes "I'm cool with" as an approval ;)

> 
> Paul, if you feel like taking on the plumbing that Erik's sketched out above,
I
> certainly wouldn't object--I don't consider it a high priority, just a nice to
> have at some point.  And I'd certainly understand if you don't want to do it. 
> If you aren't planning to do it, I'll file a bug on it and prioritize it in
with
> the other downloads bugs.

We have (well, Paul has) a non-negligible backlog of work to rip-out the
URLRequestContext. I'd suggest filing a bug for now (this is not a regression or
functional change - we are just formally documenting what used to be a lost
TODO) and then letting it be prioritized accordingly.

[Randy Smith (Not in Mondays), 2012-10-21 01:04:46]

On 2012/10/21 01:02:11, erikwright wrote:
> On 2012/10/21 00:12:47, rdsmith wrote:
> > On 2012/10/17 23:06:23, rdsmith wrote:
> > > On 2012/10/17 23:04:07, erikwright wrote:
> > > > On 2012/10/17 21:25:47, rdsmith wrote:
> > > > > On 2012/10/15 20:39:33, erikwright wrote:
> > > > > > On 2012/10/15 16:06:20, rdsmith wrote:
> > > > > > > On 2012/10/15 14:51:57, erikwright wrote:
> > > > > > > > I did a fair bit of looking into this.
> > > > > > > > 
> > > > > > > > The direct "client" in this case is
> > > > > > ResourceDispatcherHostImpl::BeginRequest,
> > > > > > > > which creates the Request and also sets up all the resource
> > handlers.
> > > > > > > > 
> > > > > > > > If BeginRequest _actually_knew_ the referrer charset, we would
be
> > > > golden.
> > > > > I
> > > > > > > > would just plug it into the BufferedResourceHandler that is
> created
> > > for
> > > > > the
> > > > > > > > request.
> > > > > > > > 
> > > > > > > > Unfortunately, BeginRequest does _not_ receive this info in
> > > > > > > > ResourceHostMsg_Request.
> > > > > > > > 
> > > > > > > > I think a pretty reasonable (perhaps even "right") alternative
is
> > for
> > > > the
> > > > > > > > ChromeDownloadManagerDelegate to know the charsets for recently
> > > received
> > > > > > > > documents. A FIFO cache mapping URLs to charsets would be more
> than
> > > > > > > sufficient.
> > > > > > > > 
> > > > > > > > It could have that information via
> > > > > ChromeNetworkDelegate::OnHeadersReceived.
> > > > > > > > 
> > > > > > > > So I would do this:
> > > > > > > > 
> > > > > > > > 1) Define a class ReferrerCharsetCache. It would be owned by the
> > > > profile.
> > > > > > > > 
> > > > > > > > 2) Pass a pointer to (1) to ChromeNetworkDelegate. It should
> notify
> > > the
> > > > > > > > ReferrerCharsetCache in OnHeadersReceived.
> > > > > > > > 
> > > > > > > > 3) Pass a pointer to (1) to ChromeDownloadManagerDelegate. It
> should
> > > > query
> > > > > > for
> > > > > > > a
> > > > > > > > cached referrer charset in CheckVisitedReferrerBeforeDone
> > > > > > > 
> > > > > > > Well, that would certainly work, but I wince at doing it that
> > way--we're
> > > > > > > creating a side channel communications medium (at least a little
> > > > unreliable,
> > > > > > > because of the cache aspect) rather than directly passing the
> > > information
> > > > > from
> > > > > > > the place that has it to the place that wants it.  Did you look at
> how
> > > > hard
> > > > > it
> > > > > > > would be to plumb the information into ResourceHostMsg_Request?
> > > > > > 
> > > > > > Personally, I don't think that the "side channel" aspect is wrong.
> From
> > an
> > > > > > encapsulation point of view, it seems more wrong to me that so many
> > > > components
> > > > > > between the renderer, the content layer, the downloads subsystem,
and
> > > > > > chrome/browser should need to be modified to implement this feature.
> > > > > 
> > > > > One point that came up while Paul and I were chatting locally is that
it
> > > isn't
> > > > > clear how long we'd have to keep entries in this cache around.  If I
> bring
> > > up
> > > > a
> > > > > couple of tabs when I start the browser, work in one of them for a
> couple
> > of
> > > > > hours, then go back and do a download in another, doesn't that run
some
> > risk
> > > > of
> > > > > blowing out the cache so that we can't get the information anymore?
> > > > > 
> > > > > > 
> > > > > > To plumb it into ResourceHostMsg_Request I believe you will need
> WebKit
> > > > > changes.
> > > > > > See the original commit message from 2009 for this feature:
> > > > > > 
> > > > > > http://codereview.chromium.org/83002
> > > > > > 
> > > > > > Is this still true? The request is originally received in
> > > > > > WebURLLoaderImpl::Context::Start, and in fact we only even see the
> > > referrer
> > > > > URL
> > > > > > because WebKit supplies it as an HTTP header on the WebURLRequest
> > object.
> > > > > > 
> > > > > > But perhaps it can be had in RenderViewImpl::willSendRequest. This
> > > function
> > > > > > appears to be called by webkit and have access to the frame that is
> > > > generating
> > > > > a
> > > > > > request. The WebFrame has a WebDocument, has an |encoding()|.
> Presumably
> > > > > that's
> > > > > > what you want. You could add it to the RequestExtraData
> > > > > > (content/common/request_extra_data.h) and unpack it in
> > > > > IPCResourceLoaderBridge.
> > > > > > From there eventually you can get it to ResourceHostMsg_Request.
> > > > > 
> > > > > [Caveat: my knowledge of the renderer and webkit is low; I asked a
> > colleague
> > > > for
> > > > > guidance in navigating this code to understand your suggestion.  ]
> > > > > 
> > > > > This sounds like it would work--not dirt simple, but not involving
> WebKit.
> > 
> > > > We'd
> > > > > have to modify the RequestExtraData,
ResourceLoaderBridge::RequestInfo,
> > > > > ResourceHostMessage_Request, and URLRequest, as well as the glue code
> > > between
> > > > > each of those.  But I don't see any reason why it wouldn't work.
> > > > > 
> > > > > Would you be ok with that plan?  I don't see any place to put the
> > > information
> > > > > once the request has been started other than the URLRequest, which I
> think
> > > you
> > > > > objected to?
> > > > > 
> > > > > (Not saying this has to happen tomorrow, or any time in particular--as
> > noted
> > > > > above, I'm just trying to figure out what the right long term plan is
so
> > > that
> > > > we
> > > > > can do short term planning that advances it instead of retarding it.)
> > > > 
> > > > There shouldn't be any need to put it in URLRequest. In
> > > > ResourceDispatcherHostImpl::BeginRequest a bunch of ResourceHandlers are
> > > created
> > > > and associated with the URLRequest. One or more of them are presumably
> > > > responsible for detecting when a download should begin, and they should
be
> > > able
> > > > to hold the referer charset for that eventuality.
> > > > 
> > > > Or, you could probably put it in and retrieve it from the
> > > > ResourceRequestInfoImpl.
> > > 
> > > Good point.  I'd be good with either of those options.
> > 
> > To be clear: Now that I think we have a vision of where we want to go
> long-term,
> > I'm cool with the original CL.  Getting things out of the URLRequestContext
> > sounds fine to me.
> 
> I don't think Rietveld formally recognizes "I'm cool with" as an approval ;)

Well, someone should fix that!  Paul??  (:-}!)

More seriously, I'm not actually approving this CL, I'm just making clear I'm
not blocking it.  I'll put it on my review queue (probably later this weekend),
or Ben or Asanka could do it if they want--it really doesn't need all three of
us.  

> > Paul, if you feel like taking on the plumbing that Erik's sketched out
above,
> I
> > certainly wouldn't object--I don't consider it a high priority, just a nice
to
> > have at some point.  And I'd certainly understand if you don't want to do
it. 
> > If you aren't planning to do it, I'll file a bug on it and prioritize it in
> with
> > the other downloads bugs.
> 
> We have (well, Paul has) a non-negligible backlog of work to rip-out the
> URLRequestContext. I'd suggest filing a bug for now (this is not a regression
or
> functional change - we are just formally documenting what used to be a lost
> TODO) and then letting it be prioritized accordingly.

Ok, will do.

[Randy Smith (Not in Mondays), 2012-10-22 15:57:39]

LGTM.

[commit-bot: I haz the power, 2012-10-22 21:57:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11092088/2001

[commit-bot: I haz the power, 2012-10-22 21:57:22]

Failed to apply patch for content/browser/download/download_resource_handler.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/download/download_resource_handler.cc
  Hunk #1 FAILED at 197.
  1 out of 1 hunk FAILED -- saving rejects to file
content/browser/download/download_resource_handler.cc.rej

Patch:       content/browser/download/download_resource_handler.cc
Index: content/browser/download/download_resource_handler.cc
diff --git a/content/browser/download/download_resource_handler.cc
b/content/browser/download/download_resource_handler.cc
index
e42d0b432b3c299548fdf68d8425900ca6d47458..446ab10fdf260c4f9e8b7e556c2a16f60b3807a5
100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -197,7 +197,6 @@ bool DownloadResourceHandler::OnResponseStarted(
 
   info->prompt_user_for_save_location =
       save_info_.prompt_for_save_location && save_info_.file_path.empty();
-  info->referrer_charset = request_->context()->referrer_charset();
   info->save_info = save_info_;
 
   BrowserThread::PostTask(

[commit-bot: I haz the power, 2012-10-23 02:50:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11092088/28001

[commit-bot: I haz the power, 2012-10-23 02:50:22]

Presubmit check for 11092088-28001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/shell
    chrome/browser/profiles

Presubmit checks took 2.2s to calculate.

[pauljensen, 2012-10-23 13:15:57]

Matt, could you take a look at the chrome/browser/profiles/profile_io_data.*
changes?
Jochen, could you take a look at the
content/shell/shell_download_manager_delegate.cc change?

To fill you in on the previous discussion: The referrer_charset field has been
set (only in Chrome) to the user's chosen default charset for the last 8 months,
which is not what it was meant to indicate.  Its meant to indicate the charset
of the web page that referred us to a particular URL/download.  Its used to come
up with a filename to save a download to.  After some discussion we've come up
with a long-term plan, crbug.com/157222, for how to plumb in the actual referrer
charset.  This plan does not involve URLRequestContext, so for now we've decided
to pull out the misnamed/broken field.  I believe this change is non-functional;
in the Chrome downloads code we simply grab the user's chosen default charset
directly.

[mmenke, 2012-10-23 16:39:23]

profiles/ LGTM

[jochen (gone - plz use gerrit), 2012-10-23 16:50:46]

content/shell lgtm

[commit-bot: I haz the power, 2012-10-23 17:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11092088/28001

[commit-bot: I haz the power, 2012-10-23 20:00:08]

Change committed as 163652