Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
336313011:
Android: compute screen orientation type in the RWHV. (Closed)
|
Created:
6 years, 6 months ago by mlamouri (slow - plz ping) Modified:
6 years, 6 months ago Reviewers:
aelias_OOO_until_Jul13 CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lock_fullscreen Project:
chromium Visibility:
Public. |
DescriptionAndroid: compute screen orientation type in the RWHV.
BUG=162827
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279709
Patch Set 1 #Patch Set 2 : move to RWHVBase #Patch Set 3 : use in all platforms #Patch Set 4 : fix typo #Patch Set 5 : fix mac build #
Messages
Total messages: 29 (0 generated)
Looks OK, but can this be a method on gfx::Display instead?
On 2014/06/19 18:38:26, aelias wrote: > Looks OK, but can this be a method on gfx::Display instead? The value is platform dependent. I could put that in the DisplayInformation java class and have a setter/getter in gfx::Display. It would be much heavier work but if you would prefer that, I can do that.
The code you wrote doesn't look platform-dependent to me since it just examines existing gfx::Display members. Are you saying the logic you wrote inferring it from the angle doesn't apply to other platforms for some reason? If so, why?
On 2014/06/19 19:05:20, aelias wrote: > The code you wrote doesn't look platform-dependent to me since it just examines > existing gfx::Display members. Are you saying the logic you wrote inferring it > from the angle doesn't apply to other platforms for some reason? If so, why? The code I wrote isn't platform dependant, indeed. That's because Android only allows me to lock to primary/secondary but doesn't allow me to know if we currently are in a primary/secondary orientation. I then need to guess. However, some other platforms, like iOS might allow that. We might also be interested to return primary/secondary depending on the preference of the user. For example, if the device is 90% of the time in a specific orientation, it will become the primary one. Which mean that on Desktop, we might simply return primary unless the orientation changes. For example, I have a +/-90 degrees rotation on my desktop screens but I would expect the orientation type to be portrait-primary, regardless of the logic I wrote there. Anyhow, I want every platform to think about how they would handle that.
That all sounds too hypothetical to me. I'm pretty sure nobody on desktop platforms particularly cares to do anything complex for this feature, so those platforms would be fine with this implementation. The feature is very mobile-oriented and desktop platforms will be happy with just always returning LandscapePrimary or PortraitPrimary depending on the bounds ratio, which is what will happen they execute this code since "angle" is always 0 there. As for Chrome for iOS, we very likely have no power to affect what UIWebKit thinks about the orientation there anyway. So I'd still suggest just moving this code to gfx::Display as is and make the Aura implementation also use it as part of this patch.
On 2014/06/19 19:51:36, aelias wrote: > That all sounds too hypothetical to me. I'm pretty sure nobody on desktop > platforms particularly cares to do anything complex for this feature, so those > platforms would be fine with this implementation. The feature is very > mobile-oriented and desktop platforms will be happy with just always returning > LandscapePrimary or PortraitPrimary depending on the bounds ratio, which is what > will happen they execute this code since "angle" is always 0 there. As for > Chrome for iOS, we very likely have no power to affect what UIWebKit thinks > about the orientation there anyway. Just for information, the angle will not be always 0 on desktop. On linux, the angle will be whatever RandR returns for example.
lgtm OK. If RandR returns a reasonable value then this code should still behave okay (though this claim is *also* hypothetical because grepping doesn't reveal see any call to SetRotationAsDegree on non-Android). Anyway, for some reason you seem very concerned that nonexistent "desktop people" will want something different and you're trying to delay the decision. In reality you're the only one who will ever think about this question in detail and desktop platforms only care about returning some kind of value to check off an web platform API support checklist. I'd recommend just making them return a value now, which will likely settle the question permanently.
Sorry, not lgtm, clicked the "quick lgtm" button on Rietveld by accident.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlamouri@chromium.org/336313011/1
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: aelias@chromium.org. Please make sure to get positive review before another CQ attempt.
On 2014/06/19 20:04:19, I haz the power (commit-bot) wrote: > A disapproval has been posted by following reviewers: mailto:aelias@chromium.org. > Please make sure to get positive review before another CQ attempt. Interesting, the commit-bot tried to land that because of your review... Anyway, I wasn't hypothetical for XRandr, see ui/views/widget/desktop_aura/desktop_screen_x11.cc: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de... Chrome OS also has set_rotation() being set on Display. Then, WebScreenInfo on Aura is populated based on the display angle: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
On 2014/06/19 20:03:00, aelias wrote: > Anyway, for some reason you seem very concerned that nonexistent "desktop > people" will want something different and you're trying to delay the decision. > In reality you're the only one who will ever think about this question in detail > and desktop platforms only care about returning some kind of value to check off > an web platform API support checklist. I'd recommend just making them return a > value now, which will likely settle the question permanently. I will do as you ask.
As discussed offline, since ui/gfx doesn't have access to Blink types, it would make sense to make this code live in RenderWidgetHostViewBase. It also might be a nice refactoring to make RWHVB generate the entire WebScreenInfo object starting from a gfx::Device (with the platform-specific part just being deciding which gfx::Display to use).
PTAL
On 2014/06/23 15:04:21, Mounir Lamouri wrote: > PTAL lgtm
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/336313011/30001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/21160)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
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/336313011/50001
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/336313011/70001
Message was sent while issue was closed.
Change committed as 279709
Message was sent while issue was closed.
On 2014/06/25 13:52:38, I haz the power (commit-bot) wrote: > Change committed as 279709 This is causing layout test failures: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=scr... http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=scr... http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=scr... Are these expected?
Message was sent while issue was closed.
On 2014/06/25 at 14:45:28, fmalita wrote: > On 2014/06/25 13:52:38, I haz the power (commit-bot) wrote: > > Change committed as 279709 > > This is causing layout test failures: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=scr... > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=scr... > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=scr... > > Are these expected? As explained on IRC, this is related to https://codereview.chromium.org/339913002 I think the best thing to do is to disable the tests (or change the expected result) until the mock is fixed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
