Closed
Bug 672166
Opened 13 years ago
Closed 13 years ago
Merge nsIScreen_MOZILLA_2_0_BRANCH into nsIScreen
Categories
(Core :: Widget, defect)
Core
Widget
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)
(deleted),
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
part 1.1: Use the old names in nsIScreen since there are JS callers and the names are not normalized
(deleted),
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•13 years ago
|
||
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 :-)
Comment 2•13 years ago
|
||
(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).
Reporter | ||
Comment 3•13 years ago
|
||
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
Reporter | ||
Updated•13 years ago
|
Reporter | ||
Comment 4•13 years ago
|
||
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
Reporter | ||
Updated•13 years ago
|
Keywords: addon-compat,
dev-doc-needed
Reporter | ||
Comment 5•13 years ago
|
||
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
Assignee | ||
Comment 6•13 years ago
|
||
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
Assignee | ||
Comment 7•13 years ago
|
||
Builds locally on gonk, gtk2, and android, tryserver for the rest.
Attachment #585114 -
Flags: superreview?(roc)
Attachment #585114 -
Flags: superreview?(roc) → superreview+
Comment 8•13 years ago
|
||
mozilla::widget::BaseScreen, please.
Have you checked if anybody uses GetRect/GetAvailRect from JS?
Assignee | ||
Comment 9•13 years ago
|
||
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?
Assignee | ||
Comment 10•13 years ago
|
||
Whiteboard: [inbound]
Comment 11•13 years ago
|
||
Because you changes the JS-exposed name.
Assignee | ||
Comment 12•13 years ago
|
||
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 ...)
Assignee | ||
Comment 13•13 years ago
|
||
Backed out
https://hg.mozilla.org/integration/mozilla-inbound/rev/a814c07a085f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad98a08690cb
Whiteboard: [inbound]
Assignee | ||
Comment 14•13 years ago
|
||
To be folded into the previous patch.
Attachment #585678 -
Flags: superreview?(roc)
Attachment #585678 -
Flags: superreview?(roc) → superreview+
Reporter | ||
Updated•13 years ago
|
Attachment #548967 -
Attachment is obsolete: true
Assignee | ||
Comment 15•13 years ago
|
||
Whiteboard: [inbound]
Comment 16•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
Reporter | ||
Comment 17•13 years ago
|
||
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).
Updated•13 years ago
|
Whiteboard: [not-fennec-11]
Comment 18•13 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•