364301 - [FIX]Eliminate consumers of GetScrollBarDimensions

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

GetScrollBarDimensions lies in many cases, and we have very few consumers of it anyway.  I think we should eliminate the few consumers and nix the interface.  This bug is for the former; the latter should get OK from the widget/gfx owners, I guess.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove all consumers

We could also stick some API on nsIScrollableFrame for this, I suppose... It'd involve creating an extra box layout state in the textarea case, but may be worth it.

Comment 2

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

We have an API on nsIScrollableFrame for this:
http://lxr.mozilla.org/mozilla/source/layout/generic/nsIScrollableFrame.h#75

 virtual nsMargin GetActualScrollbarSizes() const = 0;
 virtual nsMargin GetDesiredScrollbarSizes(nsBoxLayoutState* aState) = 0;

Can't we use GetDesiredScrollbarSizes here?

Comment 3

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

And we should definitely kill the API from nsIDeviceContext.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Use GetDesiredScrollbarSizes

Good catch!

Comment 5

[[e-mail]]

*** Bug 364233 has been marked as a duplicate of this bug. ***

Comment 6

[[e-mail]]

Comment on attachment 249076 [details] [diff] [review]
Use GetDesiredScrollbarSizes

> 
>   // Get scrollable view
>   nsIScrollableView* scrollableView;
>   result = mozXMLTermUtils::GetPresContextScrollableView(presContext,
>                                                          &scrollableView);
>   if (NS_FAILED(result) || !scrollableView)
>     return NS_ERROR_FAILURE;
> 
>-  // Get device context
>-  nsCOMPtr<nsIDeviceContext> deviceContext;
>-  result = mozXMLTermUtils::GetPresContextDeviceContext(presContext,
>-                                              getter_AddRefs(deviceContext));
>-  if (NS_FAILED(result) || !deviceContext)
>-    return NS_ERROR_FAILURE;
>-
>   // Determine twips to pixels conversion factor
>   float pixelScale;
>   pixelScale = presContext->TwipsToPixels();
> 
>-  // Get scrollbar dimensions in pixels
>-  float sbWidth, sbHeight;
>-  deviceContext->GetScrollBarDimensions(sbWidth, sbHeight);
>+  // Get scrollbar dimensions in pixels.  Ideally we'd measure the actual scrollbars here.
>+  float sbWidth = 15.0, sbHeight = 15.0;
Presumably there is a way to get the root scroll frame for a pres context
(what are my chances of getting a review if I write the code to do it?)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Presumably there is a way to get the root scroll frame for a pres context

Probably, but since I think XMLTerm should be CVS removed I wasn't going to spend more than the bare minimum "don't go red" amount of time on it....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in.  Filed bug 364345 on removing the API.

Comment 9

[[e-mail]]

Single-line <select>s look much better with this patch :-)

I did notice one unusual artefact though, since my font size had never been larger than my scrollbar size before. Easily fixed by reducing my font size ;-)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er... why did the font size matter for anything?

Comment 11

[[e-mail]]

(In reply to comment #10)
>Er... why did the font size matter for anything?
Because the height of a textarea is N rows plus the height of a scrollbar (plus its outside borders and stuff, but they're not relevant here). If your font size is less than or equal to the scrollbar height and the vertical scrollbar isn't visible then you can fit an extra line of text in ;-)