oo[oouyang] Reporter
	Description • 9 years ago

The screen orientation api has changed to use screen.orientation.lock and screen.orientation.unlock (instead of screen.lockOrientation and screen.unlockOrientation). Does the screen orientation api also need to be changed?

See https://w3c.github.io/screen-orientation/#examples

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 1 • 9 years ago

What was the question again? What need to be changed also?

Gaia does not responsible of web/app-facing APIs, Gecko do -- We however require the Gaia-facing Gecko APIs continue to function (e.g. mozbrowserIframe methods) for the phone continue to function. Speak of, that's something to improve here (documented in bug 1043102).

	Ken Chang[:kenkai]
	Comment 4 • 9 years ago

Peter will have someone working on this.

	Andrew Overholt [:overholt]
	Comment 5 • 9 years ago

wchen is implementing the new API in bug 1043102.

	Peter Chang[:pchang]
	Comment 6 • 9 years ago

Clear ni based on comment 5.

	William Chen [:wchen] Assignee
	Comment 8 • 9 years ago

The new API adds orientation angle so I added it to screen configuration.

	Andrea Marchesini [:baku]
	Comment 13 • 9 years ago

Comment on attachment 8605686 [details] [diff] [review]
Part 1: Rename existing use of ScreenOrientation to ScreenOrientationInternal

Review of attachment 8605686 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see the point of this renaming, can you tell me why you need it?

::: dom/ipc/TabChild.cpp
@@ +2017,5 @@
>  }
>  
>  bool
>  TabChild::RecvUpdateDimensions(const nsIntRect& rect, const ScreenIntSize& size,
> +                               const ScreenOrientationInternal& orientation,

It doesn't seem so 'internal'. Why do you rename this variable type?

::: dom/ipc/TabChild.h
@@ +428,5 @@
>      /** Return the DPI of the widget this TabChild draws to. */
>      void GetDPI(float* aDPI);
>      void GetDefaultScale(double *aScale);
>  
> +    ScreenOrientationInternal GetOrientation() { return mOrientation; }

This should be GetOrientationInternal() and mOrientation should be mOrientatioInternal.

	Andrea Marchesini [:baku]
	Comment 14 • 9 years ago

Comment on attachment 8605687 [details] [diff] [review]
Part 2: Update screen configuration HAL to report orientation angle.

Review of attachment 8605687 [details] [diff] [review]:
-----------------------------------------------------------------

I took a look at this patch too just to have a global view of this new feature.

::: hal/android/AndroidHal.cpp
@@ +136,3 @@
>  
>    *aScreenConfiguration =
> +    hal::ScreenConfiguration(rect, orientation, angle, colorDepth, pixelDepth);

initialize angle only when you needed.

int16_t angle = bridge->GetScreenAngle();

or:

hal::ScreenConfiguration(rect, orientation, bridge->GetScreenAngle(), colorDepth, pixelDepth);

::: widget/android/AndroidJavaWrappers.h
@@ +561,5 @@
>      int ConnectionType() { return mConnectionType; }
>      bool IsWifi() { return mIsWifi; }
>      int DHCPGateway() { return mDHCPGateway; }
>      short ScreenOrientation() { return mScreenOrientation; }
> +    short ScreenAngle() { return mScreenAngle; }

all of these should be const.

::: widget/android/GeneratedJNIWrappers.h
@@ +663,5 @@
> +        typedef int32_t ReturnType;
> +        typedef int32_t SetterType;
> +        static constexpr char name[] = "getScreenAngle";
> +        static constexpr char signature[] =
> +                "()I";

on the same line?

::: widget/android/nsAppShell.cpp
@@ +583,5 @@
>          screen->GetColorDepth(&colorDepth);
>          screen->GetPixelDepth(&pixelDepth);
>          orientation =
>              static_cast<dom::ScreenOrientationInternal>(curEvent->ScreenOrientation());
> +        angle = curEvent->ScreenAngle();

int16_t angle = ...

	William Chen [:wchen] Assignee
	Comment 15 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #13)
> Comment on attachment 8605686 [details] [diff] [review]
> Part 1: Rename existing use of ScreenOrientation to ScreenOrientationInternal
> 
> Review of attachment 8605686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see the point of this renaming, can you tell me why you need it?
It isn't strictly necessary to rename, but the web exposed interface is named ScreenOrientation and I think it's better to let the implementation of the interface have that name instead using nsScreenOrientation. Right now ScreenOrientation is used for values passed around to HAL and this seems internal in the sense that it's not web exposed. I also don't mind calling it something other than ScreenOrientationInternal (maybe DeviceOrientation because it's closer to the hardware/device?).

	Andrea Marchesini [:baku]
	Comment 16 • 9 years ago

Comment on attachment 8605688 [details] [diff] [review]
Part 3: Implement ScreenOrientation interface.

Review of attachment 8605688 [details] [diff] [review]:
-----------------------------------------------------------------

Can you submit a new patch with these comments applied? I'm happy to review it again.

::: dom/base/ScreenOrientation.cpp
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/ScreenOrientation.h"

ScreenOrientation.h if they are in the same directory.

@@ +9,5 @@
> +using namespace mozilla::dom;
> +
> +NS_IMPL_ISUPPORTS_INHERITED0(ScreenOrientation, DOMEventTargetHelper)
> +
> +static OrientationType

use an anonymous namespace for these 2.

@@ +11,5 @@
> +NS_IMPL_ISUPPORTS_INHERITED0(ScreenOrientation, DOMEventTargetHelper)
> +
> +static OrientationType
> +InternalOrientationToType(ScreenOrientationInternal aOrientation)
> +{

can you use eScreenOrientation everywhere instead ScreenOrientationInternal ?
I mean, get rid of ScreenOrientationInternal completely is it possible?

@@ +46,5 @@
> +
> +ScreenOrientation::ScreenOrientation(nsPIDOMWindow* aWindow, nsScreen* aScreen)
> +  : DOMEventTargetHelper(aWindow), mScreen(aScreen)
> +{
> +  hal::RegisterScreenConfigurationObserver(this);

MOZ_ASSERT(aWindow);
MOZ_ASSERT(aScreen);

@@ +74,5 @@
> +  nsIDocument* aDocument, bool aIsFullScreen)
> +  : mScreenOrientation(aScreenOrientation), mPromise(aPromise),
> +    mOrientationLock(aOrientationLock), mDocument(aDocument),
> +    mIsFullScreen(aIsFullScreen)
> +{

MOZ_ASSERT(aScreenOrientation);
MOZ_ASSERT(aPromise)
MOZ_ASSERT(aDocument);

@@ +111,5 @@
> +
> +  ErrorResult rv;
> +  bool result = mScreenOrientation->LockDeviceOrientation(mOrientationLock,
> +                                                          mIsFullScreen, rv);
> +  if (rv.Failed()) {

NS_WARN_IF

@@ +115,5 @@
> +  if (rv.Failed()) {
> +    return rv.StealNSResult();
> +  }
> +
> +  if (!result) {

NS_WARN_IF

@@ +117,5 @@
> +  }
> +
> +  if (!result) {
> +    mPromise->MaybeReject(NS_ERROR_UNEXPECTED);
> +    mDocument->SetOrientationPendingPromise(nullptr);

return NS_OK;

@@ +121,5 @@
> +    mDocument->SetOrientationPendingPromise(nullptr);
> +  }
> +
> +  if (mOrientationLock &
> +      OrientationTypeToInternal(mDocument->CurrentOrientationType()) ||

This returns an enum. You should check the value.

@@ +177,5 @@
> +  return LockInternal(orientation, aRv);
> +}
> +
> +static inline void
> +AbortOrientationPromises(nsIDocShell* aDocShell)

move it into the anonymous namespace.

@@ +178,5 @@
> +}
> +
> +static inline void
> +AbortOrientationPromises(nsIDocShell* aDocShell)
> +{

MOZ_ASSERT(aDocShell);

@@ +192,5 @@
> +  int32_t childCount;
> +  aDocShell->GetChildCount(&childCount);
> +  for (int32_t i = 0; i < childCount; i++) {
> +    nsCOMPtr<nsIDocShellTreeItem> child;
> +    aDocShell->GetChildAt(i, getter_AddRefs(child));

this can fail.

@@ +204,5 @@
> +already_AddRefed<Promise>
> +ScreenOrientation::LockInternal(ScreenOrientationInternal aOrientation, ErrorResult& aRv)
> +{
> +  // Steps to apply an orientation lock as defined in spec.
> +

you can create the promise immediately and reject it in case something goes wrong instead using ErrorResult.

@@ +206,5 @@
> +{
> +  // Steps to apply an orientation lock as defined in spec.
> +
> +  nsIDocument* doc = GetResponsibleDocument();
> +  if (!doc) {

NS_WARN_IF

@@ +212,5 @@
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsPIDOMWindow> owner = GetOwner();
> +  if (!owner) {

NS_WARN_IF

@@ +217,5 @@
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsIDocShell> docShell = owner->GetDocShell();

NS_WARN_IF

@@ +223,5 @@
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(owner);

MOZ_ASSERT(go);

@@ +225,5 @@
> +  }
> +
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(owner);
> +  nsRefPtr<Promise> p = Promise::Create(go, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

@@ +229,5 @@
> +  if (aRv.Failed()) {
> +    return nullptr;
> +  }
> +
> +#if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_GONK)

you can do this check at the beginning of this method.

@@ +255,5 @@
> +
> +  nsCOMPtr<nsIRunnable> lockOrientationTask =
> +    new LockOrientationTask(this, p, aOrientation, doc,
> +                            perm == FULLSCREEN_LOCK_ALLOWED);
> +  NS_DispatchToMainThread(lockOrientationTask);

this can fail.

@@ +282,5 @@
> +  // We are fullscreen and lock has been accepted.
> +  if (aIsFullScreen && !mFullScreenListener) {
> +    mFullScreenListener = new FullScreenEventListener();
> +    aRv = target->AddSystemEventListener(NS_LITERAL_STRING("mozfullscreenchange"),
> +                                         mFullScreenListener, /* useCapture = */ true);

if (NS_WARN_IF(aRv.Failed())) {
  return false;
}

@@ +314,5 @@
> +  mFullScreenListener = nullptr;
> +}
> +
> +OrientationType
> +ScreenOrientation::DeviceType()

const

@@ +320,5 @@
> +  return mType;
> +}
> +
> +uint16_t
> +ScreenOrientation::DeviceAngle()

const

@@ +326,5 @@
> +  return mAngle;
> +}
> +
> +OrientationType
> +ScreenOrientation::GetType(ErrorResult& aRv)

const

@@ +338,5 @@
> +  return doc->CurrentOrientationType();
> +}
> +
> +uint16_t
> +ScreenOrientation::GetAngle(ErrorResult& aRv)

const

@@ +375,5 @@
> +    return LOCK_ALLOWED;
> +  }
> +
> +  if (Preferences::GetBool("dom.screenorientation.testing.non_fullscreen_lock_allow",
> +                           false) || true) {

why this "|| true" ?

@@ +384,5 @@
> +  return doc->MozFullScreen() ? FULLSCREEN_LOCK_ALLOWED : LOCK_DENIED;
> +}
> +
> +nsIDocument*
> +ScreenOrientation::GetResponsibleDocument()

const

@@ +420,5 @@
> +  mType = InternalOrientationToType(orientation);
> +
> +  if (mType != previousOrientation) {
> +    // Use of mozorientationchange is deprecated.
> +    mScreen->DispatchTrustedEvent(NS_LITERAL_STRING("mozorientationchange"));

this can fail.

@@ +424,5 @@
> +    mScreen->DispatchTrustedEvent(NS_LITERAL_STRING("mozorientationchange"));
> +  }
> +
> +  if (doc->Hidden()) {
> +    mVisibleListener = new VisibleEventListener();

what about if you already have another mVisibleListener?

@@ +425,5 @@
> +  }
> +
> +  if (doc->Hidden()) {
> +    mVisibleListener = new VisibleEventListener();
> +    doc->AddSystemEventListener(NS_LITERAL_STRING("visibilitychange"),

this can fail.

@@ +441,5 @@
> +    }
> +
> +    nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableMethod(this,
> +      &ScreenOrientation::DispatchChangeEvent);
> +    NS_DispatchToMainThread(runnable);

this can fail.

@@ +448,5 @@
> +
> +void
> +ScreenOrientation::DispatchChangeEvent()
> +{
> +  DispatchTrustedEvent(NS_LITERAL_STRING("change"));

this can fail.

@@ +479,5 @@
> +  }
> +
> +  ErrorResult rv;
> +  nsScreen* screen = win->GetScreen(rv);
> +  if (rv.Failed()) {

NS_WARN_IF

@@ +500,5 @@
> +    }
> +
> +    nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableMethod(orientation,
> +      &ScreenOrientation::DispatchChangeEvent);
> +    NS_DispatchToMainThread(runnable);

this can fail.

@@ +503,5 @@
> +      &ScreenOrientation::DispatchChangeEvent);
> +    NS_DispatchToMainThread(runnable);
> +  }
> +
> +  target->RemoveSystemEventListener(NS_LITERAL_STRING("visibilitychange"),

this can fail.

@@ +534,5 @@
> +  if (doc->MozFullScreen()) {
> +    return NS_OK;
> +  }
> +
> +  target->RemoveSystemEventListener(NS_LITERAL_STRING("mozfullscreenchange"),

this can fail.

::: dom/base/ScreenOrientation.h
@@ +32,5 @@
>  
> +class ScreenOrientation : public DOMEventTargetHelper,
> +                          public mozilla::hal::ScreenConfigurationObserver
> +{
> +  // nsScreen has deprecated API that shares implementation.

can you file a follow up to remove these deprecated methods?

@@ +61,5 @@
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +
> +  void Notify(const mozilla::hal::ScreenConfiguration& aConfiguration) override;
> +
> +private:

class FullScreenEventListener;

and move the code in the cpp file.

@@ +75,5 @@
> +  };
> +
> +  // Listener to update document's orienation lock when document becomes
> +  // visible.
> +  class VisibleEventListener final : public nsIDOMEventListener

class VisibleEventListener;
and move the code into the cpp file.

@@ +86,5 @@
> +    NS_DECL_NSIDOMEVENTLISTENER
> +  };
> +
> +  // Task to run step to lock orientation as defined in specification.
> +  class LockOrientationTask final : public nsIRunnable

This class can be moved to the cpp file.

::: dom/base/nsContentPolicy.cpp
@@ +14,5 @@
>  #include "nsXPCOM.h"
>  #include "nsContentPolicyUtils.h"
>  #include "nsContentPolicy.h"
>  #include "nsIURI.h"
> +#include "nsIDocShell.h"

why these 2 additional headers?

::: dom/base/nsDocument.cpp
@@ +12033,5 @@
>    return allowed;
>  }
>  
> +uint16_t
> +nsDocument::CurrentOrientationAngle()

const

@@ +12039,5 @@
> +  return mCurrentOrientationAngle;
> +}
> +
> +OrientationType
> +nsDocument::CurrentOrientationType()

const

@@ +12053,5 @@
> +  mCurrentOrientationAngle = aAngle;
> +}
> +
> +Promise*
> +nsDocument::GetOrientationPendingPromise()

const

::: dom/base/nsDocument.h
@@ +1264,5 @@
>    static void UnlockPointer(nsIDocument* aDoc = nullptr);
>  
> +  void SetCurrentOrientation(mozilla::dom::OrientationType aType,
> +                             uint16_t aAngle) override;
> +  uint16_t CurrentOrientationAngle() override;

const

@@ +1265,5 @@
>  
> +  void SetCurrentOrientation(mozilla::dom::OrientationType aType,
> +                             uint16_t aAngle) override;
> +  uint16_t CurrentOrientationAngle() override;
> +  mozilla::dom::OrientationType CurrentOrientationType() override;

const

@@ +1267,5 @@
> +                             uint16_t aAngle) override;
> +  uint16_t CurrentOrientationAngle() override;
> +  mozilla::dom::OrientationType CurrentOrientationType() override;
> +  void SetOrientationPendingPromise(mozilla::dom::Promise* aPromise) override;
> +  mozilla::dom::Promise* GetOrientationPendingPromise() override;

const

@@ +1675,5 @@
>    bool mAsyncFullscreenPending:1;
>  
> +  // ScreenOrientation "pending promise" as described by
> +  // http://www.w3.org/TR/screen-orientation/
> +  // TODO(wchen): Hook up to CC

Is it done in some other patch?
Just add NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOrientationPendingPromise) and UNLINK too in nsDocument.cpp.

::: dom/base/nsIDocument.h
@@ +117,5 @@
>  class EventTarget;
>  class FontFaceSet;
>  class FrameRequestCallback;
>  class ImportManager;
> +enum class OrientationType : uint32_t;

alphabetic order. This should go before ProcessingIntstruction.

@@ +1182,5 @@
> +  // ScreenOrientation related APIs
> +
> +  virtual void SetCurrentOrientation(mozilla::dom::OrientationType aType,
> +                                     uint16_t aAngle) = 0;
> +  virtual uint16_t CurrentOrientationAngle() = 0;

const

@@ +1183,5 @@
> +
> +  virtual void SetCurrentOrientation(mozilla::dom::OrientationType aType,
> +                                     uint16_t aAngle) = 0;
> +  virtual uint16_t CurrentOrientationAngle() = 0;
> +  virtual mozilla::dom::OrientationType CurrentOrientationType() = 0;

const

@@ +1185,5 @@
> +                                     uint16_t aAngle) = 0;
> +  virtual uint16_t CurrentOrientationAngle() = 0;
> +  virtual mozilla::dom::OrientationType CurrentOrientationType() = 0;
> +  virtual void SetOrientationPendingPromise(mozilla::dom::Promise* aPromise) = 0;
> +  virtual mozilla::dom::Promise* GetOrientationPendingPromise() = 0;

const

::: dom/base/nsQueryContentEventResult.h
@@ +9,5 @@
>  
>  #include "nsIQueryContentEventResult.h"
>  #include "nsString.h"
>  #include "nsRect.h"
> +#include "Units.h"

why is this needed?

::: dom/base/nsScreen.cpp
@@ +141,5 @@
>    return NS_OK;
>  }
>  
> +mozilla::dom::ScreenOrientation*
> +nsScreen::Orientation() {

const

@@ +147,4 @@
>  }
>  
>  void
>  nsScreen::GetMozOrientation(nsString& aOrientation)

const

::: dom/webidl/Screen.webidl
@@ +28,5 @@
>    [Throws]
>    readonly attribute long availLeft;
>  
>    /**
> +   * DEPRECATED, use ScreenOrientation API instead.

Would be nice to write something on stderr too.

::: dom/webidl/ScreenOrientation.webidl
@@ +30,5 @@
> +
> +[UnsafeInPrerendering]
> +interface ScreenOrientation : EventTarget {
> +  [Throws]
> +  Promise<void> lock (OrientationLockType orientation);

extra space between lock and '('

same for unlock.

	Andrea Marchesini [:baku]
	Comment 17 • 9 years ago

Comment on attachment 8605690 [details] [diff] [review]
Part 4: Check sandboxing flag for orientation lock.

Review of attachment 8605690 [details] [diff] [review]:
-----------------------------------------------------------------

Smaug, I need a feedback for these changes in nsSandboxFlags.

::: dom/base/ScreenOrientation.cpp
@@ +235,5 @@
>    // User agent does not support locking the screen orientation.c:w
>    p->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>    return p.forget();
>  #else
> +  LockPermission perm = GetLockOrientationPermission(true);

LockPermission perm = GetLockOrientationPermission(true /* check sandbox */);

::: dom/base/nsScreen.cpp
@@ +219,5 @@
>        return false;
>      }
>    }
>  
> +  switch (mScreenOrientation->GetLockOrientationPermission(false)) {

same comment here.

	Andrea Marchesini [:baku]
	Comment 18 • 9 years ago

Comment on attachment 8605691 [details] [diff] [review]
Part 5: Handle changes to active orientation lock.

Review of attachment 8605691 [details] [diff] [review]:
-----------------------------------------------------------------

You need some docshell peer to review this code.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 9 years ago

Comment on attachment 8605690 [details] [diff] [review]
Part 4: Check sandboxing flag for orientation lock.

I assume there are tests which check that orientation lock doesn't work in sandboxed iframes unless allow-orientation-lock is set.

	William Chen [:wchen] Assignee
	Comment 20 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #16)
> Comment on attachment 8605688 [details] [diff] [review]
> Part 3: Implement ScreenOrientation interface.
> 
> Review of attachment 8605688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +9,5 @@
> > +using namespace mozilla::dom;
> > +
> > +NS_IMPL_ISUPPORTS_INHERITED0(ScreenOrientation, DOMEventTargetHelper)
> > +
> > +static OrientationType
> 
> use an anonymous namespace for these 2.
The static keyword already makes the function local to the translation unit and should behave as if we were using an anonymous namespace. Is there a different reason to use an anonymous namespace here?
> @@ +11,5 @@
> > +NS_IMPL_ISUPPORTS_INHERITED0(ScreenOrientation, DOMEventTargetHelper)
> > +
> > +static OrientationType
> > +InternalOrientationToType(ScreenOrientationInternal aOrientation)
> > +{
> 
> can you use eScreenOrientation everywhere instead ScreenOrientationInternal ?
> I mean, get rid of ScreenOrientationInternal completely is it possible?
> 
Do you mean use OrientationType everywhere instead of ScreenOrientationInternal? OrientationType comes from WebIDL bindings while ScreenOrientationInternal are values that were selected to be passed into HAL APIs. If we wanted to use the same type everywhere we would need to add some internal extension to WebIDL to ensure that the enums generated in CPP exactly match the values that we use for ScreenOrientationInternal. This isn't the bug to do that kind of change (if we even wanted to have such an extension). 
> @@ +121,5 @@
> > +    mDocument->SetOrientationPendingPromise(nullptr);
> > +  }
> > +
> > +  if (mOrientationLock &
> > +      OrientationTypeToInternal(mDocument->CurrentOrientationType()) ||
> 
> This returns an enum. You should check the value.
mOrientationLock is a bitfield of ScreenOrientationInternal values and it's doing a bitwise AND to see if the document's current orientation is in the set of lock orientations.
> @@ +204,5 @@
> > +already_AddRefed<Promise>
> > +ScreenOrientation::LockInternal(ScreenOrientationInternal aOrientation, ErrorResult& aRv)
> > +{
> > +  // Steps to apply an orientation lock as defined in spec.
> > +
> 
> you can create the promise immediately and reject it in case something goes
> wrong instead using ErrorResult.
The types of errors that are checked at the beginning of the method are severe (no document, no global, no docshell) and I think they should throw at the callsite rather than being delegated into a promise (which also requires a global to create). The method is also used during unlocking which doesn’t use the promise, so it would need to throw the errors at the callsite.
> ::: dom/base/nsContentPolicy.cpp
> @@ +14,5 @@
> >  #include "nsXPCOM.h"
> >  #include "nsContentPolicyUtils.h"
> >  #include "nsContentPolicy.h"
> >  #include "nsIURI.h"
> > +#include "nsIDocShell.h"
> 
> why these 2 additional headers?
Adding the new files changed the chunking for unified builds.

	William Chen [:wchen] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8605691 [details] [diff] [review]
Part 5: Handle changes to active orientation lock.

Review of attachment 8605691 [details] [diff] [review]:
-----------------------------------------------------------------

This patch adds a getter and setter to implement an orientation lock on a browsing context:
https://w3c.github.io/screen-orientation/#dfn-orientation-lock

Also adds code to update orientation when active orientation lock changes or when we navigate:
https://w3c.github.io/screen-orientation/#dfn-active-orientation-lock

	William Chen [:wchen] Assignee
	Comment 22 • 9 years ago

Comment on attachment 8605687 [details] [diff] [review]
Part 2: Update screen configuration HAL to report orientation angle.

Review of attachment 8605687 [details] [diff] [review]:
-----------------------------------------------------------------

This patch adds the orientation angle to the ScreenConfiguration for Android and B2G in order to implement the angle property on the new orientation API.

jwillcox: Can you review the android parts?
mwu: Can you review the gonk parts?

	Michael Wu [:mwu]
	Comment 23 • 9 years ago

Comment on attachment 8605687 [details] [diff] [review]
Part 2: Update screen configuration HAL to report orientation angle.

Review of attachment 8605687 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsWindow.cpp
@@ +1084,5 @@
>  
> +static uint16_t
> +RotationToAngle(uint32_t aRotation)
> +{
> +  switch (aRotation) {

indentation is wrong in this function.

Also it seems like we could just reduce this to multiplying by 90. Or change the const to directly reflect the correct angle value.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 24 • 9 years ago

Comment on attachment 8605691 [details] [diff] [review]
Part 5: Handle changes to active orientation lock.

>@@ -10566,16 +10590,21 @@ nsDocShell::DoURILoad(nsIURI* aURI,
>   nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;
>   if (inherit) {
>     securityFlags |= nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
>   }
>   if (isSandBoxed) {
>     securityFlags |= nsILoadInfo::SEC_SANDBOXED;
>   }
> 
>+  if (OrientationLock() != 0) {
>+    SetOrientationLock(0);
>+    ScreenOrientation::UpdateActiveOrientationLock(0);
>+  }
Why do we want to call ScreenOrientation::UpdateActiveOrientationLock here even if we weren't 'active'?
(like, the docshell being in a background tab)
Also, why 0, why not some enum value?
And this stuff is in DoURILoad. Do we need to handle bfcached pages somehow?
(InternalLoad deals with bfcache loading, and if such isn't happening, calls DoURILoad)

>+  // The orientation lock as desbribed by
>+  // http://www.w3.org/TR/screen-orientation/
>+  uint32_t mOrientationLock;
This is what? OrientationLockType? If so, couldn't we use the actual enum as type?
Also, I assume we're trying to implement 
https://w3c.github.io/screen-orientation/ not http://www.w3.org/TR/screen-orientation/

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 25 • 9 years ago

Comment on attachment 8605687 [details] [diff] [review]
Part 2: Update screen configuration HAL to report orientation angle.

Review of attachment 8605687 [details] [diff] [review]:
-----------------------------------------------------------------

Android stuff looks ok to me. We're hoping to nuke a lot of the JNI stuff that this is using, but that's for another time.

::: widget/android/GeneratedJNIWrappers.h
@@ +663,5 @@
> +        typedef int32_t ReturnType;
> +        typedef int32_t SetterType;
> +        static constexpr char name[] = "getScreenAngle";
> +        static constexpr char signature[] =
> +                "()I";

This is generated code, so we would need to fix it there. I think a lot of these signatures are gigantic, so they go on the next line.

	Michael Wu [:mwu]
	Comment 26 • 9 years ago

Comment on attachment 8605687 [details] [diff] [review]
Part 2: Update screen configuration HAL to report orientation angle.

Review of attachment 8605687 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review since I've reviewed this. Just needs some small adjustments to be ready.

	Andrea Marchesini [:baku]
	Comment 27 • 9 years ago

Comment on attachment 8616213 [details] [diff] [review]
Implement ScreenOrientation interface. v2

Review of attachment 8616213 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm!

::: dom/base/ScreenOrientation.cpp
@@ +167,5 @@
> +    mPromise->MaybeReject(NS_ERROR_UNEXPECTED);
> +    mDocument->SetOrientationPendingPromise(nullptr);
> +    return NS_OK;
> +  }
> +

Can you implement a helper function to do this mOrientationLock & OrientationTypeInternal ?
Otherwise it seems an error in the code.

@@ +329,5 @@
> +  if (aIsFullScreen && !target) {
> +    return false;
> +  }
> +
> +  if (!hal::LockScreenOrientation(aOrientation)) {

NS_WARN_IF

@@ +541,5 @@
> +  if (!win) {
> +    return NS_OK;
> +  }
> +
> +  ErrorResult errorResult;

you can use rv as name and then everywhere else just continue using it instead creating a nsresult var.

::: dom/base/ScreenOrientation.h
@@ +29,5 @@
>  //eScreenOrientation_Default will use the natural orientation for the deivce,
>  //it could be PortraitPrimary or LandscapePrimary depends on display resolution
>  static const ScreenOrientationInternal eScreenOrientation_Default            = 1u << 4;
>  
> +class ScreenOrientation : public DOMEventTargetHelper,

final?

@@ +94,5 @@
> +  // uses ScreenOrientationInternal.
> +  already_AddRefed<Promise> LockInternal(ScreenOrientationInternal aOrientation,
> +                                         ErrorResult& aRv);
> +
> +  virtual ~ScreenOrientation();

Can you move this just below private:

::: dom/base/nsScreen.cpp
@@ +141,5 @@
>    return NS_OK;
>  }
>  
> +mozilla::dom::ScreenOrientation*
> +nsScreen::Orientation() const {

{ in a new line

	William Chen [:wchen] Assignee
	Comment 28 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8605691 [details] [diff] [review]
> Part 5: Handle changes to active orientation lock.
> 
> >+  if (OrientationLock() != 0) {
> >+    SetOrientationLock(0);
> >+    ScreenOrientation::UpdateActiveOrientationLock(0);
> >+  }
> Why do we want to call ScreenOrientation::UpdateActiveOrientationLock here
> even if we weren't 'active'?
Ah, we should be checking that we're active.
> (like, the docshell being in a background tab)
> Also, why 0, why not some enum value?
> And this stuff is in DoURILoad. Do we need to handle bfcached pages somehow?
> (InternalLoad deals with bfcache loading, and if such isn't happening, calls
> DoURILoad)
I've moved this stuff to InternalLoad since it does look like we care about bfcache loading as the spec says to lock the orientation to default orientation on navigation.
> >+  // The orientation lock as desbribed by
> >+  // http://www.w3.org/TR/screen-orientation/
> >+  uint32_t mOrientationLock;
> This is what? OrientationLockType? If so, couldn't we use the actual enum as
> type?
Its type is ScreenOrientationInternal. As defined by the spec it's an unordered set of OrientationType, but in our implementation it's a bitfield of values representing orientation types. I've updated the type to be explicitly ScreenOrientationInternal.
> Also, I assume we're trying to implement 
> https://w3c.github.io/screen-orientation/ not
> http://www.w3.org/TR/screen-orientation/
Yes. The github url is the draft and the w3 url is the published version. I could change the URL if you prefer.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 29 • 9 years ago

Comment on attachment 8623949 [details] [diff] [review]
Part 5: Handle changes to active orientation lock. v2

> 
>+  // Whenever a top-level browsing context is navigated, the user agent MUST
>+  // lock the orientation of the document to the document's default
>+  // orientation. We don't explicitly check for a top-level browsing context
>+  // here because orientation is only set on top-level browsing contexts.
>+  if (OrientationLock() != eScreenOrientation_None) {
>+    SetOrientationLock(eScreenOrientation_None);
>+
>+    if (mIsActive) {
>+      ScreenOrientation::UpdateActiveOrientationLock(eScreenOrientation_None);
>+    }
>+  }
As far as I see, the spec has a bug (which is then replicated here in the implementation).
Why should we do anything to orientation lock when doing just a fragment navigation.
Filed
https://github.com/w3c/screen-orientation/issues/87


>+  // The orientation lock as desbribed by
>+  // http://www.w3.org/TR/screen-orientation/
I would prefer link to the most resent version of the spec.


Could you ping the spec author about the fragment navigation case.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 30 • 9 years ago

I wonder also what should happen if beforeunload listener ends up causing navigation to stop

	William Chen [:wchen] Assignee
	Comment 31 • 9 years ago

Marcos, see comment 29 and 30. Just some ideas after quickly looking over the specs: maybe a more precise place to set the default orientation would be after step 11 in the "navigate" steps [1] which happens after the fragment and unload case Olli mentioned. Or perhaps set default orientation when unloading the top-level browsing context's active document.

[1] http://www.w3.org/TR/html5/browsers.html#navigate

	William Chen [:wchen] Assignee
	Comment 32 • 9 years ago

Comment on attachment 8605686 [details] [diff] [review]
Part 1: Rename existing use of ScreenOrientation to ScreenOrientationInternal

see comment 15

	Marcos Caceres [:marcosc]
	Comment 33 • 9 years ago

(In reply to William Chen [:wchen] from comment #31)
> Marcos, see comment 29 and 30. Just some ideas after quickly looking over
> the specs: maybe a more precise place to set the default orientation would
> be after step 11 in the "navigate" steps [1] which happens after the
> fragment and unload case Olli mentioned. Or perhaps set default orientation
> when unloading the top-level browsing context's active document.
> 
> [1] http://www.w3.org/TR/html5/browsers.html#navigate

good suggestions, I've added the comments to the bug #87 on github. I'll discuss with Mounir and update the spec soon. Annevk also made some comments there about matching fullscreen API's behavior.

	Andrea Marchesini [:baku]
	Comment 34 • 9 years ago

Comment on attachment 8605686 [details] [diff] [review]
Part 1: Rename existing use of ScreenOrientation to ScreenOrientationInternal

Review of attachment 8605686 [details] [diff] [review]:
-----------------------------------------------------------------

I'm very very sorry about this delay. I prefer DeviceOrientation instead ScreenOrientationInternal.

::: dom/base/ScreenOrientation.h
@@ +11,5 @@
>  namespace dom {
>  
> +// Make sure that any change to ScreenOrientationInternal values are
> +// also made in mobile/android/base/GeckoScreenOrientation.java
> +typedef uint32_t ScreenOrientationInternal;

DeviceOrientation, I like it more.

::: dom/ipc/TabChild.h
@@ +428,5 @@
>      /** Return the DPI of the widget this TabChild draws to. */
>      void GetDPI(float* aDPI);
>      void GetDefaultScale(double *aScale);
>  
> +    ScreenOrientationInternal GetOrientation() { return mOrientation; }

DeviceOrientation GetOrientation() const { ...

::: gfx/layers/composite/AsyncCompositionManager.h
@@ +110,5 @@
>      mLayersUpdated = true;
>      mTargetConfig = aTargetConfig;
>    }
>  
> +  bool RequiresReorientation(mozilla::dom::ScreenOrientationInternal aOrientation)

const

	William Chen [:wchen] Assignee
	Comment 35 • 9 years ago

The interaction of screen orientation with navigation hasn't been spec'ed yet, but we would still like this new API to help solve high priority web compat issues.

So far the only guidance for navigation is to do whatever we are currently doing for fullscreen, but from my reading, the spec only exits fullscreen when we "traverse the history by a delta" and I don't think that's enough because it doesn't get called by main navigate algorithm.

Also, when I tested, gecko didn't exit fullscreen when navigating to a new different origin webpage and I don't think that's the right thing to do for orientation. Blink behavior differs as well.

For now, I've just moved the code that sets orientation to default further down in the navigation algorithm so that it's past the short-circuit navigation and past the beforeunload check.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 37 • 9 years ago

Comment on attachment 8645084 [details] [diff] [review]
Part 5: Handle changes to active orientation lock. v3

>+  // Whenever a top-level browsing context is navigated, the user agent MUST
>+  // lock the orientation of the document to the document's default
>+  // orientation. We don't explicitly check for a top-level browsing context
>+  // here because orientation is only set on top-level browsing contexts.
>+  if (OrientationLock() != eScreenOrientation_None) {
I think we need still some assertion here that we are the top level browsing context.
#ifdef DEBUG
       nsCOMPtr<nsIDocShellTreeItem> parent;
       GetSameTypeParent(getter_AddRefs(parent));
       MOZ_ASSERT(!parent);
#endif

>+  // The orientation lock as described by
>+  // http://www.w3.org/TR/screen-orientation/
I would use the non-TR link. 
http://www.w3.org/TR is for TRash

	William Chen [:wchen] Assignee
	Comment 38 • 9 years ago

addressing review comment on part 2

	Pulsebot
	Comment 39 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d800ffe8ceb
https://hg.mozilla.org/integration/mozilla-inbound/rev/afee399c0f2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3413e043886c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f32d297a97e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c09474c608b
https://hg.mozilla.org/integration/mozilla-inbound/rev/be65c38abed9

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1d800ffe8ceb
https://hg.mozilla.org/mozilla-central/rev/afee399c0f2f
https://hg.mozilla.org/mozilla-central/rev/3413e043886c
https://hg.mozilla.org/mozilla-central/rev/4f32d297a97e
https://hg.mozilla.org/mozilla-central/rev/2c09474c608b
https://hg.mozilla.org/mozilla-central/rev/be65c38abed9

	Andrew Overholt [:overholt]
	Comment 41 • 9 years ago

William, please sent an intent to ship email: https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Email_templates

	Kohei Yoshino [:kohei]
	Comment 42 • 9 years ago

Added site compatibility doc:

https://www.fxsitecompat.com/en-US/docs/2015/screen-orientation-api-has-been-unprefixed/