Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
163433010:
Use Display changes for screen orientation changes. (Closed)
|
Created:
6 years, 10 months ago by mlamouri (slow - plz ping) Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@orientation_tests Visibility:
Public. |
DescriptionUse Display changes for screen orientation changes.
This is the only reliable solution: we get an event from DisplayManager
every time a display change. If that change is a rotation change from
the display we care about, we can act on it. Using sensors or
onConfigurationChange is unreliable.
BUG=342714
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252846
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : update tests #
Total comments: 29
Patch Set 4 : review comments #
Total comments: 4
Patch Set 5 : moar review comments #
Total comments: 8
Patch Set 6 : #Patch Set 7 : fix gc tests #Patch Set 8 : rebase #
Messages
Total messages: 20 (0 generated)
This is fairly temporary given that we are working on a new API that will allow lock/unlock in addition of reading the screen orientation values. Things might very likely move to a specific class later.
FriendlyReviewReminder :)
I like it, thanks! just some suggestion to make it even clearer and simpler for the future refactorings. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:442: private static class DisplayOrientationListener nit: make it non-static, and remove mContentViewCore: non-static inner classes have an implicit handle to the outer class and can access their members. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:445: private int previousOrientation; nit: s/previous/mPrevious/ https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:762: sendOrientationChangeEvent(getDisplayOrientation()); see below. looks like getDisplayOrientation() needs just a context... if that's right: 1. move getDisplayOrientation() as a method in DisplayOrientationListener 2. on its contructor, call sendOrientationChangeEvent(). https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:764: mDisplayListener = new DisplayOrientationListener(this); nit: pass in mContext, and make DisplayOrientationListener constructor deal with DisplayManager https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:889: mDisplayListener = null; ditto, expose a destroy() method in mDisplayListener, and let it deal with displayManager. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1576: not needed? https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2174: (WindowManager) getContext().getSystemService(Context.WINDOW_SERVICE); yeah, looks like this can then be a method inside the new inner class. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2195: private void sendOrientationChangeEvent(int orientation) { and this is cool! ContentViewCore proper will deal with the "native" call, whilst the new class is the one centralizing all screen orientation related tasks. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java (right): https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:43: private int mTargetOrientation; nit: final
https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:442: private static class DisplayOrientationListener On 2014/02/20 11:05:18, bulach wrote: > nit: make it non-static, and remove mContentViewCore: non-static inner classes > have an implicit handle to the outer class and can access their members. Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:445: private int previousOrientation; On 2014/02/20 11:05:18, bulach wrote: > nit: s/previous/mPrevious/ Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:762: sendOrientationChangeEvent(getDisplayOrientation()); On 2014/02/20 11:05:18, bulach wrote: > see below. > looks like getDisplayOrientation() needs just a context... > if that's right: > 1. move getDisplayOrientation() as a method in DisplayOrientationListener > 2. on its contructor, call sendOrientationChangeEvent(). Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:764: mDisplayListener = new DisplayOrientationListener(this); On 2014/02/20 11:05:18, bulach wrote: > nit: pass in mContext, and make DisplayOrientationListener constructor deal with > DisplayManager Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:889: mDisplayListener = null; On 2014/02/20 11:05:18, bulach wrote: > ditto, expose a destroy() method in mDisplayListener, and let it deal with > displayManager. Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1576: On 2014/02/20 11:05:18, bulach wrote: > not needed? Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2174: (WindowManager) getContext().getSystemService(Context.WINDOW_SERVICE); On 2014/02/20 11:05:18, bulach wrote: > yeah, looks like this can then be a method inside the new inner class. Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java (right): https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:43: private int mTargetOrientation; On 2014/02/20 11:05:18, bulach wrote: > nit: final Done.
This new patch should apply the previous comments. PTAL.
+ some mostly code-stylistic nits: https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:767: (DisplayManager) mContext.getSystemService(Context.DISPLAY_SERVICE); should the indent here be +8?, wrapped line indents seem to be +8 elsewhere in this class. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:886: (DisplayManager) mContext.getSystemService(Context.DISPLAY_SERVICE); indentation https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2193: * TODO(husky): Add a hook for mock orientations. is this still valid? husky is not on the team for some time now. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java (right): https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:62: .getRotation()) { indent? https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:81: private static int orientationTypeToAngle(int orientation) { align with /** above https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:119: new OrientationChangeListenerCriteria(orientationTypeToAngle(orientation)); according to android style this should be +8sp, see http://source.android.com/source/code-style.html#use-spaces-for-indentation I see there were some inconsistencies wrt indentation originally. I guess it's best to keep that consistent where possible. +also in the new code elsewhere in this class.
lgtm, tiny nit and feel free to go ahead once tim is happy! thanks! https://codereview.chromium.org/163433010/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/163433010/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:448: private final Context mContext; my bad, sorry... now that this class is no longer static, getContext() should work. https://codereview.chromium.org/163433010/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:456: (DisplayManager) mContext.getSystemService(Context.DISPLAY_SERVICE); nit: indent by another 4 for continuation
All comments applied. Tim, PTAL. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:767: (DisplayManager) mContext.getSystemService(Context.DISPLAY_SERVICE); On 2014/02/20 12:09:02, timvolodine wrote: > should the indent here be +8?, wrapped line indents seem to be +8 elsewhere in > this class. Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:886: (DisplayManager) mContext.getSystemService(Context.DISPLAY_SERVICE); On 2014/02/20 12:09:02, timvolodine wrote: > indentation Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2193: * TODO(husky): Add a hook for mock orientations. On 2014/02/20 12:09:02, timvolodine wrote: > is this still valid? husky is not on the team for some time now. Good catch. I actually meant to remove it. A test could call sendOrientationChangeEvent() and pass an arbitrary value. Somehow, the TODO is fixed. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java (right): https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:62: .getRotation()) { On 2014/02/20 12:09:02, timvolodine wrote: > indent? Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:81: private static int orientationTypeToAngle(int orientation) { On 2014/02/20 12:09:02, timvolodine wrote: > align with /** above Done. https://codereview.chromium.org/163433010/diff/90001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:119: new OrientationChangeListenerCriteria(orientationTypeToAngle(orientation)); On 2014/02/20 12:09:02, timvolodine wrote: > according to android style this should be +8sp, see > http://source.android.com/source/code-style.html#use-spaces-for-indentation > > I see there were some inconsistencies wrt indentation originally. I guess it's > best to keep that consistent where possible. +also in the new code elsewhere in > this class. Done. https://codereview.chromium.org/163433010/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/163433010/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:448: private final Context mContext; On 2014/02/20 12:18:58, bulach wrote: > my bad, sorry... now that this class is no longer static, getContext() should > work. Done. https://codereview.chromium.org/163433010/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:456: (DisplayManager) mContext.getSystemService(Context.DISPLAY_SERVICE); On 2014/02/20 12:18:58, bulach wrote: > nit: indent by another 4 for continuation Done.
lgtm happy modulo nits ;) https://codereview.chromium.org/163433010/diff/290001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java (right): https://codereview.chromium.org/163433010/diff/290001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:62: .getRotation()) { indent +8 https://codereview.chromium.org/163433010/diff/290001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:81: private static int orientationTypeToAngle(int orientation) { align pls https://codereview.chromium.org/163433010/diff/290001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:169: ActivityInfo.SCREEN_ORIENTATION_REVERSE_PORTRAIT); +8 https://codereview.chromium.org/163433010/diff/290001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:200: ActivityInfo.SCREEN_ORIENTATION_REVERSE_LANDSCAPE); +8
https://codereview.chromium.org/163433010/diff/290001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java (right): https://codereview.chromium.org/163433010/diff/290001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:62: .getRotation()) { On 2014/02/20 12:40:42, timvolodine wrote: > indent +8 Done. https://codereview.chromium.org/163433010/diff/290001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:81: private static int orientationTypeToAngle(int orientation) { On 2014/02/20 12:40:42, timvolodine wrote: > align pls Done. https://codereview.chromium.org/163433010/diff/290001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:169: ActivityInfo.SCREEN_ORIENTATION_REVERSE_PORTRAIT); On 2014/02/20 12:40:42, timvolodine wrote: > +8 Done. https://codereview.chromium.org/163433010/diff/290001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ScreenOrientationTest.java:200: ActivityInfo.SCREEN_ORIENTATION_REVERSE_LANDSCAPE); On 2014/02/20 12:40:42, timvolodine wrote: > +8 Done.
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlamouri@chromium.org/163433010/330001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlamouri@chromium.org/163433010/330001
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlamouri@chromium.org/163433010/80002
Message was sent while issue was closed.
Change committed as 252846
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/178253002/ by jdduke@chromium.org. The reason for reverting is: This is causing runtime issues on pre-JB MR1 devices, as DisplayManager.DisplayListener was added in API level 17. A compat-layer equivalent will be needed before landing.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
