651019 - Remove nsIDeviceContext wrappers around nsIScreen

Status: NEW

(Core :: Graphics, enhancement)

Description

[Zack Weinberg (:zwol)]

The very few users of the nsIDeviceContext screen size and depth methods should be able to get an nsIScreen object directly and use that instead.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: preliminary patch

Here is a work-in-progress patch.  Although the nsDeviceContext methods in question are gone, the replacement code for the various places that used them may not be right, and it might be appropriate to add helpers that retrieve an appropriate nsIScreen given various things (especially an nsPresContext).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 596145 [details] [diff] [review]
preliminary patch

I don't actually know much about this stuff, sorry...

Comment 3

[Zack Weinberg (:zwol)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> I don't actually know much about this stuff, sorry...

Do you know who would?

Comment 4

[Boris Zbarsky [:bzbarsky]]

roc is probably a reasonably choice.  Maybe some of the graphics folks?

Comment 5

[Mozilla RelEng Bot]

Try run for b8ac7217b53a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b8ac7217b53a
Results (out of 210 total builds):
    exception: 2
    success: 91
    warnings: 103
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b8ac7217b53a

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 596145 [details] [diff] [review]
preliminary patch

Review of attachment 596145 [details] [diff] [review]:
-----------------------------------------------------------------

It would be helpful to break this patch down into a bunch of small patches that remove independent usages of the nsIDeviceContext methods, plus another patch that removes unused code.

::: layout/base/nsLayoutUtils.cpp
@@ +4633,5 @@
> +    vArea.height = aPresContext->AppUnitsToDevPixels(vArea.height);
> +    nsCOMPtr<nsIScreen> screen;
> +    nsCOMPtr<nsIScreenManager> screenmgr = services::GetScreenManager();
> +    screenmgr->ScreenForRect(vArea.x, vArea.y, vArea.width, vArea.height,
> +                             getter_AddRefs(screen));

I don't understand this. There's no reference to the window associated with the prescontext, or the position of the prescontext within its window, so this can't possibly be right? I mean, vArea is relative to the prescontext itself, but ScreenForRect requires screen coordinates, so there's a mismatch.

Comment 7

[Zack Weinberg (:zwol)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)

> It would be helpful to break this patch down into a bunch of small patches
> that remove independent usages of the nsIDeviceContext methods, plus another
> patch that removes unused code.

Can do.  That'll also make it easier to debug.

> ::: layout/base/nsLayoutUtils.cpp
> @@ +4633,5 @@
> > +    vArea.height = aPresContext->AppUnitsToDevPixels(vArea.height);
> > +    nsCOMPtr<nsIScreen> screen;
> > +    nsCOMPtr<nsIScreenManager> screenmgr = services::GetScreenManager();
> > +    screenmgr->ScreenForRect(vArea.x, vArea.y, vArea.width, vArea.height,
> > +                             getter_AddRefs(screen));
> 
> I don't understand this. There's no reference to the window associated with
> the prescontext, or the position of the prescontext within its window, so
> this can't possibly be right? I mean, vArea is relative to the prescontext
> itself, but ScreenForRect requires screen coordinates, so there's a mismatch.

In fact, it doesn't work.  I was pretty much stabbing in the dark, here.  Do you have any suggestions for how to go about this properly?  The technique formerly used by the device context is to walk up the docshell tree looking for the nearest widget with a native window, but it seems like there has to be a better way.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should add an API to get an nsIScreen from an nsIWidget, to get the "dominant screen" for the widget (this could just get the widget's client rect and ask for the dominant screen for that rect, but it might make sense to allow the API to be platform-specific). Then you can use nsIFrame::GetNearestWidget with that API.

Comment 9

[Zack Weinberg (:zwol)]

That sounds doable in principle.  I don't know how to get a frame given a prescontext, but probably there's a frame nearby in all these places.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could add an API to get the enclosing widget for a Prescontext too. Get the RootPresContext, look for its view manager's root view's widget.

Comment 11

[Benjamin Smedberg]

Resetting owner to default per Zack's request.