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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mounir, Assigned: mounir)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — 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)
>@@ -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?
Attachment #657395 - Flags: review?(justin.lebar+bug)
Attached patch Patch v2Splinter Review
Attachment #657395 - Attachment is obsolete: true
Attachment #659499 - Flags: review?(justin.lebar+bug)
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.
Attachment #659499 - Flags: review?(justin.lebar+bug) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: