Add the chromeframe tag to the user agent header at runtime instead of static...

Add the chromeframe tag to the user agent header at runtime instead of statically in the registry.TEST=Try disabling GCF and see if the chromeframe tag in the user agent is still set.  It should not be. Also make sure you don't have an older version installed... the chromeframe tag should not be in the registry - if it is, you've got an older version still registered.  This should fix the issue with going to wave.google.com after disabling chrome frame and seeing the white page of death.R=amitBUG=22760
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29370

[amit, 2009-10-02 22:02:04]

Thanks Tommi for this investigation and fix, it looks pretty good!  Now I have
an idea that should blow your pants away, but wait, you aren't wearing any today
:)

So the idea is to make the Post Platform registry key with REG_EXPAND_SZ and
when we get loaded set the appropriate environment variable. So set the post
platform key to something like 'app'='%chromeframe% and set the %chromeframe% in
the process environment whenever we get loaded. This will change the way UA
looks now, but will be vastly simpler and robust.

Makes sense?

http://codereview.chromium.org/259025/diff/1/5
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/1/5#newcode299
Line 299: std::string ret;
I am wondering if we can just use Tokenize. Also, I am not sure if we have to be
in a certain place in the UA string. If we have some liberty, then we can just
append 'chromeframe' to the UA if it's not already there?

[tommi (sloooow) - chröme, 2009-10-05 17:15:18]

On 2009/10/02 22:02:04, amit wrote:
> Thanks Tommi for this investigation and fix, it looks pretty good!  Now I have
> an idea that should blow your pants away, but wait, you aren't wearing any
today
> :)
> 
> So the idea is to make the Post Platform registry key with REG_EXPAND_SZ and
> when we get loaded set the appropriate environment variable. So set the post
> platform key to something like 'app'='%chromeframe% and set the %chromeframe%
in
> the process environment whenever we get loaded. This will change the way UA
> looks now, but will be vastly simpler and robust.
> 
> Makes sense?
> 

Vastly simpler? yes.  more robust? don't think so because the value is ignored
:)

[tommi (sloooow) - chröme, 2009-10-05 17:15:29]

http://codereview.chromium.org/259025/diff/1/5
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/1/5#newcode299
Line 299: std::string ret;
On 2009/10/02 22:02:04, amit wrote:
> I am wondering if we can just use Tokenize. Also, I am not sure if we have to
be
> in a certain place in the UA string. If we have some liberty, then we can just
> append 'chromeframe' to the UA if it's not already there? 

We should be in the product/comment section of the first user agent string. 
Here's some useful info:
http://www.texsoft.it/index.php?c=software&m=sw.php.useragent&l=it

[amit, 2009-10-05 17:43:12]

On Mon, Oct 5, 2009 at 10:15 AM, <tommi@chromium.org> wrote:

>
> http://codereview.chromium.org/259025/diff/1/5
> File chrome_frame/html_utils.cc (right):
>
> http://codereview.chromium.org/259025/diff/1/5#newcode299
> Line 299: std::string ret;
> On 2009/10/02 22:02:04, amit wrote:
>
>> I am wondering if we can just use Tokenize. Also, I am not sure if we
>>
> have to be
>
>> in a certain place in the UA string. If we have some liberty, then we
>>
> can just
>
>> append 'chromeframe' to the UA if it's not already there?
>>
>
> We should be in the product/comment section of the first user agent
> string.  Here's some useful info:
> http://www.texsoft.it/index.php?c=software&m=sw.php.useragent&l=it
>
> Makes sense, 'Post Platform' is an attempt to be there and it's not
perfect. Is there any other way we could leverage the power of
REG_EXPAND_SZ, maybe adding something under the 'User Agent'?  I think the
current implementation is quite good with a separate VTable patch than the
sink wrap, but I'd be very hapy if we could still avoid it.

>
> http://codereview.chromium.org/259025
>

[tommi (sloooow) - chröme, 2009-10-05 18:10:42]

The only thing I can think of registry wise is the "User Agent" value
under.HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Internet
Settings
However, that value is set by IE and is of type REG_SZ.  I don't think we
should change it.

On Mon, Oct 5, 2009 at 1:41 PM, Amit Joshi <amit@chromium.org> wrote:

>
>
> On Mon, Oct 5, 2009 at 10:15 AM, <tommi@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/259025/diff/1/5
>> File chrome_frame/html_utils.cc (right):
>>
>> http://codereview.chromium.org/259025/diff/1/5#newcode299
>> Line 299: std::string ret;
>> On 2009/10/02 22:02:04, amit wrote:
>>
>>> I am wondering if we can just use Tokenize. Also, I am not sure if we
>>>
>> have to be
>>
>>> in a certain place in the UA string. If we have some liberty, then we
>>>
>> can just
>>
>>> append 'chromeframe' to the UA if it's not already there?
>>>
>>
>> We should be in the product/comment section of the first user agent
>> string.  Here's some useful info:
>> http://www.texsoft.it/index.php?c=software&m=sw.php.useragent&l=it
>>
>> Makes sense, 'Post Platform' is an attempt to be there and it's not
> perfect. Is there any other way we could leverage the power of
> REG_EXPAND_SZ, maybe adding something under the 'User Agent'?  I think the
> current implementation is quite good with a separate VTable patch than the
> sink wrap, but I'd be very hapy if we could still avoid it.
>
>>
>> http://codereview.chromium.org/259025
>>
>
>

[amit, 2009-10-06 00:15:22]

Yeah :( I did some more research, looked at 'UA tokens' but that too seems
like just values. Will continue reviewing it

On Mon, Oct 5, 2009 at 11:10 AM, Tommi <tommi@chromium.org> wrote:

> The only thing I can think of registry wise is the "User Agent" value
> under.HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Internet
> Settings
> However, that value is set by IE and is of type REG_SZ.  I don't think we
> should change it.
>
> On Mon, Oct 5, 2009 at 1:41 PM, Amit Joshi <amit@chromium.org> wrote:
>
>>
>>
>> On Mon, Oct 5, 2009 at 10:15 AM, <tommi@chromium.org> wrote:
>>
>>>
>>> http://codereview.chromium.org/259025/diff/1/5
>>> File chrome_frame/html_utils.cc (right):
>>>
>>> http://codereview.chromium.org/259025/diff/1/5#newcode299
>>> Line 299: std::string ret;
>>> On 2009/10/02 22:02:04, amit wrote:
>>>
>>>> I am wondering if we can just use Tokenize. Also, I am not sure if we
>>>>
>>> have to be
>>>
>>>> in a certain place in the UA string. If we have some liberty, then we
>>>>
>>> can just
>>>
>>>> append 'chromeframe' to the UA if it's not already there?
>>>>
>>>
>>> We should be in the product/comment section of the first user agent
>>> string.  Here's some useful info:
>>> http://www.texsoft.it/index.php?c=software&m=sw.php.useragent&l=it
>>>
>>> Makes sense, 'Post Platform' is an attempt to be there and it's not
>> perfect. Is there any other way we could leverage the power of
>> REG_EXPAND_SZ, maybe adding something under the 'User Agent'?  I think the
>> current implementation is quite good with a separate VTable patch than the
>> sink wrap, but I'd be very hapy if we could still avoid it.
>>
>>>
>>> http://codereview.chromium.org/259025
>>>
>>
>>
>

[amit, 2009-10-06 09:08:19]

First pass at it. This is a big effort and I like the tests :)

http://codereview.chromium.org/259025/diff/1/5
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/1/5#newcode299
Line 299: std::string ret;
I would lean towards avoiding the parsing logic. It would be OK if the resulting
UA not in ideal form but just reasonable and predictable.

Also, thanks for this link, it's really detailed. Maybe we just check for the
presence of 'chromeframe' in the UA and simply append the string
'chromeframe/<version> at the end if not already there? Similar to what Opera
does?

http://codereview.chromium.org/259025/diff/1/5#newcode385
Line 385: std::string value(it.values());
need this only if the name is 'user-agent', move down?

http://codereview.chromium.org/259025/diff/1/2
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/259025/diff/1/2#newcode71
Line 71: vtable_patch::DynamicPatchManager
We need a DynamicPatchManager because there are different kinds of sinks? I was
hoping that the sink for HTTP and HTTPS would be the same.

http://codereview.chromium.org/259025/diff/1/2#newcode116
Line 116: service_provider.QueryFrom(to_patch);
DoQueryService(...)?

http://codereview.chromium.org/259025/diff/1/3
File chrome_frame/protocol_sink_wrap.h (right):

http://codereview.chromium.org/259025/diff/1/3#newcode46
Line 46: class DelegateQueryInterface {
I like the idea to factor out generic functionality. I am wondering what would
happen if we end up inheriting from  DelegateQueryInterface<> for more than one
interface. The macro would probably complain for duplicate definitions of
CheckOutgoingInterface. 

Maybe CheckOutgoingInterface and  QueryInterfaceIfDelegateSupports are just
utility template functions and T supports delegate() method?

[tommi (sloooow) - chröme, 2009-10-06 15:18:56]

thanks! I'll upload a new patch in a few minutes and ping you back.  just wanted
to send you some of the comments before I do.

http://codereview.chromium.org/259025/diff/1/5
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/1/5#newcode299
Line 299: std::string ret;
On 2009/10/06 09:08:23, amit wrote:
> I would lean towards avoiding the parsing logic. It would be OK if the
resulting
> UA not in ideal form but just reasonable and predictable.
> 

The way it is now is predictable.  chromeframe is always the last entry in the
first product section of the header.  The only case where we wouldn't be last is
when someone modifies the headers further (which I think we should assume will
happen).

> Also, thanks for this link, it's really detailed. Maybe we just check for the
> presence of 'chromeframe' in the UA and simply append the string
> 'chromeframe/<version> at the end if not already there? 
Similar to what Opera
> does?
> 

If we do that then the string can be in a random and non standard location and
parsing it will be harder for webdevs, no?
>

http://codereview.chromium.org/259025/diff/1/5#newcode385
Line 385: std::string value(it.values());
On 2009/10/06 09:08:23, amit wrote:
> need this only if the name is 'user-agent', move down?

we always need it.  if name is user-agent, then we add the tag, otherwise we use
the existing value.

http://codereview.chromium.org/259025/diff/1/2
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/259025/diff/1/2#newcode71
Line 71: vtable_patch::DynamicPatchManager
On 2009/10/06 09:08:23, amit wrote:
> We need a DynamicPatchManager because there are different kinds of sinks? I
was
> hoping that the sink for HTTP and HTTPS would be the same. 

the sinks for http and https are the same but the html sinks vs script sinks
aren't unfortunately.

http://codereview.chromium.org/259025/diff/1/2#newcode116
Line 116: service_provider.QueryFrom(to_patch);
On 2009/10/06 09:08:23, amit wrote:
> DoQueryService(...)?

will do.

http://codereview.chromium.org/259025/diff/1/3
File chrome_frame/protocol_sink_wrap.h (right):

http://codereview.chromium.org/259025/diff/1/3#newcode46
Line 46: class DelegateQueryInterface {
On 2009/10/06 09:08:23, amit wrote:
> I like the idea to factor out generic functionality. I am wondering what would
> happen if we end up inheriting from  DelegateQueryInterface<> for more than
one
> interface. The macro would probably complain for duplicate definitions of
> CheckOutgoingInterface. 
> 
> Maybe CheckOutgoingInterface and  QueryInterfaceIfDelegateSupports are just
> utility template functions and T supports delegate() method?

good idea.  I'll ping you again when I've done that.

[tommi (sloooow) - chröme, 2009-10-06 18:54:38]

new changes uploaded.

[amit, 2009-10-07 21:36:03]

one more pass at it ...

http://codereview.chromium.org/259025/diff/6001/6005
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/6001/6005#newcode305
Line 305: index++;
index could roll over the length here?

http://codereview.chromium.org/259025/diff/6001/6005#newcode325
Line 325: index++;
Index could overflow the valid length here and elsewhere. In general I am
thinking we should just be appending it at the end. Confirmed with Alex that it
doesn't matter much where it is. More so since the check is just an existence of
the tag.

http://codereview.chromium.org/259025/diff/6001/6002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/259025/diff/6001/6002#newcode210
Line 210: PatchHttpNegotiate(prot_sink);
We will do this for *every* request. Maybe we could do this only for top level
requests. Also, this can be moved into MaybeWrapSink since that's common,
instead of doing it twice.

http://codereview.chromium.org/259025/diff/6001/6002#newcode258
Line 258: wchar_t* headers = *additional_headers;
A formal argument is overridden using a local variable, intended?

http://codereview.chromium.org/259025/diff/6001/6002#newcode266
Line 266: std::string ascii_headers(WideToASCII(headers));
what if the user agent header is present in the |headers| but not in
|additional_headers|?

http://codereview.chromium.org/259025/diff/6001/6003
File chrome_frame/protocol_sink_wrap.h (right):

http://codereview.chromium.org/259025/diff/6001/6003#newcode219
Line 219: IInternetProtocolSink* delegate() const {
nit: Should return IUnknown?

http://codereview.chromium.org/259025/diff/6001/6007
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/259025/diff/6001/6007#newcode242
Line 242: "Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.2)
Gecko/20030208 "
80

[tommi (sloooow) - chröme, 2009-10-08 17:24:58]

more updates on the way, I'll let you know.

http://codereview.chromium.org/259025/diff/6001/6005
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/6001/6005#newcode305
Line 305: index++;
On 2009/10/07 21:36:04, amit wrote:
> index could roll over the length here?
isspace should avoid that, right?  it's ok if index points to the \0

http://codereview.chromium.org/259025/diff/6001/6005#newcode325
Line 325: index++;
On 2009/10/07 21:36:04, amit wrote:
> Index could overflow the valid length here and elsewhere. In general I am
> thinking we should just be appending it at the end. Confirmed with Alex that
it
> doesn't matter much where it is. More so since the check is just an existence
of
> the tag. 

I'm changing the code as suggested (add to the end of the string).
the value of index here should however always be safe.

http://codereview.chromium.org/259025/diff/6001/6002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/259025/diff/6001/6002#newcode210
Line 210: PatchHttpNegotiate(prot_sink);
On 2009/10/07 21:36:04, amit wrote:
> We will do this for *every* request. Maybe we could do this only for top level
> requests. Also, this can be moved into MaybeWrapSink since that's common,
> instead of doing it twice.

Currently MaybeWrapSink does not detect requests from scripts.  We don't patch
for every request, most of the time we will only do a lookup.

http://codereview.chromium.org/259025/diff/6001/6002#newcode258
Line 258: wchar_t* headers = *additional_headers;
On 2009/10/07 21:36:04, amit wrote:
> A formal argument is overridden using a local variable, intended?
oops. thanks!

http://codereview.chromium.org/259025/diff/6001/6002#newcode266
Line 266: std::string ascii_headers(WideToASCII(headers));
On 2009/10/07 21:36:04, amit wrote:
> what if the user agent header is present in the |headers| but not in
> |additional_headers|?

additional_headers is the only variable we can modify.  additional_headers
should also override other headers.
In practice though, User-Agent isn't there.

http://codereview.chromium.org/259025/diff/6001/6003
File chrome_frame/protocol_sink_wrap.h (right):

http://codereview.chromium.org/259025/diff/6001/6003#newcode219
Line 219: IInternetProtocolSink* delegate() const {
On 2009/10/07 21:36:04, amit wrote:
> nit: Should return IUnknown?

I changed it to IInternetProtocolSink since that's the actual delegate of the
class and it's IUnknown derived.  I figured it might be useful for some other
cases, but I can change it?

http://codereview.chromium.org/259025/diff/6001/6007
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/259025/diff/6001/6007#newcode242
Line 242: "Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.2)
Gecko/20030208 "
On 2009/10/07 21:36:04, amit wrote:
> 80

Done.

[amit, 2009-10-08 17:39:24]

nice, I'll wait for the new patch.

http://codereview.chromium.org/259025/diff/6001/6002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/259025/diff/6001/6002#newcode210
Line 210: PatchHttpNegotiate(prot_sink);
On 2009/10/08 17:24:59, tommi wrote:
> On 2009/10/07 21:36:04, amit wrote:
> > We will do this for *every* request. Maybe we could do this only for top
level
> > requests. Also, this can be moved into MaybeWrapSink since that's common,
> > instead of doing it twice.
> 
> Currently MaybeWrapSink does not detect requests from scripts.  We don't patch
> for every request, most of the time we will only do a lookup.

What I meant was that MaybeWrapSink is called in both places so we could simply
put this check there at the top. 
Also, I was wondering if there is a way to avoid this lookup for every request
by just doing this for top level requests.

http://codereview.chromium.org/259025/diff/6001/6002#newcode266
Line 266: std::string ascii_headers(WideToASCII(headers));
On 2009/10/08 17:24:59, tommi wrote:
> On 2009/10/07 21:36:04, amit wrote:
> > what if the user agent header is present in the |headers| but not in
> > |additional_headers|?
> 
> additional_headers is the only variable we can modify.  additional_headers
> should also override other headers.
> In practice though, User-Agent isn't there.
> 

Just to be safe, we should check for UA in both places, first in
additional_headers and then in headers if it is not there. Then add/replace the
UA header in additional_headers.

[tommi (sloooow) - chröme, 2009-10-08 17:59:03]

changes uploaded

http://codereview.chromium.org/259025/diff/6001/6002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/259025/diff/6001/6002#newcode210
Line 210: PatchHttpNegotiate(prot_sink);
> What I meant was that MaybeWrapSink is called in both places so we could
simply
> put this check there at the top. 
> Also, I was wondering if there is a way to avoid this lookup for every request
> by just doing this for top level requests.

Call to PatchHttpNegotiate moved to MaybeWrapSink.

Requests from scripts aren't top level, so we'd miss out on those requests.

http://codereview.chromium.org/259025/diff/6001/6002#newcode266
Line 266: std::string ascii_headers(WideToASCII(headers));
> Just to be safe, we should check for UA in both places, first in
> additional_headers and then in headers if it is not there. Then add/replace
the
> UA header in additional_headers.

sorry, I'm still not following...  We already handle the cases for
additional_header (if the user agent is there and if it is not there).

But for checking |headers|...

1) let's say we find that the User-Agent is in headers.
2) ...
3) profit?

:)

[tommi (sloooow) - chröme, 2009-10-14 17:11:50]

new changes uploaded.  as discussed offline I figured out a way to only patch
once at startup.  the ua header won't be tagged for every request (e.g. embedded
images), but top level URLs and script requests will.  I tested with IE6-8.

[ananta, 2009-10-14 17:39:59]

Looks great. Thanks for all the user agent tests :)

http://codereview.chromium.org/259025/diff/15001/15006
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/259025/diff/15001/15006#newcode29
Line 29: #include <atlsecurity.h> // NOLINT
Should this be moved to the top?

http://codereview.chromium.org/259025/diff/15001/15004
File chrome_frame/html_utils.h (right):

http://codereview.chromium.org/259025/diff/15001/15004#newcode122
Line 122: // Parses a user agent string and adds chrome frame to the
product/comment
We no longer add them to the comment section. Please update the comments.

http://codereview.chromium.org/259025/diff/15001/15008
File chrome_frame/vtable_patch_manager.cc (right):

http://codereview.chromium.org/259025/diff/15001/15008#newcode94
Line 94: DynamicPatchManager::DynamicPatchManager(const MethodPatchInfo*
patch_prototype)
Disable this class for now as we don't use it.

http://codereview.chromium.org/259025/diff/15001/15009
File chrome_frame/vtable_patch_manager.h (right):

http://codereview.chromium.org/259025/diff/15001/15009#newcode9
Line 9: 
Remove newline?

[tommi (sloooow) - chröme, 2009-10-14 18:13:05]

http://codereview.chromium.org/259025/diff/15001/15006
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/259025/diff/15001/15006#newcode29
Line 29: #include <atlsecurity.h> // NOLINT
On 2009/10/14 17:40:00, ananta wrote:
> Should this be moved to the top?

Done.

http://codereview.chromium.org/259025/diff/15001/15004
File chrome_frame/html_utils.h (right):

http://codereview.chromium.org/259025/diff/15001/15004#newcode122
Line 122: // Parses a user agent string and adds chrome frame to the
product/comment
On 2009/10/14 17:40:00, ananta wrote:
> We no longer add them to the comment section. Please update the comments.
> 

Done.

http://codereview.chromium.org/259025/diff/15001/15008
File chrome_frame/vtable_patch_manager.cc (right):

http://codereview.chromium.org/259025/diff/15001/15008#newcode94
Line 94: DynamicPatchManager::DynamicPatchManager(const MethodPatchInfo*
patch_prototype)
On 2009/10/14 17:40:00, ananta wrote:
> Disable this class for now as we don't use it.
> 

Done.

http://codereview.chromium.org/259025/diff/15001/15009
File chrome_frame/vtable_patch_manager.h (right):

http://codereview.chromium.org/259025/diff/15001/15009#newcode9
Line 9: 
On 2009/10/14 17:40:00, ananta wrote:
> Remove newline?

I checked against the chrome source and this seems to be the way #includes are
separated.  See for instance base/message_pump_win.h, base/mime_util_linux.cc

[amit, 2009-10-14 22:36:10]

looking better and I am still PIA :)

http://codereview.chromium.org/259025/diff/9002/14004
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/9002/14004#newcode296
Line 296: static char cf_user_agent[kArraySize] = {0};
would be simpler to use std::string?

http://codereview.chromium.org/259025/diff/9002/14004#newcode304
Line 304: wsprintfA(cf_user_agent, "%s/%i.%i", kChromeFrameUserAgent, high,
low);
If I understand this correctly:
%s corresponds to: sizeof(kChromeFrameUserAgent)
/%i.%i corresponds to: sizeof("XX/YYY")?
Then we need to use the %2i.%3i or the array might overflow? A static
std::string will simplify things.
Also the convention seems to be to terminate those with a ';'? so in this form:
chromeframe/2.0;

http://codereview.chromium.org/259025/diff/9002/14004#newcode311
Line 311: std::string AddChromeFrameToUserAgentValue(const std::string& value) {
Looks nice and compact now :)

http://codereview.chromium.org/259025/diff/9002/14002
File chrome_frame/protocol_sink_wrap.h (right):

http://codereview.chromium.org/259025/diff/9002/14002#newcode41
Line 41: // See COM_INTERFACE_BLIND_DELEGATE below for details.
This is nice and generic now, consider moving into utils?

http://codereview.chromium.org/259025/diff/15011/16005
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/15011/16005#newcode14
Line 14: 
nit: no newline needed between includes (this one and the above that).

http://codereview.chromium.org/259025/diff/15011/16005#newcode296
Line 296: _pAtlModule->m_csStaticDataInitAndTypeInfo.Lock();
nit: I think, using a base::singleton for a std::string is more portable. Also,
is it better to use CComModule::[Enter/Leave]StaticDataCriticalSection?

http://codereview.chromium.org/259025/diff/15011/16005#newcode328
Line 328: bool AddChromeFrameUATagToRequestHeaders(const std::string&
http_headers,
If you like the new suggested code in ProtocolSinkWrap then this may not be
needed anymore.

http://codereview.chromium.org/259025/diff/15011/16005#newcode385
Line 385: std::string GetHeaderValue(const std::string& headers,
nit: I am wondering if we can factor out this code with
AddChromeFrameUATagToRequestHeaders. Also, avoid parsing headers multiple times.

Maybe a helper like:
net::HttpUtil::HeadersIterator FindHeader(net::HttpUtil::HeadersIterator begin,
const char* header_name)?

http://codereview.chromium.org/259025/diff/15011/16002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/259025/diff/15011/16002#newcode101
Line 101: // Use our UrlmonUrlRequest class as we need a temporary object that
This doesn't need to be part of ProtocolSinkWrap and should be done in bho.cc
and irrespective of whether we are patching for protocol sink or OnHttpEquiv.
Also, it would be nice to wrap this in its own small helper.

http://codereview.chromium.org/259025/diff/15011/16002#newcode282
Line 282: std::string new_headers;
I think at this point, we should move all the subsequent code to a helper:
HRESULT AppendChromeFrameHeaders(LPCWSTR headers, LPCWSTR additional_headers,
std::string* new_headers); 
A benefit of this is that we can now write unit tests for this function instead
of a helper. Also we could test with different combinations of |headers| and
|additional_headers|

http://codereview.chromium.org/259025/diff/15011/16002#newcode284
Line 284: if (*additional_headers) {
Consider the following logic. I think it's simpler and readable...

std::string ascii_additional_headers(WideToASCII(*additional_headers));
net::HttpUtil::HeadersIterator additional_headers_begin(
    ascii_additional_headers.begin(), ascii_additional_headers.end(), "\r\n");

// Todo: Implement
// net::HttpUtil::HeadersIterator FindHttpHeader(
//    net::HttpUtil::HeadersIterator begin, const char* header);

net::HttpUtil::HeadersIterator user_agent_header = FindHttpHeader(
    additional_headers_begin, kUserAgentHeader);
if (!user_agent_header) {
  // Find if UA header is present in the original headers.
  std::string ascii_original_headers(WideToASCII(headers));
  net::HttpUtil::HeadersIterator original_headers_begin(
      ascii_original_headers.begin(), ascii_original_headers.end(), "\r\n");

  user_agent_header = FindHttpHeader(additional_headers_begin,
                                     kUserAgentHeader);
}

std::string user_agent_value;
if (user_agent_header) {
  user_agent_value = AddChromeFrameToUserAgentValue(user_agent_header.values());
} else {
  user_agent_value = GetDefaultUserAgentHeaderWithCFTag();
}

// Build new headers, skip the existing user agent value from existing headers.
std::string new_headers;
net::HttpUtil::HeadersIterator it = additional_headers_begin;
while (it && it.GetNext()) {
   std::string name(it.name());
   std::string value(it.values());
   if (!LowerCaseEqualsASCII(name, "user-agent"))
     *new_headers += name + ": " + value + "\r\n";
}

new_headers += user_agent_value;
new_headers += "\r\n\r\n";

...

http://codereview.chromium.org/259025/diff/15011/16007
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/259025/diff/15011/16007#newcode219
Line 219: TEST_F(HtmlUtilUnittest, AddChromeFrameToUserAgentValue) {
nice tests! :)

[tommi (sloooow) - chröme, 2009-10-15 15:00:48]

I'll ping later today when I've split the sources up, changed the parsing code
and included the BeginningTransaction test.

http://codereview.chromium.org/259025/diff/9002/14004
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/9002/14004#newcode296
Line 296: static char cf_user_agent[kArraySize] = {0};
On 2009/10/14 22:36:10, amit wrote:
> would be simpler to use std::string?

it would be more code but same amount of lines.  this way there's no destructor
:)

http://codereview.chromium.org/259025/diff/9002/14004#newcode304
Line 304: wsprintfA(cf_user_agent, "%s/%i.%i", kChromeFrameUserAgent, high,
low);
On 2009/10/14 22:36:10, amit wrote:
> If I understand this correctly:
> %s corresponds to: sizeof(kChromeFrameUserAgent)

no sizeof().  just kChromeFrameUserAgent.

> /%i.%i corresponds to: sizeof("XX/YYY")?
> Then we need to use the %2i.%3i or the array might overflow? A static
> std::string will simplify things.
> Also the convention seems to be to terminate those with a ';'? so in this
form:
> chromeframe/2.0;
> 

hmm... either this is an old comment, or you're seeing an old version of the
patch.  you should be seeing the format string as simply "%s/%i.%i"

http://codereview.chromium.org/259025/diff/9002/14002
File chrome_frame/protocol_sink_wrap.h (right):

http://codereview.chromium.org/259025/diff/9002/14002#newcode41
Line 41: // See COM_INTERFACE_BLIND_DELEGATE below for details.
On 2009/10/14 22:36:10, amit wrote:
> This is nice and generic now, consider moving into utils?

Done.

http://codereview.chromium.org/259025/diff/15011/16005
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/15011/16005#newcode14
Line 14: 
On 2009/10/14 22:36:10, amit wrote:
> nit: no newline needed between includes (this one and the above that).

Done.

http://codereview.chromium.org/259025/diff/15011/16005#newcode296
Line 296: _pAtlModule->m_csStaticDataInitAndTypeInfo.Lock();
On 2009/10/14 22:36:10, amit wrote:
> nit: I think, using a base::singleton for a std::string is more portable.
Also,
> is it better to use CComModule::[Enter/Leave]StaticDataCriticalSection?

I agree on the portability point but do you know of a lock that's available for
global construction?  I'd rather not create a new one just for this.

As for CComModule::Enter/LeaveStaticDataCriticalSection, I had already tried it
- that critical section is uninitialized (does not seem to be used at all).

http://codereview.chromium.org/259025/diff/15011/16005#newcode328
Line 328: bool AddChromeFrameUATagToRequestHeaders(const std::string&
http_headers,
On 2009/10/14 22:36:10, amit wrote:
> If you like the new suggested code in ProtocolSinkWrap then this may not be
> needed anymore. 
/me likes

http://codereview.chromium.org/259025/diff/15011/16005#newcode385
Line 385: std::string GetHeaderValue(const std::string& headers,
On 2009/10/14 22:36:10, amit wrote:
> nit: I am wondering if we can factor out this code with
> AddChromeFrameUATagToRequestHeaders. Also, avoid parsing headers multiple
times.
> 
> Maybe a helper like:
> net::HttpUtil::HeadersIterator FindHeader(net::HttpUtil::HeadersIterator
begin,
> const char* header_name)?
> 
yup, better.

http://codereview.chromium.org/259025/diff/15011/16002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/259025/diff/15011/16002#newcode101
Line 101: // Use our UrlmonUrlRequest class as we need a temporary object that
On 2009/10/14 22:36:10, amit wrote:
> This doesn't need to be part of ProtocolSinkWrap and should be done in bho.cc
> and irrespective of whether we are patching for protocol sink or OnHttpEquiv.
> Also, it would be nice to wrap this in its own small helper.

agreed. I decided to keep it with the other patching code for now.
so.. new files for the httpnegotiate patch?

http://codereview.chromium.org/259025/diff/15011/16002#newcode282
Line 282: std::string new_headers;
On 2009/10/14 22:36:10, amit wrote:
> I think at this point, we should move all the subsequent code to a helper:
> HRESULT AppendChromeFrameHeaders(LPCWSTR headers, LPCWSTR additional_headers,
> std::string* new_headers); 
> A benefit of this is that we can now write unit tests for this function
instead
> of a helper. Also we could test with different combinations of |headers| and
> |additional_headers|

Agree.  I actually have a unit test for this function, but it's a bit messy at
the moment since I need to add the ProtocolSinkWrap code to the unit tests.

http://codereview.chromium.org/259025/diff/15011/16002#newcode284
Line 284: if (*additional_headers) {
sounds good.  I'll get cracking on this and upload the other smaller changes in
the meantime.  I'll ping you again for review - let me know what you think about
some of my other replies

[amit, 2009-10-15 15:12:36]

http://codereview.chromium.org/259025/diff/9002/14004
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/9002/14004#newcode296
Line 296: static char cf_user_agent[kArraySize] = {0};
On 2009/10/15 15:00:48, tommi wrote:
> On 2009/10/14 22:36:10, amit wrote:
> > would be simpler to use std::string?
> 
> it would be more code but same amount of lines.  this way there's no
destructor
> :)

I am not sure how these comments got in, they are probably from the last time. I
think we already had this conversation. Please ignore this one and the next :)

[tommi (sloooow) - chröme, 2009-10-15 20:01:41]

latest set of patches uploaded.
I've split the files, added tests, moved the calls to apply the patch to the
bho, added new files etc etc.

however, upon looking at the implementation of HttpHeaderIterator, I don't think
it's a very efficient struct to be throwing around and copy-constructing too
often.
Instead I think it would make sense to offer a find method in the
HttpHeaderIterator class itself.

If you don't mind, I would like to skip that refactoring in this patch and do it
in a separate patch (to avoid going into the third review week).

[tommi (sloooow) - chröme, 2009-10-15 21:08:07]

fyi - this patch is now depending on another patch:
http://codereview.chromium.org/273072

[tommi (sloooow) - chröme, 2009-10-15 21:23:41]

latest refactoring changes uploaded.

[amit, 2009-10-15 23:53:34]

lgtm with a couple of nits. Great stuff!

http://codereview.chromium.org/259025/diff/22001/22011
File chrome_frame/http_negotiate.cc (right):

http://codereview.chromium.org/259025/diff/22001/22011#newcode108
Line 108: scoped_ptr<HttpUtil::HeadersIterator> user_agent_header;
why not keep around the iterator instance created from ascii_headers for later?
Maybe if you have a rewind() to the iterator, that would enable you to reuse it?
In that case you can also have a static instance as opposed to creating it on
the heap.

http://codereview.chromium.org/259025/diff/22001/22012
File chrome_frame/test/http_negotiate_unittest.cc (right):

http://codereview.chromium.org/259025/diff/22001/22012#newcode55
Line 55: TEST_F(HttpNegotiateTest, BeginningTransaction) {
Excellent tests!

[tommi (sloooow) - chröme, 2009-10-16 02:14:02]

On Thu, Oct 15, 2009 at 7:53 PM, <amit@chromium.org> wrote:

> lgtm with a couple of nits. Great stuff!
>
yay!

>
>
> http://codereview.chromium.org/259025/diff/22001/22011
> File chrome_frame/http_negotiate.cc (right):
>
> http://codereview.chromium.org/259025/diff/22001/22011#newcode108
> Line 108: scoped_ptr<HttpUtil::HeadersIterator> user_agent_header;
> why not keep around the iterator instance created from ascii_headers for
> later? Maybe if you have a rewind() to the iterator, that would enable
> you to reuse it? In that case you can also have a static instance as
> opposed to creating it on the heap.
>

sounds good.  I'll update the other patch first and then ping you back.


>
> http://codereview.chromium.org/259025/diff/22001/22012
> File chrome_frame/test/http_negotiate_unittest.cc (right):
>
> http://codereview.chromium.org/259025/diff/22001/22012#newcode55
> Line 55: TEST_F(HttpNegotiateTest, BeginningTransaction) {
> Excellent tests!
>
>
> http://codereview.chromium.org/259025
>

[tommi (sloooow) - chröme, 2009-10-16 21:08:18]

changes done and uploaded.  note that they're subject to
http://codereview.chromium.org/276067 being committed.

http://codereview.chromium.org/259025/diff/22001/22011
File chrome_frame/http_negotiate.cc (right):

http://codereview.chromium.org/259025/diff/22001/22011#newcode108
Line 108: scoped_ptr<HttpUtil::HeadersIterator> user_agent_header;
On 2009/10/15 23:53:34, amit wrote:
> why not keep around the iterator instance created from ascii_headers for
later?
> Maybe if you have a rewind() to the iterator, that would enable you to reuse
it?
> In that case you can also have a static instance as opposed to creating it on
> the heap.

Done.

[amit, 2009-10-16 22:08:13]

lgtm. Looks wonderful, a few nits below.

http://codereview.chromium.org/259025/diff/23001/24004
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/23001/24004#newcode325
Line 325: bool AddChromeFrameUATagToRequestHeaders(const std::string&
http_headers,
Remove?

http://codereview.chromium.org/259025/diff/23001/24004#newcode382
Line 382: std::string GetHeaderValue(const std::string& headers,
Remove?

http://codereview.chromium.org/259025/diff/23001/24003
File chrome_frame/html_utils.h (right):

http://codereview.chromium.org/259025/diff/23001/24003#newcode132
Line 132: bool AddChromeFrameUATagToRequestHeaders(const std::string&
http_headers,
Remove?

http://codereview.chromium.org/259025/diff/23001/24003#newcode151
Line 151: std::string GetHeaderValue(const std::string& headers, const char*
header_name);
Remove?

http://codereview.chromium.org/259025/diff/23001/24010
File chrome_frame/http_negotiate.cc (right):

http://codereview.chromium.org/259025/diff/23001/24010#newcode127
Line 127: if (!user_agent_value.length())
empty()?

[tommi (sloooow) - chröme, 2009-10-16 22:16:06]

http://codereview.chromium.org/259025/diff/23001/24004
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/259025/diff/23001/24004#newcode325
Line 325: bool AddChromeFrameUATagToRequestHeaders(const std::string&
http_headers,
On 2009/10/16 22:08:13, amit wrote:
> Remove?

Done.

http://codereview.chromium.org/259025/diff/23001/24004#newcode382
Line 382: std::string GetHeaderValue(const std::string& headers,
On 2009/10/16 22:08:13, amit wrote:
> Remove?

Done.

http://codereview.chromium.org/259025/diff/23001/24003
File chrome_frame/html_utils.h (right):

http://codereview.chromium.org/259025/diff/23001/24003#newcode132
Line 132: bool AddChromeFrameUATagToRequestHeaders(const std::string&
http_headers,
On 2009/10/16 22:08:13, amit wrote:
> Remove?

Done.

http://codereview.chromium.org/259025/diff/23001/24003#newcode151
Line 151: std::string GetHeaderValue(const std::string& headers, const char*
header_name);
On 2009/10/16 22:08:13, amit wrote:
> Remove?

Done.

http://codereview.chromium.org/259025/diff/23001/24010
File chrome_frame/http_negotiate.cc (right):

http://codereview.chromium.org/259025/diff/23001/24010#newcode127
Line 127: if (!user_agent_value.length())
On 2009/10/16 22:08:13, amit wrote:
> empty()?

Done.