Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
648733003:
Implement ScreenOrientationDelegate on ChromeOS (Closed)
|
Created:
6 years, 2 months ago by jonross Modified:
6 years, 1 month ago Reviewers:
Avi (use Gerrit), mlamouri (slow - plz ping), oshima, Rick Byers, flackr, achuithb, tbarzic CC:
chromium-reviews, sadrul, jam, darin-cc_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionImplement ScreenOrientationDelegate on ChromeOS
Extend content::ScreenOrientationDelegate within ash to provide functionality on ChromeOS.
Also provides a fix for a bug in ScreenOrientationProvider that was found during development.
TEST=Manual testing on TouchView device with an HTML page that performs ScreenOrientation requests.
BUG=396760
Committed: https://crrev.com/4f3952e7d0172a50d26a6eb57660623db70000db
Cr-Commit-Position: refs/heads/master@{#303898}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Generalize rotation locking #
Total comments: 25
Patch Set 3 : Rebase #Patch Set 4 : Move ScreenOrientationDelegate #
Total comments: 2
Patch Set 5 : #
Total comments: 4
Patch Set 6 : Restrict Rotation Locking to a single window #
Total comments: 3
Patch Set 7 : #Patch Set 8 : Tests #
Total comments: 7
Patch Set 9 : #
Total comments: 10
Patch Set 10 : Rebase #Patch Set 11 : Shift experimental feature to ash #
Total comments: 6
Patch Set 12 : #
Total comments: 1
Patch Set 13 : Move ash code into ash/content #
Total comments: 4
Patch Set 14 : #Messages
Total messages: 64 (12 generated)
jonross@chromium.org changed reviewers: + flackr@chromium.org
Hey Rob, Could you review the ash implementation of ScreenOrientationDelegate?
jonross@chromium.org changed reviewers: + mlamouri@chromium.org
Hey Mounir, This review is an ash implementation of ScreenOrientationDelegate, to provide ChromeOS functionality. There is a fix to an issue in ScreenOrientationProvider that I found while developing this change.
In addition of the comments below, shouldn't you call ScreenOrientationDispatcherHost::OnOrientationChange()? https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... File ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc (right): https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:33: controller->SetRotationLocked(false); Why not, but technically, that should not happen. Why not having a DCHECK? https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:37: controller->LockRotation(gfx::Display::ROTATE_90); Two things worry me here: 1. we should allow rotating from 'portrait-primary' to 'portrait-secondary' if locked in 'portrait'; 2. you are making assumption about the device natural orientation https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:42: controller->LockRotation(gfx::Display::ROTATE_180); Same regarding the natural orientation for the 'portrait-secondary' and 'landscape-secondary'. https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:45: controller->SetRotationLocked(true); 'any' doesn't mean locked to the current orientation but allow all orientations to be used. Is that the unlocked behaviour on ChromeOS? https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:51: controller->LockRotation(gfx::Display::ROTATE_0); Same comment regarding the natural orientation.
https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... File ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc (right): https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:37: controller->LockRotation(gfx::Display::ROTATE_90); On 2014/10/13 09:45:44, Mounir Lamouri wrote: > Two things worry me here: > 1. we should allow rotating from 'portrait-primary' to 'portrait-secondary' if > locked in 'portrait'; > 2. you are making assumption about the device natural orientation 1) Had not realized that. The ChromeOS user rotation currently does not allow that. I'll check with UX to see how we would have to update our lock notifications. 2) Yes these are some assumptions for natural orientation. Would you have recommendations on determining this?
On 2014/10/14 14:46:57, jonross wrote: > 2) Yes these are some assumptions for natural orientation. Would you have > recommendations on determining this? On Android, when the display is at its natural orientation, the rotation angle is expected to be 0 [1]. Can we make the same assumption in ChromeOS? In which case, we could use that information with the display size to guess the "*-primary" and "*-secondary" orientations. For example, see [2]. [1] http://developer.android.com/reference/android/view/Display.html#getRotation() [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...
I made the applying of rotation lock more generic. It now verifies the initial display to determine the natural orientation. All locks are applied based on this. For orientations that can accept primary/secondary I've reach out to UX to determine how we want to change the lock displayed to users. I've added a todo to update maximize mode to allow rotation between angles of the same orientation. In the interim I have these lock rotation if the current angle matches the orientation. Otherwise it locks to the primary angle for that orientation. https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... File ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc (right): https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:33: controller->SetRotationLocked(false); On 2014/10/13 09:45:44, Mounir Lamouri wrote: > Why not, but technically, that should not happen. Why not having a DCHECK? Done. https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:37: controller->LockRotation(gfx::Display::ROTATE_90); On 2014/10/14 14:46:57, jonross wrote: > On 2014/10/13 09:45:44, Mounir Lamouri wrote: > > Two things worry me here: > > 1. we should allow rotating from 'portrait-primary' to 'portrait-secondary' if > > locked in 'portrait'; > > 2. you are making assumption about the device natural orientation > > 1) Had not realized that. The ChromeOS user rotation currently does not allow > that. I'll check with UX to see how we would have to update our lock > notifications. > 2) Yes these are some assumptions for natural orientation. Would you have > recommendations on determining this? Done. https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:45: controller->SetRotationLocked(true); On 2014/10/13 09:45:44, Mounir Lamouri wrote: > 'any' doesn't mean locked to the current orientation but allow all orientations > to be used. Is that the unlocked behaviour on ChromeOS? Correct it is unlocked. Merging with the DCHEcK for LockDefault, as they should both be unlocked states. https://codereview.chromium.org/648733003/diff/1/ash/system/chromeos/screen_o... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:51: controller->LockRotation(gfx::Display::ROTATE_0); On 2014/10/13 09:45:44, Mounir Lamouri wrote: > Same comment regarding the natural orientation. Done.
It's fairly confusing that the rotation lock state is managed by maximize controller. Can we make the screen orientation delegate the rotation lock controller and have maximize mode deal with it instead? https://codereview.chromium.org/648733003/diff/170001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/648733003/diff/170001/ash/ash.gyp#newcode324 ash/ash.gyp:324: 'system/chromeos/screen_orientation/screen_orientation_delegate_ash.h', Need to update BUILD.gn https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... File ash/system/chromeos/screen_orientation/DEPS (right): https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/DEPS:2: "+content/public/browser", You should probably name the specific file (i.e. +content/public/browser/screen_orientation_delegate.h) https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... File ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc (right): https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:5: #include "ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.h" This should maybe be in ash/display/screen_orientation_delegate_chromeos.{cc,h}. ash/system is for things on the system tray I believe. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:35: } Move this to a helper function (GetDisplayNaturalOrientation?) and call from initializer list. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:54: DCHECK(!controller->rotation_locked()); Won't this be true if the user manually locked a rotation? https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:78: default: Should probably handle all expected WebScreenOrientationLockType's and make default a NOTREACHED() and/or not have default (if the compile time check works). https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:90: Shell::GetInstance()->maximize_mode_controller()->SetRotationLocked(false); How does this interact with a user-locked orientation? https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:112: // two angles of an orientation. I'm a little unclear on what you mean here? Can you add an example? Is this e.g. from ROTATE_0 to ROTATE_180?
It would be great if you could update us with the UX decision regarding 'portrait' and 'landscape' locking. I would really like that to be implemented with flipping being allowed. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... File ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc (right): https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:28: rotation == gfx::Display::ROTATE_180) && nit: one space is missing to align this line https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:35: } On 2014/10/14 20:08:18, flackr wrote: > Move this to a helper function (GetDisplayNaturalOrientation?) and call from > initializer list. +1 with that, it would make this algorithm a bit more readable. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:54: DCHECK(!controller->rotation_locked()); On 2014/10/14 20:08:17, flackr wrote: > Won't this be true if the user manually locked a rotation? LockDefault is not expected to be called. You should DCHECK() if you end up with that lock_orientation value. Regarding 'any', I think you should explicitely unlock if that's ChromeOS behaviour. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:78: default: On 2014/10/14 20:08:18, flackr wrote: > Should probably handle all expected WebScreenOrientationLockType's and make > default a NOTREACHED() and/or not have default (if the compile time check > works). +1 https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:115: return; I'm a bit confused by that. Is it expected? I mean, is that something that _can_ happen? I feel like we should whether fallback to primary or DCHECK but doing nothing is unlikely a good idea. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... File ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.h (right): https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.h:32: nit: no empty line I guess?
https://codereview.chromium.org/648733003/diff/170001/content/public/browser/... File content/public/browser/screen_orientation_provider.cc (right): https://codereview.chromium.org/648733003/diff/170001/content/public/browser/... content/public/browser/screen_orientation_provider.cc:61: return; I moved this fix in https://codereview.chromium.org/655273003/ given that you are OOO for the rest of the week.
https://codereview.chromium.org/648733003/diff/170001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/648733003/diff/170001/ash/ash.gyp#newcode324 ash/ash.gyp:324: 'system/chromeos/screen_orientation/screen_orientation_delegate_ash.h', On 2014/10/14 20:08:17, flackr wrote: > Need to update BUILD.gn Currently ash/BUILD.gn refers to the source list from gyps. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... File ash/system/chromeos/screen_orientation/DEPS (right): https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/DEPS:2: "+content/public/browser", On 2014/10/14 20:08:17, flackr wrote: > You should probably name the specific file (i.e. > +content/public/browser/screen_orientation_delegate.h) Done. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... File ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc (right): https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:5: #include "ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.h" On 2014/10/14 20:08:17, flackr wrote: > This should maybe be in ash/display/screen_orientation_delegate_chromeos.{cc,h}. > > ash/system is for things on the system tray I believe. Done. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:28: rotation == gfx::Display::ROTATE_180) && On 2014/10/15 15:39:33, Mounir Lamouri wrote: > nit: one space is missing to align this line Done. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:35: } On 2014/10/14 20:08:18, flackr wrote: > Move this to a helper function (GetDisplayNaturalOrientation?) and call from > initializer list. Done. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:54: DCHECK(!controller->rotation_locked()); On 2014/10/15 15:39:33, Mounir Lamouri wrote: > On 2014/10/14 20:08:17, flackr wrote: > > Won't this be true if the user manually locked a rotation? > > LockDefault is not expected to be called. You should DCHECK() if you end up with > that lock_orientation value. > > Regarding 'any', I think you should explicitely unlock if that's ChromeOS > behaviour. Since it should not be called I'll use NOTREACHED, as a user can manually lock the rotation. Chaning Any to unlock, which is the default behaviour on ChromeOS. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:78: default: On 2014/10/15 15:39:33, Mounir Lamouri wrote: > On 2014/10/14 20:08:18, flackr wrote: > > Should probably handle all expected WebScreenOrientationLockType's and make > > default a NOTREACHED() and/or not have default (if the compile time check > > works). > > +1 Done. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:112: // two angles of an orientation. On 2014/10/14 20:08:17, flackr wrote: > I'm a little unclear on what you mean here? Can you add an example? Is this e.g. > from ROTATE_0 to ROTATE_180? Correct, it would be to allow the web page to lock the device to either portrait, or either landscape. So that 0/180 and 90/270 were allowed, while other orientations are not. Updated the note. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.cc:115: return; On 2014/10/15 15:39:33, Mounir Lamouri wrote: > I'm a bit confused by that. Is it expected? I mean, is that something that _can_ > happen? I feel like we should whether fallback to primary or DCHECK but doing > nothing is unlikely a good idea. This is a valid state for unit tests on ash. Since accelerometer rotation is only supported on internal displays, we opted to exit early on this condition rather than fail. Otherwise a large number of unrelated tests would have had to have been updated. https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... File ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.h (right): https://codereview.chromium.org/648733003/diff/170001/ash/system/chromeos/scr... ash/system/chromeos/screen_orientation/screen_orientation_delegate_ash.h:32: On 2014/10/15 15:39:33, Mounir Lamouri wrote: > nit: no empty line I guess? Done.
https://codereview.chromium.org/648733003/diff/210001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/210001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:58: Shell::GetInstance()->maximize_mode_controller(); Copying my previous comment inline to help it gain visibility: It's confusing that the rotation lock state is managed by maximize controller. Can we make the screen orientation delegate the rotation lock controller and have maximize mode talk to it instead? Maybe this makes sense as a follow-on refactoring.
I have added a TODO to move the control of the rotation lock to ScreenOrientationDelegate. A follow up change will update MaximizeModeController, TrayRotationLock, and other areas that are currently using MaximizeModeController to perform these locks. stevet@ will be back to the office on Monday. I will speak with him then to get a decision on how we want ChromeOS to handle rotation lock to a set of orientations (ei 90/270, 0/180.) There is currently a TODO for this as well. I would want to have that change be a follow up to having moved rotation lock into ScreenOrientationDelegate. https://codereview.chromium.org/648733003/diff/210001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/210001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:58: Shell::GetInstance()->maximize_mode_controller(); On 2014/10/22 21:28:04, flackr wrote: > Copying my previous comment inline to help it gain visibility: > > It's confusing that the rotation lock state is managed by maximize > controller. Can we make the screen orientation delegate the rotation lock > controller and have maximize mode talk to it instead? > > Maybe this makes sense as a follow-on refactoring. I've added a TODO, as that change would need to touch more areas than just MaximizeModeController
https://codereview.chromium.org/648733003/diff/230001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/648733003/diff/230001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:157: SetDisplayRotation(Shell::GetInstance()->display_manager(), rotation); So if multiple consumers try to lock different orientations the last one wins? Do the earlier ones need to be informed they lost the lock?
https://codereview.chromium.org/648733003/diff/230001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/648733003/diff/230001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:157: SetDisplayRotation(Shell::GetInstance()->display_manager(), rotation); On 2014/10/27 15:12:06, flackr wrote: > So if multiple consumers try to lock different orientations the last one wins? > Do the earlier ones need to be informed they lost the lock? For the Javascript api, currently when full-screen is toggled the lock is lost. The consumer at the time will be notified of the loss. With our restriction to full-screen tabs there is only ever one consumer at a time. Any ash-side consumers, they receive notifications via MaximizeModeController::Observer
https://codereview.chromium.org/648733003/diff/230001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/648733003/diff/230001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:157: SetDisplayRotation(Shell::GetInstance()->display_manager(), rotation); On 2014/10/27 18:18:48, jonross wrote: > On 2014/10/27 15:12:06, flackr wrote: > > So if multiple consumers try to lock different orientations the last one wins? > > Do the earlier ones need to be informed they lost the lock? > > For the Javascript api, currently when full-screen is toggled the lock is lost. > The consumer at the time will be notified of the loss. > > With our restriction to full-screen tabs there is only ever one consumer at a > time. This does not seem to be the case. I can open http://blogs.sitepointstatic.com/examples/tech/full-screen/index.html in two windows, fullscreen one, alt tab, and fullscreen the other. > > Any ash-side consumers, they receive notifications via > MaximizeModeController::Observer I'm worried about conflating the user lock with the application locks. For example what if? User locks rotation to landscape, then opens a fullscreen app requesting lock to portrait? Have we forgotten about the user lock at this point? When the app unlocks will the user lock be lost?
https://codereview.chromium.org/648733003/diff/230001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/648733003/diff/230001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:157: SetDisplayRotation(Shell::GetInstance()->display_manager(), rotation); On 2014/10/27 18:45:19, flackr wrote: > On 2014/10/27 18:18:48, jonross wrote: > > On 2014/10/27 15:12:06, flackr wrote: > > > So if multiple consumers try to lock different orientations the last one > wins? > > > Do the earlier ones need to be informed they lost the lock? > > > > For the Javascript api, currently when full-screen is toggled the lock is > lost. > > The consumer at the time will be notified of the loss. > > > > With our restriction to full-screen tabs there is only ever one consumer at a > > time. > > This does not seem to be the case. I can open > http://blogs.sitepointstatic.com/examples/tech/full-screen/index.html in two > windows, fullscreen one, alt tab, and fullscreen the other. > > > > > Any ash-side consumers, they receive notifications via > > MaximizeModeController::Observer > > I'm worried about conflating the user lock with the application locks. For > example what if? > > User locks rotation to landscape, then opens a fullscreen app requesting lock to > portrait? Have we forgotten about the user lock at this point? When the app > unlocks will the user lock be lost? Definitely a problem. So on Chrome OS the active fullscreen tab can be changed without exiting fullscreen. The ScreenOrientation api doesn't listen to this, so the apps do not know. ScreenOrientation accepts one lock per RenderFrameHost, however separate apps are separate hosts. Chrome OS currently has a central lock. No separation for user vs app. Requests that arrive all have different ids, even from the same given page. So a page can overwrite its old lock with a new one, yet not reuse the id.
Updated so the ScreenOrientationDelegate is provided the WebContents requesting a lock/unlock. This way platforms where web contents can be changed without exiting fullscreen can track locks appropriately. For this change ChromeOS will only allow one window to lock the orientation at a time. In a follow up it will begin allowing each window to have a lock, and will re-apply locks upon window activation changes.
We had a discussion with stevet@ regarding how ChromeOS will handle rotation locks. Locking to an orientation of Landscape/Portrait will allow switching between the two angles of said orientation. Each window will have a rotation lock, and will be applied when it is brought back to the foreground. Requests while in the background will be saved, and applied upon the next foregrounding. User locks will be cached and re-applied when there is no foreground app with a lock. I plan on applying these changes in a follow up review, where ScreenOrientationDelegateChromeOS will become the owner of rotation locks for ChromeOS.
https://codereview.chromium.org/648733003/diff/250001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/250001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:67: return false; Until we get the more complex model using, since this interferes with the user lock lets change this to also fail to lock if the user lock is currently applied. I.e. if (locked && requesting_window != locking_window_)
https://codereview.chromium.org/648733003/diff/250001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/250001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:67: return false; On 2014/10/29 16:50:47, flackr wrote: > Until we get the more complex model using, since this interferes with the user > lock lets change this to also fail to lock if the user lock is currently > applied. I.e. if (locked && requesting_window != locking_window_) Slightly different check added to exit. Will exit on (!locking_window_ && locked) which defines user lock. Allows web page to change its orientation lock if desired.
Some tests? Otherwise looks good. https://codereview.chromium.org/648733003/diff/250001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/250001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:67: return false; On 2014/10/29 18:34:02, jonross wrote: > On 2014/10/29 16:50:47, flackr wrote: > > Until we get the more complex model using, since this interferes with the user > > lock lets change this to also fail to lock if the user lock is currently > > applied. I.e. if (locked && requesting_window != locking_window_) > > Slightly different check added to exit. > > Will exit on (!locking_window_ && locked) which defines user lock. Allows web > page to change its orientation lock if desired. Looks like it effectively evaluates to the same.
Now with unittests
LGTM with some nits https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/Copyright (c) 2012/Copyright 2014 https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos_unittest.cc:171: delegate()->Lock(content1.get(), blink::WebScreenOrientationLockLandscape); EXPECT_TRUE? https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos_unittest.cc:172: delegate()->Lock(content2.get(), blink::WebScreenOrientationLockPortrait); EXPECT_FALSE? https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos_unittest.cc:201: Nice tests!
Hi Mounir, Would you be able to take another look that this change? https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/10/30 18:11:21, flackr wrote: > s/Copyright (c) 2012/Copyright 2014 Oops https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos_unittest.cc:171: delegate()->Lock(content1.get(), blink::WebScreenOrientationLockLandscape); On 2014/10/30 18:11:21, flackr wrote: > EXPECT_TRUE? Done. https://codereview.chromium.org/648733003/diff/290001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos_unittest.cc:172: delegate()->Lock(content2.get(), blink::WebScreenOrientationLockPortrait); On 2014/10/30 18:11:21, flackr wrote: > EXPECT_FALSE? Done.
https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:22: if (display_manager->HasInternalDisplay()) { nit: I think it would be better to do an early return in that case: if (!display_manager->HasInternalDisplay()) return blink::WebScreenOrientationLockLandscape; https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:33: return blink::WebScreenOrientationLockPortrait; I find this very hard to read. Also, this implicit way of using the default value is confusing. What about something like: switch(info.rotation) { case gfx::Display::ROTATE_0: case gfx::Display::ROTATE_180: return size.height() >= size.width() ? blink::WebScreenOrientationLockPortrait : blink::WebScreenOrientationLockLandscape; case gfx::Display::ROTATE_90: case gfx::Display::ROTATE_180: return size.height() < size.width() ? blink::WebScreenOrientationLockPortrait : blink::WebScreenOrientationLockLandscape; } https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:92: LockToRotationMatchingOrientation(lock_orientation); I guess that could be: case blink::WebScreenOrientationLockPortrait: case blink::WebScreenOrientationLockLandscape: LockToRotationMatchingOrientation(lock_orientation); https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.h (right): https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.h:34: virtual void Unlock(content::WebContents* web_contents) override; I think you can remove |virtual| from those methods override. The new coding style no longer requires |virtual| when you have |override|, as far as I know. https://codereview.chromium.org/648733003/diff/310001/content/public/browser/... File content/public/browser/screen_orientation_provider.cc (right): https://codereview.chromium.org/648733003/diff/310001/content/public/browser/... content/public/browser/screen_orientation_provider.cc:75: lock_applied_ = delegate_->Lock(web_contents(), lock_orientation); I'm really uncomfortable with that change especially because it is driven by temporary code. If the implementation was complete, you wouldn't need that, right? In that case, could you keep a broken but not exposed implementation? For example, lock would always appear as unsupported unless experimental web features is enabled?
I've removed the changes made to content that handled the incomplete ash implementation. The ash implementation is now tied to the flag we have used for the rest of TouchView feature development. https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:22: if (display_manager->HasInternalDisplay()) { On 2014/11/01 00:30:40, Mounir Lamouri wrote: > nit: I think it would be better to do an early return in that case: > if (!display_manager->HasInternalDisplay()) > return blink::WebScreenOrientationLockLandscape; Done. https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:33: return blink::WebScreenOrientationLockPortrait; On 2014/11/01 00:30:40, Mounir Lamouri wrote: > I find this very hard to read. Also, this implicit way of using the default > value is confusing. > > What about something like: > switch(info.rotation) { > case gfx::Display::ROTATE_0: > case gfx::Display::ROTATE_180: > return size.height() >= size.width() ? > blink::WebScreenOrientationLockPortrait : > blink::WebScreenOrientationLockLandscape; > case gfx::Display::ROTATE_90: > case gfx::Display::ROTATE_180: > return size.height() < size.width() ? > blink::WebScreenOrientationLockPortrait : > blink::WebScreenOrientationLockLandscape; > } Done. https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:92: LockToRotationMatchingOrientation(lock_orientation); On 2014/11/01 00:30:40, Mounir Lamouri wrote: > I guess that could be: > case blink::WebScreenOrientationLockPortrait: > case blink::WebScreenOrientationLockLandscape: > LockToRotationMatchingOrientation(lock_orientation); Done. https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.h (right): https://codereview.chromium.org/648733003/diff/310001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.h:34: virtual void Unlock(content::WebContents* web_contents) override; On 2014/11/01 00:30:40, Mounir Lamouri wrote: > I think you can remove |virtual| from those methods override. The new coding > style no longer requires |virtual| when you have |override|, as far as I know. Done. https://codereview.chromium.org/648733003/diff/310001/content/public/browser/... File content/public/browser/screen_orientation_provider.cc (right): https://codereview.chromium.org/648733003/diff/310001/content/public/browser/... content/public/browser/screen_orientation_provider.cc:75: lock_applied_ = delegate_->Lock(web_contents(), lock_orientation); On 2014/11/01 00:30:40, Mounir Lamouri wrote: > I'm really uncomfortable with that change especially because it is driven by > temporary code. If the implementation was complete, you wouldn't need that, > right? In that case, could you keep a broken but not exposed implementation? For > example, lock would always appear as unsupported unless experimental web > features is enabled? That SGTM. I'll reset the delegate to always Lock. The partial implementation in ash will be hidden behind the flag we use for TouchView experiments. (Which is currently the only state where we will support the functionality) The web experiments flag is not exposed to ash, and exposing it for a temporary change does not feel right.
Hi Mounir, Would you be able to take another look that this change?
lgtm with nits. Feel free to address the user lock vs application lock later. Sorry for the delay. For some reasons, I did not see your message but only your ping. https://codereview.chromium.org/648733003/diff/350001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/350001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:42: NOTREACHED(); nit: do you need a default here? Wouldn't the compilation break if there is no |case| for a known value? https://codereview.chromium.org/648733003/diff/350001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:45: return blink::WebScreenOrientationLockLandscape; nit: NOTREACHED()? https://codereview.chromium.org/648733003/diff/350001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:81: // Exit on user set rotation lock. Is that on purpose? It's quite the opposite behaviour from Android where the user rotation lock is only used as the default orientation. The application lock will always overwrite it, which I think makes sense because some applications might not even work. unless they are on the correct orientation.
https://codereview.chromium.org/648733003/diff/350001/ash/display/screen_orie... File ash/display/screen_orientation_delegate_chromeos.cc (right): https://codereview.chromium.org/648733003/diff/350001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:42: NOTREACHED(); On 2014/11/06 22:56:52, Mounir Lamouri wrote: > nit: do you need a default here? Wouldn't the compilation break if there is no > |case| for a known value? Done. https://codereview.chromium.org/648733003/diff/350001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:45: return blink::WebScreenOrientationLockLandscape; On 2014/11/06 22:56:53, Mounir Lamouri wrote: > nit: NOTREACHED()? Done. https://codereview.chromium.org/648733003/diff/350001/ash/display/screen_orie... ash/display/screen_orientation_delegate_chromeos.cc:81: // Exit on user set rotation lock. On 2014/11/06 22:56:52, Mounir Lamouri wrote: > Is that on purpose? It's quite the opposite behaviour from Android where the > user rotation lock is only used as the default orientation. The application lock > will always overwrite it, which I think makes sense because some applications > might not even work. unless they are on the correct orientation. We had planned this as a part of the temporary lock solution. Once we have user vs application lock we were going to re-apply user lock once applications exited. I'll remove it in the interim for consistency with Android.
jonross@chromium.org changed reviewers: + oshima@chromium.org
oshima@chromium.org: Please review changes in ash/display* ash/shell.(h,cc) In this review I am implementing the first stage of the Screen Orientation API, for Chrome OS.
jonross@chromium.org changed reviewers: + avi@chromium.org
avi@chromium.org: Please review changes in content/public/browser/* In this review I am implementing the first stage of the Screen Orientation API, for Chrome OS. This requires some updates to the content side.
I have high level question about the design of the API. I think the API (or underlying mechanism) should be arc friendly so that it works well with touch view. Have you consult with arc team about this? For example, multiple app should be able to specify its own desired orientation, and the system should switch when the app becomes active (in fullcreen, which is true in touch view mode). https://codereview.chromium.org/648733003/diff/370001/ash/display/DEPS File ash/display/DEPS (right): https://codereview.chromium.org/648733003/diff/370001/ash/display/DEPS#newcode8 ash/display/DEPS:8: "+third_party/WebKit/public/platform/WebScreenOrientationLockType.h", I want to control dependency from ash to content. Can you create ash/content/display and move new files there?
On 2014/11/11 01:06:27, oshima wrote: > I have high level question about the design of the API. > I think the API (or underlying mechanism) should be arc friendly so that it > works well with touch view. Have you consult with arc team about this? > > For example, multiple app should be able to specify its own desired orientation, > and the system should switch when the app becomes active (in fullcreen, which is > true in touch view mode). My understanding is that what you described is the goal that will be achieved in follow-ups.
On 2014/11/11 10:10:15, Mounir Lamouri wrote: > On 2014/11/11 01:06:27, oshima wrote: > > I have high level question about the design of the API. > > I think the API (or underlying mechanism) should be arc friendly so that it > > works well with touch view. Have you consult with arc team about this? > > > > For example, multiple app should be able to specify its own desired > orientation, > > and the system should switch when the app becomes active (in fullcreen, which > is > > true in touch view mode). > > My understanding is that what you described is the goal that will be achieved in > follow-ups. We have spoken offline. What ohsima would like to see will be supported with the follow ups to this review. I will reach out to the arc team to let them know about our work on it.
On 2014/11/11 22:46:52, jonross wrote: > On 2014/11/11 10:10:15, Mounir Lamouri wrote: > > On 2014/11/11 01:06:27, oshima wrote: > > > I have high level question about the design of the API. > > > I think the API (or underlying mechanism) should be arc friendly so that it > > > works well with touch view. Have you consult with arc team about this? > > > > > > For example, multiple app should be able to specify its own desired > > orientation, > > > and the system should switch when the app becomes active (in fullcreen, > which > > is > > > true in touch view mode). > > > > My understanding is that what you described is the goal that will be achieved > in > > follow-ups. > > We have spoken offline. What ohsima would like to see will be supported with the > follow ups to this review. I will reach out to the arc team to let them know > about our work on it. Thanks for the update. If you move content dependent code, this should be ready to go.
Patchset #13 (id:390001) has been deleted
The ash dependencies upon content have been moved into ash/content/
lgtm with nits https://codereview.chromium.org/648733003/diff/410001/ash/content/display/DEPS File ash/content/display/DEPS (right): https://codereview.chromium.org/648733003/diff/410001/ash/content/display/DEP... ash/content/display/DEPS:5: "+content/public/browser/web_contents.h", nit: you can just add "content/public/browser" use specific_include_rules for content/public/test like specific_include_rules = { "*test.cc": [ "+content/public/test" ] } https://codereview.chromium.org/648733003/diff/410001/ash/content/display/scr... File ash/content/display/screen_orientation_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/648733003/diff/410001/ash/content/display/scr... ash/content/display/screen_orientation_delegate_chromeos_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: nuke (c)
https://codereview.chromium.org/648733003/diff/410001/ash/content/display/DEPS File ash/content/display/DEPS (right): https://codereview.chromium.org/648733003/diff/410001/ash/content/display/DEP... ash/content/display/DEPS:5: "+content/public/browser/web_contents.h", On 2014/11/12 00:17:31, oshima wrote: > nit: you can just add "content/public/browser" > > use specific_include_rules for content/public/test like > > specific_include_rules = { > "*test.cc": [ > "+content/public/test" > ] > } Done. https://codereview.chromium.org/648733003/diff/410001/ash/content/display/scr... File ash/content/display/screen_orientation_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/648733003/diff/410001/ash/content/display/scr... ash/content/display/screen_orientation_delegate_chromeos_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/11/12 00:17:32, oshima wrote: > nit: nuke (c) Done.
Hello Avi, Would you be able to provide an owners review to the changes in content/public/browser/* ? Thanks, Jon
lgtm content/public/browser/* looks fine.
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648733003/430001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by jonross@chromium.org
jonross@chromium.org changed reviewers: + rbyers@chromium.org
Hi Rick, Could you provide a DEPS review as an owner of third_party/WebKit/public/* In this change I have added a dependency to: third_party/WebKit/public/platform/WebScreenOrientationLockType.h in: ash/content/display/DEPS In this review I am implementing the W3C Screen Orientation specification within ChromeOS. Thanks, Jon
On 2014/11/12 18:59:01, jonross wrote: > Hi Rick, > > Could you provide a DEPS review as an owner of third_party/WebKit/public/* > > In this change I have added a dependency to: > third_party/WebKit/public/platform/WebScreenOrientationLockType.h > > in: > ash/content/display/DEPS Adding a dependency on WebKit/public/platform from within ash is certainly fine with me from the blink side (public/ is the interface to blink, chrome/ depends on it, I don't see any reason why ash shouldn't). But it's really up to ash owners, not me. So ash/content/display/DEPS LGTM > > In this review I am implementing the W3C Screen Orientation specification within > ChromeOS. > > Thanks, > Jon
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648733003/430001
Message was sent while issue was closed.
Committed patchset #14 (id:430001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/4f3952e7d0172a50d26a6eb57660623db70000db Cr-Commit-Position: refs/heads/master@{#303898}
Message was sent while issue was closed.
achuith@chromium.org changed reviewers: + achuith@chromium.org
Message was sent while issue was closed.
https://uberchromegw.corp.google.com/i/chromiumos.chromium/builders/X86%20%28... https://uberchromegw.corp.google.com/i/chromiumos.chromium/builders/X86%20%28... chromeos-chrome-41.0.2219.0_alpha-r1: FAILED: i686-pc-linux-gnu-g++ -B/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/binutils-bin/2.24-gold -MMD -MF obj/ash/content/display/ash.screen_orientation_delegate_chromeos.o.d -DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DTOOLKIT_VIEWS=1 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_CRAS=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_X11=1 -DUSE_XI2_MT=2 -DIMAGE_LOADER_EXTENSION=1 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DUSE_UDEV -DDONT_EMBED_BUILD_METADATA -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_LOAD_COMPLETION_HACKS=1 -DASH_IMPLEMENTATION -DGL_GLEXT_PROTOTYPES -DMOJO_USE_SYSTEM_IMPL -DLIBPEERCONNECTION_LIB=1 -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1 -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT -DSK_FM_NEW_MATCH_FAMILY_STYLE_CHARACTER=1 -DSK_USE_SCALED_FONTMETRICS -DSK_SUPPORT_LEGACY_TEXTRENDERMODE -DSK_IGNORE_GPU_LAYER_HOISTING -DSK_USE_POSIX_THREADS -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DUSE_LIBPCI=1 -DUSE_GLIB=1 -DUSE_NSS=1 -DOS_CHROMEOS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen -I../../../../../../../home/chrome-bot/chrome_root/src -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/khronos -I../../../../../../../home/chrome-bot/chrome_root/src/gpu -I../../../../../../../home/chrome-bot/chrome_root/src/skia/config -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/WebKit/Source -Igen/angle -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/WebKit -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/src/core -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/core -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/effects -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pdf -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/gpu -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/lazy -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pathops -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pipe -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/ports -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/utils -I../../../../../../../home/chrome-bot/chrome_root/src/skia/ext -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/icu/source/i18n -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/icu/source/common -Igen/ui/resources -Igen/ash/resources -Igen/ash/strings -Igen/protoc_out -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/protobuf -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/protobuf/src -Igen/ui/chromeos/resources -Igen/ui/chromeos/strings -I/usr/include32 -Werror -pthread -fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Wno-unused-local-typedefs -pthread -I/build/x86-generic/usr/include/glib-2.0 -I/build/x86-generic/usr/lib/glib-2.0/include -I/build/x86-generic/usr/include/dbus-1.0 -I/build/x86-generic/usr/lib/dbus-1.0/include -msse2 -mfpmath=sse -mmmx -m32 --sysroot=/build/x86-generic/ -O2 -fno-ident -fdata-sections -ffunction-sections -funwind-tables -g -pipe -march=i686 -mfpmath=sse -mmmx -msse -msse2 -msse3 -D__google_stl_debug_vector=1 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -std=gnu++11 -Wno-narrowing -Wno-literal-suffix -c ../../../../../../../home/chrome-bot/chrome_root/src/ash/content/display/screen_orientation_delegate_chromeos.cc -o obj/ash/content/display/ash.screen_orientation_delegate_chromeos.o chromeos-chrome-41.0.2219.0_alpha-r1: ../../../../../../../home/chrome-bot/chrome_root/src/ash/content/display/screen_orientation_delegate_chromeos.cc: In function 'blink::WebScreenOrientationLockType {anonymous}::GetDisplayNaturalOrientation()': chromeos-chrome-41.0.2219.0_alpha-r1: ../../../../../../../home/chrome-bot/chrome_root/src/ash/content/display/screen_orientation_delegate_chromeos.cc:43:1: error: control reaches end of non-void function [-Werror=return-type] chromeos-chrome-41.0.2219.0_alpha-r1: } chromeos-chrome-41.0.2219.0_alpha-r1: ^ chromeos-chrome-41.0.2219.0_alpha-r1: cc1plus: all warnings being treated as errors chromeos-chrome-41.0.2219.0_alpha-r1:
Message was sent while issue was closed.
achuith@chromium.org changed reviewers: + tbarzic@chromium.org
Message was sent while issue was closed.
Adding Toni as cros gardener. I believe this has caused the x86-bot to go red and should probably be reverted.
Message was sent while issue was closed.
On 2014/11/13 01:17:07, achuithb wrote: > Adding Toni as cros gardener. I believe this has caused the x86-bot to go red > and should probably be reverted. already reverted: https://crrev.com/61aaa96a45aaa556a96ee19b87f0e512129a27d7
Message was sent while issue was closed.
On 2014/11/13 01:20:17, tbarzic wrote: > On 2014/11/13 01:17:07, achuithb wrote: > > Adding Toni as cros gardener. I believe this has caused the x86-bot to go red > > and should probably be reverted. > > already reverted: https://crrev.com/61aaa96a45aaa556a96ee19b87f0e512129a27d7 I have also landed a patch that addresses the error seen on daisy: https://codereview.chromium.org/723633003/
Message was sent while issue was closed.
On 2014/11/13 01:28:50, jonross wrote: > On 2014/11/13 01:20:17, tbarzic wrote: > > On 2014/11/13 01:17:07, achuithb wrote: > > > Adding Toni as cros gardener. I believe this has caused the x86-bot to go > red > > > and should probably be reverted. > > > > already reverted: https://crrev.com/61aaa96a45aaa556a96ee19b87f0e512129a27d7 > > I have also landed a patch that addresses the error seen on daisy: > https://codereview.chromium.org/723633003/ Ah thanks! The bot hasn't gone green yet so I thought this was still an outstanding issue, but looks like there's something else wrong with it now |
