Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26129009:
Cache DeviceDisplayInfo data in shared structure on native side to avoid frequent JNI calls. (Closed)
|
Created:
7 years, 2 months ago by ostap Modified:
7 years, 2 months ago CC:
chromium-reviews, Inactive, lgombos Base URL:
https://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCache DeviceDisplayInfo data in shared structure on native side to avoid frequent JNI calls.
BUG=265008
Partial fix for bug 265008.
GetDisplayWidth and GetDisplayHeight are called for tile creation.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230472
Patch Set 1 #
Total comments: 15
Patch Set 2 : Updated patch by review comments. #
Total comments: 7
Patch Set 3 : Updated by review comments. #
Total comments: 4
Patch Set 4 : Updated by review comments. #
Total comments: 7
Patch Set 5 : Rebased and updated by review comments. #Patch Set 6 : Rebased and updated after cl 28053002 . #
Total comments: 15
Patch Set 7 : Updated by review comments. #
Total comments: 12
Patch Set 8 : Updated by review comments. #Patch Set 9 : Re-uploaded by Yaron request. #
Total comments: 2
Patch Set 10 : Patch for commit. #Patch Set 11 : Fix clang build issues. #
Messages
Total messages: 28 (0 generated)
Partial fix for bug 265008. GetDisplayWidth and GetDisplayHeight are called for tile creation. Thread attach + jni call could take up to 12 ms on Samsung Galaxy Note 2 (longer on slower devices).
https://codereview.chromium.org/26129009/diff/1/ui/android/java/src/org/chrom... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/26129009/diff/1/ui/android/java/src/org/chrom... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:117: mAppContext.registerComponentCallbacks( nit: indent 4 (also below) https://codereview.chromium.org/26129009/diff/1/ui/android/java/src/org/chrom... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:122: } nit: add linebreak below https://codereview.chromium.org/26129009/diff/1/ui/android/java/src/org/chrom... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:155: private native void nativeUpdateSharedDisplayInfo(int display_height, java style should be displayHeight. Please update this and other variables https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:12: using base::android::AttachCurrentThread; Inline this https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:17: nit: remove https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:46: struct ScopedReadLock { Use the locking primitives from base if you still need beyond LazyInstance https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:66: scoped_ptr<SharedDisplayInfo> SharedDisplayInfo::instance_; Use base::LazyInstance https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:99: JNIEnv* env, jobject obj, jint display_height, In C++, these should be one per line per style guide. https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:106: void SharedDisplayInfo::UpdateDisplayInfo(JNIEnv*, jobject, jint display_height, Same as above https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:117: instance->dip_scale_ = static_cast<int>(dip_scale); dip_scale_ is a double https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:139: j_device_info_.Reset(Java_DeviceDisplayInfo_create(env, It's not clear to me why you need this anymore. LazyInstance will ensure you're constructed once. The native c-tor can then call registerListener. https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... File ui/gfx/android/device_display_info.h (right): https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.h:13: #include "base/memory/scoped_ptr.h" You'll be able to remove by changing to LazyInstance in just the cc file https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.h:64: void InvokeUpdate(); never called
https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:139: j_device_info_.Reset(Java_DeviceDisplayInfo_create(env, On 2013/10/10 11:52:46, Yaron wrote: The problem that constructor is called inside the write lock and registerListener calls back with UpdateDisplayInfo which also needs write lock. I'll check what base primitives can do. For pthread_rwlock "Results are undefined if the calling thread holds the read-write lock (whether a read or write lock) at the time the call is made" .
https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... File ui/gfx/android/device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/1/ui/gfx/android/device_display... ui/gfx/android/device_display_info.cc:46: struct ScopedReadLock { On 2013/10/10 11:52:46, Yaron wrote: > Use the locking primitives from base if you still need beyond LazyInstance I'm sorry, I didn't find read/write lock implementation in base. Using simple base::Lock would decrease reads performance.
1. Move SharedDeviceDisplayInfo to the separate file. 2. Use LazyInstance instead of scoped_ptr 3. Replaced RW lock with base::Lock . Reading values are really fast, so there is almost no difference in speed.
https://codereview.chromium.org/26129009/diff/8001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java (right): https://codereview.chromium.org/26129009/diff/8001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:120: public void registerListener() { private https://codereview.chromium.org/26129009/diff/8001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:125: updateNativeSharedDisplayInfo(); indent 4 for block-level (other places below too). Please also correct the C++ code https://codereview.chromium.org/26129009/diff/8001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/gfx/DeviceDisplayInfo.java:157: public static DeviceDisplayInfo createWithListener(Context context) { Make private. This is unneeded from the java layer https://codereview.chromium.org/26129009/diff/8001/ui/gfx/android/shared_devi... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/8001/ui/gfx/android/shared_devi... ui/gfx/android/shared_device_display_info.cc:71: display_height_ = static_cast<int>(display_height); Add lock_->AssertAcquired(); https://codereview.chromium.org/26129009/diff/8001/ui/gfx/android/shared_devi... File ui/gfx/android/shared_device_display_info.h (right): https://codereview.chromium.org/26129009/diff/8001/ui/gfx/android/shared_devi... ui/gfx/android/shared_device_display_info.h:18: class GFX_EXPORT SharedDeviceDisplayInfo { Doesn't actually need to be exported, right? https://codereview.chromium.org/26129009/diff/8001/ui/gfx/android/shared_devi... ui/gfx/android/shared_device_display_info.h:21: static SharedDeviceDisplayInfo* Instance(); No need to expose this publicly. Move g_instance into an anonymous namespace in the cc file. That way UpdateSharedDisplayInfo can access it without making it visible. https://codereview.chromium.org/26129009/diff/8001/ui/gfx/android/shared_devi... ui/gfx/android/shared_device_display_info.h:36: base::Lock& lock() { return lock_; } Prefer not to expose this either. Locking should be internal to this class.
Patch was updated by review comments.
https://codereview.chromium.org/26129009/diff/17001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/17001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:49: void UpdateSharedDeviceDisplayInfo(JNIEnv* env, This should be in anon namespace at the top of the file. https://codereview.chromium.org/26129009/diff/17001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:57: display_width, bits_per_pixel, bits_per_component, dip_scale); Nit: Indent 2 more. Please fix throughout. https://codereview.chromium.org/26129009/diff/17001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.h (right): https://codereview.chromium.org/26129009/diff/17001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:31: static void InvokeUpdate(JNIEnv*, Nit: don't omit parameter names https://codereview.chromium.org/26129009/diff/17001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:48: static SharedDeviceDisplayInfo* Instance(); Any reason this doesn't use base/memory/singleton.h and instead rolls its own?
Patch was updated by review comments.
Please reply to comments directly (e.g. when you click on a comment link, you can reply there) when you've addressed them (e.g. using the "Done" link). Thanks! https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:15: void UpdateSharedDeviceDisplayInfo(JNIEnv* env, Surround this with the anon namespace (it can still be in the gfx namespace so it doesn't need to have gfx:: when calling the class), like so: namespace gfx { namespace { ... } // namespace ... } // namespace gfx https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.h (right): https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:58: friend class DefaultSingletonTraits<SharedDeviceDisplayInfo>; This should go right after the private: line. https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:59: friend class Singleton<SharedDeviceDisplayInfo>; Is this needed? I don't see other singletons using it (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/base/android/activ... )
https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.h (right): https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:59: friend class Singleton<SharedDeviceDisplayInfo>; On 2013/10/15 15:13:07, Alexei Svitkine wrote: > Is this needed? I don't see other singletons using it (e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/base/android/activ... > ) Yes it is needed because it Singleton refers to GetInstance() and it is private. ActivityStatus has GetInstance() public.
https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:28: int SharedDeviceDisplayInfo::GetDisplayHeight() { How about making these non-static and to be used through the singleton? Then, they can be simpler: int SharedDeviceDisplayInfo::GetDisplayHeight() { base::AutoLock(lock_); return display_height_; } And have it be called via: SharedDeviceDisplayInfo::GetInstance()->GetDisplayHeight() Then you can make the GetInstance() function public like other singletons and remove the extra friend decl. This matches more closely how other singletons are used in Chromium.
https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:15: void UpdateSharedDeviceDisplayInfo(JNIEnv* env, On 2013/10/15 15:13:07, Alexei Svitkine wrote: > Surround this with the anon namespace (it can still be in the gfx namespace so > it doesn't need to have gfx:: when calling the class), like so: > > namespace gfx { > > namespace { > > ... > > } // namespace > > ... > > } // namespace gfx If I do so it is not visible from JNI. I get this: gen/ui/gfx/jni/DeviceDisplayInfo_jni.h:30:13: error: 'void UpdateSharedDeviceDisplayInfo(JNIEnv*, jobject, jint, jint, jint, jint, jdouble)' declared 'static' but never defined [-Werror=unused-function] And I didn't find any place that includes JNI functions into anon namespace.
On 2013/10/15 19:41:26, ostap wrote: > https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... > File ui/gfx/android/shared_device_display_info.cc (right): > > https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... > ui/gfx/android/shared_device_display_info.cc:15: void > UpdateSharedDeviceDisplayInfo(JNIEnv* env, > On 2013/10/15 15:13:07, Alexei Svitkine wrote: > > Surround this with the anon namespace (it can still be in the gfx namespace so > > it doesn't need to have gfx:: when calling the class), like so: > > > > namespace gfx { > > > > namespace { > > > > ... > > > > } // namespace > > > > ... > > > > } // namespace gfx > > If I do so it is not visible from JNI. > I get this: > gen/ui/gfx/jni/DeviceDisplayInfo_jni.h:30:13: error: 'void > UpdateSharedDeviceDisplayInfo(JNIEnv*, jobject, jint, jint, jint, jint, > jdouble)' declared 'static' but never defined [-Werror=unused-function] > > And I didn't find any place that includes JNI functions into anon namespace. True, I think in this case it needs to be exposed to be accessible from JNI. Overall, sorry but I lost track: what's the status of this issue? Note you'll probably have to rebase after https://codereview.chromium.org/28053002/
On 2013/10/22 00:19:57, Yaron wrote: > Overall, sorry but I lost track: what's the status of this issue? Note you'll > probably have to rebase after https://codereview.chromium.org/28053002/ Alexei Svitkine have made very valuable suggestions and the patch have got a lot of updates. I'll rebase it and upload tomorrow. Thanks.
Patch was rebased.
https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/24001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:15: void UpdateSharedDeviceDisplayInfo(JNIEnv* env, On 2013/10/15 19:41:27, ostap wrote: > On 2013/10/15 15:13:07, Alexei Svitkine wrote: > > Surround this with the anon namespace (it can still be in the gfx namespace so > > it doesn't need to have gfx:: when calling the class), like so: > > > > namespace gfx { > > > > namespace { > > > > ... > > > > } // namespace > > > > ... > > > > } // namespace gfx > > If I do so it is not visible from JNI. > I get this: > gen/ui/gfx/jni/DeviceDisplayInfo_jni.h:30:13: error: 'void > UpdateSharedDeviceDisplayInfo(JNIEnv*, jobject, jint, jint, jint, jint, > jdouble)' declared 'static' but never defined [-Werror=unused-function] > > And I didn't find any place that includes JNI functions into anon namespace. Got it. Then, please re-add the static keyword to this and add a comment explaining it's done that way for JNI. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:12: using base::android::AttachCurrentThread; Nit: You only use this once below, just inline it. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:13: using base::android::ScopedJavaLocalRef; Nit: You don't seem to be using this, remove it. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:15: namespace gfx { Nit: Add a blank line after this. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:90: SharedDeviceDisplayInfo* SharedDeviceDisplayInfo::GetInstance() { Order this consistently between .cc and .h file (i.e. either move this up or move it down in the header so that the functions are in the same order). https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:94: SharedDeviceDisplayInfo::SharedDeviceDisplayInfo() { Please init all the instance variables to default values in the ctor. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.h (right): https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:12: #include "ui/gfx/gfx_export.h" You're importing this but not using GFX_EXPORT. Either add GFX_EXPORT to SharedDeviceDisplayInfo (if it will be used outside of gfx) or remove this include. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:53: base::Lock& lock() { return lock_; } No need for this private accessor, just use |lock_| directly.
https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:12: using base::android::AttachCurrentThread; On 2013/10/22 19:15:24, Alexei Svitkine wrote: > Nit: You only use this once below, just inline it. Done. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:13: using base::android::ScopedJavaLocalRef; On 2013/10/22 19:15:24, Alexei Svitkine wrote: > Nit: You don't seem to be using this, remove it. Done. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:15: namespace gfx { On 2013/10/22 19:15:24, Alexei Svitkine wrote: > Nit: Add a blank line after this. Done. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:90: SharedDeviceDisplayInfo* SharedDeviceDisplayInfo::GetInstance() { On 2013/10/22 19:15:24, Alexei Svitkine wrote: > Order this consistently between .cc and .h file (i.e. either move this up or > move it down in the header so that the functions are in the same order). Done. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:94: SharedDeviceDisplayInfo::SharedDeviceDisplayInfo() { On 2013/10/22 19:15:24, Alexei Svitkine wrote: > Please init all the instance variables to default values in the ctor. Done. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.cc:94: SharedDeviceDisplayInfo::SharedDeviceDisplayInfo() { On 2013/10/22 19:15:24, Alexei Svitkine wrote: > Please init all the instance variables to default values in the ctor. Done. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... File ui/gfx/android/shared_device_display_info.h (right): https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:12: #include "ui/gfx/gfx_export.h" On 2013/10/22 19:15:24, Alexei Svitkine wrote: > You're importing this but not using GFX_EXPORT. > > Either add GFX_EXPORT to SharedDeviceDisplayInfo (if it will be used outside of > gfx) or remove this include. Removed. https://codereview.chromium.org/26129009/diff/64001/ui/gfx/android/shared_dev... ui/gfx/android/shared_device_display_info.h:53: base::Lock& lock() { return lock_; } On 2013/10/22 19:15:24, Alexei Svitkine wrote: > No need for this private accessor, just use |lock_| directly. Done.
LGTM % some final comments and nits Please wait for Yaron's review too. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/gfx_jni_r... File ui/gfx/android/gfx_jni_registrar.cc (right): https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/gfx_jni_r... ui/gfx/android/gfx_jni_registrar.cc:17: gfx::SharedDeviceDisplayInfo::RegisterSharedDeviceDisplayInfo }, Nit: Remove gfx:: prefix here and below since this is in the gfx namespace already. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:18: jobject obj, Nit: Align params. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:30: SharedDeviceDisplayInfo* SharedDeviceDisplayInfo::GetInstance() { Nit: Add "// static" above this. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:61: return smallest_dip_width_; Can you add a DCHECK_NE(0, smallest_dip_width_); here and similarly to the other functions above? (This way, it will detect if this gets called before the values got initialized through JNI.) https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:64: bool SharedDeviceDisplayInfo::RegisterSharedDeviceDisplayInfo(JNIEnv* env) { Nit: Add "// static" above this. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:90: Nit: Remove empty line.
https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/gfx_jni_r... File ui/gfx/android/gfx_jni_registrar.cc (right): https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/gfx_jni_r... ui/gfx/android/gfx_jni_registrar.cc:17: gfx::SharedDeviceDisplayInfo::RegisterSharedDeviceDisplayInfo }, On 2013/10/22 21:01:54, Alexei Svitkine wrote: > Nit: Remove gfx:: prefix here and below since this is in the gfx namespace > already. Done. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:18: jobject obj, On 2013/10/22 21:01:54, Alexei Svitkine wrote: > Nit: Align params. Done. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:30: SharedDeviceDisplayInfo* SharedDeviceDisplayInfo::GetInstance() { On 2013/10/22 21:01:54, Alexei Svitkine wrote: > Nit: Add "// static" above this. Done. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:61: return smallest_dip_width_; On 2013/10/22 21:01:54, Alexei Svitkine wrote: > Can you add a DCHECK_NE(0, smallest_dip_width_); here and similarly to the other > functions above? (This way, it will detect if this gets called before the values > got initialized through JNI.) Done. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:64: bool SharedDeviceDisplayInfo::RegisterSharedDeviceDisplayInfo(JNIEnv* env) { On 2013/10/22 21:01:54, Alexei Svitkine wrote: > Nit: Add "// static" above this. Done. https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:90: On 2013/10/22 21:01:54, Alexei Svitkine wrote: > Nit: Remove empty line. Done.
It looks like part of the patch was eaten. I'm seeing "error: old chunk mismatch" on some files. Can you re-upload? On Tue, Oct 22, 2013 at 2:40 PM, <sl.ostapenko@samsung.com> wrote: > > https://codereview.chromium.**org/26129009/diff/124001/ui/** > gfx/android/gfx_jni_registrar.**cc<https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/gfx_jni_registrar.cc> > File ui/gfx/android/gfx_jni_**registrar.cc (right): > > https://codereview.chromium.**org/26129009/diff/124001/ui/** > gfx/android/gfx_jni_registrar.**cc#newcode17<https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/gfx_jni_registrar.cc#newcode17> > ui/gfx/android/gfx_jni_**registrar.cc:17: > gfx::SharedDeviceDisplayInfo::**RegisterSharedDeviceDisplayInf**o }, > On 2013/10/22 21:01:54, Alexei Svitkine wrote: > >> Nit: Remove gfx:: prefix here and below since this is in the gfx >> > namespace > >> already. >> > > Done. > > > https://codereview.chromium.**org/26129009/diff/124001/ui/** > gfx/android/shared_device_**display_info.cc<https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_device_display_info.cc> > File ui/gfx/android/shared_device_**display_info.cc (right): > > https://codereview.chromium.**org/26129009/diff/124001/ui/** > gfx/android/shared_device_**display_info.cc#newcode18<https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_device_display_info.cc#newcode18> > ui/gfx/android/shared_device_**display_info.cc:18: jobject obj, > On 2013/10/22 21:01:54, Alexei Svitkine wrote: > >> Nit: Align params. >> > > Done. > > > https://codereview.chromium.**org/26129009/diff/124001/ui/** > gfx/android/shared_device_**display_info.cc#newcode30<https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_device_display_info.cc#newcode30> > ui/gfx/android/shared_device_**display_info.cc:30: > SharedDeviceDisplayInfo* SharedDeviceDisplayInfo::**GetInstance() { > On 2013/10/22 21:01:54, Alexei Svitkine wrote: > >> Nit: Add "// static" above this. >> > > Done. > > > https://codereview.chromium.**org/26129009/diff/124001/ui/** > gfx/android/shared_device_**display_info.cc#newcode61<https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_device_display_info.cc#newcode61> > ui/gfx/android/shared_device_**display_info.cc:61: return > smallest_dip_width_; > On 2013/10/22 21:01:54, Alexei Svitkine wrote: > >> Can you add a DCHECK_NE(0, smallest_dip_width_); here and similarly to >> > the other > >> functions above? (This way, it will detect if this gets called before >> > the values > >> got initialized through JNI.) >> > > Done. > > > https://codereview.chromium.**org/26129009/diff/124001/ui/** > gfx/android/shared_device_**display_info.cc#newcode64<https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_device_display_info.cc#newcode64> > ui/gfx/android/shared_device_**display_info.cc:64: bool > SharedDeviceDisplayInfo::**RegisterSharedDeviceDisplayInf**o(JNIEnv* env) > { > On 2013/10/22 21:01:54, Alexei Svitkine wrote: > >> Nit: Add "// static" above this. >> > > Done. > > https://codereview.chromium.**org/26129009/diff/124001/ui/** > gfx/android/shared_device_**display_info.cc#newcode90<https://codereview.chromium.org/26129009/diff/124001/ui/gfx/android/shared_device_display_info.cc#newcode90> > ui/gfx/android/shared_device_**display_info.cc:90: > On 2013/10/22 21:01:54, Alexei Svitkine wrote: > >> Nit: Remove empty line. >> > > Done. > > https://codereview.chromium.**org/26129009/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/22 22:18:31, Yaron wrote: > It looks like part of the patch was eaten. I'm seeing "error: old chunk > mismatch" on some files. Can you re-upload? > Done
lgtm Thanks for the patch https://codereview.chromium.org/26129009/diff/304001/ui/gfx/android/shared_de... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/304001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:12: using base::android::AttachCurrentThread; Nit: remove
https://codereview.chromium.org/26129009/diff/304001/ui/gfx/android/shared_de... File ui/gfx/android/shared_device_display_info.cc (right): https://codereview.chromium.org/26129009/diff/304001/ui/gfx/android/shared_de... ui/gfx/android/shared_device_display_info.cc:12: using base::android::AttachCurrentThread; On 2013/10/22 22:58:30, Yaron wrote: > Nit: remove Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/26129009/364001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/26129009/544001
Message was sent while issue was closed.
Change committed as 230472 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
