Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2620001:
A new way of hooking internet protocols. (Closed)
|
Created:
10 years, 6 months ago by stoyan Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionA new way of hooking internet protocols.
TEST=chrome frame tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49265
Patch Set 1 #Patch Set 2 : '' #
Total comments: 66
Patch Set 3 : '' #
Total comments: 16
Patch Set 4 : '' #
Total comments: 29
Patch Set 5 : '' #Patch Set 6 : '' #
Messages
Total messages: 12 (0 generated)
Excellent work and thanks for getting this up so quickly. Can't wait to see this getting into dev channel :) http://codereview.chromium.org/2620001/diff/9001/10002 File chrome_frame/bind_context_info.h (right): http://codereview.chromium.org/2620001/diff/9001/10002#newcode84 chrome_frame/bind_context_info.h:84: void put_prot_data(ProtData* data) { nit: use set to be consistent with others http://codereview.chromium.org/2620001/diff/9001/10003 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2620001/diff/9001/10003#newcode133 chrome_frame/protocol_sink_wrap.cc:133: std::string bindstatus2str(ULONG bind_status) { nice helper. Move this to a common place like utils so that others can use this? Also shouldn't this be available only in non-official (DEBUG) builds? http://codereview.chromium.org/2620001/diff/9001/10003#newcode232 chrome_frame/protocol_sink_wrap.cc:232: " error: " << error << " Text: " << (result_text ? result_text : L""); DCHECK(!is_undetermined())? http://codereview.chromium.org/2620001/diff/9001/10003#newcode244 chrome_frame/protocol_sink_wrap.cc:244: LPOLESTR bind_ctx_string = NULL; CComHeapPtr? http://codereview.chromium.org/2620001/diff/9001/10003#newcode249 chrome_frame/protocol_sink_wrap.cc:249: IBindCtx* pbc = reinterpret_cast<IBindCtx*>(StringToInt(bind_ctx_string)); Do they AddRef this pointer? Just making sure :) Also, do we need an explicit QI on this for IBindCtx to validate and AddRef? http://codereview.chromium.org/2620001/diff/9001/10003#newcode294 chrome_frame/protocol_sink_wrap.cc:294: RendererType DetermineRendererType(void* buffer, DWORD size, bool last_chance) { check validity of buffer, size etc? http://codereview.chromium.org/2620001/diff/9001/10003#newcode317 chrome_frame/protocol_sink_wrap.cc:317: InternetProtocol_Read_Fn read_fun, const wchar_t* url) nit: alignment http://codereview.chromium.org/2620001/diff/9001/10003#newcode339 chrome_frame/protocol_sink_wrap.cc:339: HRESULT ProtData::Read(void* buffer, ULONG size, ULONG* size_read) { validate input args? http://codereview.chromium.org/2620001/diff/9001/10003#newcode356 chrome_frame/protocol_sink_wrap.cc:356: // User buffer is greater than what we have. do we have to do this or we can simply return less data and wait for the next read? http://codereview.chromium.org/2620001/diff/9001/10003#newcode370 chrome_frame/protocol_sink_wrap.cc:370: if (status_code == BINDSTATUS_DIRECTBIND) { switch? http://codereview.chromium.org/2620001/diff/9001/10003#newcode412 chrome_frame/protocol_sink_wrap.cc:412: if (first_data_notification_ == false) { nit: !first_data_notification_ http://codereview.chromium.org/2620001/diff/9001/10003#newcode419 chrome_frame/protocol_sink_wrap.cc:419: if (!IsTextHtml(suggested_mime_type_.c_str())) { are we guaranteed to have a valid suggested_mime_type_ when we reach here? It's better to check in any case. http://codereview.chromium.org/2620001/diff/9001/10003#newcode436 chrome_frame/protocol_sink_wrap.cc:436: bool last_chance = false; nit: last_chance = (hr == S_OK || hr == S_FALSE)? :) http://codereview.chromium.org/2620001/diff/9001/10003#newcode450 chrome_frame/protocol_sink_wrap.cc:450: if (renderer_type_ == CHROME) { Is this happening for every ReportData? http://codereview.chromium.org/2620001/diff/9001/10003#newcode478 chrome_frame/protocol_sink_wrap.cc:478: HRESULT ProtData::FillBuffer() { nit: this helper attempts to read enough data to make a determination. So how about something like ReadContentSniffSize? http://codereview.chromium.org/2620001/diff/9001/10003#newcode508 chrome_frame/protocol_sink_wrap.cc:508: ScopedComPtr<IBindCtx> pbc; nit: bind_context or bind_ctx if you prefer a slightly shorter version :) http://codereview.chromium.org/2620001/diff/9001/10003#newcode538 chrome_frame/protocol_sink_wrap.cc:538: prot_data = new ProtData(protocol, read_fun, url); handing over the read_fun to ProtData is a bit scary. We have to make sure that we are always calling it with the same instance this was supposed to be called for. http://codereview.chromium.org/2620001/diff/9001/10003#newcode600 chrome_frame/protocol_sink_wrap.cc:600: HRESULT hr = prot_data->Read(buffer, size, size_read); DCHECK(prot_data->protocol_ == protocol)? http://codereview.chromium.org/2620001/diff/9001/10003#newcode641 chrome_frame/protocol_sink_wrap.cc:641: HRESULT hr = obj_->QueryInterface(riid, ppvObj); if (obj) http://codereview.chromium.org/2620001/diff/9001/10003#newcode649 chrome_frame/protocol_sink_wrap.cc:649: IUnknown* obj_; initialize obj_ http://codereview.chromium.org/2620001/diff/9001/10003#newcode652 chrome_frame/protocol_sink_wrap.cc:652: static void HookCTrans(IInternetProtocol* p) { nit: rename to HookTransactionVtable? http://codereview.chromium.org/2620001/diff/9001/10003#newcode669 chrome_frame/protocol_sink_wrap.cc:669: hr = session->RegisterNameSpace(&factory, CLSID_NULL, L"611", 0, 0, 0); nit: "611" -> kDummyNameSpace http://codereview.chromium.org/2620001/diff/9001/10004 File chrome_frame/protocol_sink_wrap.h (right): http://codereview.chromium.org/2620001/diff/9001/10004#newcode41 chrome_frame/protocol_sink_wrap.h:41: enum RendererType { Share this with the one declared in urlmon_bind_status_callback.h? http://codereview.chromium.org/2620001/diff/9001/10004#newcode62 chrome_frame/protocol_sink_wrap.h:62: public IInternetProtocolSink { since we are wrapping original sink, it is better to support/forward most the interfaces that the original sink implements. This is so that QI on the other interface for IInternetProtocolSink would come back to us.
great! please run lint http://codereview.chromium.org/2620001/diff/9001/10002 File chrome_frame/bind_context_info.h (right): http://codereview.chromium.org/2620001/diff/9001/10002#newcode93 chrome_frame/bind_context_info.h:93: scoped_refptr<ProtData> prot_data_; keep with other private members http://codereview.chromium.org/2620001/diff/9001/10003 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2620001/diff/9001/10003#newcode75 chrome_frame/protocol_sink_wrap.cc:75: static const char* const bindstatus_txt[] = { nit: only one space after char* http://codereview.chromium.org/2620001/diff/9001/10003#newcode133 chrome_frame/protocol_sink_wrap.cc:133: std::string bindstatus2str(ULONG bind_status) { suggest using the same naming convention as for other functions http://codereview.chromium.org/2620001/diff/9001/10003#newcode244 chrome_frame/protocol_sink_wrap.cc:244: LPOLESTR bind_ctx_string = NULL; indent http://codereview.chromium.org/2620001/diff/9001/10003#newcode258 chrome_frame/protocol_sink_wrap.cc:258: // TODO: check the url scheme for http/https. TODO(stoyan): http://codereview.chromium.org/2620001/diff/9001/10003#newcode374 chrome_frame/protocol_sink_wrap.cc:374: // if (status_code == BINDSTATUS_DECODING) { remove? http://codereview.chromium.org/2620001/diff/9001/10003#newcode443 chrome_frame/protocol_sink_wrap.cc:443: // TODO: How we are going to send BSCF_FIRSTDATANOTIFICATION then? todo(stoyan): http://codereview.chromium.org/2620001/diff/9001/10003#newcode456 chrome_frame/protocol_sink_wrap.cc:456: // TODO: DLOG same here http://codereview.chromium.org/2620001/diff/9001/10003#newcode461 chrome_frame/protocol_sink_wrap.cc:461: // comment? http://codereview.chromium.org/2620001/diff/9001/10003#newcode464 chrome_frame/protocol_sink_wrap.cc:464: // bytes_avail remove? http://codereview.chromium.org/2620001/diff/9001/10003#newcode605 chrome_frame/protocol_sink_wrap.cc:605: struct FakeProtocol : public CComObjectRootEx<CComSingleThreadModel>, class?
http://codereview.chromium.org/2620001/diff/9001/10002 File chrome_frame/bind_context_info.h (right): http://codereview.chromium.org/2620001/diff/9001/10002#newcode84 chrome_frame/bind_context_info.h:84: void put_prot_data(ProtData* data) { On 2010/06/04 01:08:53, amit wrote: > nit: use set to be consistent with others Done. http://codereview.chromium.org/2620001/diff/9001/10002#newcode93 chrome_frame/bind_context_info.h:93: scoped_refptr<ProtData> prot_data_; On 2010/06/04 09:01:53, tommi wrote: > keep with other private members Done. http://codereview.chromium.org/2620001/diff/9001/10003 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2620001/diff/9001/10003#newcode75 chrome_frame/protocol_sink_wrap.cc:75: static const char* const bindstatus_txt[] = { On 2010/06/04 09:01:53, tommi wrote: > nit: only one space after char* Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode133 chrome_frame/protocol_sink_wrap.cc:133: std::string bindstatus2str(ULONG bind_status) { On 2010/06/04 01:08:53, amit wrote: > nice helper. Move this to a common place like utils so that others can use this? > Also shouldn't this be available only in non-official (DEBUG) builds? Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode232 chrome_frame/protocol_sink_wrap.cc:232: " error: " << error << " Text: " << (result_text ? result_text : L""); On 2010/06/04 01:08:53, amit wrote: > DCHECK(!is_undetermined())? Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode249 chrome_frame/protocol_sink_wrap.cc:249: IBindCtx* pbc = reinterpret_cast<IBindCtx*>(StringToInt(bind_ctx_string)); On 2010/06/04 01:08:53, amit wrote: > Do they AddRef this pointer? Just making sure :) > Also, do we need an explicit QI on this for IBindCtx to validate and AddRef? yes. http://codereview.chromium.org/2620001/diff/9001/10003#newcode258 chrome_frame/protocol_sink_wrap.cc:258: // TODO: check the url scheme for http/https. On 2010/06/04 09:01:53, tommi wrote: > TODO(stoyan): Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode294 chrome_frame/protocol_sink_wrap.cc:294: RendererType DetermineRendererType(void* buffer, DWORD size, bool last_chance) { On 2010/06/04 01:08:53, amit wrote: > check validity of buffer, size etc? No. http://codereview.chromium.org/2620001/diff/9001/10003#newcode317 chrome_frame/protocol_sink_wrap.cc:317: InternetProtocol_Read_Fn read_fun, const wchar_t* url) On 2010/06/04 01:08:53, amit wrote: > nit: alignment > Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode339 chrome_frame/protocol_sink_wrap.cc:339: HRESULT ProtData::Read(void* buffer, ULONG size, ULONG* size_read) { On 2010/06/04 01:08:53, amit wrote: > validate input args? Done. Checking for size_read. http://codereview.chromium.org/2620001/diff/9001/10003#newcode356 chrome_frame/protocol_sink_wrap.cc:356: // User buffer is greater than what we have. On 2010/06/04 01:08:53, amit wrote: > do we have to do this or we can simply return less data and wait for the next > read? This is what the urlmon code does. http://codereview.chromium.org/2620001/diff/9001/10003#newcode370 chrome_frame/protocol_sink_wrap.cc:370: if (status_code == BINDSTATUS_DIRECTBIND) { On 2010/06/04 01:08:53, amit wrote: > switch? I don't like to indent code. http://codereview.chromium.org/2620001/diff/9001/10003#newcode374 chrome_frame/protocol_sink_wrap.cc:374: // if (status_code == BINDSTATUS_DECODING) { On 2010/06/04 09:01:53, tommi wrote: > remove? Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode412 chrome_frame/protocol_sink_wrap.cc:412: if (first_data_notification_ == false) { On 2010/06/04 01:08:53, amit wrote: > nit: !first_data_notification_ Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode419 chrome_frame/protocol_sink_wrap.cc:419: if (!IsTextHtml(suggested_mime_type_.c_str())) { On 2010/06/04 01:08:53, amit wrote: > are we guaranteed to have a valid suggested_mime_type_ when we reach here? It's > better to check in any case. No. We may have not received mime_type at all, but it does not matter. http://codereview.chromium.org/2620001/diff/9001/10003#newcode436 chrome_frame/protocol_sink_wrap.cc:436: bool last_chance = false; On 2010/06/04 01:08:53, amit wrote: > nit: last_chance = (hr == S_OK || hr == S_FALSE)? :) Not good for coverage. http://codereview.chromium.org/2620001/diff/9001/10003#newcode443 chrome_frame/protocol_sink_wrap.cc:443: // TODO: How we are going to send BSCF_FIRSTDATANOTIFICATION then? On 2010/06/04 09:01:53, tommi wrote: > todo(stoyan): Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode450 chrome_frame/protocol_sink_wrap.cc:450: if (renderer_type_ == CHROME) { On 2010/06/04 01:08:53, amit wrote: > Is this happening for every ReportData? No. http://codereview.chromium.org/2620001/diff/9001/10003#newcode456 chrome_frame/protocol_sink_wrap.cc:456: // TODO: DLOG On 2010/06/04 09:01:53, tommi wrote: > same here Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode461 chrome_frame/protocol_sink_wrap.cc:461: // On 2010/06/04 09:01:53, tommi wrote: > comment? Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode464 chrome_frame/protocol_sink_wrap.cc:464: // bytes_avail On 2010/06/04 09:01:53, tommi wrote: > remove? Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode478 chrome_frame/protocol_sink_wrap.cc:478: HRESULT ProtData::FillBuffer() { On 2010/06/04 01:08:53, amit wrote: > nit: this helper attempts to read enough data to make a determination. So how > about something like ReadContentSniffSize? No. I like FillBuffer more. http://codereview.chromium.org/2620001/diff/9001/10003#newcode508 chrome_frame/protocol_sink_wrap.cc:508: ScopedComPtr<IBindCtx> pbc; On 2010/06/04 01:08:53, amit wrote: > nit: bind_context or bind_ctx if you prefer a slightly shorter version :) Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode538 chrome_frame/protocol_sink_wrap.cc:538: prot_data = new ProtData(protocol, read_fun, url); On 2010/06/04 01:08:53, amit wrote: > handing over the read_fun to ProtData is a bit scary. We have to make sure that > we are always calling it with the same instance this was supposed to be called > for. I do not understand what do you mean. If we are here - in Hook_Start - it means we are in same vtable as Hook_Read and hence the original read_fun is relevant. http://codereview.chromium.org/2620001/diff/9001/10003#newcode600 chrome_frame/protocol_sink_wrap.cc:600: HRESULT hr = prot_data->Read(buffer, size, size_read); On 2010/06/04 01:08:53, amit wrote: > DCHECK(prot_data->protocol_ == protocol)? pointless. we have just called ProtData::DataFromProtocol http://codereview.chromium.org/2620001/diff/9001/10003#newcode605 chrome_frame/protocol_sink_wrap.cc:605: struct FakeProtocol : public CComObjectRootEx<CComSingleThreadModel>, On 2010/06/04 09:01:53, tommi wrote: > class? Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode641 chrome_frame/protocol_sink_wrap.cc:641: HRESULT hr = obj_->QueryInterface(riid, ppvObj); On 2010/06/04 01:08:53, amit wrote: > if (obj) What are the chances? http://codereview.chromium.org/2620001/diff/9001/10003#newcode652 chrome_frame/protocol_sink_wrap.cc:652: static void HookCTrans(IInternetProtocol* p) { On 2010/06/04 01:08:53, amit wrote: > nit: rename to HookTransactionVtable? Done. http://codereview.chromium.org/2620001/diff/9001/10003#newcode669 chrome_frame/protocol_sink_wrap.cc:669: hr = session->RegisterNameSpace(&factory, CLSID_NULL, L"611", 0, 0, 0); On 2010/06/04 01:08:53, amit wrote: > nit: "611" -> kDummyNameSpace too long. http://codereview.chromium.org/2620001/diff/9001/10004 File chrome_frame/protocol_sink_wrap.h (right): http://codereview.chromium.org/2620001/diff/9001/10004#newcode41 chrome_frame/protocol_sink_wrap.h:41: enum RendererType { On 2010/06/04 01:08:53, amit wrote: > Share this with the one declared in urlmon_bind_status_callback.h? If we are going to remove urlmon_bind_status_callback.h this is not very useful exercise. http://codereview.chromium.org/2620001/diff/9001/10004#newcode62 chrome_frame/protocol_sink_wrap.h:62: public IInternetProtocolSink { On 2010/06/04 01:08:53, amit wrote: > since we are wrapping original sink, it is better to support/forward most the > interfaces that the original sink implements. This is so that QI on the other > interface for IInternetProtocolSink would come back to us. Inside urlmon.dll QI for IInternetProtocolSink is used only when handlers/filters are created.
http://codereview.chromium.org/2620001/diff/20001/21003 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2620001/diff/20001/21003#newcode83 chrome_frame/protocol_sink_wrap.cc:83: ScopedComPtr<IInternetProtocolSink> ProtocolSinkWrap::CreateNewSink( check input arguments or at least a DCHECK? http://codereview.chromium.org/2620001/diff/20001/21003#newcode242 chrome_frame/protocol_sink_wrap.cc:242: memcpy(buffer, buffer_ + buffer_pos_, bytes_to_copy); buffer and size_read are coming in directly from the hook, shouldn't we validate them before using here? http://codereview.chromium.org/2620001/diff/20001/21003#newcode292 chrome_frame/protocol_sink_wrap.cc:292: return delegate->ReportProgress(status_code, status_text); check/dcheck input arg before using? http://codereview.chromium.org/2620001/diff/20001/21003#newcode361 chrome_frame/protocol_sink_wrap.cc:361: void ProtData::UpdateUrl(const wchar_t* url) { nit: set_url? http://codereview.chromium.org/2620001/diff/20001/21003#newcode556 chrome_frame/protocol_sink_wrap.cc:556: CComObjectStackEx<FakeProtocol> prot; if (IS_PATCHED(CTransaction)) or a DCHECK? http://codereview.chromium.org/2620001/diff/20001/21003#newcode560 chrome_frame/protocol_sink_wrap.cc:560: const wchar_t* const kDummyNameSpace = L"611"; nice, can we actually use this constant below? :) http://codereview.chromium.org/2620001/diff/20001/21003#newcode566 chrome_frame/protocol_sink_wrap.cc:566: hr = ::CreateAsyncBindCtxEx(0, 0, 0, 0, bc.Receive(), 0); Check return value or DCHECK to catch failures at each step? We should log in case this crucial step fails for any reason. http://codereview.chromium.org/2620001/diff/20001/21004 File chrome_frame/protocol_sink_wrap.h (right): http://codereview.chromium.org/2620001/diff/20001/21004#newcode111 chrome_frame/protocol_sink_wrap.h:111: RendererType get_renderer_type() { nit: chrome convention is to drop get and just use renderer_type()
On 2010/06/04 16:47:32, stoyan wrote: > http://codereview.chromium.org/2620001/diff/9001/10003#newcode294 > chrome_frame/protocol_sink_wrap.cc:294: RendererType DetermineRendererType(void* > buffer, DWORD size, bool last_chance) { > On 2010/06/04 01:08:53, amit wrote: > > check validity of buffer, size etc? > > No. This is a global helper receiving a pointer as input. Why would you not want to check the validity of input arguments? > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode317 > chrome_frame/protocol_sink_wrap.cc:317: InternetProtocol_Read_Fn read_fun, const > wchar_t* url) > On 2010/06/04 01:08:53, amit wrote: > > nit: alignment > > > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode339 > chrome_frame/protocol_sink_wrap.cc:339: HRESULT ProtData::Read(void* buffer, > ULONG size, ULONG* size_read) { > On 2010/06/04 01:08:53, amit wrote: > > validate input args? > > Done. Checking for size_read. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode356 > chrome_frame/protocol_sink_wrap.cc:356: // User buffer is greater than what we > have. > On 2010/06/04 01:08:53, amit wrote: > > do we have to do this or we can simply return less data and wait for the next > > read? > > This is what the urlmon code does. So in some cases the original read could return E_PENDING and we would end up returning data with that error code? > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode370 > chrome_frame/protocol_sink_wrap.cc:370: if (status_code == > BINDSTATUS_DIRECTBIND) { > On 2010/06/04 01:08:53, amit wrote: > > switch? > > I don't like to indent code. Please be considerate of your fellow programmers :) I think using a switch makes this code much more readable. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode374 > chrome_frame/protocol_sink_wrap.cc:374: // if (status_code == > BINDSTATUS_DECODING) { > On 2010/06/04 09:01:53, tommi wrote: > > remove? > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode412 > chrome_frame/protocol_sink_wrap.cc:412: if (first_data_notification_ == false) { > On 2010/06/04 01:08:53, amit wrote: > > nit: !first_data_notification_ > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode419 > chrome_frame/protocol_sink_wrap.cc:419: if > (!IsTextHtml(suggested_mime_type_.c_str())) { > On 2010/06/04 01:08:53, amit wrote: > > are we guaranteed to have a valid suggested_mime_type_ when we reach here? > It's > > better to check in any case. > > No. We may have not received mime_type at all, but it does not matter. In that case wouldn't we fire up a ReportProgress of BINDSTATUS_MIMETYPEAVAILABLE with an empty string? Secondly, looking at code it seems we could send the BINDSTATUS_MIMETYPEAVAILABLE notification more than once which is not necessarily a good thing. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode436 > chrome_frame/protocol_sink_wrap.cc:436: bool last_chance = false; > On 2010/06/04 01:08:53, amit wrote: > > nit: last_chance = (hr == S_OK || hr == S_FALSE)? :) > > Not good for coverage. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode443 > chrome_frame/protocol_sink_wrap.cc:443: // TODO: How we are going to send > BSCF_FIRSTDATANOTIFICATION then? > On 2010/06/04 09:01:53, tommi wrote: > > todo(stoyan): > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode450 > chrome_frame/protocol_sink_wrap.cc:450: if (renderer_type_ == CHROME) { > On 2010/06/04 01:08:53, amit wrote: > > Is this happening for every ReportData? > > No. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode456 > chrome_frame/protocol_sink_wrap.cc:456: // TODO: DLOG > On 2010/06/04 09:01:53, tommi wrote: > > same here > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode461 > chrome_frame/protocol_sink_wrap.cc:461: // > On 2010/06/04 09:01:53, tommi wrote: > > comment? > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode464 > chrome_frame/protocol_sink_wrap.cc:464: // bytes_avail > On 2010/06/04 09:01:53, tommi wrote: > > remove? > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode478 > chrome_frame/protocol_sink_wrap.cc:478: HRESULT ProtData::FillBuffer() { > On 2010/06/04 01:08:53, amit wrote: > > nit: this helper attempts to read enough data to make a determination. So how > > about something like ReadContentSniffSize? > > No. I like FillBuffer more. Again, please be considerate to your fellow citizens :) I think it deserves a more readable name. FillBuffer does not tell me what this function is doing. Neither there are comments to this effect. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode508 > chrome_frame/protocol_sink_wrap.cc:508: ScopedComPtr<IBindCtx> pbc; > On 2010/06/04 01:08:53, amit wrote: > > nit: bind_context or bind_ctx if you prefer a slightly shorter version :) > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode538 > chrome_frame/protocol_sink_wrap.cc:538: prot_data = new ProtData(protocol, > read_fun, url); > On 2010/06/04 01:08:53, amit wrote: > > handing over the read_fun to ProtData is a bit scary. We have to make sure > that > > we are always calling it with the same instance this was supposed to be called > > for. > > I do not understand what do you mean. If we are here - in Hook_Start - it means > we are in same vtable as Hook_Read and hence the original read_fun is relevant. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode600 > chrome_frame/protocol_sink_wrap.cc:600: HRESULT hr = prot_data->Read(buffer, > size, size_read); > On 2010/06/04 01:08:53, amit wrote: > > DCHECK(prot_data->protocol_ == protocol)? > > pointless. we have just called ProtData::DataFromProtocol > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode605 > chrome_frame/protocol_sink_wrap.cc:605: struct FakeProtocol : public > CComObjectRootEx<CComSingleThreadModel>, > On 2010/06/04 09:01:53, tommi wrote: > > class? > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode641 > chrome_frame/protocol_sink_wrap.cc:641: HRESULT hr = obj_->QueryInterface(riid, > ppvObj); > On 2010/06/04 01:08:53, amit wrote: > > if (obj) > > What are the chances? It's a separate detail that currently the class factory is being used only from one place where this member is correctly set. However, the factory class on its own, relies on this member being valid. So shouldn't it be validated or at least DCHECKed? > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode652 > chrome_frame/protocol_sink_wrap.cc:652: static void > HookCTrans(IInternetProtocol* p) { > On 2010/06/04 01:08:53, amit wrote: > > nit: rename to HookTransactionVtable? > > Done. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode669 > chrome_frame/protocol_sink_wrap.cc:669: hr = > session->RegisterNameSpace(&factory, CLSID_NULL, L"611", 0, 0, 0); > On 2010/06/04 01:08:53, amit wrote: > > nit: "611" -> kDummyNameSpace > > too long. > > http://codereview.chromium.org/2620001/diff/9001/10004 > File chrome_frame/protocol_sink_wrap.h (right): > > http://codereview.chromium.org/2620001/diff/9001/10004#newcode41 > chrome_frame/protocol_sink_wrap.h:41: enum RendererType { > On 2010/06/04 01:08:53, amit wrote: > > Share this with the one declared in urlmon_bind_status_callback.h? > > If we are going to remove urlmon_bind_status_callback.h this is not very useful > exercise. > > http://codereview.chromium.org/2620001/diff/9001/10004#newcode62 > chrome_frame/protocol_sink_wrap.h:62: public IInternetProtocolSink { > On 2010/06/04 01:08:53, amit wrote: > > since we are wrapping original sink, it is better to support/forward most the > > interfaces that the original sink implements. This is so that QI on the other > > interface for IInternetProtocolSink would come back to us. > > Inside urlmon.dll QI for IInternetProtocolSink is used only when > handlers/filters are created. I hope that's true for all current and future versions. If, however, that does not turn out to be a case, it will lead to bugs that are quite hard to find.
http://codereview.chromium.org/2620001/diff/20001/21003 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2620001/diff/20001/21003#newcode83 chrome_frame/protocol_sink_wrap.cc:83: ScopedComPtr<IInternetProtocolSink> ProtocolSinkWrap::CreateNewSink( On 2010/06/04 21:36:23, amit wrote: > check input arguments or at least a DCHECK? Done. http://codereview.chromium.org/2620001/diff/20001/21003#newcode242 chrome_frame/protocol_sink_wrap.cc:242: memcpy(buffer, buffer_ + buffer_pos_, bytes_to_copy); On 2010/06/04 21:36:23, amit wrote: > buffer and size_read are coming in directly from the hook, shouldn't we validate > them before using here? How can you validate "size"? If it passes invalid buffer - let it crash. http://codereview.chromium.org/2620001/diff/20001/21003#newcode292 chrome_frame/protocol_sink_wrap.cc:292: return delegate->ReportProgress(status_code, status_text); On 2010/06/04 21:36:23, amit wrote: > check/dcheck input arg before using? ? check for what? http://codereview.chromium.org/2620001/diff/20001/21003#newcode361 chrome_frame/protocol_sink_wrap.cc:361: void ProtData::UpdateUrl(const wchar_t* url) { On 2010/06/04 21:36:23, amit wrote: > nit: set_url? Update implies "there was a previous value". http://codereview.chromium.org/2620001/diff/20001/21003#newcode556 chrome_frame/protocol_sink_wrap.cc:556: CComObjectStackEx<FakeProtocol> prot; On 2010/06/04 21:36:23, amit wrote: > if (IS_PATCHED(CTransaction)) or a DCHECK? Done. http://codereview.chromium.org/2620001/diff/20001/21003#newcode560 chrome_frame/protocol_sink_wrap.cc:560: const wchar_t* const kDummyNameSpace = L"611"; On 2010/06/04 21:36:23, amit wrote: > nice, can we actually use this constant below? :) Done. Removed. http://codereview.chromium.org/2620001/diff/20001/21003#newcode566 chrome_frame/protocol_sink_wrap.cc:566: hr = ::CreateAsyncBindCtxEx(0, 0, 0, 0, bc.Receive(), 0); On 2010/06/04 21:36:23, amit wrote: > Check return value or DCHECK to catch failures at each step? We should log in > case this crucial step fails for any reason. Done. http://codereview.chromium.org/2620001/diff/20001/21004 File chrome_frame/protocol_sink_wrap.h (right): http://codereview.chromium.org/2620001/diff/20001/21004#newcode111 chrome_frame/protocol_sink_wrap.h:111: RendererType get_renderer_type() { On 2010/06/04 21:36:23, amit wrote: > nit: chrome convention is to drop get and just use renderer_type() Done.
On 2010/06/04 21:55:58, amit wrote: > On 2010/06/04 16:47:32, stoyan wrote: > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode294 > > chrome_frame/protocol_sink_wrap.cc:294: RendererType > DetermineRendererType(void* > > buffer, DWORD size, bool last_chance) { > > On 2010/06/04 01:08:53, amit wrote: > > > check validity of buffer, size etc? > > > > No. > This is a global helper receiving a pointer as input. Why would you not want to > check the validity of input arguments? How are you going to check validity of the buffer? It can never be zero anyway. > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode317 > > chrome_frame/protocol_sink_wrap.cc:317: InternetProtocol_Read_Fn read_fun, > const > > wchar_t* url) > > On 2010/06/04 01:08:53, amit wrote: > > > nit: alignment > > > > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode339 > > chrome_frame/protocol_sink_wrap.cc:339: HRESULT ProtData::Read(void* buffer, > > ULONG size, ULONG* size_read) { > > On 2010/06/04 01:08:53, amit wrote: > > > validate input args? > > > > Done. Checking for size_read. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode356 > > chrome_frame/protocol_sink_wrap.cc:356: // User buffer is greater than what we > > have. > > On 2010/06/04 01:08:53, amit wrote: > > > do we have to do this or we can simply return less data and wait for the > next > > > read? > > > > This is what the urlmon code does. > So in some cases the original read could return E_PENDING and we would end up > returning data with that error code? > > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode370 > > chrome_frame/protocol_sink_wrap.cc:370: if (status_code == > > BINDSTATUS_DIRECTBIND) { > > On 2010/06/04 01:08:53, amit wrote: > > > switch? > > > > I don't like to indent code. > Please be considerate of your fellow programmers :) > I think using a switch makes this code much more readable. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode374 > > chrome_frame/protocol_sink_wrap.cc:374: // if (status_code == > > BINDSTATUS_DECODING) { > > On 2010/06/04 09:01:53, tommi wrote: > > > remove? > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode412 > > chrome_frame/protocol_sink_wrap.cc:412: if (first_data_notification_ == false) > { > > On 2010/06/04 01:08:53, amit wrote: > > > nit: !first_data_notification_ > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode419 > > chrome_frame/protocol_sink_wrap.cc:419: if > > (!IsTextHtml(suggested_mime_type_.c_str())) { > > On 2010/06/04 01:08:53, amit wrote: > > > are we guaranteed to have a valid suggested_mime_type_ when we reach here? > > It's > > > better to check in any case. > > > > No. We may have not received mime_type at all, but it does not matter. > In that case wouldn't we fire up a ReportProgress of > BINDSTATUS_MIMETYPEAVAILABLE with an empty string? Secondly, looking at code it > seems we could send the BINDSTATUS_MIMETYPEAVAILABLE notification more than once > which is not necessarily a good thing. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode436 > > chrome_frame/protocol_sink_wrap.cc:436: bool last_chance = false; > > On 2010/06/04 01:08:53, amit wrote: > > > nit: last_chance = (hr == S_OK || hr == S_FALSE)? :) > > > > Not good for coverage. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode443 > > chrome_frame/protocol_sink_wrap.cc:443: // TODO: How we are going to send > > BSCF_FIRSTDATANOTIFICATION then? > > On 2010/06/04 09:01:53, tommi wrote: > > > todo(stoyan): > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode450 > > chrome_frame/protocol_sink_wrap.cc:450: if (renderer_type_ == CHROME) { > > On 2010/06/04 01:08:53, amit wrote: > > > Is this happening for every ReportData? > > > > No. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode456 > > chrome_frame/protocol_sink_wrap.cc:456: // TODO: DLOG > > On 2010/06/04 09:01:53, tommi wrote: > > > same here > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode461 > > chrome_frame/protocol_sink_wrap.cc:461: // > > On 2010/06/04 09:01:53, tommi wrote: > > > comment? > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode464 > > chrome_frame/protocol_sink_wrap.cc:464: // bytes_avail > > On 2010/06/04 09:01:53, tommi wrote: > > > remove? > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode478 > > chrome_frame/protocol_sink_wrap.cc:478: HRESULT ProtData::FillBuffer() { > > On 2010/06/04 01:08:53, amit wrote: > > > nit: this helper attempts to read enough data to make a determination. So > how > > > about something like ReadContentSniffSize? > > > > No. I like FillBuffer more. > Again, please be considerate to your fellow citizens :) > I think it deserves a more readable name. FillBuffer does not tell me what this > function is doing. Neither there are comments to this effect. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode508 > > chrome_frame/protocol_sink_wrap.cc:508: ScopedComPtr<IBindCtx> pbc; > > On 2010/06/04 01:08:53, amit wrote: > > > nit: bind_context or bind_ctx if you prefer a slightly shorter version :) > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode538 > > chrome_frame/protocol_sink_wrap.cc:538: prot_data = new ProtData(protocol, > > read_fun, url); > > On 2010/06/04 01:08:53, amit wrote: > > > handing over the read_fun to ProtData is a bit scary. We have to make sure > > that > > > we are always calling it with the same instance this was supposed to be > called > > > for. > > > > I do not understand what do you mean. If we are here - in Hook_Start - it > means > > we are in same vtable as Hook_Read and hence the original read_fun is > relevant. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode600 > > chrome_frame/protocol_sink_wrap.cc:600: HRESULT hr = prot_data->Read(buffer, > > size, size_read); > > On 2010/06/04 01:08:53, amit wrote: > > > DCHECK(prot_data->protocol_ == protocol)? > > > > pointless. we have just called ProtData::DataFromProtocol > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode605 > > chrome_frame/protocol_sink_wrap.cc:605: struct FakeProtocol : public > > CComObjectRootEx<CComSingleThreadModel>, > > On 2010/06/04 09:01:53, tommi wrote: > > > class? > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode641 > > chrome_frame/protocol_sink_wrap.cc:641: HRESULT hr = > obj_->QueryInterface(riid, > > ppvObj); > > On 2010/06/04 01:08:53, amit wrote: > > > if (obj) > > > > What are the chances? > It's a separate detail that currently the class factory is being used only from > one place where this member is correctly set. However, the factory class on its > own, relies on this member being valid. So shouldn't it be validated or at least > DCHECKed? > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode652 > > chrome_frame/protocol_sink_wrap.cc:652: static void > > HookCTrans(IInternetProtocol* p) { > > On 2010/06/04 01:08:53, amit wrote: > > > nit: rename to HookTransactionVtable? > > > > Done. > > > > http://codereview.chromium.org/2620001/diff/9001/10003#newcode669 > > chrome_frame/protocol_sink_wrap.cc:669: hr = > > session->RegisterNameSpace(&factory, CLSID_NULL, L"611", 0, 0, 0); > > On 2010/06/04 01:08:53, amit wrote: > > > nit: "611" -> kDummyNameSpace > > > > too long. > > > > http://codereview.chromium.org/2620001/diff/9001/10004 > > File chrome_frame/protocol_sink_wrap.h (right): > > > > http://codereview.chromium.org/2620001/diff/9001/10004#newcode41 > > chrome_frame/protocol_sink_wrap.h:41: enum RendererType { > > On 2010/06/04 01:08:53, amit wrote: > > > Share this with the one declared in urlmon_bind_status_callback.h? > > > > If we are going to remove urlmon_bind_status_callback.h this is not very > useful > > exercise. > > > > http://codereview.chromium.org/2620001/diff/9001/10004#newcode62 > > chrome_frame/protocol_sink_wrap.h:62: public IInternetProtocolSink { > > On 2010/06/04 01:08:53, amit wrote: > > > since we are wrapping original sink, it is better to support/forward most > the > > > interfaces that the original sink implements. This is so that QI on the > other > > > interface for IInternetProtocolSink would come back to us. > > > > Inside urlmon.dll QI for IInternetProtocolSink is used only when > > handlers/filters are created. > I hope that's true for all current and future versions. If, however, that does > not turn out to be a case, it will lead to bugs that are quite hard to find.
http://codereview.chromium.org/2620001/diff/26002/28002 File chrome_frame/bind_context_info.h (right): http://codereview.chromium.org/2620001/diff/26002/28002#newcode114 chrome_frame/bind_context_info.h:114: #endif // CHROME_FRAME_BIND_CONTEXT_INFO_ don't delete this \n. If you do, future try runs will fail. http://codereview.chromium.org/2620001/diff/26002/28003 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2620001/diff/26002/28003#newcode76 chrome_frame/protocol_sink_wrap.cc:76: DLOG(INFO) << __FUNCTION__ << " " << this; format this as 0x%80X? Makes the log more readable. http://codereview.chromium.org/2620001/diff/26002/28003#newcode137 chrome_frame/protocol_sink_wrap.cc:137: IBindCtx* BindCtxFromIBindInfo(IInternetBindInfo* bind_info) { Should the return value perhaps be ScopedComPtr<IBindCtx> to be consistent with other methods you're adding? In general it feels awkward to me to return refcounted objects like this. Some of the chrome code returns an object that is not refcounted (a bad idea imho) and here you're returning an object that has been AddRefed. I see you're careful to use the Attach() method rather than the constructor but I think it would be an easy mistake for someone to make to use a constructor and mistakenly add one reference too many. http://codereview.chromium.org/2620001/diff/26002/28003#newcode197 chrome_frame/protocol_sink_wrap.cc:197: UTF8ToWide((char*)buffer, size, &html_contents); fix cast http://codereview.chromium.org/2620001/diff/26002/28003#newcode278 chrome_frame/protocol_sink_wrap.cc:278: // Should we fire MIMESTATUSAVAILABLE s well? is this a todo? http://codereview.chromium.org/2620001/diff/26002/28003#newcode319 chrome_frame/protocol_sink_wrap.cc:319: // TODO: We may attempt to remove ourselves from the bind context. TODO(stoyan) http://codereview.chromium.org/2620001/diff/26002/28003#newcode511 chrome_frame/protocol_sink_wrap.cc:511: transaction_.QueryFrom(protocol_sink); should we do a NOTREACHED if this ever fails? http://codereview.chromium.org/2620001/diff/26002/28003#newcode553 chrome_frame/protocol_sink_wrap.cc:553: HRESULT hr = vtable_patch::PatchInterfaceMethods(p, CTransaction_PatchInfo); move to top of function http://codereview.chromium.org/2620001/diff/26002/28003#newcode557 chrome_frame/protocol_sink_wrap.cc:557: return; return statement not necessary http://codereview.chromium.org/2620001/diff/26002/28003#newcode572 chrome_frame/protocol_sink_wrap.cc:572: DLOG_IF(ERROR, FAILED(hr)) << "Failed to register namespace"; DLOG_IF(FATAL, ... ? http://codereview.chromium.org/2620001/diff/26002/28004 File chrome_frame/protocol_sink_wrap.h (right): http://codereview.chromium.org/2620001/diff/26002/28004#newcode71 chrome_frame/protocol_sink_wrap.h:71: IInternetProtocolSink* sink, ProtData* prot_data); fix indent. http://codereview.chromium.org/2620001/diff/26002/28007 File chrome_frame/utils.cc (right): http://codereview.chromium.org/2620001/diff/26002/28007#newcode1081 chrome_frame/utils.cc:1081: bool IBrowserServicePatchEnabled() { Suggest IsBrowserServicePatchEnabled() or IsIBrowserServicePatchEnabled() http://codereview.chromium.org/2620001/diff/26002/28007#newcode1154 chrome_frame/utils.cc:1154: #define ADD_PI_FLAG(x) if (flags & x) { s.append(#x ## " "); flags &= ~x; } should the if be: if ((flags & x) == x) ? http://codereview.chromium.org/2620001/diff/26002/28007#newcode1182 chrome_frame/utils.cc:1182: ADD_BSCF_FLAG(BSCF_INTERMEDIATEDATANOTIFICATION) indentation here is strange http://codereview.chromium.org/2620001/diff/26002/28007#newcode1193 chrome_frame/utils.cc:1193: } missing trailing \n (at least according to this diff) http://codereview.chromium.org/2620001/diff/26002/28008 File chrome_frame/utils.h (right): http://codereview.chromium.org/2620001/diff/26002/28008#newcode455 chrome_frame/utils.h:455: std::string bindstatus2str(ULONG bind_status); use standard naming conventions
http://codereview.chromium.org/2620001/diff/26002/28002 File chrome_frame/bind_context_info.h (right): http://codereview.chromium.org/2620001/diff/26002/28002#newcode114 chrome_frame/bind_context_info.h:114: #endif // CHROME_FRAME_BIND_CONTEXT_INFO_ On 2010/06/07 11:11:22, tommi wrote: > don't delete this \n. If you do, future try runs will fail. Done. http://codereview.chromium.org/2620001/diff/26002/28003 File chrome_frame/protocol_sink_wrap.cc (right): http://codereview.chromium.org/2620001/diff/26002/28003#newcode76 chrome_frame/protocol_sink_wrap.cc:76: DLOG(INFO) << __FUNCTION__ << " " << this; On 2010/06/07 11:11:22, tommi wrote: > format this as 0x%80X? Makes the log more readable. Hm. Ok. http://codereview.chromium.org/2620001/diff/26002/28003#newcode137 chrome_frame/protocol_sink_wrap.cc:137: IBindCtx* BindCtxFromIBindInfo(IInternetBindInfo* bind_info) { On 2010/06/07 11:11:22, tommi wrote: > Should the return value perhaps be ScopedComPtr<IBindCtx> to be consistent with > other methods you're adding? > In general it feels awkward to me to return refcounted objects like this. Some > of the chrome code returns an object that is not refcounted (a bad idea imho) > and here you're returning an object that has been AddRefed. I see you're > careful to use the Attach() method rather than the constructor but I think it > would be an easy mistake for someone to make to use a constructor and mistakenly > add one reference too many. Done. http://codereview.chromium.org/2620001/diff/26002/28003#newcode197 chrome_frame/protocol_sink_wrap.cc:197: UTF8ToWide((char*)buffer, size, &html_contents); On 2010/06/07 11:11:22, tommi wrote: > fix cast Done. http://codereview.chromium.org/2620001/diff/26002/28003#newcode319 chrome_frame/protocol_sink_wrap.cc:319: // TODO: We may attempt to remove ourselves from the bind context. On 2010/06/07 11:11:22, tommi wrote: > TODO(stoyan) Done. http://codereview.chromium.org/2620001/diff/26002/28003#newcode557 chrome_frame/protocol_sink_wrap.cc:557: return; On 2010/06/07 11:11:22, tommi wrote: > return statement not necessary Done. http://codereview.chromium.org/2620001/diff/26002/28003#newcode572 chrome_frame/protocol_sink_wrap.cc:572: DLOG_IF(ERROR, FAILED(hr)) << "Failed to register namespace"; On 2010/06/07 11:11:22, tommi wrote: > DLOG_IF(FATAL, ... > ? Done. http://codereview.chromium.org/2620001/diff/26002/28004 File chrome_frame/protocol_sink_wrap.h (right): http://codereview.chromium.org/2620001/diff/26002/28004#newcode71 chrome_frame/protocol_sink_wrap.h:71: IInternetProtocolSink* sink, ProtData* prot_data); On 2010/06/07 11:11:22, tommi wrote: > fix indent. Done. http://codereview.chromium.org/2620001/diff/26002/28007 File chrome_frame/utils.cc (right): http://codereview.chromium.org/2620001/diff/26002/28007#newcode1081 chrome_frame/utils.cc:1081: bool IBrowserServicePatchEnabled() { On 2010/06/07 11:11:22, tommi wrote: > Suggest IsBrowserServicePatchEnabled() or IsIBrowserServicePatchEnabled() Done. http://codereview.chromium.org/2620001/diff/26002/28007#newcode1154 chrome_frame/utils.cc:1154: #define ADD_PI_FLAG(x) if (flags & x) { s.append(#x ## " "); flags &= ~x; } On 2010/06/07 11:11:22, tommi wrote: > should the if be: > if ((flags & x) == x) > ? Why? These flags are single bits. http://codereview.chromium.org/2620001/diff/26002/28007#newcode1182 chrome_frame/utils.cc:1182: ADD_BSCF_FLAG(BSCF_INTERMEDIATEDATANOTIFICATION) On 2010/06/07 11:11:22, tommi wrote: > indentation here is strange Done. http://codereview.chromium.org/2620001/diff/26002/28008 File chrome_frame/utils.h (right): http://codereview.chromium.org/2620001/diff/26002/28008#newcode455 chrome_frame/utils.h:455: std::string bindstatus2str(ULONG bind_status); On 2010/06/07 11:11:22, tommi wrote: > use standard naming conventions What do you mean?
lgtm http://codereview.chromium.org/2620001/diff/26002/28008 File chrome_frame/utils.h (right): http://codereview.chromium.org/2620001/diff/26002/28008#newcode455 chrome_frame/utils.h:455: std::string bindstatus2str(ULONG bind_status); On 2010/06/07 20:40:24, stoyan wrote: > On 2010/06/07 11:11:22, tommi wrote: > > use standard naming conventions > > What do you mean? CamelCaps instead of alllowercasenames :)
Thanks for keeping up with the suggestions. LGTM with Tommi's concerns addressed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
