This bug is to track the implementation of the screen orientation API in gonk: - getting orientation - orientation change listener - orientation lock/unlock

Comment on attachment 614322 [details] [diff] [review] Rename orientation API: remove 'moz' from the names I can't review this. Maybe smaug can?

test result: https://tbpl.mozilla.org/?tree=Try&rev=a8c2c47db7f5 (with only the difference of using ifneq (,$(filter-out android gonk,$(MOZ_WIDGET_TOOLKIT))) in hal/Makefile.in)

Comment on attachment 614323 [details] [diff] [review] Implement orientation API backend in gonk Review of attachment 614323 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly right to me. There's just one edge condition that I think should be handled better. ::: hal/gonk/GonkHal.cpp @@ +592,5 @@ > +namespace { > + > +bool sScreenOrientationNotificationEnabled = false; > + > +nsIScreen *GetPrimaryScreen() This appears to be a local helper function. Please mark functions static where possible. @@ +597,5 @@ > +{ > + nsresult result; > + nsCOMPtr<nsIScreenManager> screenMgr = > + do_GetService("@mozilla.org/gfx/screenmanager;1", &result); > + if (NS_FAILED(result)) { NS_ENSURE_TRUE(screenMgr, NULL); should be simpler. Also, we can just use the single arg version of do_GetService if we just check screenMgr. @@ +603,5 @@ > + return NULL; > + } > + nsCOMPtr<nsIScreen> screen; > + screenMgr->GetPrimaryScreen(getter_AddRefs(screen)); > + return screen; This function should probably return already_AddRefed<nsIScreen>. This line would then be |return screen.forget();| @@ +630,5 @@ > + return NS_OK; > +} > + > +nsresult ConvertToScreenRotation(dom::ScreenOrientation aOrientation, > + PRUint32 *result) I think this function can just be done as a switch statement. @@ +636,5 @@ > + if (aOrientation & dom::eScreenOrientation_Portrait) { > + if (aOrientation & dom::eScreenOrientation_PortraitPrimary) { > + *result = nsIScreen::ROTATION_0_DEG; > + } > + else { } else { is the style here I think, though there is a terrible mix of different styles in this file that we'll need to fix. @@ +719,5 @@ > + return; > + } > + > + // notify dom > + if (sScreenOrientationNotificationEnabled) { if (!sScreenOrientationNotificationEnabled) return; @@ +732,5 @@ > +void > +OrientationSensorObserver::Enable(PRUint32 aOrientations) > +{ > + MOZ_ASSERT(aOrientations); > + MOZ_ASSERT(aOrientations != dom::eScreenOrientation_EndGuard); Might as well go for aOrientations < dom::eScreenOrientation_EndGuard @@ +817,5 @@ > + else { > + sOrientationSensorObserver.Disable(); > + } > + > + if (prevRotation != rotation) { if (prevRotation == rotation) return true; @@ +818,5 @@ > + sOrientationSensorObserver.Disable(); > + } > + > + if (prevRotation != rotation) { > + result = screen->SetRotation(rotation); One condition I'm worried about here is if the user is holding the phone upside down, the screen is rotated 180 degrees, and an app locks the orientation to portrait. It seems like that would cause the screen to briefly rotate to 0 degrees before the next sensor event causes it to rotate back to 180 degrees.

Is there a reasonable stable specification for orientation? If not, we should keep the prefixes.

Comment on attachment 614323 [details] [diff] [review] Implement orientation API backend in gonk Review of attachment 614323 [details] [diff] [review]: ----------------------------------------------------------------- Some random comments about this patch: - You are assuming a specific device orientation is equivalent to a specific screen orientation but is that true for all devices? For example, on Android and iOS, when orientation is 0, a phone will be in portrait-primary but a tablet will be in landscape-primary... - Please, do not use orientation & eScreenOrientation_Foo but orientation == eScreenOrientation_Foo unless you really need to.

Comment on attachment 614322 [details] [diff] [review] Rename orientation API: remove 'moz' from the names Review of attachment 614322 [details] [diff] [review]: ----------------------------------------------------------------- There is no reason to remove 'moz' prefix. This is out of scope of the Gonk backend and it's way too early compared to the WebAPI policy regarding APIs.

Orientation API backend v2. Changes from v1: * changes per comment #5, and no extra rotation when loosening from 'portrait-secondary' to 'portrait' * add support for default device orienatition == landscape

To be added to github

regression test: https://tbpl.mozilla.org/?tree=Try&rev=4b8df0acf676

Comment on attachment 615299 [details] [diff] [review] Implement orientation API backend in gonk (v2) Review of attachment 615299 [details] [diff] [review]: ----------------------------------------------------------------- It looks generally good but there are a few things I would like to see fixed, amongst them, the coding style that is not very well respected (note that I pointed some lines but far from all so you should do a double-check). ::: hal/gonk/GonkHal.cpp @@ +706,5 @@ > + void Disable(); > + > +private: > + PRTime mLastUpdate; > + PRUint32 mOrientations; I would rename this mAllowedOrientations. @@ +751,5 @@ > + if (NS_FAILED(res)) > + return; > + > + if ((mOrientations & orientation) == dom::eScreenOrientation_None) > + return; // the desired orientation is not allowed Does that mean if neither lock or unlock are called, there will be no orientationchange events? @@ +755,5 @@ > + return; // the desired orientation is not allowed > + > + PRTime now = PR_Now(); > + MOZ_ASSERT(now > mLastUpdate); > + if (now - mLastUpdate < sMinUpdateInterval) Is that because of some platform specificities? @@ +785,5 @@ > + > +void > +OrientationSensorObserver::Disable() > +{ > + MOZ_ASSERT(mOrientations < dom::eScreenOrientation_EndGuard); I don't think you need that. @@ +789,5 @@ > + MOZ_ASSERT(mOrientations < dom::eScreenOrientation_EndGuard); > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (mOrientations & > + (dom::eScreenOrientation_Landscape | dom::eScreenOrientation_Portrait)) { This is equivalent to: if (mOrientations != dom::eScreenOrientation_None) { @@ +820,5 @@ > +GetCurrentScreenOrientation(dom::ScreenOrientation* aScreenOrientation) > +{ > + nsCOMPtr<nsIScreen> screen = GetPrimaryScreen(); > + if (!screen) > + return; You need to set a value. nit: coding style says you should put { } here. @@ +827,5 @@ > + nsresult res; > + res = screen->GetRotation(&rotation); > + if (NS_FAILED(res)) { > + NS_ERROR("Can't get screen rotation"); > + return; You need to set a value. @@ +830,5 @@ > + NS_ERROR("Can't get screen rotation"); > + return; > + } > + > + // aScreenOrientation will not be changed if conversion fails You should not do that. @@ +839,5 @@ > +LockScreenOrientation(const dom::ScreenOrientation& aOrientation) > +{ > + if (aOrientation <= dom::eScreenOrientation_None || > + aOrientation >= dom::eScreenOrientation_EndGuard) > + return false; // invalid value ScreenOrientation is an enum so what you are checking shouldn't happen. I would call MOZ_ASSERT instead of returning false.

Updated patch per comment #12

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #12) > @@ +751,5 @@ > > + if (NS_FAILED(res)) > > + return; > > + > > + if ((mOrientations & orientation) == dom::eScreenOrientation_None) > > + return; // the desired orientation is not allowed > > Does that mean if neither lock or unlock are called, there will be no > orientationchange events? Yes. By default screen orientation using sensor is disabled. It is enabled only after screen.mozUnlockOrientation(), screen.mozLockOrientation('portrait') or screen.mozLockOrientation('landscape') is called. But sensor might request landscape while only portrait is allowed and we don't fire dom event on such case. > > @@ +755,5 @@ > > + return; // the desired orientation is not allowed > > + > > + PRTime now = PR_Now(); > > + MOZ_ASSERT(now > mLastUpdate); > > + if (now - mLastUpdate < sMinUpdateInterval) > > Is that because of some platform specificities? > I do this because we don't know in what interval sensor notification is triggered. On android sensor API has setDelay() to control the reporting interval, but this is not necessarily implemented by vendor. So we might as well specify a minimal update interval to not rotate the screen too frequently in some corner cases.

Comment on attachment 615629 [details] [diff] [review] Implement orientation API backend in gonk (v3) Review of attachment 615629 [details] [diff] [review]: ----------------------------------------------------------------- r- because of all the nits, the coding style and the events/delay I would like to understand. Please, could you change all NULL to nsnull? Also you are abusing NS_ENSURE_FOO() with no second argument. You should not do that. In addition, comments should be written in proper English: "this is not a nice comment" "But this one is quite better." (In reply to Cervantes Yu from comment #14) > Yes. By default screen orientation using sensor is disabled. It is enabled > only after screen.mozUnlockOrientation(), > screen.mozLockOrientation('portrait') > or screen.mozLockOrientation('landscape') is called. But sensor might request > landscape while only portrait is allowed and we don't fire dom event on such > case. Does that mean 'orientationchange' events will never be fired if neither 'lock' nor 'unlock' have been called? > I do this because we don't know in what interval sensor notification is > triggered. On android sensor API has setDelay() to control the reporting > interval, but this is not necessarily implemented by vendor. So we might > as well specify a minimal update interval to not rotate the screen too > frequently in some corner cases. I'm really curious if you have find situations where this is really needed. I mean, given that we receive sensor events and then calculate the current orientation, that seems unlikely that the result of that computation will change very often (except if you crazily move you phone with a 45 degree movement to make it change from ladscape to portrait as fast as possible...). In another hand, adding that delay might create a sensation of UI slowness when intentionally moving from orientation X to Y and then moving back. ::: hal/gonk/GonkHal.cpp @@ +597,5 @@ > } > > +namespace { > + > +bool sScreenOrientationNotificationEnabled = false; I guess you could add |static| like you did for everything else below. @@ +619,5 @@ > + {nsIScreen::ROTATION_90_DEG, dom::eScreenOrientation_LandscapePrimary}, > + {nsIScreen::ROTATION_270_DEG, dom::eScreenOrientation_LandscapeSecondary}, > + {nsIScreen::ROTATION_90_DEG, dom::eScreenOrientation_Landscape} > +}; > +#define DEFAULT_LANDSCAPE_OFFSET 3 Could you do: static const PRUint8 sDefaultLandscape = 3; @@ +628,5 @@ > +static void > +DetectDefaultOrientation() > +{ > + nsCOMPtr<nsIScreen> screen = GetPrimaryScreen(); > + NS_ENSURE_TRUE(screen,); Oh, I should have seen that before. Please do that instead: if (!screen) { return; } This applies to all NS_ENSURE_FOO() with a missing second argument. @@ +633,5 @@ > + > + PRInt32 left, top, width, height; > + nsresult rv = screen->GetRect(&left, &top, &width, &height); > + NS_ENSURE_SUCCESS(rv,); > + PRUint32 rotation; nit: leave an empty line before |PRUint32 rotation;|. @@ +640,5 @@ > + > + if (width < height) { > + if (rotation == nsIScreen::ROTATION_0_DEG || > + rotation == nsIScreen::ROTATION_180_DEG) { > + sOrientationOffset = 0; // default portrait Please add |static const PRUint8 sDefaultPortrait = 0;| and use the value here instead of the hard-coded 0. @@ +649,5 @@ > + if (rotation == nsIScreen::ROTATION_0_DEG || > + rotation == nsIScreen::ROTATION_180_DEG) { > + sOrientationOffset = DEFAULT_LANDSCAPE_OFFSET; > + } else { > + sOrientationOffset = 0; // default portrait ditto @@ +656,5 @@ > +} > + > +static nsresult > +ConvertToDomOrientation(PRUint32 aRotation, > + dom::ScreenOrientation *result) Maybe you could write a comment explaining what this method is doing in details. It's not straightforward. @@ +661,5 @@ > +{ > + pthread_once(&detection_once_control, DetectDefaultOrientation); > + > + const int numOrientations = > + sizeof(sOrientationMappings) / sizeof(sOrientationMappings[0]); You could make this a |static const| so it will not be computed everytime you go into the method. You can also make this a |static const| in the anonymous namespace given that it's used in two methods. @@ +662,5 @@ > + pthread_once(&detection_once_control, DetectDefaultOrientation); > + > + const int numOrientations = > + sizeof(sOrientationMappings) / sizeof(sOrientationMappings[0]); > + for (int i = 0; i < numOrientations; i++) { nit: leave a blank line above this. @@ +671,5 @@ > + (sOrientationMappings[adjusted][1]); > + return NS_OK; > + } > + } > + *result = dom::eScreenOrientation_None; nit: leave a blank line before. @@ +677,5 @@ > +} > + > +static nsresult > +ConvertToScreenRotation(dom::ScreenOrientation aOrientation, > + PRUint32 *result) Same comments than above apply for this method too. @@ +710,5 @@ > + > +private: > + PRTime mLastUpdate; > + PRUint32 mAllowedOrientations; > + static const PRTime sMinUpdateInterval = 200 * 1000; // 200 ms Could you do | = 200 * PR_USEC_PER_MSEC;| instead. @@ +720,5 @@ > + // sensor will call us on the main thread > + MOZ_ASSERT(NS_IsMainThread()); > + > + MOZ_ASSERT(aSensorData.sensor() == SensorType::SENSOR_ORIENTATION); > + InfallibleTArray<float> values = aSensorData.values(); nit: keep the two MOZ_ASSERT in the same block and leave a blank line before |InfallibleTArray<float> ...;| @@ +721,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + MOZ_ASSERT(aSensorData.sensor() == SensorType::SENSOR_ORIENTATION); > + InfallibleTArray<float> values = aSensorData.values(); > + // float azimuth = values[0]; // unused Why do you keep this commented if it's not used? You could just put a comment explaining why it's not used instead. @@ +724,5 @@ > + InfallibleTArray<float> values = aSensorData.values(); > + // float azimuth = values[0]; // unused > + float pitch = values[1]; > + float roll = values[2]; > + PRUint32 rotation; nit: leave a blank line between |float ...;| and |PRUint32 rotation;| and doesn't leave one between the |rotation| declaration and the rotation setting block. @@ +739,5 @@ > + return; // don't rotate > + } > + > + PRUint32 currRotation; > + nsresult rv; Don't declare that variable here. @@ +743,5 @@ > + nsresult rv; > + > + nsCOMPtr<nsIScreen> screen = GetPrimaryScreen(); > + NS_ENSURE_TRUE(screen,); > + rv = screen->GetRotation(&currRotation); |nsresult rv = ...| @@ +757,5 @@ > + } > + > + PRTime now = PR_Now(); > + MOZ_ASSERT(now > mLastUpdate); > + NS_ENSURE_TRUE(now - mLastUpdate >= sMinUpdateInterval,); Oh... this is evil. Don't use NS_ENSURE_TRUE for that... And, wouldn't be better to do the timing check at the beginning of the function so you prevent using cycles for nothing? @@ +773,5 @@ > +void > +OrientationSensorObserver::Enable(PRUint32 aOrientations) > +{ > + MOZ_ASSERT(dom::eScreenOrientation_None < aOrientations); > + MOZ_ASSERT(aOrientations < dom::eScreenOrientation_EndGuard); You can merge this to: MOZ_ASSERT(aOrientation > dom::eScreenOrientation_None && aOrientation < dom::eScreenOrientation_EndGuard); @@ +776,5 @@ > + MOZ_ASSERT(dom::eScreenOrientation_None < aOrientations); > + MOZ_ASSERT(aOrientations < dom::eScreenOrientation_EndGuard); > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (mAllowedOrientations == dom::eScreenOrientation_None) { Put a comment explaining that this check means OrientationSensorObserver wasn't enabled yet and ::Enable() can be called n times. I had to look around calls to understand that. @@ +787,5 @@ > +OrientationSensorObserver::Disable() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (mAllowedOrientations != dom::eScreenOrientation_None) { Same here, equals none means already disabled. Actually, I would prefer an early return but that's more an aesthetic thing. @@ +831,5 @@ > +bool > +LockScreenOrientation(const dom::ScreenOrientation& aOrientation) > +{ > + MOZ_ASSERT(dom::eScreenOrientation_None < aOrientations); > + MOZ_ASSERT(aOrientations < dom::eScreenOrientation_EndGuard); You can merge this to: MOZ_ASSERT(aOrientation > dom::eScreenOrientation_None && aOrientation < dom::eScreenOrientation_EndGuard); @@ +843,5 @@ > + > + nsCOMPtr<nsIScreen> screen = GetPrimaryScreen(); > + NS_ENSURE_TRUE(screen, false); > + > + PRUint32 prevRotation; I would call this |currentRotation|. @@ +847,5 @@ > + PRUint32 prevRotation; > + nsresult rv = screen->GetRotation(&prevRotation); > + NS_ENSURE_SUCCESS(rv, false); > + > + dom::ScreenOrientation prevOrientation = dom::eScreenOrientation_None; and |currentOrientation|. @@ +852,5 @@ > + rv = ConvertToDomOrientation(prevRotation, &prevOrientation); > + NS_ENSURE_SUCCESS(rv, false); > + > + if (prevOrientation & aOrientation) > + return true; // don't rotate if we are in the requested orientation(s) I think it would be nicer to write this this way: // Don't rotate if the current orientation match one of the requested orientations. if (prevOrientation & aOrientation) { return true; } @@ +856,5 @@ > + return true; // don't rotate if we are in the requested orientation(s) > + > + PRUint32 rotation; > + rv = ConvertToScreenRotation(aOrientation, &rotation); > + // will return false on invalid orientation value nit: comments on top of block are always more readable.

Comment on attachment 615629 [details] [diff] [review] Implement orientation API backend in gonk (v3) Review of attachment 615629 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly ok other than the misuse of NS_ENSURE_TRUE. nsnull is mostly an unnecessary mozillaism these days so I don't consider it a requirement in platform specific code. If anything, we should be moving to nullptr. ::: hal/gonk/GonkHal.cpp @@ +611,5 @@ > + screenMgr->GetPrimaryScreen(getter_AddRefs(screen)); > + return screen.forget(); > +} > + > +static PRUint32 sOrientationMappings [][2] = { I suggest using an array of structs here instead of a 2d array so you can access the values with meaningful member names. @@ +661,5 @@ > +{ > + pthread_once(&detection_once_control, DetectDefaultOrientation); > + > + const int numOrientations = > + sizeof(sOrientationMappings) / sizeof(sOrientationMappings[0]); Use ArrayLength - https://hg.mozilla.org/mozilla-central/file/719a2fb28324/mfbt/Util.h#l335 @@ +663,5 @@ > + > + const int numOrientations = > + sizeof(sOrientationMappings) / sizeof(sOrientationMappings[0]); > + for (int i = 0; i < numOrientations; i++) { > + if (aRotation == sOrientationMappings[i][0]) { if (aRotation != sOrientationMappings[i][0]) continue; Similar comment applies to the other conversion function. @@ +757,5 @@ > + } > + > + PRTime now = PR_Now(); > + MOZ_ASSERT(now > mLastUpdate); > + NS_ENSURE_TRUE(now - mLastUpdate >= sMinUpdateInterval,); NS_ENSURE_TRUE should only be used for things that aren't expected to be false under normal circumstances. It will spew messages on debug builds. @@ +852,5 @@ > + rv = ConvertToDomOrientation(prevRotation, &prevOrientation); > + NS_ENSURE_SUCCESS(rv, false); > + > + if (prevOrientation & aOrientation) > + return true; // don't rotate if we are in the requested orientation(s) s/match/matches/ if you want to use mounir's comment wording. Either way sounds fine to me.

(In reply to comment #15) > > Please, could you change all NULL to nsnull? I would change the added NULL to nsnull, which doesn't hurt except creating an inconsistency in this file. But modifying all other NULL's is out of the scope of this patch. > Does that mean 'orientationchange' events will never be fired if neither 'lock' > nor 'unlock' have been called? Yes. 'unlock' or 'lock' has to be called for 'orientationchange' event to be fired. > I'm really curious if you have find situations where this is really needed. I > mean, given that we receive sensor events and then calculate the current > orientation, that seems unlikely that the result of that computation will change > very often (except if you crazily move you phone with a 45 degree movement to > make it change from ladscape to portrait as fast as possible...). > In another hand, adding that delay might create a sensation of UI slowness when > intentionally moving from orientation X to Y and then moving back. 150 to 200 ms is barely human-perceptible latency in some researches like this one: http://dl.acm.org/citation.cfm?id=634255&dl=ACM&coll=DL&CFID=77879211&CFTOKEN=72949065 Requiring a minimal 200 sec between screen rotations should not result in a feeling of UI slowness. > And, wouldn't be better to do the timing check at the beginning of the function > so you prevent using cycles for nothing? PR_Now() doesn't come with its cost. In my measurement, time spent in PR_Now() and getting nsIScreen's rotation is about 1:1.3. Though getting current screen rotation is more expensive, the screen rotation remains the same most of the time. Comparing sensor values with thresholds is much cheaper than PR_Now(), which performs a system call. Given these reasons, I would leave the time checking here.

Updated patch per comment #15 and #16

(In reply to Cervantes Yu from comment #17) > > Does that mean 'orientationchange' events will never be fired if neither 'lock' > > nor 'unlock' have been called? > > Yes. 'unlock' or 'lock' has to be called for 'orientationchange' event to be > fired. That means if none of those are called, there will be no event. That is wrong... We should always get this event even if there is no lock.

Comment on attachment 616064 [details] [diff] [review] Refactor battery updater using the uevent poller (v4) Review of attachment 616064 [details] [diff] [review]: ----------------------------------------------------------------- Still r- because I don't understand why the events are not allowed to be sent unless luck() or unlock() is called... ::: hal/gonk/GonkHal.cpp @@ +664,5 @@ > + } > + > + PRInt32 left, top, width, height; > + nsresult rv = screen->GetRect(&left, &top, &width, &height); > + if (NS_FAILED(rv)) { if (NS_FAILED(screen->GetRect(&left, &top, &width, &height))) { return; } @@ +670,5 @@ > + } > + > + PRUint32 rotation; > + rv = screen->GetRotation(&rotation); > + if (NS_FAILED(rv)) { ditto @@ +708,5 @@ > + > + for (int i = 0; i < sNumOrientations; i++) { > + if (aRotation == sOrientationMappings[i].mScreenRotation) { > + // Adjustment for default orientation. > + int adjusted = (i + sOrientationOffset) % sNumOrientations; When I asked you to put some comments for the method, I mostly wanted you to explain a bit more what this was doing. This is not straightforward enough to be explained IMO. And, "Adjustment for default orientation." is really not giving more information than |int adjusted = someStuffRegardingOrientation;|.. @@ +737,5 @@ > + > + for (int i = 0; i < sNumOrientations; i++) { > + if (aOrientation == sOrientationMappings[i].mDomOrientation) { > + // Adjustment for default orientation. > + int adjusted = (i + sOrientationOffset) % sNumOrientations; ditto @@ +801,5 @@ > + } > + > + PRUint32 currRotation; > + nsresult rv = screen->GetRotation(&currRotation); > + if (NS_FAILED(rv) || rotation == currRotation) { You could do: if (NS_FAILED(screen->GetRotation(&currRotation)) || rotation == currRotation) { return; } @@ +807,5 @@ > + } > + > + dom::ScreenOrientation orientation; > + rv = ConvertToDomOrientation(rotation, &orientation); > + if (NS_FAILED(rv)) { if (NS_FAILED(ConvertToDomOrientation(rotation, &orientation)) { return; } @@ +824,5 @@ > + } > + mLastUpdate = now; > + > + rv = screen->SetRotation(rotation); > + if (NS_FAILED(rv)) { if (NS_FAILED(screen->SetRotation(rotation)) { return; } @@ +844,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (mAllowedOrientations == dom::eScreenOrientation_None) { > + // Register if the observer wasn't enabled yet. This method can be called > + // multiple times, where succesive calls only change allowed orientations. That comment could even be made a method comment. @@ +857,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (mAllowedOrientations != dom::eScreenOrientation_None) { > + // Unregister if the observer was enabled. This method can be called > + // multiple times, where succesive calls have no effect. ditto @@ +894,5 @@ > + } > + > + PRUint32 rotation; > + nsresult rv = screen->GetRotation(&rotation); > + if (NS_FAILED(rv)) { if (NS_FAILED(screen->GetRotation(&rotation)) { return; }

Updated patch per comment #20. The major change is enable/disable automatic screen orientation in nsAppShell so lock() or unlock() doesn't need to be called for onorientationchange event to be fired.

Comment on attachment 616513 [details] [diff] [review] Implement orientation API backend in gonk (v5) Review of attachment 616513 [details] [diff] [review]: ----------------------------------------------------------------- You are trying to use very dirty hacks to Enable/Disable the OrientationSensorObserver. What I would recommend is doing the following: class OrientationSensorObserver : public hal::ISensorObserver { public: OrientationSensorObserver () : mLastUpdate(0) , mAllowedOrientations (dom::eScreenOrientation_Portrait | dom::eScreenOrientation_Landscape) { } void Notify(const hal::SensorData& aSensorData); // NOTE: those are drafts of what should the method do. // Please, write clean non-inline implementations. void Enable() { // TODO: you should probably check that this is not called twice or it will react correctly. hal::RegisterSensorObserver(hal::SENSOR_ORIENTATION, this); } void Disable() { // TODO: ditto hal::UnregisterSensorObserver(hal::SENSOR_ORIENTATION, this); } void Lock(PRUint32 aAllowedOrientation) { mAllowedOrientations = aAllowedOrientations; } void Unlock() { mAllowedOrientations = dom::eScreenOrientation_Portrait | dom::eScreenOrientation_Landscape; } private: PRTime mLastUpdate; PRUint32 mAllowedOrientations; // 200 ms, the latency which is barely perceptible by human. static const PRTime sMinUpdateInterval = 200 * PR_USEC_PER_MSEC; }; You can then make the hal methods call Lock() / Unlock() and nsAppShell() call Enable()/Disable(). Right now, it's not doable because nsAppShell can't access OrientationSensorObserver. There should be different ways to fix that.

This reminds me another question: when multiprocess work is done, does the observer run in the child process or parent process? The answer looks to be in child since it's surrounded in PROXY_IF_SANDBOXED(). If it's in the child process then nsAppShell has no direct access to it and a new method should be added to PHal.ipdl. Then a new question arises: does it make sense to get screen orientation from child process? I can't think of any reason to access the nsIScreen object in a child process through IPC calls. If the above reasoning is valid, then there is another bug in Hal.cpp that screen orientation shouldn't be proxied. Screen orientation methods in namespace hal should call their hal_impl:: counterparts.

OK, I got it wrong. Scratch comment #23. Then it'd be better to open an interface from GonkHal.cpp to let nsAppShell to fix that.

Updated patch per comment #22. The changes are: * Added methods to namespace hal for enabling/disabling automatic screen orientation on application startup/shutdown. The the methods only need to run on the main process. * Change the observer as recommended, with the difference that Lock() and Unlock() methods of the observer should consider if the observer needs to be enabled/disabled. This is to avoid unnecessary notifications from the sensor when the screen is locked to only one orientation.

Comment on attachment 616933 [details] [diff] [review] Implement orientation API backend in gonk (v6) Review of attachment 616933 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok to me aside from some nits. The hal changes probably need cjones' review. ::: hal/gonk/GonkHal.cpp @@ +633,5 @@ > + return screen.forget(); > +} > + > +struct OrientationMapping > +{ { on the same line as struct OrientationMapping @@ +638,5 @@ > + PRUint32 mScreenRotation; > + dom::ScreenOrientation mDomOrientation; > +}; > + > +static OrientationMapping sOrientationMappings [] = { no space between sOrientationMapping and [] @@ +647,5 @@ > + {nsIScreen::ROTATION_270_DEG, dom::eScreenOrientation_LandscapeSecondary}, > + {nsIScreen::ROTATION_90_DEG, dom::eScreenOrientation_Landscape} > +}; > + > +const static int sNumOrientations = ArrayLength(sOrientationMappings); Inline this into where it's used so it's obvious what array the variable is for. @@ +649,5 @@ > +}; > + > +const static int sNumOrientations = ArrayLength(sOrientationMappings); > +const static int sDefaultLandscape = 3; > +const static int sDeafultPortrait = 0; default spelled wrong @@ +754,5 @@ > +public: > + OrientationSensorObserver () > + : mEnabled(false), > + mLastUpdate(0), > + mAllowedOrientations (sDefaultOrientations) no space between mAllowedOrientations and () @@ +906,5 @@ > +} > + > +// Note that all operations with sOrientationSensorObserver > +// should be on the main thread. > +OrientationSensorObserver sOrientationSensorObserver; static if possible

Comment on attachment 616933 [details] [diff] [review] Implement orientation API backend in gonk (v6) Mounir's review is sufficient.

Comment on attachment 616933 [details] [diff] [review] Implement orientation API backend in gonk (v6) Review of attachment 616933 [details] [diff] [review]: ----------------------------------------------------------------- I really dislike that design. I don't think we should add new hal methods here. GonkHal code is run in the parent process and nsAppShell code too and they want to communicate with eachother so we should find a way for them to communicate in a better way that doesn't involve inter-process communication and abstraction layers... I would suggest that you move the orientation observer code from hal/ to widget/gonk/ (like widget/gonk/OrientationObserver.cpp). nsAppShell could enable/disable it at run-time/shutdown and hal would do what it is currently doing. The reason to do that is that hal shouldn't be accessed from outside without using the hal interface but adding something to the interface requires all backends to have the method and that enabling/disabling is very specific to Gonk. In the other hand, we can make things in widget/ accessible from code inside hal/. Does that sound reasonable?

Comment on attachment 621759 [details] [diff] [review] Implement orientation API in gonk (v7) Review of attachment 621759 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/OrientationObserver.cpp @@ +147,5 @@ > + > +} // Anonymous namespace > + > +OrientationObserver* > +OrientationObserver::Instance() Please have this allocate a new orientationobserver that is held by a nsAutoPtr and registered with ClearOnShutdown. The constructor should register the sensor listener and call DetectDefaultOrientation and the destructor should unregister the sensor listener.

Comment on attachment 621759 [details] [diff] [review] Implement orientation API in gonk (v7) Review of attachment 621759 [details] [diff] [review]: ----------------------------------------------------------------- Could you update that patch to apply on top of bug 745145? ::: hal/Makefile.in @@ +143,5 @@ > + $(NULL) > +endif > + > +# Fallbacks for backends implements on Android and Gonk. > +ifneq (,$(filter-out android gonk,$(MOZ_WIDGET_TOOLKIT))) Chris Jones already has a patch changing that. ::: hal/gonk/GonkHal.cpp @@ +601,5 @@ > > +void > +EnableScreenOrientationNotifications() > +{ > + MOZ_ASSERT(NS_IsMainThread()); Not needed. @@ +609,5 @@ > + > +void > +DisableScreenOrientationNotifications() > +{ > + MOZ_ASSERT(NS_IsMainThread()); Not neded. ::: widget/gonk/Makefile.in @@ +44,5 @@ > $(NULL) > > include $(DEPTH)/config/autoconf.mk > > +ifeq ($(MOZ_WIDGET_TOOLKIT), gonk) No need for that, you are in widget/gonk/... @@ +46,5 @@ > include $(DEPTH)/config/autoconf.mk > > +ifeq ($(MOZ_WIDGET_TOOLKIT), gonk) > +EXPORTS = \ > + OrientationObserver.h EXPORTS = OrientationObserver.h should be enough ::: widget/gonk/OrientationObserver.cpp @@ +22,5 @@ > +}; > + > +static OrientationMapping sOrientationMappings[] = { > + {nsIScreen::ROTATION_0_DEG, eScreenOrientation_PortraitPrimary}, > + {nsIScreen::ROTATION_180_DEG, eScreenOrientation_PortraitSecondary}, nit: 2 spaces for indentation, not 4 @@ +121,5 @@ > + * dom::eScreenOrientation_PortraitPrimary. > + * @return NS_OK on success. NS_ILLEGAL_VALUE on failure. > + */ > +nsresult > +ConvertToDomOrientation(PRUint32 aRotation, ScreenOrientation *aResult) Have a look at how Chris Jones did that in bug 745145. ::: widget/gonk/OrientationObserver.h @@ +14,5 @@ > +namespace hal { > +class SensorData; > +typedef mozilla::Observer<SensorData> ISensorObserver; > +} > +} nit: } // namespace hal } // namespace mozilla @@ +24,5 @@ > +class OrientationObserver : public ISensorObserver { > +public: > + OrientationObserver() > + : mAutoOrientationEnabled(false), > + mNotificationEnabled(false), nit: commas at the beginning of the attribute would be better Foo() : mFoo , mBar {} @@ +44,5 @@ > + void GetCurrentScreenOrientation(ScreenOrientation* aOrientation); > + bool LockScreenOrientation(ScreenOrientation aOrientation); > + void UnlockScreenOrientation(); > + > + static OrientationObserver* Instance(); nit: we usually use |GetInstance()| for singleton, I think. @@ +47,5 @@ > + > + static OrientationObserver* Instance(); > + > +private: > + bool AutoOrientationEnabled(); Please, remove that method, no need to have a method with the same visibility than the attribute.

Update the patch per comment #30. Also rebase the patch after the fix for bug 745145.

Update patch per comment #31.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #31) > Comment on attachment 621759 [details] [diff] [review] > Implement orientation API in gonk (v7) > > Review of attachment 621759 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +121,5 @@ > > + * dom::eScreenOrientation_PortraitPrimary. > > + * @return NS_OK on success. NS_ILLEGAL_VALUE on failure. > > + */ > > +nsresult > > +ConvertToDomOrientation(PRUint32 aRotation, ScreenOrientation *aResult) > > Have a look at how Chris Jones did that in bug 745145. > I would prefer not to change it for now. The lookup table is needed for conversion in both directions. We can change it later if you feel it necessary.

Resolve conflict from change https://hg.mozilla.org/integration/mozilla-inbound/rev/727b2eb545bd

Test on try: https://tbpl.mozilla.org/?tree=Try&rev=c2b1155f363c

You need my review before pushing that patch. I will try to do that later today.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #37) > You need my review before pushing that patch. I will try to do that later > today. The code is all in widget and I already pushed it. https://hg.mozilla.org/integration/mozilla-inbound/rev/a420a91d1b8c

(In reply to Michael Wu [:mwu] from comment #38) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #37) > > You need my review before pushing that patch. I will try to do that later > > today. > > The code is all in widget and I already pushed it. > > https://hg.mozilla.org/integration/mozilla-inbound/rev/a420a91d1b8c Asking a review to someone who r- the patch then ignoring the reviewer by pushing the patch is not a good behavior even if the person has no peer/owner status of the module.

(Firefox's cache changed a few metadata value by mistake, restring them.)

Implementation no longer matches spec.

Hi Marcos, It seems like we should file a new bug for the spec change. So, I will close this bug and then reopen the Bug 1131470 for the new spec. Thank you.