Closed Bug 672166 Opened 13 years ago Closed 13 years ago

Merge nsIScreen_MOZILLA_2_0_BRANCH into nsIScreen

Categories

(Core :: Widget, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: emorley, Assigned: cjones)

References

Details

(Keywords: addon-compat, dev-doc-complete, Whiteboard: [not-fennec-11])

Attachments

(2 files, 2 obsolete files)

Broken out from bug 617539, since the original patch which received r+ failed try, so needs further work. http://mxr.mozilla.org/mozilla-central/search?string=nsIScreen_MOZILLA_2_0_BRANCH
Attached patch WIP (obsolete) (deleted) — Splinter Review
Is the patch from bug 617539, updated to tip and a couple of NS_IMPL_ISUPPORTS2 -> NS_IMPL_ISUPPORTS1 corrections. Generates the following compile errors on win32 when linking XUL: { nsScreenWin.obj : error LNK2001: unresolved external symbol "public: virtual unsigned int __stdcall nsScreenWin::LockMinimumBrightness(unsigned int)" (?LockMinimumBrightness@nsScreenWin@@UAGII@Z) nsScreenWin.obj : error LNK2001: unresolved external symbol "public: virtual unsigned int __stdcall nsScreenWin::UnlockMinimumBrightness(unsigned int)" (?UnlockMinimumBrightness@nsScreenWin@@UAGII@Z) xul.dll : fatal error LNK1120: 2 unresolved externals } ...since not all of the implementations of nsIScreen implemented nsIScreen_MOZILLA_2_0_BRANCH. I'm aiming to come back to this soon, but if someone else fixes this up before then, I won't be offended :-)
(Mid-aired) Problem here is that not all nsIScreen implementations also implement nsIScreen_MOZILLA_2_0_BRANCH. Either we need to implement these functions everywhere, or we should rename the interface (nsIScreenMobile, perhaps).
Looks like bug 616664 comments 28-30 give us the answer (note to self: check hg blame sooner next time!). (Comment #28) bsmedberg > I tried to remove the _MOZILLA_2_0 interface added here post-2.0, but it > seems that not every screen implementation implements the new interface, > only the android implementation does. If so, should this really be a unified > interface? Or should this be nsIScreenBrightness and left as a separate > interface? (Comment #29) azakai > I don't have an opinion about unified or separate, but I do think this would > be useful in other places than Android. It would be nice to not have the > display go to sleep on desktop too (like Flash video). (Comment #30) roc > I think we should make everything implement the new interface, with a dummy > implementation for non-Android. So presumably dummy interface for now, let someone implement it if desired later...
Assignee: nobody → bmo
Status: NEW → ASSIGNED
Depends on: 649274, 616664
Attached patch WIP v2 (obsolete) (deleted) — Splinter Review
Attaching this WIP so I can remember where I got to, having deleted the build log this morning once already when I clobbered :-/ Win32 build gives the following: { c:\mozilla\mozilla-central\widget\src\windows\nsScreenWin.h(49) : warning C4584: 'nsScreenWin' : base-class 'nsIScreen' is already a base-class of 'mozilla::widget::BrightnessLockingWidget' ../../../dist/include\nsIScreen.h(25) : see declaration of 'nsIScreen' ../../../../widget/src/shared\WidgetUtils.h(68) : see declaration of 'mozilla::widget::BrightnessLockingWidget' nsScreenWin.obj : error LNK2001: unresolved external symbol "public: virtual unsigned int __stdcall nsScreenWin::LockMinimumBrightness(unsigned int)" (?LockMinimumBrightness@nsScreenWin@@UAGII@Z) nsScreenWin.obj : error LNK2001: unresolved external symbol "public: virtual unsigned int __stdcall nsScreenWin::UnlockMinimumBrightness(unsigned int)" (?UnlockMinimumBrightness@nsScreenWin@@UAGII@Z) xul.dll : fatal error LNK1120: 2 unresolved externals }
Attachment #546486 - Attachment is obsolete: true
Not working on this at the moment, throwing back in the pool for others to take if wanted. May come back to it later if someone else hasn't sorted it in the meantime :-)
Assignee: bmo → nobody
Status: ASSIGNED → NEW
This is a serious detriment to adding new APIs to nsIScreen, which we need to do. I'm fixing this by adding an nsBaseScreen with fallback impls for the new APIs, which are really only implementable on newer mobile devices.
Assignee: nobody → jones.chris.g
Blocks: 714416
Builds locally on gonk, gtk2, and android, tryserver for the rest.
Attachment #585114 - Flags: superreview?(roc)
Attachment #585114 - Flags: superreview?(roc) → superreview+
mozilla::widget::BaseScreen, please. Have you checked if anybody uses GetRect/GetAvailRect from JS?
I started down the BaseScreen path but aborted because all the other *Base* are nsBase*. Let's move them all over wholesale in a followup. Didn't check. Why do you ask?
Because you changes the JS-exposed name.
Oh, bollocks. I thought that got normalized :(. We do have a JS caller of GetAvailRect(), but it's only tabbrowser.xml. No one will notice if that breaks, right? Heh heh heh ... AFAICT we don't have JS callers of GetRect. Thanks for the catch. Backing out. (Aside: this was clean on tryserver, which means we're not testing tab dragging ...)
Attachment #585678 - Flags: superreview?(roc) → superreview+
Attachment #548967 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
Spotted a comment remnant: http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseScreen.h#78 (Currently running HD recovery on my main machine :'-(, so don't have a repo around, otherwise I'd create the patch).
Whiteboard: [not-fennec-11]
The stuff added in the nsIScreen_MOZILLA_2_0_BRANCH interface was never documented; now it is, but in nsIScreen. https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScreen Also mentioned on Firefox 12 for developers.
Depends on: 766933
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: