Implement HTTP headers sniffing in the ProtocolSinkWrap, instead of relying o...

Implement HTTP headers sniffing in the ProtocolSinkWrap, instead of relying on IHttpNegotiate patch.
Moved some code based on over-conservative assumptions to execute earlier in pipeline.
Will prevent BUG=38480
to manifest itself.
BUG=47879
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52337

[tommi, 2010-07-12 21:58:40]

lgtm.

http://codereview.chromium.org/2987001/diff/16001/17002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/2987001/diff/16001/17002#newcode199
chrome_frame/protocol_sink_wrap.cc:199: char buffer[32] = "x-ua-compatible";
use the kUACompatibleHttpHeader constant

http://codereview.chromium.org/2987001/diff/16001/17002#newcode200
chrome_frame/protocol_sink_wrap.cc:200: DWORD len = 32;
sizeof(buffer)?

http://codereview.chromium.org/2987001/diff/16001/17002#newcode204
chrome_frame/protocol_sink_wrap.cc:204: if (StrStrIA(buffer, "chrome=1")) {
create a constant for "chrome=1"?

http://codereview.chromium.org/2987001/diff/16001/17002#newcode316
chrome_frame/protocol_sink_wrap.cc:316: if (renderer_type_ == UNDETERMINED)
two of these return statements aren't necessary and you could omit the
UNDETERMINED check.

could be as simple as:

if (render_type_ == CHROME) {
 DLOG(INFO) << "Forwarding BINDSTATUS_MIMETYPEAVAILABLE "
            << kChromeMimeType;
  delegate->ReportProgress(BINDSTATUS_MIMETYPEAVAILABLE, kChromeMimeType)
} else if (render_type_ == OTHER) {
  FireSugestedMimeType(delegate);
}

return S_OK;

http://codereview.chromium.org/2987001/diff/16001/17002#newcode366
chrome_frame/protocol_sink_wrap.cc:366: // This is the first data notification
we forward, since up to now we hold.
"since up to now we hold" ... seems to be something missing from that sentence.

[amit, 2010-07-12 22:27:07]

This is nice because it will avoid crash 38480!
lgtm with what Tommi said.

http://codereview.chromium.org/2987001/diff/16001/17002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/2987001/diff/16001/17002#newcode314
chrome_frame/protocol_sink_wrap.cc:314: renderer_type_ =
DetermineRendererTypeFromMetaData(suggested_mime_type_,
we should call this only if we get a valid 'info' otherwise, based on the
DetermineRendererTypeFromMetaData implementation we'd never report a mime type
for text/html if we don't get hold of IWinInetHttpInfo

http://codereview.chromium.org/2987001/diff/16001/17002#newcode316
chrome_frame/protocol_sink_wrap.cc:316: if (renderer_type_ == UNDETERMINED)
On 2010/07/12 21:58:41, tommi wrote:
> two of these return statements aren't necessary and you could omit the
> UNDETERMINED check.
> 
> could be as simple as:
> 
> if (render_type_ == CHROME) {
>  DLOG(INFO) << "Forwarding BINDSTATUS_MIMETYPEAVAILABLE "
>             << kChromeMimeType;
>   delegate->ReportProgress(BINDSTATUS_MIMETYPEAVAILABLE, kChromeMimeType)
> } else if (render_type_ == OTHER) {
>   FireSugestedMimeType(delegate);
> }
> 
> return S_OK;
I would personally prefer a switch. But anyway you do it, a comment explaining
each choice and what needs to happen will help a lot.

[stoyan, 2010-07-12 22:58:57]

http://codereview.chromium.org/2987001/diff/16001/17002
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/2987001/diff/16001/17002#newcode199
chrome_frame/protocol_sink_wrap.cc:199: char buffer[32] = "x-ua-compatible";
kUACompatibleHttpHeader is wchar.
Additionally if constant is used then it's not easy to pick the buffer size.

http://codereview.chromium.org/2987001/diff/16001/17002#newcode200
chrome_frame/protocol_sink_wrap.cc:200: DWORD len = 32;
On 2010/07/12 21:58:41, tommi wrote:
> sizeof(buffer)?

Done.

http://codereview.chromium.org/2987001/diff/16001/17002#newcode314
chrome_frame/protocol_sink_wrap.cc:314: renderer_type_ =
DetermineRendererTypeFromMetaData(suggested_mime_type_,
If we don't get IWinInetHttpInfo, we still can have metatag inside the content,
so we hold reporting MIME type until we fetch enough data.
I'm going to fix the TODO in ShouldWrapCallback() anyway, so we care only for
HTTP/HTTPS requests, where IWinInetHttpInfo should be available.

http://codereview.chromium.org/2987001/diff/16001/17002#newcode316
chrome_frame/protocol_sink_wrap.cc:316: if (renderer_type_ == UNDETERMINED)
On 2010/07/12 22:27:07, amit wrote:
> On 2010/07/12 21:58:41, tommi wrote:
> > two of these return statements aren't necessary and you could omit the
> > UNDETERMINED check.
> > 
> > could be as simple as:
> > 
> > if (render_type_ == CHROME) {
> >  DLOG(INFO) << "Forwarding BINDSTATUS_MIMETYPEAVAILABLE "
> >             << kChromeMimeType;
> >   delegate->ReportProgress(BINDSTATUS_MIMETYPEAVAILABLE, kChromeMimeType)
> > } else if (render_type_ == OTHER) {
> >   FireSugestedMimeType(delegate);
> > }
> > 
> > return S_OK;
> I would personally prefer a switch. But anyway you do it, a comment explaining
> each choice and what needs to happen will help a lot.
> 

Done.

http://codereview.chromium.org/2987001/diff/16001/17002#newcode366
chrome_frame/protocol_sink_wrap.cc:366: // This is the first data notification
we forward, since up to now we hold.
On 2010/07/12 21:58:41, tommi wrote:
> "since up to now we hold" ... seems to be something missing from that
sentence.

Done.