Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Merge nsIScreen_MOZILLA_2_0_BRANCH into nsIScreen

RESOLVED FIXED in mozilla12

Status

()

Core
Widget
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: emorley, Assigned: cjones)

Tracking

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

Trunk
mozilla12
addon-compat, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [not-fennec-11])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 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

6 years ago
Created attachment 546486 [details] [diff] [review]
WIP

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

6 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

6 years ago
Depends on: 649274, 616664
(Reporter)

Comment 4

6 years ago
Created attachment 548967 [details] [diff] [review]
WIP v2

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

6 years ago
Keywords: addon-compat, dev-doc-needed
(Reporter)

Comment 5

6 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
Created attachment 585114 [details] [diff] [review]
Get rid of nsIScreen_MOZILLA_2_0_BRANCH and create nsBaseScreen for shared code

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?
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d814cd18b3
Whiteboard: [inbound]
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 ...)
Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/a814c07a085f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad98a08690cb
Whiteboard: [inbound]
Created attachment 585678 [details] [diff] [review]
part 1.1: Use the old names in nsIScreen since there are JS callers and the names are not normalized

To be folded into the previous patch.
Attachment #585678 - Flags: superreview?(roc)
Attachment #585678 - Flags: superreview?(roc) → superreview+
(Reporter)

Updated

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

Comment 17

6 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.
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Updated

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