Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288383002:
Use common code to set HttpNetworkSession::Param pointers. (Closed)
DescriptionUse common code to set HttpNetworkSession::Param pointers.
This CL is a refactor in which HttpNetworkSession "components", which
are passed to the constructor as pointers stored in a struct
HttpNetworkSession::Params, are initiatized directly from the context
under construction. This has two benefits:
1) It ensures the network session associated with a context is
consistent with the components for the context, and
2) it forces all session components to be initialized in a common code
block.
The second benefit is one step towards making sure can create isolated
contexts where the session components are unique from other contexts.
BUG=519373
Committed: https://crrev.com/ea309f76bd0f29ee2a5f2abfff449947df42225e
Cr-Commit-Position: refs/heads/master@{#345429}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Move network session creation into ConstructSystemRequestContext(). #Patch Set 3 : Apply same refactor for construction of ProxyScriptFetcherContext. #Patch Set 4 : Remove check, http_cache_enabled_ || !channel_id_service. #
Total comments: 21
Patch Set 5 : Address comments. #Patch Set 6 : Accidentally deleted code is restored. #
Total comments: 16
Patch Set 7 : Address suggestions. #Patch Set 8 : Rebase to r345348. #
Depends on Patchset: Messages
Total messages: 43 (15 generated)
wjmaclean@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@ - This is a (rough?) first draft of the re-factor I talked to you about. It compiles and basically runs. There are a few log warnings, but I'm not sure which of these are from this patch, and which are due to the current ToT state. I'm running some try jobs to see how much I may have broken. This CL addresses main/otr/system/proxy_fetcher contexts; everything that seems to rely on IOThread::InitializeNetworkSessionParametersFromGlobals(). I've changed my idea slightly from what we discussed: in this CL I'm concentrating on splitting the HttpNetworkSession::Params into "components" (pointers passed into the constructor) and "settings" (primary values that are local copies, and in some cases may be system-wide defaults). There are some "// WJM" comments that will disappear before the final (landable) version comes along ... I'm using these to mark sections of the code I may need to come back to at some point. This CL doesn't attempt to do any isolation of the main/system contexts yet, but hopefully will make that easier when the time comes. Let me know what you think of the direction.
https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1107: net::HttpNetworkSession::Params* params) { Not for this CL, but may make sense to separate arguments that are just settings the HttpNetworkSession accept, and arguments that are objects that actually do stuff. This would mostly set the latter. Could also then reasonably make the settings object an optional thing to set directly on the builder. Then we'd just have to worry about how to handle everything else. Could even get rid of all these "CopyToIfSet" things, and just have globals have a copy of this object, with settings overridden just according to flags. Anyhow, I'm just thinking out loud here. Certainly fine to go in some other direction. https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1111: params->cert_policy_enforcer = globals.cert_policy_enforcer.get(); // WJM I have no idea what this does - should probably talk to the SSL people about this (davidben, rsleevi) https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1112: params->host_mapping_rules = globals.host_mapping_rules.get(); // WJM host_mapping_rules is for command line options (Mostly used for debugging, I guess?), so it should be fine. https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1227: ConstructSystemRequestContext(globals_, net_log_)); Everything below this line may make more sense in ConstructSystemRequestContext (And the system_url_request_job_factory construction line, too). Does have a bit more side effects that way, but that's also how the builder will work, when we switch to it.
Oops...Lost my main comment - I think this looks like a pretty reasonable first step.
wjmaclean@chromium.org changed reviewers: + davidben@chromium.org
davidben@ - could you please comment on whether cert_policy_enforcer can be safely shared between HttpNetworkSessions? See code/comment at: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...
I've moved the network session creation code into ConstructSystemRequestContext() as suggested. Do we want to consider doing the same for ConstructProxyScriptFetcherContext() too? I read a comment suggesting that code was going away, in which case it might not be worth it. I'll also look into creating the separate settings object for the non-component settings. https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1111: params->cert_policy_enforcer = globals.cert_policy_enforcer.get(); // WJM On 2015/08/13 15:37:38, mmenke wrote: > I have no idea what this does - should probably talk to the SSL people about > this (davidben, rsleevi) I've looped in davidben@ to comment on this. https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1112: params->host_mapping_rules = globals.host_mapping_rules.get(); // WJM On 2015/08/13 15:37:38, mmenke wrote: > host_mapping_rules is for command line options (Mostly used for debugging, I > guess?), so it should be fine. Acknowledged. https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1227: ConstructSystemRequestContext(globals_, net_log_)); On 2015/08/13 15:37:38, mmenke wrote: > Everything below this line may make more sense in ConstructSystemRequestContext > (And the system_url_request_job_factory construction line, too). Does have a > bit more side effects that way, but that's also how the builder will work, when > we switch to it. Done.
On 2015/08/14 13:29:19, wjmaclean wrote: > I've moved the network session creation code into > ConstructSystemRequestContext() as suggested. Do we want to consider doing the > same for ConstructProxyScriptFetcherContext() too? I read a comment suggesting > that code was going away, in which case it might not be worth it. I think that's reasonable. That comment has been around for quite a while, so I suspect it's not going away anytime soon, though may be worth asking eroman about it. > I'll also look into creating the separate settings object for the non-component > settings. > > https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... > chrome/browser/io_thread.cc:1111: params->cert_policy_enforcer = > globals.cert_policy_enforcer.get(); // WJM > On 2015/08/13 15:37:38, mmenke wrote: > > I have no idea what this does - should probably talk to the SSL people about > > this (davidben, rsleevi) > > I've looped in davidben@ to comment on this. > > https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... > chrome/browser/io_thread.cc:1112: params->host_mapping_rules = > globals.host_mapping_rules.get(); // WJM > On 2015/08/13 15:37:38, mmenke wrote: > > host_mapping_rules is for command line options (Mostly used for debugging, I > > guess?), so it should be fine. > > Acknowledged. > > https://codereview.chromium.org/1288383002/diff/1/chrome/browser/io_thread.cc... > chrome/browser/io_thread.cc:1227: ConstructSystemRequestContext(globals_, > net_log_)); > On 2015/08/13 15:37:38, mmenke wrote: > > Everything below this line may make more sense in > ConstructSystemRequestContext > > (And the system_url_request_job_factory construction line, too). Does have a > > bit more side effects that way, but that's also how the builder will work, > when > > we switch to it. > > Done.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288383002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288383002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
I think this CL is ready to graduate from [wip] to regular status: may we do a formal review cycle now? (Thanks!)
Still want to do another careful pass over this - I'm paranoid about changing behavior in subtle ways. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1526: // now that I got rid of refcounting URLRequestContexts. Hrm...Wonder how doable this is. Not suggesting you take it on, but if I could get this done myself, make some of the future refactors a little simpler. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1552: "466432 IOThread::InitAsync::SetProtocolHandler")); This should be renamed "466432 IOThread::ConstructProxyScriptFetcherContext" and probably moved to the start of this method. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1572: globals->proxy_script_fetcher_url_request_job_factory.get()); Suggest we put job factory initialization last, since it involves the most code. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1581: globals->http_server_properties->GetWeakPtr()); This last one wasn't being done on the proxy context before, I believe? I don't believe it matters, but want to try and minimize functional changes in this CL, for now. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1592: "466432 IOThread::InitAsync::HttpNetorkSession::Start")); "466432 IOThread::ConstructProxyScriptFetcherContext2" https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1593: TRACE_EVENT_BEGIN0("startup", "IOThread::InitAsync:HttpNetworkSession"); These two TRACE_EVENTS are weird. Suggest moving them back into IOThread::Init, making them surround this method call, and renaming them to IOThread::Init::ProxyScriptFetcherRequestContext. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1303: params.ssl_session_cache_shard = GetSSLSessionCacheShard(); If you're wondering what this is (I know I was...), I think it's for SSL session resumption. If you had an SSL connection to a site, you can resume the old encrypted session, and save a round trip when setting up a connection. The way at least one of our SSL libraries works is that SSL sessions are global state, I believe, so we need to add a string to differentiate SSL sessions, to prevent us from, do things like resuming a non-incognito SSL session in an incognito session. https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:256: void URLRequestContextBuilder::SetHttpNetworkSessionComponents( Definition order should match declaration order. https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... net/url_request/url_request_context_builder.h:92: URLRequestContext* context); nit: Per google style guide, inputs should go before outputs, and the context is basically an input. I believe you can also make the context a const reference, too. https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... net/url_request/url_request_context_builder.h:92: URLRequestContext* context); This should go after the constructor and destructor.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288383002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288383002/80001
mmenke@ - I've addressed your comments, ptal? https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1526: // now that I got rid of refcounting URLRequestContexts. On 2015/08/17 20:13:04, mmenke wrote: > Hrm...Wonder how doable this is. Not suggesting you take it on, but if I could > get this done myself, make some of the future refactors a little simpler. Acknowledged. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1552: "466432 IOThread::InitAsync::SetProtocolHandler")); On 2015/08/17 20:13:04, mmenke wrote: > This should be renamed "466432 IOThread::ConstructProxyScriptFetcherContext" and > probably moved to the start of this method. Done. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1572: globals->proxy_script_fetcher_url_request_job_factory.get()); On 2015/08/17 20:13:03, mmenke wrote: > Suggest we put job factory initialization last, since it involves the most code. Done. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1581: globals->http_server_properties->GetWeakPtr()); On 2015/08/17 20:13:04, mmenke wrote: > This last one wasn't being done on the proxy context before, I believe? I don't > believe it matters, but want to try and minimize functional changes in this CL, > for now. Adding this was necessary because we need the http_server_properties for the HttpNetworkSession construction (it has a CHECK requiring it https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...). Before this use to get set directly on the params in IOThread::InitializeNetworkSessionParamsFromGlobals() called from IOThread::Init(), but since the refactored code pulls this value from the context, it needs to be set on the context in order to match how other contexts are built. If we're really worried it will change something we can clear it again after line 1586 below. But it would be nice if this context could be consistent with other contexts. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1592: "466432 IOThread::InitAsync::HttpNetorkSession::Start")); On 2015/08/17 20:13:04, mmenke wrote: > "466432 IOThread::ConstructProxyScriptFetcherContext2" Done. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1593: TRACE_EVENT_BEGIN0("startup", "IOThread::InitAsync:HttpNetworkSession"); On 2015/08/17 20:13:04, mmenke wrote: > These two TRACE_EVENTS are weird. Suggest moving them back into IOThread::Init, > making them surround this method call, and renaming them to > IOThread::Init::ProxyScriptFetcherRequestContext. Done. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1303: params.ssl_session_cache_shard = GetSSLSessionCacheShard(); On 2015/08/17 20:13:04, mmenke wrote: > If you're wondering what this is (I know I was...), I think it's for SSL session > resumption. If you had an SSL connection to a site, you can resume the old > encrypted session, and save a round trip when setting up a connection. The way > at least one of our SSL libraries works is that SSL sessions are global state, I > believe, so we need to add a string to differentiate SSL sessions, to prevent us > from, do things like resuming a non-incognito SSL session in an incognito > session. Acknowledged. https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:256: void URLRequestContextBuilder::SetHttpNetworkSessionComponents( On 2015/08/17 20:13:04, mmenke wrote: > Definition order should match declaration order. Done. https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... net/url_request/url_request_context_builder.h:92: URLRequestContext* context); On 2015/08/17 20:13:04, mmenke wrote: > This should go after the constructor and destructor. Done. Sorry about that ... I thought static functions went ahead, but that's just faulty memory on my part :-) https://codereview.chromium.org/1288383002/diff/60001/net/url_request/url_req... net/url_request/url_request_context_builder.h:92: URLRequestContext* context); On 2015/08/17 20:13:04, mmenke wrote: > nit: Per google style guide, inputs should go before outputs, and the context > is basically an input. > > I believe you can also make the context a const reference, too. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This looks really good. https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1288383002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:1581: globals->http_server_properties->GetWeakPtr()); On 2015/08/18 14:17:22, wjmaclean wrote: > On 2015/08/17 20:13:04, mmenke wrote: > > This last one wasn't being done on the proxy context before, I believe? I > don't > > believe it matters, but want to try and minimize functional changes in this > CL, > > for now. > > Adding this was necessary because we need the http_server_properties for the > HttpNetworkSession construction (it has a CHECK requiring it > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...). > > Before this use to get set directly on the params in > IOThread::InitializeNetworkSessionParamsFromGlobals() called from > IOThread::Init(), but since the refactored code pulls this value from the > context, it needs to be set on the context in order to match how other contexts > are built. > > If we're really worried it will change something we can clear it again after > line 1586 below. But it would be nice if this context could be consistent with > other contexts. Ahh...It was being set on the session, but not the context. Which seems like a bug, though a harmless one - everything in net accesses them through the session. https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1482: net::NetLog* net_log) { fix indent https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1511: globals->ssl_config_service.get()); You're doing this twice. https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1530: // See IOThread::Globals for details. Method level comments should go with the definition, not the declaration. https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1531: // static optional nit: The latest google style guide says nothing about adding static comments, it was from an earlier version, so could just remove all the "// statics" from this file...Fine to leave them in, though. Up to you. https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1576: tracked_objects::ScopedTracker tracking_profile15( nit: Should probably renumber these to match their strings (1/2/3 instead of 6/4/5) https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1578: "466432 IOThread::InitAsync::HttpNetorkSession::End")); IOThread::ConstructProxyScriptFetcherContext3? https://codereview.chromium.org/1288383002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1288383002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:215: // static nit: Remove static (No longer in Google style guide) https://codereview.chromium.org/1288383002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1288383002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.h:93: static void SetHttpNetworkSessionComponents( Should have a comment about what it does
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288383002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288383002/120001
I think I've addressed everything, ptal :-) https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1482: net::NetLog* net_log) { On 2015/08/18 18:30:32, mmenke wrote: > fix indent Done. https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1511: globals->ssl_config_service.get()); On 2015/08/18 18:30:32, mmenke wrote: > You're doing this twice. Thanks for spotting this ... fixed! https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1530: // See IOThread::Globals for details. On 2015/08/18 18:30:32, mmenke wrote: > Method level comments should go with the definition, not the declaration. Done. https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1531: // static On 2015/08/18 18:30:32, mmenke wrote: > optional nit: The latest google style guide says nothing about adding static > comments, it was from an earlier version, so could just remove all the "// > statics" from this file...Fine to leave them in, though. Up to you. Done. (Removed throughout this file.) https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1576: tracked_objects::ScopedTracker tracking_profile15( On 2015/08/18 18:30:32, mmenke wrote: > nit: Should probably renumber these to match their strings (1/2/3 instead of > 6/4/5) Done. https://codereview.chromium.org/1288383002/diff/100001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1578: "466432 IOThread::InitAsync::HttpNetorkSession::End")); On 2015/08/18 18:30:32, mmenke wrote: > IOThread::ConstructProxyScriptFetcherContext3? Done. https://codereview.chromium.org/1288383002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1288383002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:215: // static On 2015/08/18 18:30:32, mmenke wrote: > nit: Remove static (No longer in Google style guide) Done. https://codereview.chromium.org/1288383002/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1288383002/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.h:93: static void SetHttpNetworkSessionComponents( On 2015/08/18 18:30:32, mmenke wrote: > Should have a comment about what it does Done.
LGTM, though per earlier discussion, I think we should hold off on landing until after branch, given how easy it would be to miss a breakage in this CL, and have no tests fail. Should be fine to land Monday. Happy to start reviewing another CL on top of this one in the meantime - don't want this to delay you.
On 2015/08/18 19:00:05, mmenke wrote: > LGTM, though per earlier discussion, I think we should hold off on landing until > after branch, given how easy it would be to miss a breakage in this CL, and have > no tests fail. Should be fine to land Monday. > > Happy to start reviewing another CL on top of this one in the meantime - don't > want this to delay you. Sure thing ... I was sort of thinking along the same lines since there's virtually no bake-time left before branch. I'll continue working ahead ...
(I have a giant email and code review hole to dig myself out of, so I'll defer to Matt here, unless one of you really wants me to give it a separate look.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
davidben@chromium.org changed reviewers: - davidben@chromium.org
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288383002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288383002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/08/25 16:38:42, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) So much for Tuesday morning optimism ... rebasing now. ;-)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288383002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288383002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1288383002/#ps140001 (title: "Rebase to r345348.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288383002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288383002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ea309f76bd0f29ee2a5f2abfff449947df42225e Cr-Commit-Position: refs/heads/master@{#345429} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
