672166 - Merge nsIScreen_MOZILLA_2_0_BRANCH into nsIScreen

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Ed Morley [:emorley]]

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

Comment 1

[Ed Morley [:emorley]]

Attachment: 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 :-)

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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).

Comment 3

[Ed Morley [:emorley]]

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...

Comment 4

[Ed Morley [:emorley]]

Attachment: 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
}

Comment 5

[Ed Morley [:emorley]]

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 :-)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

mozilla::widget::BaseScreen, please.

Have you checked if anybody uses GetRect/GetAvailRect from JS?

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64d814cd18b3

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Because you changes the JS-exposed name.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/a814c07a085f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad98a08690cb

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45e9963f21e9

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/45e9963f21e9

Comment 17

[Ed Morley [:emorley]]

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

Comment 18

[Eric Shepherd [:sheppy]]

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.