Closed
Bug 787538
Opened 13 years ago
Closed 13 years ago
Screen Orientation API: make sure the event listener is disconnected if the lock is rejected by the backend
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: mounir, Assigned: mounir)
Details
Attachments
(1 file, 1 obsolete file)
6.22 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
Right now, in some situations, we create an event listener to remove the orientation lock when we are no longer in fullscreen. However, some times, we can happen to be not locking. In that case, we don't need to keep that event listener.
Attachment #657395 -
Flags: review?(justin.lebar+bug)
Comment 1•13 years ago
|
||
>@@ -435,16 +443,20 @@ nsScreen::MozLockOrientation(const jsval
>
> target->AddSystemEventListener(NS_LITERAL_STRING("mozfullscreenchange"),
> mEventListener, /* useCapture = */ true);
> canLockOrientation = true;
> } while(0);
>
> if (canLockOrientation) {
> *aReturn = hal::LockScreenOrientation(orientation);
>+
>+ if (!*aReturn) {
>+ DisconnectEventListener();
>+ }
> }
>
> return NS_OK;
> }
I know we bikeshedded over this code a lot in the last review, but shouldn't
you simply wait to add the event listener until after we've called
hal::LockScreenOrientation and observed that it succeeded?
Updated•13 years ago
|
Attachment #657395 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #657395 -
Attachment is obsolete: true
Attachment #659499 -
Flags: review?(justin.lebar+bug)
Comment 3•13 years ago
|
||
This is good!
> + // This is only for compilers that don't understand that the previous switch
> + // will always return.
> return NS_OK;
Can you add a MOZ_NOT_REACHED() here? r=me with that change.
Updated•13 years ago
|
Attachment #659499 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 4•13 years ago
|
||
Target Milestone: --- → mozilla18
Comment 5•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•