Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1092283003:
[Chromecast] CastSysInfo implementation. (Closed)
|
Created:
5 years, 8 months ago by derekjchow1 Modified:
5 years, 8 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, halliwell Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] CastSysInfo implementation.
Adds CastSysInfo. This class is platform dependent and provides
some basic information about the platform it is running on.
R=gunsch@chromium.org,byungchul@chromium.org
BUG=
Committed: https://crrev.com/aee21a7b4f2dbbcee1fb2f2d812fc711df7f7fc1
Cr-Commit-Position: refs/heads/master@{#326424}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address some comments from patchset 1 #
Total comments: 6
Patch Set 3 : Updated comments. #
Total comments: 6
Patch Set 4 : Remove Init/Finalize #Patch Set 5 : Fix CastSysInfoShlib comment. #Patch Set 6 : Remove shlib #
Total comments: 4
Patch Set 7 : cast_sys_info_util.h #
Total comments: 4
Patch Set 8 : Fix nits. #
Messages
Total messages: 29 (4 generated)
PTAL. Note I've added a static CastSysInfo::Create function.
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:24: static CastSysInfo* Create(); scoped_ptr<CastSysInfo> https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info_shlib.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info_shlib.h:21: static CastSysInfo* Create(const std::vector<std::string>& argv); You should have cast_sys_info_shlib.cc: CastSysInfo* CastSysInfo::Create() { return CastSysInfoShlib::Create( base::CommandLine::ForCurrentProcess()->argv()); }
Thanks for adding this. https://codereview.chromium.org/1092283003/diff/1/chromecast/base/DEPS File chromecast/base/DEPS (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/base/DEPS#newcode4 chromecast/base/DEPS:4: "+chromecast/public", this can go in chromecast/DEPS. https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. while you're here, can you add a chromecast/public/DEPS file that *removes* all other dependencies that chromecast/ has? e.g. "-chromecast" "-base" "-net" etc https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:24: static CastSysInfo* Create(); On 2015/04/20 18:00:24, byungchul wrote: > scoped_ptr<CastSysInfo> No. scoped_ptr is a Chromium dependency. Leave as-is.
+CC halliwell
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:24: static CastSysInfo* Create(); On 2015/04/20 18:02:31, gunsch wrote: > On 2015/04/20 18:00:24, byungchul wrote: > > scoped_ptr<CastSysInfo> > > No. scoped_ptr is a Chromium dependency. Leave as-is. Hmm.. Unless we want to make all CastSysInfo in shlib, I think it should be out of CastSysInfo. Otherwise, it overlaps with CastSysInfoShlib::Create() and makes confusions.
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:24: static CastSysInfo* Create(); On 2015/04/20 18:10:19, byungchul wrote: > On 2015/04/20 18:02:31, gunsch wrote: > > On 2015/04/20 18:00:24, byungchul wrote: > > > scoped_ptr<CastSysInfo> > > > > No. scoped_ptr is a Chromium dependency. Leave as-is. > > Hmm.. Unless we want to make all CastSysInfo in shlib, I think it should be out > of CastSysInfo. Otherwise, it overlaps with CastSysInfoShlib::Create() and makes > confusions. I'm fine if we want to put a convenience function that wraps it into somewhere like chromecast/base/cast_sys_info.h. But I think we should keep all Chromium includes out of chromecast/public/.
halliwell@chromium.org changed reviewers: + halliwell@chromium.org
https://codereview.chromium.org/1092283003/diff/1/chromecast/base/cast_sys_in... File chromecast/base/cast_sys_info_dummy.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/base/cast_sys_in... chromecast/base/cast_sys_info_dummy.h:22: const std::string GetSystemReleaseChannel() override; const is pointless on return by value. https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:14: // processes as well as cast_shell browser process. All informations are I don't think "informations" is a word :) https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:29: virtual void Finalize() = 0; What do Initialize/Finalize do? Seems like a holdover from chromecast service pattern. If I'm just going to create one of these as a throwaway object to quickly get some system info, would be nicer if it just used ctor/dtor to initialize/finalize.
Commit comment: "dependant" --> "dependent"
https://codereview.chromium.org/1092283003/diff/1/chromecast/base/DEPS File chromecast/base/DEPS (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/base/DEPS#newcode4 chromecast/base/DEPS:4: "+chromecast/public", On 2015/04/20 18:02:31, gunsch wrote: > this can go in chromecast/DEPS. Done. https://codereview.chromium.org/1092283003/diff/1/chromecast/base/cast_sys_in... File chromecast/base/cast_sys_info_dummy.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/base/cast_sys_in... chromecast/base/cast_sys_info_dummy.h:22: const std::string GetSystemReleaseChannel() override; On 2015/04/20 18:12:18, halliwell wrote: > const is pointless on return by value. Done. https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/04/20 18:02:31, gunsch wrote: > while you're here, can you add a chromecast/public/DEPS file that *removes* all > other dependencies that chromecast/ has? e.g. > > "-chromecast" > "-base" > "-net" > > etc Done. It'd be better if we could specify that files in this directory should not depend on anything though. https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:14: // processes as well as cast_shell browser process. All informations are On 2015/04/20 18:12:18, halliwell wrote: > I don't think "informations" is a word :) Done. https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:24: static CastSysInfo* Create(); On 2015/04/20 18:11:49, gunsch wrote: > On 2015/04/20 18:10:19, byungchul wrote: > > On 2015/04/20 18:02:31, gunsch wrote: > > > On 2015/04/20 18:00:24, byungchul wrote: > > > > scoped_ptr<CastSysInfo> > > > > > > No. scoped_ptr is a Chromium dependency. Leave as-is. > > > > Hmm.. Unless we want to make all CastSysInfo in shlib, I think it should be > out > > of CastSysInfo. Otherwise, it overlaps with CastSysInfoShlib::Create() and > makes > > confusions. > > I'm fine if we want to put a convenience function that wraps it into somewhere > like chromecast/base/cast_sys_info.h. But I think we should keep all Chromium > includes out of chromecast/public/. Done. Moved to base/cast_sys_info.h and renamed CreateSysInfo to avoid confusion. https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:29: virtual void Finalize() = 0; On 2015/04/20 18:12:18, halliwell wrote: > What do Initialize/Finalize do? Seems like a holdover from chromecast service > pattern. If I'm just going to create one of these as a throwaway object to > quickly get some system info, would be nicer if it just used ctor/dtor to > initialize/finalize. Our Android code uses it to do some Java initialization, so I don't think we can easily remove these functions.
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:29: virtual void Finalize() = 0; On 2015/04/20 21:15:32, derekjchow1 wrote: > On 2015/04/20 18:12:18, halliwell wrote: > > What do Initialize/Finalize do? Seems like a holdover from chromecast service > > pattern. If I'm just going to create one of these as a throwaway object to > > quickly get some system info, would be nicer if it just used ctor/dtor to > > initialize/finalize. > > Our Android code uses it to do some Java initialization, so I don't think we can > easily remove these functions. Ok. So what are the rules for client using this class? How do I use Initialize and Finalize as a caller?
Public API comments could use some work. We should try to hold those to a very high standard (even though some of their sources currently don't). Thanks. https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... chromecast/public/cast_sys_info.h:54: // Returns the wifi interface. the wifi interface name? https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... chromecast/public/cast_sys_info.h:56: // Returns the soft AP interface. software AP interface name. https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... File chromecast/public/cast_sys_info_shlib.h (right): https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... chromecast/public/cast_sys_info_shlib.h:17: // Defines a static function in a class since gcc makes warning if visibility This isn't really a great description of this class or how to implement it, just a note. Can this doc be rewritten?
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:29: virtual void Finalize() = 0; On 2015/04/20 21:19:43, halliwell wrote: > On 2015/04/20 21:15:32, derekjchow1 wrote: > > On 2015/04/20 18:12:18, halliwell wrote: > > > What do Initialize/Finalize do? Seems like a holdover from chromecast > service > > > pattern. If I'm just going to create one of these as a throwaway object to > > > quickly get some system info, would be nicer if it just used ctor/dtor to > > > initialize/finalize. > > > > Our Android code uses it to do some Java initialization, so I don't think we > can > > easily remove these functions. > > Ok. So what are the rules for client using this class? How do I use Initialize > and Finalize as a caller? This is only relevant for ATV, but for correctness, we should call Initialize before we use the class and Finalize before we destroy it. I completely agree with you though, we should remove these functions. I think we can refactor our internal code a little bit to get rid of this corner case. https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... chromecast/public/cast_sys_info.h:54: // Returns the wifi interface. On 2015/04/20 21:32:25, gunsch wrote: > the wifi interface name? Done. https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... chromecast/public/cast_sys_info.h:56: // Returns the soft AP interface. On 2015/04/20 21:32:25, gunsch wrote: > software AP interface name. Done. https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... File chromecast/public/cast_sys_info_shlib.h (right): https://codereview.chromium.org/1092283003/diff/20001/chromecast/public/cast_... chromecast/public/cast_sys_info_shlib.h:17: // Defines a static function in a class since gcc makes warning if visibility On 2015/04/20 21:32:25, gunsch wrote: > This isn't really a great description of this class or how to implement it, just > a note. Can this doc be rewritten? Documented the function.
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:29: virtual void Finalize() = 0; On 2015/04/20 22:10:52, derekjchow1 wrote: > On 2015/04/20 21:19:43, halliwell wrote: > > On 2015/04/20 21:15:32, derekjchow1 wrote: > > > On 2015/04/20 18:12:18, halliwell wrote: > > > > What do Initialize/Finalize do? Seems like a holdover from chromecast > > service > > > > pattern. If I'm just going to create one of these as a throwaway object > to > > > > quickly get some system info, would be nicer if it just used ctor/dtor to > > > > initialize/finalize. > > > > > > Our Android code uses it to do some Java initialization, so I don't think we > > can > > > easily remove these functions. > > > > Ok. So what are the rules for client using this class? How do I use > Initialize > > and Finalize as a caller? > > This is only relevant for ATV, but for correctness, we should call Initialize > before we use the class and Finalize before we destroy it. > > I completely agree with you though, we should remove these functions. I think we > can refactor our internal code a little bit to get rid of this corner case. Ok, but if that's all there is to it, we could just move the Initialize code into the ctor and Finalize into the dtor ... ? :) And if there's more to it, that should be documented? Also, if it's just an ATV thing, is there a way to remove this from the public interface and restrict the Initialize/Finalize calls to ATV?
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:29: virtual void Finalize() = 0; I believe there may actually be more to it. Byungchul, can you comment? I agree that since we're changing to shlib model from previous public-interface model, we should prefer ctor/dtor if possible. https://codereview.chromium.org/1092283003/diff/40001/chromecast/public/cast_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/40001/chromecast/public/cast_... chromecast/public/cast_sys_info.h:46: // This describes system version which may be different with different with --> different than https://codereview.chromium.org/1092283003/diff/40001/chromecast/public/cast_... File chromecast/public/cast_sys_info_shlib.h (right): https://codereview.chromium.org/1092283003/diff/40001/chromecast/public/cast_... chromecast/public/cast_sys_info_shlib.h:17: // Defines a static function in a class since gcc makes warning if visibility Still: this is not really a good definition of this class. If anything, this was appropriate as a review mail to explain during this header's initial review, but doesn't belong with the code once checked in.
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:29: virtual void Finalize() = 0; On 2015/04/20 22:34:49, gunsch wrote: > I believe there may actually be more to it. Byungchul, can you comment? I agree > that since we're changing to shlib model from previous public-interface model, > we should prefer ctor/dtor if possible. When it was instantiated in chromecast service client, it should be consistent with other, injecting dependencies in ctor, being initialized by chromecast service later. Now, I am fine without Init/Finalize in cast sysinfo because it is out of chromecast service client. https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info_shlib.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info_shlib.h:21: static CastSysInfo* Create(const std::vector<std::string>& argv); On 2015/04/20 18:00:24, byungchul wrote: > You should have cast_sys_info_shlib.cc: > > CastSysInfo* CastSysInfo::Create() { > return CastSysInfoShlib::Create( > base::CommandLine::ForCurrentProcess()->argv()); > } ping on this comment~ https://codereview.chromium.org/1092283003/diff/40001/chromecast/base/cast_sy... File chromecast/base/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/40001/chromecast/base/cast_sy... chromecast/base/cast_sys_info.h:17: static scoped_ptr<CastSysInfo> CreateSysInfo(); remove static https://codereview.chromium.org/1092283003/diff/40001/chromecast/public/cast_... File chromecast/public/cast_sys_info_shlib.h (right): https://codereview.chromium.org/1092283003/diff/40001/chromecast/public/cast_... chromecast/public/cast_sys_info_shlib.h:17: // Defines a static function in a class since gcc makes warning if visibility On 2015/04/20 22:34:49, gunsch wrote: > Still: this is not really a good definition of this class. If anything, this was > appropriate as a review mail to explain during this header's initial review, but > doesn't belong with the code once checked in. Agree that this is not a comment to define this. But, at the same item, this is the only purpose of this class, and should be here for those who will see this code in the future. Does it work for you if it is rephrased like: A static class for CastSysInfo shared library visible from cast_shell. gcc makes warning if visibility attribute is set on global functions. ?
https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info.h:29: virtual void Finalize() = 0; On 2015/04/20 23:51:14, byungchul wrote: > On 2015/04/20 22:34:49, gunsch wrote: > > I believe there may actually be more to it. Byungchul, can you comment? I > agree > > that since we're changing to shlib model from previous public-interface model, > > we should prefer ctor/dtor if possible. > > When it was instantiated in chromecast service client, it should be consistent > with other, injecting dependencies in ctor, being initialized by chromecast > service later. > > Now, I am fine without Init/Finalize in cast sysinfo because it is out of > chromecast service client. Removed. https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... File chromecast/public/cast_sys_info_shlib.h (right): https://codereview.chromium.org/1092283003/diff/1/chromecast/public/cast_sys_... chromecast/public/cast_sys_info_shlib.h:21: static CastSysInfo* Create(const std::vector<std::string>& argv); On 2015/04/20 23:51:14, byungchul wrote: > On 2015/04/20 18:00:24, byungchul wrote: > > You should have cast_sys_info_shlib.cc: > > > > CastSysInfo* CastSysInfo::Create() { > > return CastSysInfoShlib::Create( > > base::CommandLine::ForCurrentProcess()->argv()); > > } > > ping on this comment~ As discussed offline we might statically link cast_sys_info. https://codereview.chromium.org/1092283003/diff/40001/chromecast/base/cast_sy... File chromecast/base/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/40001/chromecast/base/cast_sy... chromecast/base/cast_sys_info.h:17: static scoped_ptr<CastSysInfo> CreateSysInfo(); On 2015/04/20 23:51:14, byungchul wrote: > remove static Done. https://codereview.chromium.org/1092283003/diff/40001/chromecast/public/cast_... File chromecast/public/cast_sys_info_shlib.h (right): https://codereview.chromium.org/1092283003/diff/40001/chromecast/public/cast_... chromecast/public/cast_sys_info_shlib.h:17: // Defines a static function in a class since gcc makes warning if visibility On 2015/04/20 23:51:14, byungchul wrote: > On 2015/04/20 22:34:49, gunsch wrote: > > Still: this is not really a good definition of this class. If anything, this > was > > appropriate as a review mail to explain during this header's initial review, > but > > doesn't belong with the code once checked in. > > Agree that this is not a comment to define this. But, at the same item, this is > the only purpose of this class, and should be here for those who will see this > code in the future. > Does it work for you if it is rephrased like: > > A static class for CastSysInfo shared library visible from cast_shell. gcc makes > warning if visibility attribute is set on global functions. > > ? Done.
lcwu@chromium.org changed reviewers: + lcwu@chromium.org
https://codereview.chromium.org/1092283003/diff/70010/chromecast/public/cast_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/70010/chromecast/public/cast_... chromecast/public/cast_sys_info.h:35: // Returns product code name of the device (eg: anchovy, salami, fugu, ...). Remove the code name examples from the comment.
https://codereview.chromium.org/1092283003/diff/70010/chromecast/chromecast.gyp File chromecast/chromecast.gyp (right): https://codereview.chromium.org/1092283003/diff/70010/chromecast/chromecast.g... chromecast/chromecast.gyp:323: 'base/cast_sys_info.h', I think having the same name here (as the one in public/) is confusing. I would suggest changing it to something like create_sys_info.h. (Or if we expect to add additional utils to this file, maybe something like cast_sys_info_util.h.)
https://codereview.chromium.org/1092283003/diff/70010/chromecast/chromecast.gyp File chromecast/chromecast.gyp (right): https://codereview.chromium.org/1092283003/diff/70010/chromecast/chromecast.g... chromecast/chromecast.gyp:323: 'base/cast_sys_info.h', On 2015/04/22 21:57:54, lcwu1 wrote: > I think having the same name here (as the one in public/) is confusing. I would > suggest changing it to something like create_sys_info.h. (Or if we expect to add > additional utils to this file, maybe something like cast_sys_info_util.h.) Done. https://codereview.chromium.org/1092283003/diff/70010/chromecast/public/cast_... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/70010/chromecast/public/cast_... chromecast/public/cast_sys_info.h:35: // Returns product code name of the device (eg: anchovy, salami, fugu, ...). On 2015/04/22 20:51:40, lcwu1 wrote: > Remove the code name examples from the comment. Done.
lgtm % nit https://codereview.chromium.org/1092283003/diff/110001/chromecast/public/cast... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/110001/chromecast/public/cast... chromecast/public/cast_sys_info.h:43: // Returns device manufacturer (eg: Google, Sony, ...). no need to mention other companies in the comments, "Google" example is sufficient
lgtm https://codereview.chromium.org/1092283003/diff/110001/chromecast/chromecast.gyp File chromecast/chromecast.gyp (right): https://codereview.chromium.org/1092283003/diff/110001/chromecast/chromecast.... chromecast/chromecast.gyp:330: 'base/cast_sys_info_simple.cc', nit: I would change the file name to cast_sys_info_util_simple.cc
https://codereview.chromium.org/1092283003/diff/110001/chromecast/chromecast.gyp File chromecast/chromecast.gyp (right): https://codereview.chromium.org/1092283003/diff/110001/chromecast/chromecast.... chromecast/chromecast.gyp:330: 'base/cast_sys_info_simple.cc', On 2015/04/22 22:32:17, lcwu1 wrote: > nit: I would change the file name to cast_sys_info_util_simple.cc Done. https://codereview.chromium.org/1092283003/diff/110001/chromecast/public/cast... File chromecast/public/cast_sys_info.h (right): https://codereview.chromium.org/1092283003/diff/110001/chromecast/public/cast... chromecast/public/cast_sys_info.h:43: // Returns device manufacturer (eg: Google, Sony, ...). On 2015/04/22 22:25:25, gunsch wrote: > no need to mention other companies in the comments, "Google" example is > sufficient Done.
The CQ bit was checked by derekjchow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lcwu@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1092283003/#ps130001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092283003/130001
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/aee21a7b4f2dbbcee1fb2f2d812fc711df7f7fc1 Cr-Commit-Position: refs/heads/master@{#326424} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
