Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
629393002:
Chromium side of geofencing event dispatching. (Closed)
|
Created:
6 years, 2 months ago by Marijn Kruisselbrink Modified:
6 years, 1 month ago Reviewers:
michaeln, Avi (use Gerrit), dominicc (has gone to gerrit), falken, Michael van Ouwerkerk, palmer, Nate Chapin CC:
chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+serviceworker, mkwst+moarreviews-ipc_chromium.org, mkwst+moarreviews-renderer_chromium.org, nhiroki, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@geofencing_serviceworker Project:
chromium Visibility:
Public. |
DescriptionChromium side of geofencing event dispatching.
BUG=383125
Committed: https://crrev.com/1c397381ff0444275cbc932eb750c86888f5669c
Cr-Commit-Position: refs/heads/master@{#301965}
Patch Set 1 #
Total comments: 6
Patch Set 2 : nits #Patch Set 3 : rebase #
Total comments: 3
Patch Set 4 : add todos #
Total comments: 8
Patch Set 5 : rebase after manager refactor #Patch Set 6 : rebase on top of other CL #Patch Set 7 : rebase #
Total comments: 4
Patch Set 8 : rebase #Patch Set 9 : address comments #
Total comments: 5
Patch Set 10 : nit #Messages
Total messages: 38 (8 generated)
dominicc@chromium.org changed reviewers: + dominicc@chromium.org
DBCs inline. https://codereview.chromium.org/629393002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/629393002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_version.h:205: // Sends geofencing event to the associated embedded woker and asynchronously nit: woker https://codereview.chromium.org/629393002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_version.h:206: // calls |callback| when it errors out or it gets response from the worker to nit: response needs an article--a, the
mvanouwerkerk@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
I'd recommend adding michaeln as a reviewer. You are in the same time zone, right? Also, you could then drop by his desk for a personal chat more easily. https://codereview.chromium.org/629393002/diff/1/content/browser/geofencing/g... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/1/content/browser/geofencing/g... content/browser/geofencing/geofencing_manager.cc:27: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Nit: use DCHECK_CURRENTLY_ON here and below.
mek@chromium.org changed reviewers: + michaeln@chromium.org
+michaeln: can you take a look at this CL? https://codereview.chromium.org/629393002/diff/1/content/browser/geofencing/g... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/1/content/browser/geofencing/g... content/browser/geofencing/geofencing_manager.cc:27: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2014/10/08 16:42:00, Michael van Ouwerkerk wrote: > Nit: use DCHECK_CURRENTLY_ON here and below. Done. https://codereview.chromium.org/629393002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/629393002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_version.h:205: // Sends geofencing event to the associated embedded woker and asynchronously On 2014/10/07 01:52:05, dominicc wrote: > nit: woker Done. https://codereview.chromium.org/629393002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_version.h:206: // calls |callback| when it errors out or it gets response from the worker to On 2014/10/07 01:52:05, dominicc wrote: > nit: response needs an article--a, the Done. (and also fixed this in all the other places in this file)
https://codereview.chromium.org/629393002/diff/70001/content/browser/geofenci... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/70001/content/browser/geofenci... content/browser/geofencing/geofencing_manager.cc:28: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( I'm not sure if this is correct or not. Somebody who understands where partition boundaries are and why should take a look at this. You may need to store information about which 'partition' a page registering the region belongs to? Have you thought about storing registrations per-storagepartition and having a GeofencingManager for each? Lets say i have two user profiles for Fred and Sally, both of which have registered regions. Suppose Sally is using the browser and Fred isn't around. When you enter a region registered to Fred, what is supposed to happen? Now suppose Freds data is deleted. What prevents the mixing of data across boundaries that shouldn't be mixed? https://codereview.chromium.org/629393002/diff/70001/content/browser/geofenci... File content/browser/geofencing/geofencing_manager.h (right): https://codereview.chromium.org/629393002/diff/70001/content/browser/geofenci... content/browser/geofencing/geofencing_manager.h:69: void RegisterRegion(BrowserContext* browser_context, What if this is an incognito profile?
https://codereview.chromium.org/629393002/diff/70001/content/browser/geofenci... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/70001/content/browser/geofenci... content/browser/geofencing/geofencing_manager.cc:28: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( On 2014/10/13 00:22:32, michaeln wrote: > I'm not sure if this is correct or not. Somebody who understands where partition > boundaries are and why should take a look at this. You may need to store > information about which 'partition' a page registering the region belongs to? This part is at least the same as what push messaging does to deliver push messages (in src/content/browser/push_messaging_router.cc). There too it looks up the storage partition by browser context and origin. > Have you thought about storing registrations per-storagepartition and having a > GeofencingManager for each? Lets say i have two user profiles for Fred and > Sally, both of which have registered regions. Suppose Sally is using the browser > and Fred isn't around. When you enter a region registered to Fred, what is > supposed to happen? Now suppose Freds data is deleted. > > What prevents the mixing of data across boundaries that shouldn't be mixed? It's complicated. At some level all geofencing registration will have to be combined before being handed of to the platform specific geofencing system (some things can just only be done with access to all registrations, not just those specific to a browser context or storage partition). But when it comes to persistence, at least part of that will almost certainly mean having something with an identical scope and lifetime as ServiceWorkerContextCore. I'm not sure if ServiceWorkerContextObserver already has all the right hooks to fully make this possible though. I've added TODOs to geofencing_manager.h to somehow handle ServiceWorkerRegistrations going away/being unregistered, and hopefully handling that case will also "automatically" handle the case of a BrowserContext going away (like in the case of an incognito profile being destroyed) since presumably destroying a browser context involves destroying all its service worker registrations. The question of what should happen with the two user profiles you mention is an interesting one though. I'd say that at least on android (which is a simpler case there is no multi-profile support) geofencing events, push messages, etc should all be delivered and wake up a service worker even when chrome is not currently running. Continuing that reasoning to a multi-profile situation would imply that events should be delivered/wake up service workers also in profiles that currently aren't active. But while that might be nice, it also causes problems. On windows/linux/mac where there is nothing like a password protecting multiple profiles this probably would still work reasonably well (although any UI that is the result of an event sent to a profile that wasn't currently active might be confusing), but on chromeos this really becomes problematic. So in short, I'm not sure what the solution will be here/what the desired behavior is. I think as a start just limiting events to when a profile is active is good enough, but before this is launched the "real" background story will have to be figured out (but that is something that effects more than just the geofencing API).
looks great except for picking the partition which confounds me? https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:28: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( I don't see how this can be correct? An app embeds a webview, the webview is isolated from the normal web. The webview runs content that registers a sw at some origin. The user browses the normal web and runs content that also registers a sw for that origin. This function is called and is given 'origin' as input. What gets returned? https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:65: service_worker_registration->active_version()->DispatchGeofencingEvent( need to be more defensive about active_version() being null, if a registration occurs during installation there may be no active version
https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:28: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( On 2014/10/14 02:20:40, michaeln wrote: > I don't see how this can be correct? > > An app embeds a webview, the webview is isolated from the normal web. The > webview runs content that registers a sw at some origin. > > The user browses the normal web and runs content that also registers a sw for > that origin. > > This function is called and is given 'origin' as input. What gets returned? Ah yes, you're right of course. That also means that the push messaging code has the same fundamental problem. There too it is assumed that the combination of BrowserContext and origin is enough to determine the StoragePartition and ServiceWorkerContextWrapper to deliver an event to. I could do an ugly hack and replace every BrowserContext* reference in GeofencingManager with a StoragePartition*, but maybe it's better to just first improve/refactor GeofencingManager to add a per-storage-partition geofencing context, so I think that's what I'll do. https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:65: service_worker_registration->active_version()->DispatchGeofencingEvent( On 2014/10/14 02:20:40, michaeln wrote: > need to be more defensive about active_version() being null, if a registration > occurs during installation there may be no active version Hmm, okay... So what I think should happen if a geofence is registered and triggered during installation but before activation, it that the geofencing event gets delivered as soon as the service worker got activated? Is there currently something I can call on the ServiceWorkerRegistration that will do the "call this when you have an active version" bit that I need for that?
falken@chromium.org changed reviewers: + falken@chromium.org
https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:65: service_worker_registration->active_version()->DispatchGeofencingEvent( On 2014/10/14 19:16:55, Marijn Kruisselbrink wrote: > On 2014/10/14 02:20:40, michaeln wrote: > > need to be more defensive about active_version() being null, if a registration > > occurs during installation there may be no active version > Hmm, okay... So what I think should happen if a geofence is registered and > triggered during installation but before activation, it that the geofencing > event gets delivered as soon as the service worker got activated? Is there > currently something I can call on the ServiceWorkerRegistration that will do the > "call this when you have an active version" bit that I need for that? I haven't been closely following... There's a few cases when there won't be an active version: 1) During installation before activation. 2) The browser started with a waiting SW. Currently it won't get activated until you navigate to a page in its scope. 3) The registration is "dead" with no associated SWs in it (but I think you wouldn't find the registration in this case). Also if there's an active version, but also waiting and/or installing versions in the pipeline, do you want to dispatch the event to those versions also? You could use SWRegistration AddListener and watch for OnVersionAttributesChanged events then dispatch the event as appropriate.
https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:28: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( On 2014/10/14 19:16:55, Marijn Kruisselbrink wrote: > On 2014/10/14 02:20:40, michaeln wrote: > > I don't see how this can be correct? > > > > An app embeds a webview, the webview is isolated from the normal web. The > > webview runs content that registers a sw at some origin. > > > > The user browses the normal web and runs content that also registers a sw for > > that origin. > > > > This function is called and is given 'origin' as input. What gets returned? > Ah yes, you're right of course. That also means that the push messaging code has > the same fundamental problem. There too it is assumed that the combination of > BrowserContext and origin is enough to determine the StoragePartition and > ServiceWorkerContextWrapper to deliver an event to. > I could do an ugly hack and replace every BrowserContext* reference in > GeofencingManager with a StoragePartition*, but maybe it's better to just first > improve/refactor GeofencingManager to add a per-storage-partition geofencing > context, so I think that's what I'll do. Okay, I did this refactoring of GeofencingManager in https://codereview.chromium.org/645763003/, which now makes all this code local to a specific StoragePartition. Additionally I rebased this CL on top of https://codereview.chromium.org/623823002/ (which is rebased on top of the GeofencingManager refactoring) in an attempt to keep the diffs as sane as possible. https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:65: service_worker_registration->active_version()->DispatchGeofencingEvent( On 2014/10/15 01:59:34, falken wrote: > On 2014/10/14 19:16:55, Marijn Kruisselbrink wrote: > > On 2014/10/14 02:20:40, michaeln wrote: > > > need to be more defensive about active_version() being null, if a > registration > > > occurs during installation there may be no active version > > Hmm, okay... So what I think should happen if a geofence is registered and > > triggered during installation but before activation, it that the geofencing > > event gets delivered as soon as the service worker got activated? Is there > > currently something I can call on the ServiceWorkerRegistration that will do > the > > "call this when you have an active version" bit that I need for that? > > I haven't been closely following... > > There's a few cases when there won't be an active version: > 1) During installation before activation. > 2) The browser started with a waiting SW. Currently it won't get activated until > you navigate to a page in its scope. > 3) The registration is "dead" with no associated SWs in it (but I think you > wouldn't find the registration in this case). > > Also if there's an active version, but also waiting and/or installing versions > in the pipeline, do you want to dispatch the event to those versions also? I'm not sure, I think generally events are only delivered to the active version? But yes, that all gets a bit confusing when an event is the result of code that ran in the oninstall event of a new version. For now I've just added a TODO to figure out how to deal with this in the future. > You could use SWRegistration AddListener and watch for > OnVersionAttributesChanged events then dispatch the event as appropriate. Okay, thanks.
https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/240001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:65: service_worker_registration->active_version()->DispatchGeofencingEvent( On 2014/10/15 23:55:15, Marijn Kruisselbrink wrote: > On 2014/10/15 01:59:34, falken wrote: > > On 2014/10/14 19:16:55, Marijn Kruisselbrink wrote: > > > On 2014/10/14 02:20:40, michaeln wrote: > > > > need to be more defensive about active_version() being null, if a > > registration > > > > occurs during installation there may be no active version > > > Hmm, okay... So what I think should happen if a geofence is registered and > > > triggered during installation but before activation, it that the geofencing > > > event gets delivered as soon as the service worker got activated? Is there > > > currently something I can call on the ServiceWorkerRegistration that will do > > the > > > "call this when you have an active version" bit that I need for that? > > > > I haven't been closely following... > > > > There's a few cases when there won't be an active version: > > 1) During installation before activation. > > 2) The browser started with a waiting SW. Currently it won't get activated > until > > you navigate to a page in its scope. > > 3) The registration is "dead" with no associated SWs in it (but I think you > > wouldn't find the registration in this case). > > > > Also if there's an active version, but also waiting and/or installing versions > > in the pipeline, do you want to dispatch the event to those versions also? > I'm not sure, I think generally events are only delivered to the active version? > But yes, that all gets a bit confusing when an event is the result of code that > ran in the oninstall event of a new version. For now I've just added a TODO to > figure out how to deal with this in the future. TODO sounds good. We sometimes dispatch events to non-active versions: 'install' and 'activate' (these occur during registration), postMessage can also be done on non-active versions.
michael/michael: All the dependencies for this CL have landed now, so could you please take another look?
lgtm This looks like it dispatches events to the Service Worker a lot like how push and sync do. Long-term we probably should have an easier way to build functionality on top of Service Worker.
lgtm with nits https://codereview.chromium.org/629393002/diff/310001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/310001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:311: return; I think you'll want to keep track of such failures. Maybe add a TODO for UMA instrumentation to track this failure? https://codereview.chromium.org/629393002/diff/310001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.h (right): https://codereview.chromium.org/629393002/diff/310001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.h:135: void DispatchGeofencingEvent(blink::WebGeofencingEventType event_type, Please document these methods. I can't tell from this header what the difference is between 'DispatchGeofencingEvent' and 'DeliverGeofencingEvent'.
On 2014/10/27 04:56:38, falken wrote: > lgtm > > This looks like it dispatches events to the Service Worker a lot like how push > and sync do. Long-term we probably should have an easier way to build > functionality on top of Service Worker. Yeah, it would be nice to be able to do this without having to modify ServiceWorkerVersion. And largely that already is possible I think. Instead of having a DispatchGeofencingEvent method on ServiceWorker I could have called a slightly modified ServiceWorkerVersion::SendMessage that deals with the "the event has been delivered" callback somehow generically. Although that still would be somewhat complicated. But yes, it would be nice to avoid all this mostly duplicate code in ServiceWorkerVersion for each separate event type (especially since I(&you) missed one spot in ServiceWorkerVersion I should have changed to add this new event type, the OnStopped method).
https://codereview.chromium.org/629393002/diff/310001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.cc (right): https://codereview.chromium.org/629393002/diff/310001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.cc:311: return; On 2014/10/27 15:57:40, Michael van Ouwerkerk wrote: > I think you'll want to keep track of such failures. Maybe add a TODO for UMA > instrumentation to track this failure? Done. https://codereview.chromium.org/629393002/diff/310001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.h (right): https://codereview.chromium.org/629393002/diff/310001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.h:135: void DispatchGeofencingEvent(blink::WebGeofencingEventType event_type, On 2014/10/27 15:57:40, Michael van Ouwerkerk wrote: > Please document these methods. I can't tell from this header what the difference > is between 'DispatchGeofencingEvent' and 'DeliverGeofencingEvent'. Done.
Patchset #9 (id:350001) has been deleted
mek@chromium.org changed reviewers: + avi@chromium.org, dcheng@chromium.org
+dcheng for content/common/service_worker/service_worker_messages.h and WebKit/public (DEPS target path) OWNERS +avi for content/*/DEPS OWNERS
Sorry, I'm really busy with blinkon stuff this week. If this is urgent, you may want to find another IPC reviewer.
mek@chromium.org changed reviewers: + japhet@chromium.org, palmer@chromium.org - dcheng@chromium.org
On 2014/10/27 18:36:21, dcheng wrote: > Sorry, I'm really busy with blinkon stuff this week. If this is urgent, you may > want to find another IPC reviewer. Thanks for letting me know. +palmer for IPC OWNERS +japhet for DEPS target (WebKit/public) OWNERS
On 2014/10/27 18:41:22, Marijn Kruisselbrink wrote: > On 2014/10/27 18:36:21, dcheng wrote: > > Sorry, I'm really busy with blinkon stuff this week. If this is urgent, you > may > > want to find another IPC reviewer. > Thanks for letting me know. > > +palmer for IPC OWNERS > +japhet for DEPS target (WebKit/public) OWNERS content DEPS lgtm
lgtm 2
https://codereview.chromium.org/629393002/diff/370001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.h (right): https://codereview.chromium.org/629393002/diff/370001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.h:152: // Called when delivering of a geofence event to a service worker has finished Nit: "...when delivery of..." https://codereview.chromium.org/629393002/diff/370001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/629393002/diff/370001/content/browser/service... content/browser/service_worker/service_worker_version.cc:382: DCHECK_EQ(ACTIVATED, status()) << status(); What are the consequences of continuing in a production build, if status() != ACTIVATED?
https://codereview.chromium.org/629393002/diff/370001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/629393002/diff/370001/content/browser/service... content/browser/service_worker/service_worker_version.cc:382: DCHECK_EQ(ACTIVATED, status()) << status(); On 2014/10/27 22:27:37, Chromium Palmer wrote: > What are the consequences of continuing in a production build, if status() != > ACTIVATED? nothing harmful, this just catches cases where the geofencemanager attempts to raise the event into a script that is still somewhere in the installation/activation pipeline (see line 345 in geofencemanager). the intent is to only raise this event (and push events) into versions that are fully activated.
palmer/japhet: PTAL https://codereview.chromium.org/629393002/diff/370001/content/browser/geofenc... File content/browser/geofencing/geofencing_manager.h (right): https://codereview.chromium.org/629393002/diff/370001/content/browser/geofenc... content/browser/geofencing/geofencing_manager.h:152: // Called when delivering of a geofence event to a service worker has finished On 2014/10/27 22:27:36, Chromium Palmer wrote: > Nit: "...when delivery of..." Done. https://codereview.chromium.org/629393002/diff/370001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/629393002/diff/370001/content/browser/service... content/browser/service_worker/service_worker_version.cc:382: DCHECK_EQ(ACTIVATED, status()) << status(); On 2014/10/27 22:27:37, Chromium Palmer wrote: > What are the consequences of continuing in a production build, if status() != > ACTIVATED? as michaeln said, this is purely about correctness with regard to following the spec/delivering the event to the right service worker version. The worst that can happen here is that an event gets delivered to the wrong version of a service worker.
> content/browser/service_worker/service_worker_version.cc:382: > DCHECK_EQ(ACTIVATED, status()) << status(); > On 2014/10/27 22:27:37, Chromium Palmer wrote: > > What are the consequences of continuing in a production build, if status() != > > ACTIVATED? > as michaeln said, this is purely about correctness with regard to following the > spec/delivering the event to the right service worker version. The worst that > can happen here is that an event gets delivered to the wrong version of a > service worker. I'm inclined to say that a correctness check dependent on what has happened at run-time should be a run-time if ... else ..., not a DCHECK.
On 2014/10/29 20:48:09, Chromium Palmer wrote:
> > content/browser/service_worker/service_worker_version.cc:382:
> > DCHECK_EQ(ACTIVATED, status()) << status();
> > On 2014/10/27 22:27:37, Chromium Palmer wrote:
> > > What are the consequences of continuing in a production build, if status()
> !=
> > > ACTIVATED?
> > as michaeln said, this is purely about correctness with regard to following
> the
> > spec/delivering the event to the right service worker version. The worst
that
> > can happen here is that an event gets delivered to the wrong version of a
> > service worker.
>
> I'm inclined to say that a correctness check dependent on what has happened at
> run-time should be a run-time if ... else ..., not a DCHECK.
It's not really dependent on what has happened at runtime, This DCHECK makes
sure that the code in GeofencingManager picks the correct ServiceWorkerVersion
to call this method on. There are no situations, other than bugs in
GeofencingManager (or other code that might call this method) that should be
able to cause this DCHECK to fail. So this case seems to exactly match what the
coding style says DCHECKs should be used for
('Use DCHECK() or NOTREACHED() as assertions, e.g. to document pre- and
post-conditions. A DCHECK() means "this condition must always be true", not
"this condition is normally true, but perhaps not in exceptional cases."')
OK. LGTM.
The CQ bit was checked by mek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/629393002/380001
DEPS changes LGTM
Message was sent while issue was closed.
Committed patchset #10 (id:380001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/1c397381ff0444275cbc932eb750c86888f5669c Cr-Commit-Position: refs/heads/master@{#301965} |
