|
|
DescriptionThis exposes a way to see if there is a Service Worker controlling two URLs.
This is used for the app banner manager to determine if it should
show a banner for the current page.
BUG=452825
Committed: https://crrev.com/f23c9142c04fc5d10e9164294d61095ff578b4a8
Cr-Commit-Position: refs/heads/master@{#315024}
Patch Set 1 #Patch Set 2 : Change interface, use it #Patch Set 3 : Finish off chrome side #Patch Set 4 : Tidy up #Patch Set 5 : Rebase #
Total comments: 3
Patch Set 6 : Android compile #
Total comments: 11
Patch Set 7 : Feedback #
Total comments: 2
Patch Set 8 : Rename #Patch Set 9 : Updated comment #
Total comments: 6
Patch Set 10 : Rebase and naming / comment tweaks #
Messages
Total messages: 37 (13 generated)
benwells@chromium.org changed reviewers: + falken@chromium.org
falken: early review for the SW stuff which I don't think will change. The app banner stuff isn't quite done yet but wanted to send it out.
benwells@chromium.org changed reviewers: + dfalcantara@chromium.org
+dfalcantara for c/b/android/banners
Patchset #4 (id:60001) has been deleted
https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/a... File chrome/browser/android/banners/app_banner_manager.h (right): https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/a... chrome/browser/android/banners/app_banner_manager.h:151: // completed. What does "may" mean in this context? Might be destroyed, or is allowed to be destroyed? I'm wary of the former, because the Java-side AppBannerManager is supposed to own the native AppBannerManager, but I'm guessing it's the latter.
https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/a... File chrome/browser/android/banners/app_banner_manager.h (right): https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/a... chrome/browser/android/banners/app_banner_manager.h:151: // completed. On 2015/02/04 17:28:01, dfalcantara wrote: > What does "may" mean in this context? Might be destroyed, or is allowed to be > destroyed? I'm wary of the former, because the Java-side AppBannerManager is > supposed to own the native AppBannerManager, but I'm guessing it's the latter. It means 'might'. The ServiceWorkerContext is owned by the storage partition, which is owned by the profile. The banner manager is (I think) owned by the tab. So if the banner manager starts a call, then the tab is closed before the call is completed, it could be gone. This case should be handled through the combination of the web pointer and the IOThread handler.
On 2015/02/04 17:35:36, benwells wrote: > https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/a... > File chrome/browser/android/banners/app_banner_manager.h (right): > > https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/a... > chrome/browser/android/banners/app_banner_manager.h:151: // completed. > On 2015/02/04 17:28:01, dfalcantara wrote: > > What does "may" mean in this context? Might be destroyed, or is allowed to be > > destroyed? I'm wary of the former, because the Java-side AppBannerManager is > > supposed to own the native AppBannerManager, but I'm guessing it's the latter. > > It means 'might'. The ServiceWorkerContext is owned by the storage partition, > which is owned by the profile. The banner manager is (I think) owned by the tab. > So if the banner manager starts a call, then the tab is closed before the call > is completed, it could be gone. > > This case should be handled through the combination of the web pointer and the > IOThread handler. That should read 'weak pointer'.
Banners, lgtm. https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/a... File chrome/browser/android/banners/app_banner_manager.h (right): https://chromiumcodereview.appspot.com/893793005/diff/100001/chrome/browser/a... chrome/browser/android/banners/app_banner_manager.h:151: // completed. On 2015/02/04 17:35:36, benwells wrote: > On 2015/02/04 17:28:01, dfalcantara wrote: > > What does "may" mean in this context? Might be destroyed, or is allowed to be > > destroyed? I'm wary of the former, because the Java-side AppBannerManager is > > supposed to own the native AppBannerManager, but I'm guessing it's the latter. > > It means 'might'. The ServiceWorkerContext is owned by the storage partition, > which is owned by the profile. The banner manager is (I think) owned by the tab. > So if the banner manager starts a call, then the tab is closed before the call > is completed, it could be gone. > > This case should be handled through the combination of the web pointer and the > IOThread handler. Ah, alright. As long as the ownership is being tracked properly then this should be fine. Can you flip that "may be destroyed" to "might be gone" for clarity?
New patchsets have been uploaded after l-g-t-m from dfalcantara@chromium.org
benwells@chromium.org changed reviewers: + sievers@chromium.org
+sievers as content/public owner
https://chromiumcodereview.appspot.com/893793005/diff/120001/content/browser/... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://chromiumcodereview.appspot.com/893793005/diff/120001/content/browser/... content/browser/service_worker/service_worker_context_wrapper.cc:335: const CheckHasSameServiceWorkerCallback& callback) { This is a bit odd: It can be called from any thread, but the callback is invoked from the IO thread.
https://codereview.chromium.org/893793005/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/893793005/diff/120001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:335: const CheckHasSameServiceWorkerCallback& callback) { On 2015/02/04 19:46:05, sievers wrote: > This is a bit odd: It can be called from any thread, but the callback is invoked > from the IO thread. Fair enough. I've outlined some options below, it would be awesome if you could indicate what you think is the best option. If none of these are good feel free to give me another. Looking at the other functions on this class (e.g. RegisterSericeWorker), it can be called on any thread but the callback can is invoked from the UI thread. That has similar weirdness but at least is consistent with the other functions on the interface and would make the calling code simpler. Having it so that the callback goes to the same thread as the caller would probably be more consistent with the way it is called (i.e. from any thread), but that feels overly complicated. Another alternative is to make the function only callable from the IO thread, but I think that would mean making the base class (ServiceWorkerContext) ref counted which I wanted to avoid.
https://codereview.chromium.org/893793005/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/893793005/diff/120001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:295: ServiceWorkerUtils::ScopeMatches(registration->pattern(), other_url); There might be a bug here depending on the use case. If you have registration A for "/foo" and registration B for "/foo/bar", then URL "/foo/x" will fall under registration A and "foo/bar/x" will fall under B, yet CheckHasSameServiceWorker("/foo", "/foo/bar") will return true. Another bug depending on use case is the registration might not have an active worker, if it's still installing, or the waiting version is in the process of being promoted to active. You might want to also check if registration.active() is non-null? https://codereview.chromium.org/893793005/diff/120001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:298: callback.Run(has_same); subjective style nit: the use of has_same is a bit tricky. Early return on status != OK is a bit more idiomatic in SW code: if (status != OK) { callback.Run(false); return; } DCHECK(registration); callback.Run(ServiceWorkerUtils::ScopeMatches(...)); https://codereview.chromium.org/893793005/diff/120001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:335: const CheckHasSameServiceWorkerCallback& callback) { On 2015/02/05 02:09:25, benwells wrote: > On 2015/02/04 19:46:05, sievers wrote: > > This is a bit odd: It can be called from any thread, but the callback is > invoked > > from the IO thread. > > Fair enough. I've outlined some options below, it would be awesome if you could > indicate what you think is the best option. If none of these are good feel free > to give me another. > > Looking at the other functions on this class (e.g. RegisterSericeWorker), it can > be called on any thread but the callback can is invoked from the UI thread. That > has similar weirdness but at least is consistent with the other functions on the > interface and would make the calling code simpler. > > Having it so that the callback goes to the same thread as the caller would > probably be more consistent with the way it is called (i.e. from any thread), > but that feels overly complicated. > > Another alternative is to make the function only callable from the IO thread, > but I think that would mean making the base class (ServiceWorkerContext) ref > counted which I wanted to avoid. I chatted a bit offline with Ben, I think it's OK to keep the current code (perhaps making callback on UI thread if it makes sense for the caller) to be consistent with the existing functions. The code also predates this chromium-dev thread https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/uWGNk-hVlGc... and kinuko@ mentions on https://codereview.chromium.org/796393005/ the class is intended to be thread-safe so it's OK to keep the pattern. https://codereview.chromium.org/893793005/diff/120001/content/public/browser/... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/893793005/diff/120001/content/public/browser/... content/public/browser/service_worker_context.h:91: // Worker also controls |other_url|. The language in the comment is a bit wrong, right now the function just checks if there's a registration whose scope matches the URLs. "Controlling SW" implies there's a ServiceWorkerVersion that is the .controller of an open page. This should be something like "Determines if a Service Worker registration exists that matches |url| and the same registration also matches |other_url|" and maybe also "and has an active worker" depending on if you check .active().
https://codereview.chromium.org/893793005/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/893793005/diff/120001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:295: ServiceWorkerUtils::ScopeMatches(registration->pattern(), other_url); On 2015/02/05 10:26:11, falken wrote: > There might be a bug here depending on the use case. If you have registration A > for "/foo" and registration B for "/foo/bar", then URL "/foo/x" will fall under > registration A and "foo/bar/x" will fall under B, yet > CheckHasSameServiceWorker("/foo", "/foo/bar") will return true. That's interesting, I didn't realise that could be a thing. After talking to Alex Russel about the desired behavior, turns out this is actually how we want it. This makes it simple and keeps the behavior the same regardless of what other pages have been visited or the order pages are visted in. > > Another bug depending on use case is the registration might not have an active > worker, if it's still installing, or the waiting version is in the process of > being promoted to active. You might want to also check if registration.active() > is non-null? OK, done. https://codereview.chromium.org/893793005/diff/120001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:298: callback.Run(has_same); On 2015/02/05 10:26:11, falken wrote: > subjective style nit: the use of has_same is a bit tricky. Early return on > status != OK is a bit more idiomatic in SW code: > > if (status != OK) { > callback.Run(false); > return; > } > DCHECK(registration); > callback.Run(ServiceWorkerUtils::ScopeMatches(...)); Done. https://codereview.chromium.org/893793005/diff/120001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:335: const CheckHasSameServiceWorkerCallback& callback) { On 2015/02/05 10:26:11, falken wrote: > On 2015/02/05 02:09:25, benwells wrote: > > On 2015/02/04 19:46:05, sievers wrote: > > > This is a bit odd: It can be called from any thread, but the callback is > > invoked > > > from the IO thread. > > > > Fair enough. I've outlined some options below, it would be awesome if you > could > > indicate what you think is the best option. If none of these are good feel > free > > to give me another. > > > > Looking at the other functions on this class (e.g. RegisterSericeWorker), it > can > > be called on any thread but the callback can is invoked from the UI thread. > That > > has similar weirdness but at least is consistent with the other functions on > the > > interface and would make the calling code simpler. > > > > Having it so that the callback goes to the same thread as the caller would > > probably be more consistent with the way it is called (i.e. from any thread), > > but that feels overly complicated. > > > > Another alternative is to make the function only callable from the IO thread, > > but I think that would mean making the base class (ServiceWorkerContext) ref > > counted which I wanted to avoid. > > I chatted a bit offline with Ben, I think it's OK to keep the current code > (perhaps making callback on UI thread if it makes sense for the caller) to be > consistent with the existing functions. The code also predates this chromium-dev > thread > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/uWGNk-hVlGc... > and kinuko@ mentions on https://codereview.chromium.org/796393005/ the class is > intended to be thread-safe so it's OK to keep the pattern. I've changed it to reply on the UI thread. https://codereview.chromium.org/893793005/diff/120001/content/public/browser/... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/893793005/diff/120001/content/public/browser/... content/public/browser/service_worker_context.h:91: // Worker also controls |other_url|. On 2015/02/05 10:26:11, falken wrote: > The language in the comment is a bit wrong, right now the function just checks > if there's a registration whose scope matches the URLs. "Controlling SW" implies > there's a ServiceWorkerVersion that is the .controller of an open page. This > should be something like "Determines if a Service Worker registration exists > that matches |url| and the same registration also matches |other_url|" and maybe > also "and has an active worker" depending on if you check .active(). Done.
(Reg: callback threading) https://codereview.chromium.org/893793005/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/893793005/diff/120001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:335: const CheckHasSameServiceWorkerCallback& callback) { > > > Looking at the other functions on this class (e.g. RegisterSericeWorker), it > > can > > > be called on any thread but the callback can is invoked from the UI thread. > > That > > > has similar weirdness but at least is consistent with the other functions on > > the > > > interface and would make the calling code simpler. > > > > > > Having it so that the callback goes to the same thread as the caller would > > > probably be more consistent with the way it is called (i.e. from any > thread), > > > but that feels overly complicated. Yep, we ended up with the current code after a few revisions but then we found that it's become a bit messy as you write (i.e. allows to be called on any thread but actually assumes it comes from UI thread). > I've changed it to reply on the UI thread. For now I think this feels more consistent, thanks. (We can fix these at once later)
lgtm https://codereview.chromium.org/893793005/diff/140001/content/public/browser/... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/893793005/diff/140001/content/public/browser/... content/public/browser/service_worker_context.h:92: virtual void CheckHasSameServiceWorker( This name is a bit off since it's only checking whether |other_url| falls inside the scope of |url|'s registration (regardless of whether |other_url|'s registration may be different). Can you rename to "CheckHasServiceWorker" and in the comment explain that it determines whether there exists a registration that matches |url| and additionally |other_url| falls inside the scope of that registration (although it may have a longer-matching registration)?
New patchsets have been uploaded after l-g-t-m from falken@chromium.org
https://codereview.chromium.org/893793005/diff/140001/content/public/browser/... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/893793005/diff/140001/content/public/browser/... content/public/browser/service_worker_context.h:92: virtual void CheckHasSameServiceWorker( On 2015/02/05 15:46:46, falken wrote: > This name is a bit off since it's only checking whether |other_url| falls inside > the scope of |url|'s registration (regardless of whether |other_url|'s > registration may be different). Can you rename to "CheckHasServiceWorker" and in > the comment explain that it determines whether there exists a registration that > matches |url| and additionally |other_url| falls inside the scope of that > registration (although it may have a longer-matching registration)? Done.
On 2015/02/05 10:26:11, falken wrote: > > I chatted a bit offline with Ben, I think it's OK to keep the current code > (perhaps making callback on UI thread if it makes sense for the caller) to be > consistent with the existing functions. The code also predates this chromium-dev > thread > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/uWGNk-hVlGc... > and kinuko@ mentions on https://codereview.chromium.org/796393005/ the class is > intended to be thread-safe so it's OK to keep the pattern. > ok lgtm then. But maybe it should be commented somewhere in the interface for this function or the class that callback(s) are run on the IO thread, so that there are no surprises.
New patchsets have been uploaded after l-g-t-m from sievers@chromium.org
On 2015/02/05 19:47:32, sievers wrote: > On 2015/02/05 10:26:11, falken wrote: > > > > I chatted a bit offline with Ben, I think it's OK to keep the current code > > (perhaps making callback on UI thread if it makes sense for the caller) to be > > consistent with the existing functions. The code also predates this > chromium-dev > > thread > > > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/uWGNk-hVlGc... > > and kinuko@ mentions on https://codereview.chromium.org/796393005/ the class > is > > intended to be thread-safe so it's OK to keep the pattern. > > > > ok lgtm then. But maybe it should be commented somewhere in the interface for > this function or the class that callback(s) are run on the IO thread, so that > there are no surprises. Done. Thanks!
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/893793005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/android/banners/app_banner_manager.cc: While running git apply --index -3 -p1; error: patch failed: chrome/browser/android/banners/app_banner_manager.cc:140 Falling back to three-way merge... Applied patch to 'chrome/browser/android/banners/app_banner_manager.cc' with conflicts. U chrome/browser/android/banners/app_banner_manager.cc Patch: chrome/browser/android/banners/app_banner_manager.cc Index: chrome/browser/android/banners/app_banner_manager.cc diff --git a/chrome/browser/android/banners/app_banner_manager.cc b/chrome/browser/android/banners/app_banner_manager.cc index cbd87921f3bad37c1f05c645ab6d3d7d4c5da2d4..c08e3e963cc37ed6a4df2d70422b24b3b71ec00a 100644 --- a/chrome/browser/android/banners/app_banner_manager.cc +++ b/chrome/browser/android/banners/app_banner_manager.cc @@ -24,8 +24,12 @@ #include "chrome/common/chrome_switches.h" #include "chrome/common/render_messages.h" #include "content/public/browser/android/content_view_core.h" +#include "content/public/browser/browser_context.h" +#include "content/public/browser/browser_thread.h" #include "content/public/browser/navigation_details.h" #include "content/public/browser/render_frame_host.h" +#include "content/public/browser/service_worker_context.h" +#include "content/public/browser/storage_partition.h" #include "content/public/browser/web_contents.h" #include "content/public/common/frame_navigate_params.h" #include "content/public/common/manifest.h" @@ -45,7 +49,8 @@ const char kBannerTag[] = "google-play-id"; namespace banners { AppBannerManager::AppBannerManager(JNIEnv* env, jobject obj) - : weak_java_banner_view_manager_(env, obj) {} + : weak_java_banner_view_manager_(env, obj), weak_factory_(this) { +} AppBannerManager::~AppBannerManager() { } @@ -140,21 +145,34 @@ void AppBannerManager::OnDidGetManifest(const content::Manifest& manifest) { return; } - // TODO(benwells): Check triggering parameters here and if there is a meta - // tag. - - // Create an infobar to promote the manifest's app. manifest_ = manifest; - GURL icon_url = - ManifestIconSelector::FindBestMatchingIcon( - manifest.icons, - GetPreferredIconSize(), - gfx::Screen::GetScreenFor(web_contents()->GetNativeView())); - if (icon_url.is_empty()) - return; + // Check to see if there is a single service worker controlling this page + // and the manifest's start url. + Profile* profile = + Profile::FromBrowserContext(web_contents()->GetBrowserContext()); + content::StoragePartition* storage_partition = + content::BrowserContext::GetStoragePartition( + profile, web_contents()->GetSiteInstance()); + DCHECK(storage_partition); + + storage_partition->GetServiceWorkerContext()->CheckHasServiceWorker( + validated_url_, manifest.start_url, + base::Bind(&AppBannerManager::OnDidCheckHasServiceWorker, + weak_factory_.GetWeakPtr())); +} + +void AppBannerManager::OnDidCheckHasServiceWorker(bool has_same) { + if (has_same) { + // TODO(benwells): Check triggering parameters. + GURL icon_url = ManifestIconSelector::FindBestMatchingIcon( + manifest_.icons, GetPreferredIconSize(), + gfx::Screen::GetScreenFor(web_contents()->GetNativeView())); + if (icon_url.is_empty()) + return; - FetchIcon(icon_url); + FetchIcon(icon_url); + } } bool AppBannerManager::OnMessageReceived(const IPC::Message& message) {
The CQ bit was unchecked by commit-bot@chromium.org
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org
Drive-by-comment: CheckHasServiceWorker() seems to do multiple things and its behavior wouldn't be straightforward. Could you clarify what you expect the function to do? (especially, the registration equality) (A) If you don't have to take care of the equality of registrations for |url| and |other_url|, calling CheckHasServiceWorker(url, callback) for each URL might be clearer. (B) If you have to take care of the equality, it could be better to make a tailor-made function (eg, SWStorage::FindRegistrationForDocuments) to exactly check the equality in one-shot. And then rename CheckHasServiceWorker to something else. WDYT? https://codereview.chromium.org/893793005/diff/180001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager.cc (right): https://codereview.chromium.org/893793005/diff/180001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager.cc:165: void AppBannerManager::OnDidCheckHasServiceWorker(bool has_same) { |has_same| -> |has_service_worker|? https://codereview.chromium.org/893793005/diff/180001/content/public/browser/... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/893793005/diff/180001/content/public/browser/... content/public/browser/service_worker_context.h:35: typedef base::Callback<void(bool has_same_service_worker)> |has_same_service_worker| -> |has_service_worker|? https://codereview.chromium.org/893793005/diff/180001/content/public/browser/... content/public/browser/service_worker_context.h:93: // which has a longer match for for |other_url|. nit: "for for" -> "for"
On 2015/02/05 12:38:29, benwells wrote: > https://codereview.chromium.org/893793005/diff/120001/content/browser/service... > File content/browser/service_worker/service_worker_context_wrapper.cc (right): > > https://codereview.chromium.org/893793005/diff/120001/content/browser/service... > content/browser/service_worker/service_worker_context_wrapper.cc:295: > ServiceWorkerUtils::ScopeMatches(registration->pattern(), other_url); > On 2015/02/05 10:26:11, falken wrote: > > There might be a bug here depending on the use case. If you have registration > A > > for "/foo" and registration B for "/foo/bar", then URL "/foo/x" will fall > under > > registration A and "foo/bar/x" will fall under B, yet > > CheckHasSameServiceWorker("/foo", "/foo/bar") will return true. > > That's interesting, I didn't realise that could be a thing. After talking to > Alex Russel about the desired behavior, turns out this is actually how we want > it. This makes it simple and keeps the behavior the same regardless of what > other pages have been visited or the order pages are visted in. Oops... sorry, I completely missed your this comment. It's ok to land this as is (or after changing the patch to call the function twice I mentioned in (A)).
nhiroki@chromium.org changed reviewers: - nhiroki@chromium.org
https://codereview.chromium.org/893793005/diff/180001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager.cc (right): https://codereview.chromium.org/893793005/diff/180001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager.cc:165: void AppBannerManager::OnDidCheckHasServiceWorker(bool has_same) { On 2015/02/06 05:36:33, nhiroki (very slow til Mar 9) wrote: > |has_same| -> |has_service_worker|? Done. https://codereview.chromium.org/893793005/diff/180001/content/public/browser/... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/893793005/diff/180001/content/public/browser/... content/public/browser/service_worker_context.h:35: typedef base::Callback<void(bool has_same_service_worker)> On 2015/02/06 05:36:33, nhiroki (very slow til Mar 9) wrote: > |has_same_service_worker| -> |has_service_worker|? Done. https://codereview.chromium.org/893793005/diff/180001/content/public/browser/... content/public/browser/service_worker_context.h:93: // which has a longer match for for |other_url|. On 2015/02/06 05:36:34, nhiroki (very slow til Mar 9) wrote: > nit: "for for" -> "for" Done.
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/893793005/200001
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f23c9142c04fc5d10e9164294d61095ff578b4a8 Cr-Commit-Position: refs/heads/master@{#315024} |