Refactor host network

TEST=new tests added.


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

[amit, 2010-01-19 10:09:40]

The refactoring with RequestManager/Delegate is quite clean.  The cut between
thread safe and unsafe request managers and speeding up processing for thread
safe managers is nifty. Awesome work!

First set of comments, will continue reviewing it tomorrow.

http://codereview.chromium.org/545093/diff/1008/1013
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/545093/diff/1008/1013#newcode858
chrome_frame/chrome_frame_automation.cc:858: bool
ChromeFrameAutomationClient::ProcessNetworkMessage(TabProxy* tab,
nit: call it ProcessUrlRequestMessage?

http://codereview.chromium.org/545093/diff/1008/1013#newcode863
chrome_frame/chrome_frame_automation.cc:863: bool invoke = ui_thread ||
thread_safe_url_fetcher_;
nice trick!

http://codereview.chromium.org/545093/diff/1008/1013#newcode888
chrome_frame/chrome_frame_automation.cc:888: if (false == invoke) {
it's simpler if you move this up and check for if (invoke) {...}

http://codereview.chromium.org/545093/diff/1008/1013#newcode1063
chrome_frame/chrome_frame_automation.cc:1063: url_fetcher_->StopAllRequests();
I think clearing the map in StopAllRequest is important due to this!

http://codereview.chromium.org/545093/diff/1008/1014
File chrome_frame/chrome_frame_automation.h (right):

http://codereview.chromium.org/545093/diff/1008/1014#newcode351
chrome_frame/chrome_frame_automation.h:351: public:
nit: new line above

http://codereview.chromium.org/545093/diff/1008/1020
File chrome_frame/npapi_url_request.cc (right):

http://codereview.chromium.org/545093/diff/1008/1020#newcode203
chrome_frame/npapi_url_request.cc:203: 
nit: remove empty line

http://codereview.chromium.org/545093/diff/1008/1020#newcode211
chrome_frame/npapi_url_request.cc:211: request_map_[request_id] = new_request;
DCHECK that a request with the new ID should not exist in the map before adding.
Also, shouldn't we add the request to the map if Start succeeds?

http://codereview.chromium.org/545093/diff/1008/1020#newcode222
chrome_frame/npapi_url_request.cc:222: request->Read(bytes_to_read);
DCHECK and handle the case where the request is not found in the map.

http://codereview.chromium.org/545093/diff/1008/1020#newcode231
chrome_frame/npapi_url_request.cc:231: void NPAPIUrlRequestManager::StopAll() {
shouldn't we also clear the map at the end, since if a new request comes in (in
theory) the ID could get reused.

http://codereview.chromium.org/545093/diff/1008/1020#newcode272
chrome_frame/npapi_url_request.cc:272: DCHECK(request_map_.find(request->id())
!= request_map_.end());
I think we should bail out without crashing here (and other entry points like
WriteReady, Write etc.) if we fail to find a valid request due to garbage
notifydata.

http://codereview.chromium.org/545093/diff/1008/1020#newcode299
chrome_frame/npapi_url_request.cc:299: void
NPAPIUrlRequestManager::UrlNotify(const char* url, NPReason reason,
In the scenario where the connection is broken from the host browser, how the
request will be removed from the map?

http://codereview.chromium.org/545093/diff/1008/1016
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/545093/diff/1008/1016#newcode224
chrome_frame/urlmon_url_request.cc:224: public PluginUrlRequest,
nit: fix indentation

http://codereview.chromium.org/545093/diff/1008/1021
File chrome_frame/urlmon_url_request.h (right):

http://codereview.chromium.org/545093/diff/1008/1021#newcode25
chrome_frame/urlmon_url_request.h:25: class STAThread : public base::Thread {
nice helper class, please consider moving it to base/thread_win.h (at least
chrome_frame/utils.h)

[stoyan, 2010-01-19 20:07:43]

http://codereview.chromium.org/545093/diff/1008/1013
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/545093/diff/1008/1013#newcode858
chrome_frame/chrome_frame_automation.cc:858: bool
ChromeFrameAutomationClient::ProcessNetworkMessage(TabProxy* tab,
On 2010/01/19 10:09:40, amit wrote:
> nit: call it ProcessUrlRequestMessage?

Done.

http://codereview.chromium.org/545093/diff/1008/1013#newcode888
chrome_frame/chrome_frame_automation.cc:888: if (false == invoke) {
I don't understand what do you mean.
We still have to check whether msg.type() is a UrlRequest related message.

http://codereview.chromium.org/545093/diff/1008/1014
File chrome_frame/chrome_frame_automation.h (right):

http://codereview.chromium.org/545093/diff/1008/1014#newcode351
chrome_frame/chrome_frame_automation.h:351: public:
On 2010/01/19 10:09:40, amit wrote:
> nit: new line above

Done.

http://codereview.chromium.org/545093/diff/1008/1020
File chrome_frame/npapi_url_request.cc (right):

http://codereview.chromium.org/545093/diff/1008/1020#newcode203
chrome_frame/npapi_url_request.cc:203: 
On 2010/01/19 10:09:40, amit wrote:
> nit: remove empty line

Done.

http://codereview.chromium.org/545093/diff/1008/1020#newcode211
chrome_frame/npapi_url_request.cc:211: request_map_[request_id] = new_request;
On 2010/01/19 10:09:40, amit wrote:
> DCHECK that a request with the new ID should not exist in the map before
adding.

Done.

> Also, shouldn't we add the request to the map if Start succeeds? 

PluginUrlRequestDelegate::OnResponseEnd() will be called if Start fails.
Start does not need to return bool for error/success but I was not sure about
this additional AddRef() thing. I was going to ask Ananta about it.

http://codereview.chromium.org/545093/diff/1008/1020#newcode222
chrome_frame/npapi_url_request.cc:222: request->Read(bytes_to_read);
On 2010/01/19 10:09:40, amit wrote:
> DCHECK and handle the case where the request is not found in the map.

Done.

http://codereview.chromium.org/545093/diff/1008/1020#newcode231
chrome_frame/npapi_url_request.cc:231: void NPAPIUrlRequestManager::StopAll() {
IDs are not reused, right?
Map is cleared in ::OnResponseEnd. How does NPAPI works? Is the request stopped
immediatelly? If yes - then there is a problem because the map is modified in
OnResponseEnd and the iterators here are becoming invalid.

http://codereview.chromium.org/545093/diff/1008/1020#newcode272
chrome_frame/npapi_url_request.cc:272: DCHECK(request_map_.find(request->id())
!= request_map_.end());
On 2010/01/19 10:09:40, amit wrote:
> I think we should bail out without crashing here (and other entry points like
> WriteReady, Write etc.) if we fail to find a valid request due to garbage
> notifydata.

If notifyData is garbage accessing request->id() will very likely cause a crash
anyway.

http://codereview.chromium.org/545093/diff/1008/1020#newcode299
chrome_frame/npapi_url_request.cc:299: void
NPAPIUrlRequestManager::UrlNotify(const char* url, NPReason reason,
On 2010/01/19 10:09:40, amit wrote:
> In the scenario where the connection is broken from the host browser, how the
> request will be removed from the map?

In ::OnReponseEnd() callback.

http://codereview.chromium.org/545093/diff/1008/1016
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/545093/diff/1008/1016#newcode224
chrome_frame/urlmon_url_request.cc:224: public PluginUrlRequest,
On 2010/01/19 10:09:40, amit wrote:
> nit: fix indentation

Done.

http://codereview.chromium.org/545093/diff/1008/1021
File chrome_frame/urlmon_url_request.h (right):

http://codereview.chromium.org/545093/diff/1008/1021#newcode25
chrome_frame/urlmon_url_request.h:25: class STAThread : public base::Thread {
On 2010/01/19 10:09:40, amit wrote:
> nice helper class, please consider moving it to base/thread_win.h (at least
> chrome_frame/utils.h)

Done. Moved to chrome_frame/utils.h

[amit, 2010-01-20 07:13:29]

The code overall looks pretty good. I'd like to see if we can eliminate locking
for the map in UrlmonUrlRequest.

http://codereview.chromium.org/545093/diff/5055/5060
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/545093/diff/5055/5060#newcode887
chrome_frame/chrome_frame_automation.cc:887: if (false == invoke) {
nit: if (!invoke)

http://codereview.chromium.org/545093/diff/5055/5060#newcode889
chrome_frame/chrome_frame_automation.cc:889:
&ChromeFrameAutomationClient::ProcessUrlRequestMessage, tab, msg, true));
80

http://codereview.chromium.org/545093/diff/5055/5065
File chrome_frame/http_negotiate.cc (right):

http://codereview.chromium.org/545093/diff/5055/5065#newcode55
chrome_frame/http_negotiate.cc:55: class TmpBindStatusCallback : public
CComObjectRootEx<CComSingleThreadModel>,
nit: rename to something like SimpleBindStatusCallback  or
BindStatusCallbackImpl and move it to utils.h or something?

http://codereview.chromium.org/545093/diff/5055/5065#newcode58
chrome_frame/http_negotiate.cc:58: BEGIN_COM_MAP(TmpBindStatusCallback)
nit: spaces

http://codereview.chromium.org/545093/diff/5055/5065#newcode109
chrome_frame/http_negotiate.cc:109: CComObjectStackEx<TmpBindStatusCallback>
request;
cool!

http://codereview.chromium.org/545093/diff/5055/5063
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/545093/diff/5055/5063#newcode639
chrome_frame/urlmon_url_request.cc:639: // since moniker_ is not released while
we are in manager's request map.
any way to DCHECK on those conditions?

http://codereview.chromium.org/545093/diff/5055/5063#newcode1119
chrome_frame/urlmon_url_request.cc:1119: void
UrlmonUrlRequest::NotifyDelegateAndDie() {
nit: Isn't EndRequest/CleanupRequest more readable? :)
BTW, thanks for getting rid of the window crap associated with request.

http://codereview.chromium.org/545093/diff/5055/5063#newcode1302
chrome_frame/urlmon_url_request.cc:1302: lock_.Acquire();
shouldn't this happen only on one thread? Is there a way to avoid locking on
every IPC (Start/Read/End)? Maybe by doing map Add/Remove on the worker thread?

http://codereview.chromium.org/545093/diff/5055/5063#newcode1464
chrome_frame/urlmon_url_request.cc:1464:
UrlmonUrlRequestManager::UrlmonUrlRequestManager() :
nit: move colon to the next line.

http://codereview.chromium.org/545093/diff/5055/5068
File chrome_frame/urlmon_url_request.h (right):

http://codereview.chromium.org/545093/diff/5055/5068#newcode59
chrome_frame/urlmon_url_request.h:59: CComObject<WrappedBindContext>* bind_ctx_;
How about a scoped_refptr?

[ananta, 2010-01-20 18:27:00]

Looks great overall. Some comments.

http://codereview.chromium.org/545093/diff/5055/5063
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/545093/diff/5055/5063#newcode224
chrome_frame/urlmon_url_request.cc:224: class UrlmonUrlRequest
It would be great if you could add some comments here about which thread the
UrlmonUrlRequest objects live on.

http://codereview.chromium.org/545093/diff/5055/5063#newcode225
chrome_frame/urlmon_url_request.cc:225: : public
CComObjectRootEx<CComMultiThreadModel>,
Fix indentation?

http://codereview.chromium.org/545093/diff/5055/5063#newcode313
chrome_frame/urlmon_url_request.cc:313: class SendStream
It would be great if you could get rid of the SendStream class. It adds needless
complexity. It does not have to be in this CL though. I still prefer it to be
defined outside the UrlmonUrlRequest class as the class definition is way too
big.

http://codereview.chromium.org/545093/diff/5055/5063#newcode337
chrome_frame/urlmon_url_request.cc:337: std::min(static_cast<ULONG>(MAXINT),
size));
Fix indent

http://codereview.chromium.org/545093/diff/5055/5063#newcode338
chrome_frame/urlmon_url_request.cc:338:
request_->delegate_->OnReadComplete(request_->id(), buffer,
DHECK on delegate_?

http://codereview.chromium.org/545093/diff/5055/5063#newcode672
chrome_frame/urlmon_url_request.cc:672: binding_->Abort();
Please make sure to test these changes with a number of https sites like bank
sites, etc.

http://codereview.chromium.org/545093/diff/5055/5063#newcode954
chrome_frame/urlmon_url_request.cc:954: const wchar_t* url_cstr = url.c_str();
We don't need this local variable here. Just use url.c_str()

http://codereview.chromium.org/545093/diff/5055/5063#newcode1302
chrome_frame/urlmon_url_request.cc:1302: lock_.Acquire();
On 2010/01/20 07:13:29, amit wrote:
> shouldn't this happen only on one thread? Is there a way to avoid locking on
> every IPC (Start/Read/End)? Maybe by doing map Add/Remove on the worker
thread?

+1 to avoid locking.

http://codereview.chromium.org/545093/diff/5055/5063#newcode1357
chrome_frame/urlmon_url_request.cc:1357: AutoLock lock(lock_);
It would be great if we can avoid this lock here and other places.

http://codereview.chromium.org/545093/diff/5055/5063#newcode1409
chrome_frame/urlmon_url_request.cc:1409: // DCHECK_EQ(0,
UrlmonUrlRequest::instance_count_);
Why has this been commented out?

http://codereview.chromium.org/545093/diff/5055/5068
File chrome_frame/urlmon_url_request.h (right):

http://codereview.chromium.org/545093/diff/5055/5068#newcode32
chrome_frame/urlmon_url_request.h:32: void UseMonikerForUrl(IMoniker* moniker,
IBindCtx* bind_ctx,
A newline here? and some comments in general would be very helpful.

http://codereview.chromium.org/545093/diff/5055/5068#newcode33
chrome_frame/urlmon_url_request.h:33: const std::wstring& url);
Fix indentation

http://codereview.chromium.org/545093/diff/5055/5068#newcode52
chrome_frame/urlmon_url_request.h:52: //
Did you mean to add comments here?. If not please delete this.

http://codereview.chromium.org/545093/diff/5055/5068#newcode57
chrome_frame/urlmon_url_request.h:57: Lock lock_;
A one line description about each member would be great.

[stoyan, 2010-01-21 22:22:48]

http://codereview.chromium.org/545093/diff/5055/5060
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/545093/diff/5055/5060#newcode887
chrome_frame/chrome_frame_automation.cc:887: if (false == invoke) {
On 2010/01/20 07:13:29, amit wrote:
> nit: if (!invoke)

Done.

http://codereview.chromium.org/545093/diff/5055/5060#newcode889
chrome_frame/chrome_frame_automation.cc:889:
&ChromeFrameAutomationClient::ProcessUrlRequestMessage, tab, msg, true));
On 2010/01/20 07:13:29, amit wrote:
> 80

Done.

http://codereview.chromium.org/545093/diff/5055/5065
File chrome_frame/http_negotiate.cc (right):

http://codereview.chromium.org/545093/diff/5055/5065#newcode55
chrome_frame/http_negotiate.cc:55: class TmpBindStatusCallback : public
CComObjectRootEx<CComSingleThreadModel>,
On 2010/01/20 07:13:29, amit wrote:
> nit: rename to something like SimpleBindStatusCallback  or
> BindStatusCallbackImpl and move it to utils.h or something?

Done.

http://codereview.chromium.org/545093/diff/5055/5065#newcode58
chrome_frame/http_negotiate.cc:58: BEGIN_COM_MAP(TmpBindStatusCallback)
On 2010/01/20 07:13:29, amit wrote:
> nit: spaces
> 

Done.

http://codereview.chromium.org/545093/diff/5055/5063
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/545093/diff/5055/5063#newcode337
chrome_frame/urlmon_url_request.cc:337: std::min(static_cast<ULONG>(MAXINT),
size));
On 2010/01/20 18:27:00, ananta wrote:
> Fix indent

Done.

http://codereview.chromium.org/545093/diff/5055/5063#newcode954
chrome_frame/urlmon_url_request.cc:954: const wchar_t* url_cstr = url.c_str();
On 2010/01/20 18:27:00, ananta wrote:
> We don't need this local variable here. Just use url.c_str()

In MSVC's implementation c_str() is not a simple member accessor.

http://codereview.chromium.org/545093/diff/5055/5063#newcode1302
chrome_frame/urlmon_url_request.cc:1302: lock_.Acquire();
Locking is needed since UrlmonUrlRequestManager methods are called (and
therefore map is accessed) from 3 different threads. (UI, IPC, worker thread).

http://codereview.chromium.org/545093/diff/5055/5063#newcode1409
chrome_frame/urlmon_url_request.cc:1409: // DCHECK_EQ(0,
UrlmonUrlRequest::instance_count_);
On 2010/01/20 18:27:00, ananta wrote:
> Why has this been commented out?

Because it is not a public member, so it will not compile.

http://codereview.chromium.org/545093/diff/5055/5063#newcode1464
chrome_frame/urlmon_url_request.cc:1464:
UrlmonUrlRequestManager::UrlmonUrlRequestManager() :
On 2010/01/20 07:13:29, amit wrote:
> nit: move colon to the next line.

Done.

http://codereview.chromium.org/545093/diff/5055/5068
File chrome_frame/urlmon_url_request.h (right):

http://codereview.chromium.org/545093/diff/5055/5068#newcode32
chrome_frame/urlmon_url_request.h:32: void UseMonikerForUrl(IMoniker* moniker,
IBindCtx* bind_ctx,
On 2010/01/20 18:27:00, ananta wrote:
> A newline here? and some comments in general would be very helpful.

Done.

http://codereview.chromium.org/545093/diff/5055/5068#newcode33
chrome_frame/urlmon_url_request.h:33: const std::wstring& url);
On 2010/01/20 18:27:00, ananta wrote:
> Fix indentation

Done.

http://codereview.chromium.org/545093/diff/5055/5068#newcode52
chrome_frame/urlmon_url_request.h:52: //
On 2010/01/20 18:27:00, ananta wrote:
> Did you mean to add comments here?. If not please delete this.

Done.

http://codereview.chromium.org/545093/diff/5055/5068#newcode57
chrome_frame/urlmon_url_request.h:57: Lock lock_;
On 2010/01/20 18:27:00, ananta wrote:
> A one line description about each member would be great.

Done.

http://codereview.chromium.org/545093/diff/5055/5068#newcode59
chrome_frame/urlmon_url_request.h:59: CComObject<WrappedBindContext>* bind_ctx_;
On 2010/01/20 07:13:29, amit wrote:
> How about a scoped_refptr?

Done.

[stoyan, 2010-01-28 22:46:08]

ping?
I removed the lock as well.

[ananta, 2010-01-28 23:18:02]

Almost there :)

http://codereview.chromium.org/545093/diff/2083/2102
File chrome_frame/test/chrome_frame_unittests.cc (right):

http://codereview.chromium.org/545093/diff/2083/2102#newcode1952
chrome_frame/test/chrome_frame_unittests.cc:1952: class MockUrlDelegate : public
PluginUrlRequestDelegate {
Nice tests :) Can you move them to a separate file?

http://codereview.chromium.org/545093/diff/2083/2102#newcode1981
chrome_frame/test/chrome_frame_unittests.cc:1981: TEST(UrlmonUrlRequestTest,
Simple1) {
A small comment above each test as to what it is testing would be great.

http://codereview.chromium.org/545093/diff/2083/2102#newcode1987
chrome_frame/test/chrome_frame_unittests.cc:1987: server.SetUp();
Should this block be aligned?

http://codereview.chromium.org/545093/diff/2083/2102#newcode1999
chrome_frame/test/chrome_frame_unittests.cc:1999: 
Remove additional newlines here and other places below.

http://codereview.chromium.org/545093/diff/2083/2091
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/545093/diff/2083/2091#newcode450
chrome_frame/urlmon_url_request.cc:450:
status_.set_result(URLRequestStatus::FAILED, net::ERR_INVALID_URL);
ERR_METHOD_NOT_SUPPORTED?

http://codereview.chromium.org/545093/diff/2083/2091#newcode657
chrome_frame/urlmon_url_request.cc:657: const wchar_t* url_cstr = url.c_str();
Remove this and just use url.c_str()?

http://codereview.chromium.org/545093/diff/2083/2091#newcode1016
chrome_frame/urlmon_url_request.cc:1016:
ctx->QueryInterface(moniker_for_url_->bind_ctx.Receive());
DCHECK for success here?

http://codereview.chromium.org/545093/diff/2083/2091#newcode1175
chrome_frame/urlmon_url_request.cc:1175: int request_id, bool erase) {
Can we avoid the Lookup function from providing a facility to erase?. That could
live in its own function?

http://codereview.chromium.org/545093/diff/2083/2091#newcode1177
chrome_frame/urlmon_url_request.cc:1177: scoped_refptr<UrlmonUrlRequest>
request;
Remove this request variable and just return (*it).second?

http://codereview.chromium.org/545093/diff/2083/2091#newcode1207
chrome_frame/urlmon_url_request.cc:1207: done.Wait();
Can you clarify why we need to wait here?. It would be great if this can be
commented in code.

http://codereview.chromium.org/545093/diff/2083/2103
File chrome_frame/urlmon_url_request_private.h (right):

http://codereview.chromium.org/545093/diff/2083/2103#newcode272
chrome_frame/urlmon_url_request_private.h:272: 
Remove additional newline.

http://codereview.chromium.org/545093/diff/2083/2100
File chrome_frame/utils.h (right):

http://codereview.chromium.org/545093/diff/2083/2100#newcode319
chrome_frame/utils.h:319: // Thread that enters STA and have an UI message loop.
has a?

[amit, 2010-01-29 00:23:04]

Looks much better, nice cleanup. LGTM with a few nits.

http://codereview.chromium.org/545093/diff/2083/2095
File chrome_frame/npapi_url_request.cc (right):

http://codereview.chromium.org/545093/diff/2083/2095#newcode222
chrome_frame/npapi_url_request.cc:222: DCHECK(request.get());
nit: if (request) to be on the safe side?

http://codereview.chromium.org/545093/diff/2083/2095#newcode239
chrome_frame/npapi_url_request.cc:239: }
How about DCHECK(request_map_.empty()) here or at some point where we can detect
it?

http://codereview.chromium.org/545093/diff/2083/2094
File chrome_frame/plugin_url_request.h (right):

http://codereview.chromium.org/545093/diff/2083/2094#newcode65
chrome_frame/plugin_url_request.h:65: const IPC::AutomationURLRequest&
request_info) {
nit: indent

http://codereview.chromium.org/545093/diff/2083/2102
File chrome_frame/test/chrome_frame_unittests.cc (right):

http://codereview.chromium.org/545093/diff/2083/2102#newcode1952
chrome_frame/test/chrome_frame_unittests.cc:1952: class MockUrlDelegate : public
PluginUrlRequestDelegate {
On 2010/01/28 23:18:02, ananta wrote:
> Nice tests :) Can you move them to a separate file?
> 
+1

http://codereview.chromium.org/545093/diff/2083/2102#newcode2029
chrome_frame/test/chrome_frame_unittests.cc:2029:
TEST(UrlmonUrlRequestManagerTest, Simple1) {
Simple2?

http://codereview.chromium.org/545093/diff/2083/2091
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/545093/diff/2083/2091#newcode229
chrome_frame/urlmon_url_request.cc:229: std::min(static_cast<ULONG>(MAXINT),
size));
cool!

http://codereview.chromium.org/545093/diff/2083/2091#newcode659
chrome_frame/urlmon_url_request.cc:659: // Note that there's really no way for
us here to distinguish session cookies
nit: 80

http://codereview.chromium.org/545093/diff/2083/2091#newcode1044
chrome_frame/urlmon_url_request.cc:1044: 
DCHECK thread affinity in *Worker functions

http://codereview.chromium.org/545093/diff/2083/2091#newcode1045
chrome_frame/urlmon_url_request.cc:1045: scoped_ptr<MonikerForUrl>
moniker_for_url(use_moniker);
move below the if (stopping_) check?

http://codereview.chromium.org/545093/diff/2083/2091#newcode1132
chrome_frame/urlmon_url_request.cc:1132: // DCHECK_EQ(0,
UrlmonUrlRequest::instance_count_);
uncomment?

[stoyan, 2010-01-29 20:39:42]

http://codereview.chromium.org/545093/diff/2083/2095
File chrome_frame/npapi_url_request.cc (right):

http://codereview.chromium.org/545093/diff/2083/2095#newcode222
chrome_frame/npapi_url_request.cc:222: DCHECK(request.get());
On 2010/01/29 00:23:04, amit wrote:
> nit: if (request) to be on the safe side?

Done.

http://codereview.chromium.org/545093/diff/2083/2094
File chrome_frame/plugin_url_request.h (right):

http://codereview.chromium.org/545093/diff/2083/2094#newcode65
chrome_frame/plugin_url_request.h:65: const IPC::AutomationURLRequest&
request_info) {
On 2010/01/29 00:23:04, amit wrote:
> nit: indent

Done.

http://codereview.chromium.org/545093/diff/2083/2102
File chrome_frame/test/chrome_frame_unittests.cc (right):

http://codereview.chromium.org/545093/diff/2083/2102#newcode1952
chrome_frame/test/chrome_frame_unittests.cc:1952: class MockUrlDelegate : public
PluginUrlRequestDelegate {
On 2010/01/28 23:18:02, ananta wrote:
> Nice tests :) Can you move them to a separate file?
> 

Done.

http://codereview.chromium.org/545093/diff/2083/2102#newcode1952
chrome_frame/test/chrome_frame_unittests.cc:1952: class MockUrlDelegate : public
PluginUrlRequestDelegate {
On 2010/01/29 00:23:04, amit wrote:
> On 2010/01/28 23:18:02, ananta wrote:
> > Nice tests :) Can you move them to a separate file?
> > 
> +1
> 

Done.

http://codereview.chromium.org/545093/diff/2083/2102#newcode1981
chrome_frame/test/chrome_frame_unittests.cc:1981: TEST(UrlmonUrlRequestTest,
Simple1) {
On 2010/01/28 23:18:02, ananta wrote:
> A small comment above each test as to what it is testing would be great.
> 

Done.

http://codereview.chromium.org/545093/diff/2083/2102#newcode1987
chrome_frame/test/chrome_frame_unittests.cc:1987: server.SetUp();
On 2010/01/28 23:18:02, ananta wrote:
> Should this block be aligned?

Done.

http://codereview.chromium.org/545093/diff/2083/2102#newcode1999
chrome_frame/test/chrome_frame_unittests.cc:1999: 
On 2010/01/28 23:18:02, ananta wrote:
> Remove additional newlines here and other places below.
> 

Done.

http://codereview.chromium.org/545093/diff/2083/2091
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/545093/diff/2083/2091#newcode450
chrome_frame/urlmon_url_request.cc:450:
status_.set_result(URLRequestStatus::FAILED, net::ERR_INVALID_URL);
On 2010/01/28 23:18:02, ananta wrote:
> ERR_METHOD_NOT_SUPPORTED?

Done.

http://codereview.chromium.org/545093/diff/2083/2091#newcode657
chrome_frame/urlmon_url_request.cc:657: const wchar_t* url_cstr = url.c_str();
On 2010/01/28 23:18:02, ananta wrote:
> Remove this and just use url.c_str()?

Done.

http://codereview.chromium.org/545093/diff/2083/2091#newcode659
chrome_frame/urlmon_url_request.cc:659: // Note that there's really no way for
us here to distinguish session cookies
On 2010/01/29 00:23:04, amit wrote:
> nit: 80

Done.

http://codereview.chromium.org/545093/diff/2083/2091#newcode1016
chrome_frame/urlmon_url_request.cc:1016:
ctx->QueryInterface(moniker_for_url_->bind_ctx.Receive());
On 2010/01/28 23:18:02, ananta wrote:
> DCHECK for success here?
> 

Done.

http://codereview.chromium.org/545093/diff/2083/2091#newcode1044
chrome_frame/urlmon_url_request.cc:1044: 
On 2010/01/29 00:23:04, amit wrote:
> DCHECK thread affinity in *Worker functions

Done.

http://codereview.chromium.org/545093/diff/2083/2091#newcode1045
chrome_frame/urlmon_url_request.cc:1045: scoped_ptr<MonikerForUrl>
moniker_for_url(use_moniker);
On 2010/01/29 00:23:04, amit wrote:
> move below the if (stopping_) check?

MonikerForUrl will be leaked.

http://codereview.chromium.org/545093/diff/2083/2091#newcode1132
chrome_frame/urlmon_url_request.cc:1132: // DCHECK_EQ(0,
UrlmonUrlRequest::instance_count_);
On 2010/01/29 00:23:04, amit wrote:
> uncomment?

Done.

http://codereview.chromium.org/545093/diff/2083/2091#newcode1175
chrome_frame/urlmon_url_request.cc:1175: int request_id, bool erase) {
On 2010/01/28 23:18:02, ananta wrote:
> Can we avoid the Lookup function from providing a facility to erase?. That
could
> live in its own function?

Done.

http://codereview.chromium.org/545093/diff/2083/2091#newcode1177
chrome_frame/urlmon_url_request.cc:1177: scoped_refptr<UrlmonUrlRequest>
request;
On 2010/01/28 23:18:02, ananta wrote:
> Remove this request variable and just return (*it).second?

Done.

http://codereview.chromium.org/545093/diff/2083/2091#newcode1207
chrome_frame/urlmon_url_request.cc:1207: done.Wait();
On 2010/01/28 23:18:02, ananta wrote:
> Can you clarify why we need to wait here?. It would be great if this can be
> commented in code.
> 

Done. This is a sync call. Caller needs moniker before continuing. I did not
want to introduce another callback in ActiveX object.

http://codereview.chromium.org/545093/diff/2083/2103
File chrome_frame/urlmon_url_request_private.h (right):

http://codereview.chromium.org/545093/diff/2083/2103#newcode272
chrome_frame/urlmon_url_request_private.h:272: 
On 2010/01/28 23:18:02, ananta wrote:
> Remove additional newline.

Done.

http://codereview.chromium.org/545093/diff/2083/2100
File chrome_frame/utils.h (right):

http://codereview.chromium.org/545093/diff/2083/2100#newcode319
chrome_frame/utils.h:319: // Thread that enters STA and have an UI message loop.
On 2010/01/28 23:18:02, ananta wrote:
> has a?

Done.