Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
109283006:
Redirect HTML resource bytes directly to parser thread (Chrome side CL) (Closed)
|
Created:
7 years ago by oystein (OOO til 10th of July) Modified:
6 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRedirect HTML resource bytes directly to parser thread (Chrome side)
Blink side: https://codereview.chromium.org/100563004/
Note: This can't land until the Blink-side has landed.
R=jamesr@chromium.org,jam@chromium.org
BUG=277886
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275655
Patch Set 1 : #
Total comments: 26
Patch Set 2 : #
Total comments: 13
Patch Set 3 : Leak fix #
Total comments: 23
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 36 (0 generated)
Hi guys, functional but probably rough first pass at this. See the linked review for the Blink side.
I'll defer to jamesr for the review since I haven't looked at/am not familiar with the blink side. just some nits https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... content/child/resource_dispatcher.cc:641: DCHECK(request_info != NULL); nit: this dcheck isn't necessary. in release builds it does nothing. in debug builds, the crash below is just as useful of a signal to the developer. https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... content/child/resource_dispatcher.cc:644: if (request_info->buffer != NULL) { nit: "!= NULL" is redundant, convention is to skip it. same below on line 656, just do "if (!request_info) https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... File content/child/resource_dispatcher.h (right): https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... content/child/resource_dispatcher.h:72: // Callback from the WebParserResourceBridgeImpl happening when its nit: blank line above this. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... File content/child/webparserresourcebridge_impl.cc (right): https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.cc:58: , main_thread_message_loop_(main_thread_message_loop) nit: google style is comma on previous lines https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.cc:122: // Do we care about this leaking on shutdown? no https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.cc:134: , shm_size_(shm_size) ditto https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.cc:173: if (peer_ == NULL) { nit: if (!peer_). also above and below https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... File content/child/webparserresourcebridge_impl.h (right): https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:19: class WebThread; no need to forward declare classes that are used in the parent interface, this is redundant since they must have been forward declared already https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:23: class WebThreadImpl; nit: no indentation in forward declarations https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:29: class CONTENT_EXPORT WebParserResourceBridgeImpl : i don't think you need a CONTENT_EXPORT here since it's not used in unit tests. then you wouldn't need NON_EXPORTED_BASE as well https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:43: void OnReceivedData(int data_offset, nit: this can be made private? https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:46: void CreateSharedMemoryBuffer(); ditto https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:49: static webkit_glue::WebThreadImpl& parser_thread(); this can just be a static method in the cc file instead of being here
Nice, thanks :). https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... content/child/resource_dispatcher.cc:641: DCHECK(request_info != NULL); On 2013/12/17 00:44:40, jam wrote: > nit: this dcheck isn't necessary. in release builds it does nothing. in debug > builds, the crash below is just as useful of a signal to the developer. Done. https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... content/child/resource_dispatcher.cc:644: if (request_info->buffer != NULL) { On 2013/12/17 00:44:40, jam wrote: > nit: "!= NULL" is redundant, convention is to skip it. same below on line 656, > just do "if (!request_info) I fixed the possible ones, but for the request_info->buffer: error: value of type 'linked_ptr<base::SharedMemory>' is not contextually convertible to 'bool' https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... File content/child/resource_dispatcher.h (right): https://codereview.chromium.org/109283006/diff/20001/content/child/resource_d... content/child/resource_dispatcher.h:72: // Callback from the WebParserResourceBridgeImpl happening when its On 2013/12/17 00:44:40, jam wrote: > nit: blank line above this. Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... File content/child/webparserresourcebridge_impl.cc (right): https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.cc:58: , main_thread_message_loop_(main_thread_message_loop) On 2013/12/17 00:44:40, jam wrote: > nit: google style is comma on previous lines Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.cc:122: // Do we care about this leaking on shutdown? On 2013/12/17 00:44:40, jam wrote: > no Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.cc:134: , shm_size_(shm_size) On 2013/12/17 00:44:40, jam wrote: > ditto Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.cc:173: if (peer_ == NULL) { On 2013/12/17 00:44:40, jam wrote: > nit: if (!peer_). also above and below Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... File content/child/webparserresourcebridge_impl.h (right): https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:19: class WebThread; On 2013/12/17 00:44:40, jam wrote: > no need to forward declare classes that are used in the parent interface, this > is redundant since they must have been forward declared already Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:23: class WebThreadImpl; On 2013/12/17 00:44:40, jam wrote: > nit: no indentation in forward declarations Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:29: class CONTENT_EXPORT WebParserResourceBridgeImpl : On 2013/12/17 00:44:40, jam wrote: > i don't think you need a CONTENT_EXPORT here since it's not used in unit tests. > then you wouldn't need NON_EXPORTED_BASE as well Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:43: void OnReceivedData(int data_offset, On 2013/12/17 00:44:40, jam wrote: > nit: this can be made private? It's called by ParserResourceMessageFilter::OnReceivedData (the internal class in the .cc), but maybe that should be a friend instead? https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:46: void CreateSharedMemoryBuffer(); On 2013/12/17 00:44:40, jam wrote: > ditto Done. https://codereview.chromium.org/109283006/diff/20001/content/child/webparserr... content/child/webparserresourcebridge_impl.h:49: static webkit_glue::WebThreadImpl& parser_thread(); On 2013/12/17 00:44:40, jam wrote: > this can just be a static method in the cc file instead of being here Done.
I probably can't take a look at this until Wednesday but am super duper excited about this patch! To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Second draft is up for this, based on the Blink-side review.
ping!
Darin - could you suggest a good review for the resource dispatcher changes here?
The CL description here seems like something that belongs in a design doc on dev.chromium.org. That is, instead of a description of the current CL, it seems to be a description of the entire system. I think it would be good to make each CLs description more specific to what it is doing, and then link to a big picture design doc. https://codereview.chromium.org/109283006/diff/270001/webkit/child/resource_l... File webkit/child/resource_loader_bridge.h (right): https://codereview.chromium.org/109283006/diff/270001/webkit/child/resource_l... webkit/child/resource_loader_bridge.h:220: // WebThreadedResourceProvider which will be used to interact with the parser It seems unnecessary to mention the "parser thread" here. There doesn't seem to be anything about this interface that is specific to the parser.
On 2014/02/11 07:04:59, darin wrote: > The CL description here seems like something that belongs in a design doc on > http://dev.chromium.org. That is, instead of a description of the current CL, it seems > to be a description of the entire system. I think it would be good to make each > CLs description more specific to what it is doing, and then link to a big > picture design doc. > > https://codereview.chromium.org/109283006/diff/270001/webkit/child/resource_l... > File webkit/child/resource_loader_bridge.h (right): > > https://codereview.chromium.org/109283006/diff/270001/webkit/child/resource_l... > webkit/child/resource_loader_bridge.h:220: // WebThreadedResourceProvider which > will be used to interact with the parser > It seems unnecessary to mention the "parser thread" here. There doesn't seem to > be anything about this interface that is specific to the parser. Oh, maybe I'm just observing the effect of some names changing since the CL was first posted. Still a design doc would be nice.
Some more comments... This looks really neat, but I'd like to understand the overall design better. From this CL, I'm only seeing part of it. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... File content/child/webthreadedresourceprovider_impl.cc (right): https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:22: class ParserResourceMessageFilter : public IPC::ChannelProxy::MessageFilter { I think I would avoid mentioning the parser in this file. It doesn't seem like there is anything stopping Blink from making use of this infrastructure for other things besides the parser. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:129: thread_ = new webkit_glue::WebThreadImpl("HTMLParserThread"); This code doesn't seem specific to the parser. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:177: delete foregroundClient_; nit: google variable naming style is foreground_client_ https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:181: return &parser_thread(); It is not clear why resourceThread() is a method since it always returns the same value. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:225: // TODO: XSS validation and other stuff needs to happen to happen nit: TODO(username) https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:231: // the browser process until we're done with it. true, but why does this message need to be sent from the main thread? you have the request_id_ so you should be able to send the ACK directly from the IO thread.
On 2014/02/11 07:14:22, darin wrote: > Some more comments... > > This looks really neat, but I'd like to understand the overall design better. > From this CL, I'm only seeing part of it. > Thanks for taking a look Darin :). The other half of this (Blink side) is https://codereview.chromium.org/100563004/ and should give the complete picture. I can take a stab at putting together some diagrams for this, it'd probably be the easiest way of demonstrating the flow here. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... File content/child/webthreadedresourceprovider_impl.cc (right): https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:22: class ParserResourceMessageFilter : public IPC::ChannelProxy::MessageFilter { On 2014/02/11 07:14:23, darin wrote: > I think I would avoid mentioning the parser in this file. It doesn't seem like > there is anything stopping Blink from making use of this infrastructure for > other things besides the parser. Done! Yeah this has become less and less parser specific in successive iterations. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:129: thread_ = new webkit_glue::WebThreadImpl("HTMLParserThread"); On 2014/02/11 07:14:23, darin wrote: > This code doesn't seem specific to the parser. Done. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:177: delete foregroundClient_; On 2014/02/11 07:14:23, darin wrote: > nit: google variable naming style is foreground_client_ Done. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:181: return &parser_thread(); On 2014/02/11 07:14:23, darin wrote: > It is not clear why resourceThread() is a method since it always returns the > same value. resourceProviderThread() (renamed now) is implementing a Blink interface method and needs to return a blink::WebThread* for use inside Blink, whereas parser_thread (now renamed resource_provider_thread) returns a webkit_glue::WebThreadImpl& so we can post tasks to it on the Chrome-side. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:225: // TODO: XSS validation and other stuff needs to happen to happen On 2014/02/11 07:14:23, darin wrote: > nit: TODO(username) Done. https://codereview.chromium.org/109283006/diff/270001/content/child/webthread... content/child/webthreadedresourceprovider_impl.cc:231: // the browser process until we're done with it. On 2014/02/11 07:14:23, darin wrote: > true, but why does this message need to be sent from the main thread? you have > the request_id_ so you should be able to send the ACK directly from the IO > thread. Clarified this in the comment a bit; it's because the incoming data is received on both the resource thread and the main thread. If we directly ACKed from the background thread we'd risk invalidating the shared memory buffer while the main thread is still dealing with the data.
On 2014/02/11 07:14:22, darin wrote: > Some more comments... > > This looks really neat, but I'd like to understand the overall design better. > From this CL, I'm only seeing part of it. > I don't have access to put stuff on dev.chromium.org yet, but I made a brief doc with some diagrams which should hopefully illustrate the full flow here: https://docs.google.com/a/google.com/document/d/1MAkeou6F60Xcc2B_FJ325h4hWpFl... Please let me know if there's anything I should clarify further!
The Blink side of this just got LGTM'd, so I'd appreciate another look at this :). (ping)
I also left some comments in the Blink-side CL for you to consider. Overall, this looks pretty good to me. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... File content/child/threadeddataprovider.cc (right): https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:26: WebThreadImpl& background_thread, nit: google c++ style forbids non-const reference parameters. use a pointer if this needs to be non-const. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:27: base::WeakPtr<ThreadedDataProvider> nit: pass by |const base::WeakPtr<...>&| ? https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:113: int data_offset, nit: indentation https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:161: // Release the filter from our locally held member variable before A better approach here is to use the traits template parameter to RefCountedThreadSafe<...>. Perhaps all MessageFilter subclasses should be destroyed on the IO thread? This is probably a yak shave best left for another CL, but it seems like the current code in this CL is super fragile. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:167: filter_ = nullptr; i don't think we use nullptr in chromium code yet. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:173: void DestructOnMainThread(ThreadedDataProvider* dataProvider) { nit: dataProvider -> data_provider (please double check all of the other variable names) https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:219: resource_filter_active_ = true; nit: add DCHECK(background_thread_.isCurrentThread()) ? https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:229: std::vector<QueuedSharedMemoryData> emptyList; nit: emptyList -> empty_list https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:231: queued_data_.swap(emptyList); nit: you could also just call .clear() on the vector. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:255: queuedData.data = data_ptr + data_offset; nit: queuedData -> queued_data https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:275: // TODO(oysteine): SiteIsolationPolicy needs to be be checked please make sure that creis@ is aware of this. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... File content/child/threadeddataprovider.h (right): https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2014 https://codereview.chromium.org/109283006/diff/540001/webkit/child/resource_l... File webkit/child/resource_loader_bridge.h (right): https://codereview.chromium.org/109283006/diff/540001/webkit/child/resource_l... webkit/child/resource_loader_bridge.h:227: blink::WebThreadedDataReceiver* threadedDataReceiver) = 0; nit: threaded_data_receiver
Thanks! New version up; comments below. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... File content/child/threadeddataprovider.cc (right): https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:26: WebThreadImpl& background_thread, On 2014/03/19 03:51:28, darin wrote: > nit: google c++ style forbids non-const reference parameters. use a pointer if > this needs to be non-const. Done. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:27: base::WeakPtr<ThreadedDataProvider> On 2014/03/19 03:51:28, darin wrote: > nit: pass by |const base::WeakPtr<...>&| ? Done. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:113: int data_offset, On 2014/03/19 03:51:28, darin wrote: > nit: indentation Done. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:161: // Release the filter from our locally held member variable before On 2014/03/19 03:51:28, darin wrote: > A better approach here is to use the traits template parameter to > RefCountedThreadSafe<...>. Perhaps all MessageFilter subclasses should be > destroyed on the IO thread? This is probably a yak shave best left for another > CL, but it seems like the current code in this CL is super fragile. Looking through this again it seems whatever reason there was for making sure the MessageFilter always gets destructed on the I/O thread isn't there anymore, probably only present in an earlier version of the patch. So I removed the sketchiness here. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:167: filter_ = nullptr; On 2014/03/19 03:51:28, darin wrote: > i don't think we use nullptr in chromium code yet. Done. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:173: void DestructOnMainThread(ThreadedDataProvider* dataProvider) { On 2014/03/19 03:51:28, darin wrote: > nit: dataProvider -> data_provider > > (please double check all of the other variable names) Sorry about all of these; the Chrome<->Blink style context switching still trips me up. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:219: resource_filter_active_ = true; On 2014/03/19 03:51:28, darin wrote: > nit: add DCHECK(background_thread_.isCurrentThread()) ? Done. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:231: queued_data_.swap(emptyList); On 2014/03/19 03:51:28, darin wrote: > nit: you could also just call .clear() on the vector. AFAIK .clear() doesn't actually free any memory, but I guess it doesn't really matter in this case. Done. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.cc:275: // TODO(oysteine): SiteIsolationPolicy needs to be be checked On 2014/03/19 03:51:28, darin wrote: > please make sure that creis@ is aware of this. Yeah we've discussed it in the past, and I landed a warmup CL to this one a couple of weeks ago which will make the needed fix pretty trivial. I plan on getting that landed as soon as this is in. https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... File content/child/threadeddataprovider.h (right): https://codereview.chromium.org/109283006/diff/540001/content/child/threadedd... content/child/threadeddataprovider.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/03/19 03:51:28, darin wrote: > nit: 2014 Done.
Darin: Ping :)
LGTM w/ nits https://codereview.chromium.org/109283006/diff/580001/content/child/resource_... File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/109283006/diff/580001/content/child/resource_... content/child/resource_dispatcher.cc:466: if (request_info->blocked_response && alternative_data.size() > 0) { nit: it is generally preferred with STL to do !container.empty() instead of .size() > 0. https://codereview.chromium.org/109283006/diff/580001/content/child/resource_... content/child/resource_dispatcher.cc:473: if (!request_info->blocked_response || alternative_data.size() > 0) { ditto https://codereview.chromium.org/109283006/diff/580001/content/child/resource_... File content/child/resource_dispatcher.h (right): https://codereview.chromium.org/109283006/diff/580001/content/child/resource_... content/child/resource_dispatcher.h:29: class ThreadedDataProvider; nit: "class" before "struct" https://codereview.chromium.org/109283006/diff/580001/content/child/web_url_l... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/109283006/diff/580001/content/child/web_url_l... content/child/web_url_loader_impl.cc:233: blink::WebThreadedDataReceiver* threadedDataReceiver); nit: threaded_data_receiver https://codereview.chromium.org/109283006/diff/580001/content/child/web_url_l... content/child/web_url_loader_impl.cc:312: blink::WebThreadedDataReceiver* threadedDataReceiver) { nit: threaded_data_receiver https://codereview.chromium.org/109283006/diff/580001/content/child/web_url_l... content/child/web_url_loader_impl.cc:314: return bridge_->AttachThreadedDataReceiver(threadedDataReceiver); ditto https://codereview.chromium.org/109283006/diff/580001/content/child/web_url_l... content/child/web_url_loader_impl.cc:886: blink::WebThreadedDataReceiver* threadedDataReceiver) { nit: threaded_data_receiver https://codereview.chromium.org/109283006/diff/580001/content/child/web_url_l... content/child/web_url_loader_impl.cc:887: return context_->AttachThreadedDataReceiver(threadedDataReceiver); ditto https://codereview.chromium.org/109283006/diff/580001/content/child/web_url_l... File content/child/web_url_loader_impl.h (right): https://codereview.chromium.org/109283006/diff/580001/content/child/web_url_l... content/child/web_url_loader_impl.h:46: blink::WebThreadedDataReceiver* threadedDataReceiver); nit: threaded_data_receiver
Thanks Darin! All nits fixed.
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/109283006/600001
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/109283006/620001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/109283006/620001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/109283006/620001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/109283006/880001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 275655 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
