Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2393353002:
[Background loader] .h file draft outline of background loader (Closed)
Description[Background loader] .h file draft outline of background loader
BUG=659414
Committed: https://crrev.com/8215fcb61573c54217efdd82b5141926f2733133
Cr-Commit-Position: refs/heads/master@{#427567}
Patch Set 1 #
Total comments: 8
Patch Set 2 : some adjustments #
Total comments: 22
Patch Set 3 : Move files to offline_pages/ and code review adjustments #
Total comments: 18
Patch Set 4 : code review updates #Patch Set 5 : update namespace #
Messages
Total messages: 25 (8 generated)
chili@chromium.org changed reviewers: + dimich@chromium.org, dougarnett@chromium.org, petewil@chromium.org
Still working on improving this as I poke around with offliner, but this is the bare minimum interface
I understand this is WIP (Work In Progress) and is not really intended yet for actual landing soon. As such, could you please add DO NOT LAND THIS CL to the beginning of the description? Just in case :) https://codereview.chromium.org/2393353002/diff/1/components/background_loade... File components/background_loader/DEPS (right): https://codereview.chromium.org/2393353002/diff/1/components/background_loade... components/background_loader/DEPS:3: "+ui/base", What do you plan to use from it? https://codereview.chromium.org/2393353002/diff/1/components/background_loade... File components/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/1/components/background_loade... components/background_loader/background_loader_contents.h:18: typedef base::Callback<void(content::WebContents*)> WebContentsCallback; I wonder why you need to give the WebContents out like this, since I think the caller of LoadPage has (owns?) an instance of this class and can always call web_contents() from it. Perhaps you meant the callback as a signal that load ended? Happy to chat more in the office! https://codereview.chromium.org/2393353002/diff/1/components/background_loade... components/background_loader/background_loader_contents.h:32: void LoadPage(const GURL& url, const WebContentsCallback& callback); How do you plan to communicate the progress of that load to the user of this class? Some sort of Observer interface? https://codereview.chromium.org/2393353002/diff/1/components/background_loade... components/background_loader/background_loader_contents.h:74: content::BrowserContext* browser_context_; BrowserContext and Profiel are practically the same thing, you only need one of those. I think in components folder you can't refer to Profile...
Updated per comments. PTAL https://codereview.chromium.org/2393353002/diff/1/components/background_loade... File components/background_loader/DEPS (right): https://codereview.chromium.org/2393353002/diff/1/components/background_loade... components/background_loader/DEPS:3: "+ui/base", On 2016/10/06 04:36:02, Dmitry Titov wrote: > What do you plan to use from it? This was left over from when I needed MediaType for something. Removed now. https://codereview.chromium.org/2393353002/diff/1/components/background_loade... File components/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/1/components/background_loade... components/background_loader/background_loader_contents.h:18: typedef base::Callback<void(content::WebContents*)> WebContentsCallback; On 2016/10/06 04:36:02, Dmitry Titov wrote: > I wonder why you need to give the WebContents out like this, since I think the > caller of LoadPage has (owns?) an instance of this class and can always call > web_contents() from it. > > Perhaps you meant the callback as a signal that load ended? > > Happy to chat more in the office! I was playing around with different ways to offer up the internal web contents. After the discussion during last week's meeting, it makes more sense to have observer rather than a callback type structure for when to take the snapshot. https://codereview.chromium.org/2393353002/diff/1/components/background_loade... components/background_loader/background_loader_contents.h:32: void LoadPage(const GURL& url, const WebContentsCallback& callback); On 2016/10/06 04:36:02, Dmitry Titov wrote: > How do you plan to communicate the progress of that load to the user of this > class? Some sort of Observer interface? Added an observer class for this. I originally followed what the prerendering loader was doing, which essentially just says that we only need to know when the page load is done. Integrating snapshot controller and possibly taking multiple snapshots at different points in time made observer a much better choice https://codereview.chromium.org/2393353002/diff/1/components/background_loade... components/background_loader/background_loader_contents.h:74: content::BrowserContext* browser_context_; On 2016/10/06 04:36:02, Dmitry Titov wrote: > BrowserContext and Profiel are practically the same thing, you only need one of > those. I think in components folder you can't refer to Profile... removed.
Progress! More comments, some of them can be resolved by adding good comments but some can be more involved: https://codereview.chromium.org/2393353002/diff/20001/components/background_l... File components/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:26: Observer() {} no need to have constructor, usually the Observers follow the pattern: Observer { public: virtual ~Observer = default; virtual void Method1() = 0; virtual void Method2() = 0; }; So they are pure abstract classes and don't need ctor as such. https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:29: void OnPageLoaded() {} Will this correspond to OnLoad event or to some signal from SnapshotController? Who and how will determine that page is loaded? https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:32: BackgroundLoaderContents( Needs comment explaining the parameters (eg why passing the sessions_storage_namespace and where to get one). Also, the next ctor does not have one - when shoudl the user use one or another? https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:38: content::WebContents* web_contents() const { return web_contents_.get(); } I'm still not sure this should be exposed. WebContents is internal object that is constantly changing - it is unclear how it can be a 'result' of a load? Maybe it should be passed to Observer::OnPageLoaded()? Alternatively, this class could never expose web_contents and instead implement an AbstractOffliner interface that would do all the work of observing the ongoing loading process and produce a snapshot of contents. Even more alternatively, this class would expose something like TabHelper that a client can implement and attach to receive WebContentsObserver calls. https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:40: void StopLoading(); If StopLoading() is called, what call will Observer receive? https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:41: void SetObserver(Observer* observer); These usually follow patter on AddObserver/RemoveObserver and use ObserverList for implementation.
Thanks for the comments! Working on the other comments, but wanted some more thoughts about the Observer https://codereview.chromium.org/2393353002/diff/20001/components/background_l... File components/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:26: Observer() {} On 2016/10/13 01:03:26, Dmitry Titov wrote: > no need to have constructor, usually the Observers follow the pattern: > Observer { > public: > virtual ~Observer = default; > > virtual void Method1() = 0; > virtual void Method2() = 0; > }; > > So they are pure abstract classes and don't need ctor as such. I'm not too familiar about this part of C++. I was hoping to have an interface similar to the WebContentsObserver - where you only need to override the methods that you care about. Wouldn't having all the methods be virtual force you to implement all of the methods? What would be a better way to implement what I want to do? https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:29: void OnPageLoaded() {} On 2016/10/13 01:03:26, Dmitry Titov wrote: > Will this correspond to OnLoad event or to some signal from SnapshotController? > Who and how will determine that page is loaded? This will correspond to the OnLoad event from the WebContentsObserver. The WebContents determines that. I imagine when we fully integrate the SnapshotController in, this might change to provide more granular detail https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:41: void SetObserver(Observer* observer); On 2016/10/13 01:03:26, Dmitry Titov wrote: > These usually follow patter on AddObserver/RemoveObserver and use ObserverList > for implementation. I had a TODO comment somewhere for whether this should be a single observer vs list. I originally picked single for ease of management, but will change to List
On 2016/10/13 01:18:58, chili wrote: > Thanks for the comments! > > Working on the other comments, but wanted some more thoughts about the Observer > > https://codereview.chromium.org/2393353002/diff/20001/components/background_l... > File components/background_loader/background_loader_contents.h (right): > > https://codereview.chromium.org/2393353002/diff/20001/components/background_l... > components/background_loader/background_loader_contents.h:26: Observer() {} > On 2016/10/13 01:03:26, Dmitry Titov wrote: > > no need to have constructor, usually the Observers follow the pattern: > > Observer { > > public: > > virtual ~Observer = default; > > > > virtual void Method1() = 0; > > virtual void Method2() = 0; > > }; > > > > So they are pure abstract classes and don't need ctor as such. > > I'm not too familiar about this part of C++. I was hoping to have an interface > similar to the WebContentsObserver - where you only need to override the methods > that you care about. Wouldn't having all the methods be virtual force you to > implement all of the methods? What would be a better way to implement what I > want to do? WebContentsObserver is somewhat a special case - it's huge, it's derived from 2 other classes and it's not an internal class of another class. Usually Foo::Observer is a smaller abstract class: https://cs.chromium.org/search/?q=class%5CsObserver+file:.%5C.h&sq=package:ch... > > https://codereview.chromium.org/2393353002/diff/20001/components/background_l... > components/background_loader/background_loader_contents.h:29: void > OnPageLoaded() {} > On 2016/10/13 01:03:26, Dmitry Titov wrote: > > Will this correspond to OnLoad event or to some signal from > SnapshotController? > > Who and how will determine that page is loaded? > > This will correspond to the OnLoad event from the WebContentsObserver. The > WebContents determines that. I imagine when we fully integrate the > SnapshotController in, this might change to provide more granular detail Interesting. I wonder if instead of channeling some of the WebContentsObserver methods it's simpler to say that consumer of the loader can provide an impl of WebContentsObserver (as TabHelpers do) to receive all kinds of signals. This way, you won't have to grow your Observer into a subset of WCO...
https://codereview.chromium.org/2393353002/diff/20001/components/background_l... File components/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:26: Observer() {} On 2016/10/13 01:18:58, chili wrote: > On 2016/10/13 01:03:26, Dmitry Titov wrote: > > no need to have constructor, usually the Observers follow the pattern: > > Observer { > > public: > > virtual ~Observer = default; > > > > virtual void Method1() = 0; > > virtual void Method2() = 0; > > }; > > > > So they are pure abstract classes and don't need ctor as such. > > I'm not too familiar about this part of C++. I was hoping to have an interface > similar to the WebContentsObserver - where you only need to override the methods > that you care about. Wouldn't having all the methods be virtual force you to > implement all of the methods? What would be a better way to implement what I > want to do? So at least remove ctor here and define dtor as virtual default. https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:29: void OnPageLoaded() {} On 2016/10/13 01:18:58, chili wrote: > On 2016/10/13 01:03:26, Dmitry Titov wrote: > > Will this correspond to OnLoad event or to some signal from > SnapshotController? > > Who and how will determine that page is loaded? > > This will correspond to the OnLoad event from the WebContentsObserver. The > WebContents determines that. I imagine when we fully integrate the > SnapshotController in, this might change to provide more granular detail Might be helpful to mention in the class comment that the "events" are WebContentsObserver events then and per method comment what such event it corresponds to (eg, DidStartLoading? DocumentOnLoadCompletedInMainFrame?)
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2393353002/diff/20001/components/background_l... File components/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. drop the (c) it's not required. https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:6: #define COMPONENTS_BACKGROUND_LOADER_BACKGROUND_LOADER_CONTENTS_H_ About location of that file (from the meeting discussion): * Perhaps you want to start the new folder structure with new files and put it directly in the components/offline_pages/content/background_loader https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:76: BackgroundLoaderContents(); Hey, I don't understand that part. Why is this protected?
https://codereview.chromium.org/2393353002/diff/20001/components/background_l... File components/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/10/13 22:32:51, fgorski wrote: > drop the (c) it's not required. Done. https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:6: #define COMPONENTS_BACKGROUND_LOADER_BACKGROUND_LOADER_CONTENTS_H_ On 2016/10/13 22:32:51, fgorski wrote: > About location of that file (from the meeting discussion): > * Perhaps you want to start the new folder structure with new files and put it > directly in the components/offline_pages/content/background_loader Done. https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:26: Observer() {} On 2016/10/13 15:38:04, dougarnett wrote: > On 2016/10/13 01:18:58, chili wrote: > > On 2016/10/13 01:03:26, Dmitry Titov wrote: > > > no need to have constructor, usually the Observers follow the pattern: > > > Observer { > > > public: > > > virtual ~Observer = default; > > > > > > virtual void Method1() = 0; > > > virtual void Method2() = 0; > > > }; > > > > > > So they are pure abstract classes and don't need ctor as such. > > > > I'm not too familiar about this part of C++. I was hoping to have an > interface > > similar to the WebContentsObserver - where you only need to override the > methods > > that you care about. Wouldn't having all the methods be virtual force you to > > implement all of the methods? What would be a better way to implement what I > > want to do? > > So at least remove ctor here and define dtor as virtual default. Removed Observer altogether in favor of plain WebContentsObserver https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:29: void OnPageLoaded() {} On 2016/10/13 15:38:04, dougarnett wrote: > On 2016/10/13 01:18:58, chili wrote: > > On 2016/10/13 01:03:26, Dmitry Titov wrote: > > > Will this correspond to OnLoad event or to some signal from > > SnapshotController? > > > Who and how will determine that page is loaded? > > > > This will correspond to the OnLoad event from the WebContentsObserver. The > > WebContents determines that. I imagine when we fully integrate the > > SnapshotController in, this might change to provide more granular detail > > Might be helpful to mention in the class comment that the "events" are > WebContentsObserver events then and per method comment what such event it > corresponds to (eg, DidStartLoading? DocumentOnLoadCompletedInMainFrame?) Acknowledged. https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:32: BackgroundLoaderContents( On 2016/10/13 01:03:26, Dmitry Titov wrote: > Needs comment explaining the parameters (eg why passing the > sessions_storage_namespace and where to get one). Also, the next ctor does not > have one - when shoudl the user use one or another? Done. https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:38: content::WebContents* web_contents() const { return web_contents_.get(); } On 2016/10/13 01:03:26, Dmitry Titov wrote: > I'm still not sure this should be exposed. WebContents is internal object that > is constantly changing - it is unclear how it can be a 'result' of a load? Maybe > it should be passed to Observer::OnPageLoaded()? > > Alternatively, this class could never expose web_contents and instead implement > an AbstractOffliner interface that would do all the work of observing the > ongoing loading process and produce a snapshot of contents. > > Even more alternatively, this class would expose something like TabHelper that a > client can implement and attach to receive WebContentsObserver calls. removed https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:40: void StopLoading(); On 2016/10/13 01:03:26, Dmitry Titov wrote: > If StopLoading() is called, what call will Observer receive? Renamed to Cancel, per design doc. This will call Close() on WebContents, which should fire DidStopLoading() and later WebContentsDestroyed() https://codereview.chromium.org/2393353002/diff/20001/components/background_l... components/background_loader/background_loader_contents.h:76: BackgroundLoaderContents(); On 2016/10/13 22:32:51, fgorski wrote: > Hey, I don't understand that part. Why is this protected? Removed
lgtm with couple nits: https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:16: namespace background { As a global namespace, this may easily overlap with something else (although it currently does not). How about: namespace offline_pages namespace background or namespace background_loader Either one is narrower. https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:31: BackgroundLoaderContents(content::BrowserContext* browser_context); should be 'explicit' contructor: https://google.github.io/styleguide/cppguide.html#Implicit_Conversions https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:34: // Loads the URL in a WebContents. Will call observers to observe Is there incomplete sentence? https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:36: // Cancels loading on the current page. Calls Close() on internal WebContents. "loading on" -> "loading of"?
lgtm with comments. https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:11: #include "content/public/browser/web_contents.h" This file has a lot of dependencies included only through the web_contents.*\.h headers. Can we discuss in the office if we are OK with that, or whether it makes sense to be more explicit? https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:16: namespace background { On 2016/10/25 00:31:09, Dmitry Titov wrote: > As a global namespace, this may easily overlap with something else (although it > currently does not). How about: > > namespace offline_pages > namespace background > > or > namespace background_loader > > Either one is narrower. Is there a reason to put it in a different namespace than offline_pages? https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:24: // session storage should be used. Could you add an example of such case? What if nullptr is passed for the latter parameter? https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:31: BackgroundLoaderContents(content::BrowserContext* browser_context); On 2016/10/25 00:31:09, Dmitry Titov wrote: > should be 'explicit' contructor: > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions Rule applies to all constructors with a single parameter. https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:42: // content::WebContentsDelegate Implementation: s/I/i/ https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:85: std::vector<WebContentsObserver*> observers_; Is there a reason why this cannot be a base::ObserverList? https://cs.chromium.org/chromium/src/base/observer_list.h
chili@chromium.org changed reviewers: + creis@chromium.org
+creis for content/public/browser DEPS https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:11: #include "content/public/browser/web_contents.h" On 2016/10/25 12:15:36, fgorski wrote: > This file has a lot of dependencies included only through the web_contents.*\.h > headers. Can we discuss in the office if we are OK with that, or whether it > makes sense to be more explicit? Removed a few where we only need the definition. will add these to the cc file instead. Will poke you in office to see if this works https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:16: namespace background { On 2016/10/25 12:15:36, fgorski wrote: > On 2016/10/25 00:31:09, Dmitry Titov wrote: > > As a global namespace, this may easily overlap with something else (although > it > > currently does not). How about: > > > > namespace offline_pages > > namespace background > > > > or > > namespace background_loader > > > > Either one is narrower. > > Is there a reason to put it in a different namespace than offline_pages? It's in a different namespace mainly because at one point I put it in a different component. I kept it different for ease of moving it around. what do you think? https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:24: // session storage should be used. On 2016/10/25 12:15:36, fgorski wrote: > Could you add an example of such case? > What if nullptr is passed for the latter parameter? Not sure what you mean by example. session storage namespace should not be null https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:31: BackgroundLoaderContents(content::BrowserContext* browser_context); On 2016/10/25 12:15:36, fgorski wrote: > On 2016/10/25 00:31:09, Dmitry Titov wrote: > > should be 'explicit' contructor: > > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions > > Rule applies to all constructors with a single parameter. Done. https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:34: // Loads the URL in a WebContents. Will call observers to observe On 2016/10/25 00:31:09, Dmitry Titov wrote: > Is there incomplete sentence? not really, but expanded the sentence to be more clear https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:36: // Cancels loading on the current page. Calls Close() on internal WebContents. On 2016/10/25 00:31:09, Dmitry Titov wrote: > "loading on" -> "loading of"? Done. https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:42: // content::WebContentsDelegate Implementation: On 2016/10/25 12:15:36, fgorski wrote: > s/I/i/ Done. https://codereview.chromium.org/2393353002/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:85: std::vector<WebContentsObserver*> observers_; On 2016/10/25 12:15:36, fgorski wrote: > Is there a reason why this cannot be a base::ObserverList? > > https://cs.chromium.org/chromium/src/base/observer_list.h did not know it existed :)
DEPS LGTM. nit: Please include a bug number in the CL description.
lgtm
Description was changed from ========== [Background loader] .h file draft outline of background loader BUG= ========== to ========== [Background loader] .h file draft outline of background loader BUG=659414 ==========
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, fgorski@chromium.org, creis@chromium.org, dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2393353002/#ps80001 (title: "update namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Background loader] .h file draft outline of background loader BUG=659414 ========== to ========== [Background loader] .h file draft outline of background loader BUG=659414 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Background loader] .h file draft outline of background loader BUG=659414 ========== to ========== [Background loader] .h file draft outline of background loader BUG=659414 Committed: https://crrev.com/8215fcb61573c54217efdd82b5141926f2733133 Cr-Commit-Position: refs/heads/master@{#427567} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8215fcb61573c54217efdd82b5141926f2733133 Cr-Commit-Position: refs/heads/master@{#427567} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
