Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1234543003:
Report Android Build.MODEL in SystemProfileProto hardware_class. (Closed)
DescriptionReport Android Build.MODEL in SystemProfileProto hardware_class.
BUG=509018
Committed: https://crrev.com/ae416695267e33d69c903a30783fc522bf93509d
Cr-Commit-Position: refs/heads/master@{#339037}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix typo, use GetDeviceName(). #
Total comments: 5
Patch Set 3 : Remove GetDeviceName(). #
Total comments: 4
Patch Set 4 : Address jdduke's comments. #
Total comments: 4
Patch Set 5 : Address rkaplow's comments. #
Total comments: 1
Messages
Total messages: 41 (12 generated)
tdresser@chromium.org changed reviewers: + thakis@chromium.org
PTAL
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
+dfalcantara who originally added some of the current sys_info_android routines. https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc#ne... base/sys_info_android.cc:177: __system_property_get("ro.product.model", device_model_str); Hmm, shouldn't this property correspond to base::android::BuildInfo::GetInstance()->model();? I wonder why we're pulling these values directly instead of using base::android::BuildInfo.
https://codereview.chromium.org/1234543003/diff/1/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/1234543003/diff/1/base/sys_info.h#newcode55 base/sys_info.h:55: // string if machime model is unknown or an error occured. while you're here: typo machime https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc#ne... base/sys_info_android.cc:182: return base::android::BuildInfo::GetInstance()->model(); This does 14 jni calls to fill out BuildInfo and then ignores all but one of them. I guess this will only be called very rarely so it's ok?
https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc#ne... base/sys_info_android.cc:182: return base::android::BuildInfo::GetInstance()->model(); On 2015/07/10 19:52:22, Nico wrote: > This does 14 jni calls to fill out BuildInfo and then ignores all but one of > them. I guess this will only be called very rarely so it's ok? The JNI calls are only made when the singleton is constructed, and the singleton itself is used all over the place (primarily for sdk_int()). I suspect it will already have been constructed by the time we hit this getter. That said, we should probably be consistent in this class, and it's still not clear how this query differs from the above ro.product.model fetcher.
dfalcantara@chromium.org changed reviewers: + dfalcantara@chromium.org
https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc#ne... base/sys_info_android.cc:177: __system_property_get("ro.product.model", device_model_str); On 2015/07/10 19:16:05, jdduke wrote: > Hmm, shouldn't this property correspond to > base::android::BuildInfo::GetInstance()->model();? I wonder why we're pulling > these values directly instead of using base::android::BuildInfo. Probably for historic reasons that may not make sense anymore (IIRC I just moved these over from another class). If base::android::BuildInfo works and returns the same string, it's worth using it.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1234543003/diff/1/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/1234543003/diff/1/base/sys_info.h#newcode55 base/sys_info.h:55: // string if machime model is unknown or an error occured. On 2015/07/10 19:52:22, Nico wrote: > while you're here: typo machime Done. https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/1/base/sys_info_android.cc#ne... base/sys_info_android.cc:177: __system_property_get("ro.product.model", device_model_str); On 2015/07/10 20:58:11, dfalcantara wrote: > On 2015/07/10 19:16:05, jdduke wrote: > > Hmm, shouldn't this property correspond to > > base::android::BuildInfo::GetInstance()->model();? I wonder why we're pulling > > these values directly instead of using base::android::BuildInfo. > > Probably for historic reasons that may not make sense anymore (IIRC I just moved > these over from another class). If base::android::BuildInfo works and returns > the same string, it's worth using it. It doesn't look like we can get rid of |__system_property_get| in this file, as it's needed for |GetDalvikHeapSizeMB|, among others. In order to keep things consistent, I'll just use |GetDeviceName()|, which appears to be the same thing as BuildInfo::model.
https://codereview.chromium.org/1234543003/diff/40001/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/1234543003/diff/40001/base/sys_info.h#newcode56 base/sys_info.h:56: // e.g. MacPro1,1 on Mac. another example for Android might be nice. https://codereview.chromium.org/1234543003/diff/40001/base/sys_info.h#newcode134 base/sys_info.h:134: static std::string GetDeviceName(); What about removing this and updating the callsites to use HardwareModelName()? I think there are only 1 or 2 callers, and I'd rather we have but one method for getting this data. Dan any thoughts?
https://codereview.chromium.org/1234543003/diff/40001/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/1234543003/diff/40001/base/sys_info.h#newcode56 base/sys_info.h:56: // e.g. MacPro1,1 on Mac. On 2015/07/14 15:04:31, jdduke wrote: > another example for Android might be nice. Done. https://codereview.chromium.org/1234543003/diff/40001/base/sys_info.h#newcode134 base/sys_info.h:134: static std::string GetDeviceName(); On 2015/07/14 15:04:31, jdduke wrote: > What about removing this and updating the callsites to use HardwareModelName()? > I think there are only 1 or 2 callers, and I'd rather we have but one method for > getting this data. Dan any thoughts? Done.
https://chromiumcodereview.appspot.com/1234543003/diff/40001/base/sys_info.h File base/sys_info.h (right): https://chromiumcodereview.appspot.com/1234543003/diff/40001/base/sys_info.h#... base/sys_info.h:134: static std::string GetDeviceName(); On 2015/07/14 15:04:31, jdduke wrote: > What about removing this and updating the callsites to use HardwareModelName()? > I think there are only 1 or 2 callers, and I'd rather we have but one method for > getting this data. Dan any thoughts? Don't know... I'd be worried that the user agent string would change suddenly. Not sure how big a difference the two strings would make in practice.
lgtm, thanks https://codereview.chromium.org/1234543003/diff/60001/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/60001/base/sys_info_android.c... base/sys_info_android.cc:175: char device_model_str[PROP_VALUE_MAX]; Nit: Should we move this up above OperatingSystemName() (matching the declaration order.. hmm, i guess the GetAndroid methods are also out-of-order). https://codereview.chromium.org/1234543003/diff/60001/content/common/user_age... File content/common/user_agent.cc (right): https://codereview.chromium.org/1234543003/diff/60001/content/common/user_age... content/common/user_agent.cc:30: return base::SysInfo::HardwareModelName(); Nit: I think we can remove this method and just inline the SysInfo call below.
On 2015/07/14 18:44:31, dfalcantara wrote: > https://chromiumcodereview.appspot.com/1234543003/diff/40001/base/sys_info.h > File base/sys_info.h (right): > > https://chromiumcodereview.appspot.com/1234543003/diff/40001/base/sys_info.h#... > base/sys_info.h:134: static std::string GetDeviceName(); > On 2015/07/14 15:04:31, jdduke wrote: > > What about removing this and updating the callsites to use > HardwareModelName()? > > I think there are only 1 or 2 callers, and I'd rather we have but one method > for > > getting this data. Dan any thoughts? > > Don't know... I'd be worried that the user agent string would change suddenly. > Not sure how big a difference the two strings would make in practice. This change doesn't modify the string that's returned, it only changes the name of the method.
On 2015/07/14 18:47:22, tdresser wrote: > On 2015/07/14 18:44:31, dfalcantara wrote: > > https://chromiumcodereview.appspot.com/1234543003/diff/40001/base/sys_info.h > > File base/sys_info.h (right): > > > > > https://chromiumcodereview.appspot.com/1234543003/diff/40001/base/sys_info.h#... > > base/sys_info.h:134: static std::string GetDeviceName(); > > On 2015/07/14 15:04:31, jdduke wrote: > > > What about removing this and updating the callsites to use > > HardwareModelName()? > > > I think there are only 1 or 2 callers, and I'd rather we have but one method > > for > > > getting this data. Dan any thoughts? > > > > Don't know... I'd be worried that the user agent string would change suddenly. > > > Not sure how big a difference the two strings would make in practice. > > This change doesn't modify the string that's returned, it only changes the name > of the method. In that case, lgtm too.
https://codereview.chromium.org/1234543003/diff/60001/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/60001/base/sys_info_android.c... base/sys_info_android.cc:175: char device_model_str[PROP_VALUE_MAX]; On 2015/07/14 18:46:48, jdduke wrote: > Nit: Should we move this up above OperatingSystemName() (matching the > declaration order.. hmm, i guess the GetAndroid methods are also out-of-order). Done. https://codereview.chromium.org/1234543003/diff/60001/content/common/user_age... File content/common/user_agent.cc (right): https://codereview.chromium.org/1234543003/diff/60001/content/common/user_age... content/common/user_agent.cc:30: return base::SysInfo::HardwareModelName(); On 2015/07/14 18:46:48, jdduke wrote: > Nit: I think we can remove this method and just inline the SysInfo call below. Done.
The CQ bit was checked by tdresser@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1234543003/#ps80001 (title: "Address jdduke's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234543003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tdresser@chromium.org changed reviewers: + jam@chromium.org
Oops, still missing LGTM from Nico. jam@, can you take a look at content/common/user_agent.cc?
rkaplow@chromium.org changed reviewers: + rkaplow@chromium.org
Can you also update this comment? https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... https://codereview.chromium.org/1234543003/diff/80001/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/1234543003/diff/80001/base/sys_info.h#newcode57 base/sys_info.h:57: // Only implemented on OS X and Android, will return an empty string on other this actually is set for ChromeOS as well. Can you mention that?
lgtm
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1234543003/diff/80001/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/80001/base/sys_info_android.c... base/sys_info_android.cc:160: __system_property_get("ro.product.model", device_model_str); Hmm, there's a bug saying that this API is deprecated: https://code.google.com/p/chromium/issues/detail?id=392191 Is there a better API that can be used?
https://codereview.chromium.org/1234543003/diff/80001/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/1234543003/diff/80001/base/sys_info_android.c... base/sys_info_android.cc:160: __system_property_get("ro.product.model", device_model_str); On 2015/07/14 20:20:39, Alexei Svitkine wrote: > Hmm, there's a bug saying that this API is deprecated: > > https://code.google.com/p/chromium/issues/detail?id=392191 > > Is there a better API that can be used? Well, there's BuildInfo::model() which gets it's data directly from the Android Build.MODEL. Dan was worried that this might introduce some discontinuities in our data, though it looks like under the covers Build.MODEL is just getString("ro.product.model");
On 2015/07/14 20:29:57, jdduke wrote: > https://codereview.chromium.org/1234543003/diff/80001/base/sys_info_android.cc > File base/sys_info_android.cc (right): > > https://codereview.chromium.org/1234543003/diff/80001/base/sys_info_android.c... > base/sys_info_android.cc:160: __system_property_get("ro.product.model", > device_model_str); > On 2015/07/14 20:20:39, Alexei Svitkine wrote: > > Hmm, there's a bug saying that this API is deprecated: > > > > https://code.google.com/p/chromium/issues/detail?id=392191 > > > > Is there a better API that can be used? > > Well, there's BuildInfo::model() which gets it's data directly from the Android > Build.MODEL. Dan was worried that this might introduce some discontinuities in > our data, though it looks like under the covers Build.MODEL is just > getString("ro.product.model"); BuildInfo doesn't wrap all of the required calls though. We'd need to update DalvikHeapSizeMB() and DalvikHeapGrowthLimitMB() as well.
On 2015/07/14 20:34:40, tdresser wrote: > On 2015/07/14 20:29:57, jdduke wrote: > > https://codereview.chromium.org/1234543003/diff/80001/base/sys_info_android.cc > > File base/sys_info_android.cc (right): > > > > > https://codereview.chromium.org/1234543003/diff/80001/base/sys_info_android.c... > > base/sys_info_android.cc:160: __system_property_get("ro.product.model", > > device_model_str); > > On 2015/07/14 20:20:39, Alexei Svitkine wrote: > > > Hmm, there's a bug saying that this API is deprecated: > > > > > > https://code.google.com/p/chromium/issues/detail?id=392191 > > > > > > Is there a better API that can be used? > > > > Well, there's BuildInfo::model() which gets it's data directly from the > Android > > Build.MODEL. Dan was worried that this might introduce some discontinuities in > > our data, though it looks like under the covers Build.MODEL is just > > getString("ro.product.model"); > > BuildInfo doesn't wrap all of the required calls though. > We'd need to update DalvikHeapSizeMB() and DalvikHeapGrowthLimitMB() as well. Good point, we should probably have those calls routed through ActivityManager (which indirectly exposes those values via getLargeMemoryClass() and getMemoryClass()). In any case, making such changes should probably happen in a separate CL?
rkaplow@, does the comment in system_profile.proto look okay? https://codereview.chromium.org/1234543003/diff/80001/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/1234543003/diff/80001/base/sys_info.h#newcode57 base/sys_info.h:57: // Only implemented on OS X and Android, will return an empty string on other On 2015/07/14 19:43:19, rkaplow wrote: > this actually is set for ChromeOS as well. Can you mention that? Done.
lgtm
The CQ bit was checked by tdresser@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, jam@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1234543003/#ps100001 (title: "Address rkaplow's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234543003/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ae416695267e33d69c903a30783fc522bf93509d Cr-Commit-Position: refs/heads/master@{#339037}
Message was sent while issue was closed.
https://codereview.chromium.org/1234543003/diff/100001/components/metrics/pro... File components/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/1234543003/diff/100001/components/metrics/pro... components/metrics/proto/system_profile.proto:115: // The hardware_class describes the current machine model, e.g. "MacPro1,1" Can you also send a CL to update this comment in the internal copy of this proto? Thanks!
Message was sent while issue was closed.
On 2015/07/21 16:52:12, Alexei Svitkine wrote: > https://codereview.chromium.org/1234543003/diff/100001/components/metrics/pro... > File components/metrics/proto/system_profile.proto (right): > > https://codereview.chromium.org/1234543003/diff/100001/components/metrics/pro... > components/metrics/proto/system_profile.proto:115: // The hardware_class > describes the current machine model, e.g. "MacPro1,1" > Can you also send a CL to update this comment in the internal copy of this > proto? Thanks! Sorry for the late response, I was OOO. I'll update the proto. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
