Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9406001:
Factor out ResourceDispatcherHost dependent code around SSLManager (Closed)
|
Created:
8 years, 10 months ago by Takashi Toyoshima Modified:
8 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFactor out ResourceDispatcherHost dependent code around SSLManager
Because SSLManager must work with not only ResourceDispatcherHost.
BUG=53836
TEST=existing tests will cover this refactoring
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126310
Patch Set 1 #Patch Set 2 : ready for review #
Total comments: 10
Patch Set 3 : reflects wtc's review #
Total comments: 18
Patch Set 4 : reflects wtc's second review #Patch Set 5 : merge tpayne's change #Patch Set 6 : rebase #
Total comments: 1
Patch Set 7 : remove unused inclusion #
Total comments: 6
Patch Set 8 : fix an error on merging tpayne's change #
Total comments: 8
Patch Set 9 : reflects darin's review #
Total comments: 15
Patch Set 10 : rebase because of GetAssociatedRenderView, etc #Patch Set 11 : reflects Darin's second review #Patch Set 12 : catch up to trunk #Patch Set 13 : fix a typo #
Total comments: 2
Patch Set 14 : add a new line #Patch Set 15 : rebase for retry #Messages
Total messages: 32 (0 generated)
Hi Adam and Wan-Teh, I'm refactoring on SSLManager because I'd like to use it from also WebSocket. Adam looks the original author of these classes, right? I'm happy if you can review this change. WebSocket doesn't use ResourceDispatcherHost but SocketStreamDispatcherHost, that's why I decide to factor out ResourceDispatcherHost dependent code into SSLErrorHandler::Delegate class, then implement it from ResourceDispatcherHost and SocketStreamDispatcherHost. Please refer the filed issue for more details or whole change plan. Thanks.
Review comments on Patch Set 2: abarth should be the primary reviewer unless he is busy. Thank you for refactoring this code. I have some questions and suggested changes below. https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/rend... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/rend... content/browser/renderer_host/resource_dispatcher_host.cc:1489: SSLErrorHandler::instance_id instance; Nit: 'error_handler_id' seems more clear than 'instance'. https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/rend... File content/browser/renderer_host/resource_dispatcher_host.h (right): https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/rend... content/browser/renderer_host/resource_dispatcher_host.h:227: const SSLErrorHandler::instance_id& id) const; It seems that this new method can be private. This is function overloading. Please make sure this case is allowed by the Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl... or rename this function to describe its argument (e.g., GetURLRequestFromSSLErrorHandler). https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/ssl/... File content/browser/ssl/ssl_error_handler.h (right): https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:42: typedef std::pair<int, int> instance_id; instance_id doesn't meet the requirement of the Style Guide on naming conventions. I think it should be named InstanceID. Please document what the two 'int' members of instance_id are. https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:55: virtual void ContinueRequestForInstance(const instance_id& id) = 0; Please document the Delegate class and its methods. https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:115: // The id of the request associated with this object. Perhaps this member (and its type) should be named request_id_? Why can't you use the GlobalRequestID type here? GlobalRequestID doesn't seem specific to ResourceDispatcherHost.
Thanks, I reflect wtc's review comments. https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/rend... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/rend... content/browser/renderer_host/resource_dispatcher_host.cc:1489: SSLErrorHandler::instance_id instance; Now, 'request_id' looks better as instance of GlobalRequestID? https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/rend... File content/browser/renderer_host/resource_dispatcher_host.h (right): https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/rend... content/browser/renderer_host/resource_dispatcher_host.h:227: const SSLErrorHandler::instance_id& id) const; I can remove this function now, because I decide to stick using content::GlobalRequestID as a unique ID. https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/ssl/... File content/browser/ssl/ssl_error_handler.h (right): https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:42: typedef std::pair<int, int> instance_id; Thanks. I decide to use content::GlobalRequestID instead of instance_id. https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:55: virtual void ContinueRequestForInstance(const instance_id& id) = 0; On 2012/02/22 00:36:23, wtc wrote: > > Please document the Delegate class and its methods. Done. https://chromiumcodereview.appspot.com/9406001/diff/2001/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:115: // The id of the request associated with this object. OK, I stick to the original request_id_.
Patch Set 3 LGTM. I suggest some minor changes below. It would be nice to get a code review from abarth. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/rend... File content/browser/renderer_host/resource_dispatcher_host.h (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/rend... content/browser/renderer_host/resource_dispatcher_host.h:290: // SSLErrorHandler::Delegate Nit: add ':' at the end of this line. Remove the blank lines between the methods. This is recommended by Chromium Coding Style: http://www.chromium.org/developers/coding-style#TOC-Code-formatting https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... File content/browser/ssl/ssl_cert_error_handler.h (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_cert_error_handler.h:12: #include "content/public/browser/global_request_id.h" This is already included by ssl_error_handler.h. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... File content/browser/ssl/ssl_error_handler.h (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:43: // |id| as athe first argument. |id| is a copy of the second argument of Typo: athe => the https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:46: // At last, CancelSSLRequest() or ContinueSSLRequest() will be called after Nit: "At last" probably should be "Finally" or "Lastly". https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:50: // Query what kind of resource is associated with a request that generated Nit: change "Query" to "Queries" in these comments. This is recommended by the Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:52: virtual ResourceType::Type ResourceTypeForSSLRequest( Nit: I suggest changing "SSLRequest" to just "Request" in the delegate function names. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... File content/browser/ssl/ssl_manager.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_manager.cc:14: #include "content/browser/ssl/ssl_error_handler.h" Nit: this is already included by ssl_manager.h. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_manager.cc:42: << " instance_id: " << id.child_id << "," << id.request_id Nit: update "instance_id" in this comment. Perhaps "request id" or just "id"? https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... File content/browser/ssl/ssl_manager.h (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_manager.h:50: // |ContinueDespiteLastError| on the request associated with |id|. The references to |Cancel| and |ContinueDespiteLastError| in this comment should be updated to be more general.
LGTM
Thank you guys. I'll consider adding unit tests for SSLManager in another CL. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/rend... File content/browser/renderer_host/resource_dispatcher_host.h (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/rend... content/browser/renderer_host/resource_dispatcher_host.h:290: // SSLErrorHandler::Delegate On 2012/02/24 00:44:38, wtc wrote: > Nit: add ':' at the end of this line. Remove the blank lines > between the methods. This is recommended by Chromium Coding > Style: > http://www.chromium.org/developers/coding-style#TOC-Code-formatting Done. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... File content/browser/ssl/ssl_cert_error_handler.h (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_cert_error_handler.h:12: #include "content/public/browser/global_request_id.h" On 2012/02/24 00:44:38, wtc wrote: > > This is already included by ssl_error_handler.h. Done. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... File content/browser/ssl/ssl_error_handler.h (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:43: // |id| as athe first argument. |id| is a copy of the second argument of On 2012/02/24 00:44:38, wtc wrote: > > Typo: athe => the Done. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:46: // At last, CancelSSLRequest() or ContinueSSLRequest() will be called after On 2012/02/24 00:44:38, wtc wrote: > > Nit: "At last" probably should be "Finally" or "Lastly". Done. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:50: // Query what kind of resource is associated with a request that generated On 2012/02/24 00:44:38, wtc wrote: > > Nit: change "Query" to "Queries" in these comments. This is > recommended by the Style Guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments Done. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_error_handler.h:52: virtual ResourceType::Type ResourceTypeForSSLRequest( As we discussed offline, the postfix "Request" will cause name confliction in ResourceDispatcherHost. I keep to use "SSLRequest" for now. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... File content/browser/ssl/ssl_manager.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_manager.cc:14: #include "content/browser/ssl/ssl_error_handler.h" On 2012/02/24 00:44:38, wtc wrote: > > Nit: this is already included by ssl_manager.h. Done. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_manager.cc:42: << " instance_id: " << id.child_id << "," << id.request_id On 2012/02/24 00:44:38, wtc wrote: > > Nit: update "instance_id" in this comment. Perhaps > "request id" or just "id"? Done. https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... File content/browser/ssl/ssl_manager.h (right): https://chromiumcodereview.appspot.com/9406001/diff/3010/content/browser/ssl/... content/browser/ssl/ssl_manager.h:50: // |ContinueDespiteLastError| on the request associated with |id|. On 2012/02/24 00:44:38, wtc wrote: > > The references to |Cancel| and |ContinueDespiteLastError| > in this comment should be updated to be more general. Done.
Reflects http://codereview.chromium.org/9453028/ in Patch Set 5.
https://chromiumcodereview.appspot.com/9406001/diff/20002/content/browser/ssl... File content/browser/ssl/ssl_error_handler.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/20002/content/browser/ssl... content/browser/ssl/ssl_error_handler.cc:9: #include "content/browser/renderer_host/resource_dispatcher_host_request_info.h" Oops, I notice that this inclusion at line 9 also must be removed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9406001/15013
Presubmit check for 9406001-15013 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/browser/renderer_host/resource_dispatcher_host.cc,content/browser/renderer_host/resource_dispatcher_host.h Presubmit checks took 1.9s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... content/browser/renderer_host/resource_dispatcher_host.cc:418: if (!request) This changed in http://codereview.chromium.org/9453028/ and should now !request || !request-is_pending(), just like ContinueSSLRequest below.
https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... content/browser/renderer_host/resource_dispatcher_host.cc:418: if (!request) On 2012/02/28 21:33:56, tpayne wrote: > This changed in http://codereview.chromium.org/9453028/ and should now !request > || !request-is_pending(), just like ContinueSSLRequest below. tpayne: Does ContinueSSLRequest also need the !request->is_pending() test? In your CL you didn't add the !request->is_pending() test to SSLErrorHandler::CompleteContinueRequest().
https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... content/browser/renderer_host/resource_dispatcher_host.cc:418: if (!request) On 2012/02/28 21:49:44, wtc wrote: > > On 2012/02/28 21:33:56, tpayne wrote: > > This changed in http://codereview.chromium.org/9453028/ and should now > !request > > || !request-is_pending(), just like ContinueSSLRequest below. > > tpayne: Does ContinueSSLRequest also need the !request->is_pending() > test? In your CL you didn't add the !request->is_pending() > test to SSLErrorHandler::CompleteContinueRequest(). I'm not familiar enough with this code to be answer that question. Intuitively, it seems to make sense that the check be there. However, I don't see any code in the request/job that depends on that state.
I added Darin as the final owner reviewer. Darin, could you give me a stamp for landing? https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... content/browser/renderer_host/resource_dispatcher_host.cc:418: if (!request) Hum... I also don't have any confidence on the same change in CancelSSLRequest. So, I'll keep this as is for now.
https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... content/browser/renderer_host/resource_dispatcher_host.cc:418: if (!request) On 2012/02/28 22:12:07, toyoshim wrote: > Hum... > I also don't have any confidence on the same change in CancelSSLRequest. So, > I'll keep this as is for now. If you commit this as-is, the SSLUITests will go back to flaking.
To my understanding, this CL already contains a change which is equivalent to you CL, right?
On 2012/02/28 22:48:56, toyoshim wrote: > To my understanding, this CL already contains a change which is equivalent to > you CL, right? No, my CL added a check that request->is_pending() before calling SimulateSSLError. Your CL does not include that check.
https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/15013/content/browser/ren... content/browser/renderer_host/resource_dispatcher_host.cc:418: if (!request) Ah, sorry. I'm confused. I'll remove the check in ContinueSSLRequest and move here. That must be equivalent.
https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ren... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ren... content/browser/renderer_host/resource_dispatcher_host.cc:418: if (!request || !request->is_pending()) NOTE: The request switches to no longer being pending at the same time as it is removed from the pending_requests_ list maintained by the ResourceDispatcherHost. This means that testing for GetURLRequest returning NULL should be sufficient here. The extra is_pending check should only return false for requests that have not yet been started. https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... File content/browser/ssl/ssl_cert_error_handler.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... content/browser/ssl/ssl_cert_error_handler.cc:15: const content::GlobalRequestID& id, So GlobalRequestID is being used to track both URLRequests and WebSockets? https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... File content/browser/ssl/ssl_error_handler.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... content/browser/ssl/ssl_error_handler.cc:25: request_url_(delegate->URLForSSLRequest(id)), why are these delegate function calls instead of just being constructor parameters? https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... File content/browser/ssl/ssl_error_handler.h (right): https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... content/browser/ssl/ssl_error_handler.h:52: virtual ResourceType::Type ResourceTypeForSSLRequest( hmm... the ResourceType is not a property that ever changes. do you really need to query this via a Delegate interface? it could also be a property that gets pushed to the SSLErrorHandler, right? I suppose the URL could change after following HTTP redirects. Does the associated RenderViewHost / RenderProcess ever change? I wouldn't think so.
https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ren... File content/browser/renderer_host/resource_dispatcher_host.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ren... content/browser/renderer_host/resource_dispatcher_host.cc:418: if (!request || !request->is_pending()) Can I keep this as is for now? Do you mean pending_requests management will be fixed in a future CL so that the request gotten from GetURLRequest() always returns true here? https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... File content/browser/ssl/ssl_cert_error_handler.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... content/browser/ssl/ssl_cert_error_handler.cc:15: const content::GlobalRequestID& id, Yes. WebSockets require one int value, socket_id_ to identify requestor object. I'll also use GlobalRequestID to wrap socket_id_ for WebSockets. https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... File content/browser/ssl/ssl_error_handler.cc (right): https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... content/browser/ssl/ssl_error_handler.cc:25: request_url_(delegate->URLForSSLRequest(id)), I thought minimizing number of arguments would be better. But, just passing them as parameters will be simple. OK, I'll do so. https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... File content/browser/ssl/ssl_error_handler.h (right): https://chromiumcodereview.appspot.com/9406001/diff/16010/content/browser/ssl... content/browser/ssl/ssl_error_handler.h:52: virtual ResourceType::Type ResourceTypeForSSLRequest( ResourceTypeForSSLRequest() and URLForSSLRequest will be removed by passing them as constructor parameters. So, do you mean RenderViewForSSLRequest() also could be removed by passing render_process_host_id and render_view_host_id to constructor?
+brettw, sky Hi content/browser owners, This is for another CL. It's different from the one I sent a little while ago. This change already get LGTM for major parts of CL. But, I still need LGTM for content/browser. I request the review of Darin, but he seems to be busy. Could one of you guys take over this review for content/browser?
LGTM
http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... File content/browser/renderer_host/resource_dispatcher_host.cc (right): http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.cc:393: DVLOG(1) << "CancelRequestForInstance() url: " << request->url().spec(); nit: the log comment seems to implicate a function named CancelRequestForInstance. maybe you meant for this to say "CancelSSLRequest"? http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.cc:395: request->SimulateSSLError(error, *ssl_info); the ResourceDispatcherHost has so many other Cancel methods. it is not clear why this is implemented using Simulate*Error instead of one of the other Cancel methods. Maybe that indicates that this function is even mis-named? Should it be named InterruptSSLRequestWithError? http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.cc:1457: if(!RenderViewForRequest(request, &render_process_id, &render_view_id)) since you already have |info|, please just use the GetAssociatedRenderView function instead. RenderViewForRequest has to lookup the info all over again, which is a bit wasteful. http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... File content/browser/renderer_host/resource_dispatcher_host.h (right): http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.h:285: // SSLErrorHandler::Delegate: it seems like it would be better to move these functions to be listed just after the URLRequest::Delegate methods. that way the functions that are overridden from base classes are grouped together. http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_cer... File content/browser/ssl/ssl_cert_error_handler.cc (right): http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_cer... content/browser/ssl/ssl_cert_error_handler.cc:18: int render_process_id, it actually looks like we only need the render_process_id and render_view_id inside the Dispatch function. those could be bound parameters to the Dispatch function. they could just be looked up inside SSLManager::OnSSLCertificateError. that would mean that you don't need render_process_id_ or render_view_id_. it also means that you would have fewer parameters to these constructors. you could furthermore just pass the URLRequest, which would then mean that you wouldn't need the GlobalRequestID or the GURL since you can get both of those from the URLRequest. http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_err... File content/browser/ssl/ssl_error_handler.h (right): http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_err... content/browser/ssl/ssl_error_handler.h:58: }; nit: add a new line at the end of this class. http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_err... content/browser/ssl/ssl_error_handler.h:104: const content::GlobalRequestID& id, isn't there redundant information being passed here? GlobalRequestID has a process ID member. is that different than the render_process_id being passed here?
Thanks a lot, Darin. GetAssociatedRenderView() you suggest is introduced by newer revision. So, first I just rebase my CL to meet at Set 9, then apply your suggestion at Set 10. Regards, http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... File content/browser/renderer_host/resource_dispatcher_host.cc (right): http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.cc:393: DVLOG(1) << "CancelRequestForInstance() url: " << request->url().spec(); Oops, sorry for mistakes. Fixed. http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.cc:395: request->SimulateSSLError(error, *ssl_info); I'm looking into url_request.h. It has Start(), Cancel(), SimulateError(), SimulateSSLError() methods. I feel Start(), Cancel(), CancelWithError(), CancelWithSSLError() looks consistent. Anyway, I notice that SimulateError() is used from some other files. I feel it's better that this renaming CL is splited into another CL. Could you allow me to split it into another CL with a TODO comment? http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.cc:408: DVLOG(1) << "ContinueRequestForInstance() url: " << request->url().spec(); Same mistake here. Done. http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.cc:1457: if(!RenderViewForRequest(request, &render_process_id, &render_view_id)) Applied with rebase. Done. http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... File content/browser/renderer_host/resource_dispatcher_host.h (right): http://codereview.chromium.org/9406001/diff/22001/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host.h:285: // SSLErrorHandler::Delegate: I see. Also I remove their implementation just before static function. I guess it would be better place from the viewpoints of this. I also added the same style comments with net::URLRequest::Delegate like, // SSLErrorHandler::Delegate ---... http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_cer... File content/browser/ssl/ssl_cert_error_handler.cc (right): http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_cer... content/browser/ssl/ssl_cert_error_handler.cc:18: int render_process_id, WebSocket doesn't use URLRequest. Removing it from SSLManager related codes is one of the major purpose of this refactoring. resource_type and url are exported from SSLErrorHandler class. So these could not be omitted, I think. http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_err... File content/browser/ssl/ssl_error_handler.h (right): http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_err... content/browser/ssl/ssl_error_handler.h:58: }; On 2012/03/09 20:49:03, darin wrote: > nit: add a new line at the end of this class. Done. http://codereview.chromium.org/9406001/diff/22001/content/browser/ssl/ssl_err... content/browser/ssl/ssl_error_handler.h:104: const content::GlobalRequestID& id, The contents of GlobalRequestID is different from inheritance. As you said, ResourceDispatcherHost uses process ID, but SocketStreamDispatcherHost will use socket_id. This is the reason why I introduce these arguments.
> So, first I just rebase my CL to meet at Set 9, then apply your suggestion at > Set 10. Oops, it means rebase at Set 10, apply suggestion at Set 11. So, please review Set 11 again. Thanks
Darin, I rebased my change because related codes are dramatically changed. Now, Patch Set 13 is ready for review.
LGTM http://codereview.chromium.org/9406001/diff/32002/content/browser/renderer_ho... File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right): http://codereview.chromium.org/9406001/diff/32002/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host_impl.cc:1945: // SSLErrorHandler::Delegate --------------------------------------------------- nit: add a new line below here
Thanks Darin and other reviewers!! I'll land this CL via CQ. http://codereview.chromium.org/9406001/diff/32002/content/browser/renderer_ho... File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right): http://codereview.chromium.org/9406001/diff/32002/content/browser/renderer_ho... content/browser/renderer_host/resource_dispatcher_host_impl.cc:1945: // SSLErrorHandler::Delegate --------------------------------------------------- On 2012/03/12 22:10:16, darin wrote: > nit: add a new line below here Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9406001/33009
Try job failure for 9406001-33009 (retry) on linux_rel for step "check_deps". It's a second try, previously, step "check_deps" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9406001/37003
Change committed as 126310 |
