Merge nsIScreen_MOZILLA_2_0_BRANCH into nsIScreen

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: emorley, Assigned: cjones)

Tracking

({addon-compat, dev-doc-complete})

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [not-fennec-11])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

8 years ago
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

8 years ago
Posted patch WIP (obsolete) — 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).
Reporter

Comment 3

8 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

8 years ago
Depends on: 649274, 616664
Reporter

Comment 4

8 years ago
Posted patch WIP v2 (obsolete) — 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
Reporter

Updated

8 years ago
Reporter

Comment 5

8 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
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+
Reporter

Updated

8 years ago
Attachment #548967 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/45e9963f21e9
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
Reporter

Comment 17

8 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).
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.
Reporter

Updated

7 years ago
Depends on: 766933
You need to log in before you can comment on or make changes to this bug.