hi Stig, thanks for your latest patch. It looks good. My comments are below, most are just style points. The only major question is about the need for OPHttpRequestCompletionAsyncFunctor - I think we could avoid the early deletion by refing before we call into Opera.
I've made changes to fix all of my comments, except those where there's an outstanding question, and will mail out a new patch. If it's easier for you, you can just reply to my questions without sending a new patch and I'll update my copy before submitting. That way we can avoid a round of merging at each end. Steve ======================================================================== http://mondrian.corp.google.com/file/9185258///depot/googleclient/gears/opensource/gears/localserver/opera/http_request_op.cc?a=3 File //depot/googleclient/gears/opensource/gears/localserver/opera/http_request_op.cc (snapshot 3) ------------------------------------ Line 49: == ThreadMessageQueue::GetInstance()->GetCurrentThreadId()) { operator before line break ------------------------------------ Line 74: : public AsyncFunctor { Fits on one line. ------------------------------------ Line 103: listener_(NULL), 4 space indent before ':' ------------------------------------ Line 205: BrowsingContext *browsing_context) { Indent to open brace, one argument per line ------------------------------------ Line 234: HeaderEntry *new_header = new HeaderEntry; Now that you're using std::pair, there's no need to allocate on the heap. Plus std::pair has a default constructor ... HeaderEntry new_header = HeaderEntry(name, value); ------------------------------------ Line 249: bool OPHttpRequest::Send(BlobInterface* blob) { '*' on right ------------------------------------ Line 276: if (url_data_) { Is there a set of states in which url_data_ should be set? Would it be better to test for those states? See comment in OnRequestCreated(). ------------------------------------ Line 282: UTF8ToString16(charset, &charset16); Good to test the return value of UTF8ToString16. ------------------------------------ Line 307: //g_opera->AbortUrl(url_id_); Should this be here? ------------------------------------ Line 322: const unsigned short **value) { Add a comment explaining that this method is only called by the browser while we are blocked in RequestUrl(), so SetRequestHeader can't be called between successive calls. ------------------------------------ Line 335: OPHttpRequest::GetUrl() { Don't break line on return type ------------------------------------ Line 336: return url_.c_str(); Do we need to protect against this being called before url_ is set (ie before state OPEN)? ------------------------------------ Line 339: const unsigned short* Why use unsigned short* here, but wchar_t above? ------------------------------------ Line 364: if (ready_state_ == HttpRequest::OPEN) { Add a comment explaining that this method should only be called after OnRequestCreated has been called by Opera, so url_data_ should be set. Add an assert. ------------------------------------ Line 403: if (!url_data_ || url_data_->IsFinished()) { What state exactly does IsFinished() denote? ------------------------------------ Line 405: AsyncRouter::GetInstance()-> Indent 4 spaces and break at open brace. ------------------------------------ Line 408: new OPHttpRequestCompletionAsyncFunctor(this)); >> Can you explain the need to set the ready state asynchronously via the >> AsyncFunctor? Presumably this method is called by the browser. What thread >> is it >> called on? Is the ThreadMessageQueue initialised on this thread? > > Answer: > We had strange problems when calling SetReadyState(HttpRequest::COMPLETE) in > the > OPHttpRequest::OnDataReceived() method directly because that could lead to a > call to the SafeHttpRequest::RemoveNativeRequest() before we returned from > the > call into Opera from OPHttpRequest::SendImpl(). Delaying the setting of the > state > fixed that. > And the ThreadMessageQueue for the browser thread is initialized. It sounds like you need to ref the SafeHttpRequest in SendImpl while you call RequestUrl. If this is possible, I think it's a neater solution than adding an extra class and another asynchronous call. ------------------------------------ Line 451: void OPHttpRequest::OnRequestCreated(OperaUrlData* data) { Maybe add a comment explaining that this is called by Opera and data is always non-NULL. Add an assert too? ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/9185258
