Closed Bug 448830 Opened 16 years ago Closed 16 years ago

Split nsInt(Rect|Size|Point|Margin) into separate types and fix their usage.

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: reg, Assigned: reg)

References

Details

Attachments

(1 file, 6 obsolete files)

Fix harmless warnings
(deleted), patch
Details | Diff | Splinter Review
Attached patch Convert nsInt* to full types (obsolete) (deleted) — — Splinter Review 
This patch makes nsInt* into real types, not typedefs, and fixes the entire tree to make this work.  I've placed this in the layout component, for lack of a better place.

The majority of the patch is simple search and replace.  The only (possibly) controversial part is that I have removed all of the nsRect.Scale* functions, and replaced them inline conversion functions.  This is type safe and looks a lot cleaner in code.

However, there were three places where ScaleRoundOut(|Inverse) should still have been used.  Two identical invocations in the printing code (which should have been using inverse), which I just did in place, and one in nsMenuPopupFrame.cpp which was still easy to replace with these inline conversions.

The big question here is whether a lot of these should be gfxIntRect, etc.

Compiled and tested on windows, mac, linux.  I still need to get reftests working on my machines though...

Also, I have my mercurial repo up at http://www.openpave.org/cgi-bin/hgweb.cgi
Attachment #331997 - Flags: superreview?(roc)
Attachment #331997 - Flags: review?(roc)
The concept sounds good. I'll do my best to review it soon since it will rot fast.

I don't think we should be using gfxIntRect here, it would seem out of place. We might want to rationalize rect/int/size types (something I wanted to talk to people about at the summit, but forgot), but at worst nsIntRect/nsIntPoint/nsIntSize should survive as typedefs to something else, IMHO.
Oh, I forgot to mention that this is amazing work!!!
-[ref] native nsRectRef(nsRect);
+[ref] native nsRectRef(nsIntRect);
 
 [scriptable, uuid(89653afe-182f-401f-9f3c-8858d91387cd)]

Make that nsIntRectRef and rev the UUID.

+++ b/docshell/shistory/public/nsISHEntry.idl

Rev the UUID here too.

+  [ref] native nsNativeMarginRef(nsIntMargin);

nsNativeIntMarginRef and rev UUID.
As you guessed, the main issue here is the conversion methods. I agree they should be functional instead of in-place. However, I prefer member functions with the original names. But I think it would also be fine to write them as nsRect::ToIntRectPreservingCenters, nsRect::ToIntRectOut, nsRect::ToIntRectIn, and nsIntRect::ToAppUnits. dbaron, any preferences here?
Maybe even nsRect::RoundPreservingCenters, nsRect::RoundOut, nsRect::RoundIn.
Summary: Spilt nsInt(Rect|Size|Point|Margin) into seperate types and fix their usage. → Spilt nsInt(Rect|Size|Point|Margin) into separate types and fix their usage.
Summary: Spilt nsInt(Rect|Size|Point|Margin) into separate types and fix their usage. → Split nsInt(Rect|Size|Point|Margin) into separate types and fix their usage.
OK, I'll work on an updated patch.

I like the ToIntRect syntax, since it implies a change of coordinates, not just a rounding.  ToPixels also has the same property.  I think that 'nearest', or 'round', is a much shorter way of saying 'preserving centers' which is not true anyway since the rounding does not always preserve centers.  How about:

nsIntRect nsRect::ToIntRectOut(const nsRect &aRect, nscoord AppUnitsPerPixel);
nsIntRect nsRect::ToIntRectNearest(const nsRect &aRect, nscoord AppUnitsPerPixel);
nsIntRect nsRect::ToIntRectIn(const nsRect &aRect, nscoord AppUnitsPerPixel);

or even:

nsIntRect nsRect::ToOuterPixels(const nsRect &aRect, nscoord AppUnitsPerPixel);
nsIntRect nsRect::ToNearestPixels(const nsRect &aRect, nscoord AppUnitsPerPixel);
nsIntRect nsRect::ToInnerPixels(const nsRect &aRect, nscoord AppUnitsPerPixel);

The Inner/Outer terms also work for nsSize (where I've not provided methods, but should, since there are a number of places where this is done by hand), but not for nsPoint, where one would want to use Floor/Ceil?  Although I think that most people would have a feel for what Out and In did if they thought about the TopLeft corner of a rectangle.

(sorry about the spelling in the title...  seems the more I write on my dissertation the worse my spelling gets ;-()

(In reply to comment #6)
> I like the ToIntRect syntax, since it implies a change of coordinates, not
> just a rounding.  ToPixels also has the same property.  I think that
> 'nearest', or 'round', is a much shorter way of saying 'preserving centers'
> which is not true anyway since the rounding does not always preserve
> centers.

It doesn't?

> nsIntRect nsRect::ToOuterPixels(const nsRect &aRect, nscoord AppUnitsPerPixel);
> nsIntRect nsRect::ToNearestPixels(const nsRect &aRect, nscoord
> AppUnitsPerPixel);
> nsIntRect nsRect::ToInnerPixels(const nsRect &aRect, nscoord AppUnitsPerPixel);

These sound good.
Attached patch Address initial review comments (obsolete) (deleted) — — Splinter Review 
(In reply to comment #7)
> > which is not true anyway since the rounding does not always preserve
> > centers.
> 
> It doesn't?

(0.5,0.5)-(1.5,1.5) becomes (1,1)-(2,2).  A suppose it depends on your definition of centers.

The new patch has the new names, IID changes for all the interfaces I could find, and some other minor cleanup.

Just a point..  This patch requires the patch from bug 379616, which changes SVG to using app units not device pixels.
Attachment #331997 - Attachment is obsolete: true
Attachment #333038 - Flags: superreview?(roc)
Attachment #333038 - Flags: review?(roc)
Attachment #331997 - Flags: superreview?(roc)
Attachment #331997 - Flags: review?(roc)
(In reply to comment #8)
> Created an attachment (id=333038) [details]
> Address initial review comments
> 
> (In reply to comment #7)
> > > which is not true anyway since the rounding does not always preserve
> > > centers.
> > 
> > It doesn't?
> 
> (0.5,0.5)-(1.5,1.5) becomes (1,1)-(2,2).  A suppose it depends on your
> definition of centers.

OK, it preserves centers that are in the interior of the rectangle and does something arbitrary with the centers on the boundary. I still think "PreservingCenters" is much more geometrically meaningful than "Round".
If you could update this patch, now that 379616 is done I'll review this pronto and we can get it in!
Attached patch Refreshed patch (obsolete) (deleted) — — Splinter Review 
Updated patch.  It is using 'nearest' to mean 'preserving centers'.  I still think that 'snapped' might be the term people would be the most familiar with (i.e. s/ToNearestPixels/ToSnappedPixels/g).

I'm mostly offline from 2008/09/28 to 2008/11/06, so you might need to sheppard this in a little.  I can do some changes up till then.  I have built this on FreeBSD, Linux and Windows.  My mac build needs to be clobbered and my 'mac' virtually crawls... it did get to linking libxul so that means any mac specific changes should have been gone over.  I last ran reftests and mochitests last week, and all seemed OK.  There were only minor changes since then.
Attachment #333038 - Attachment is obsolete: true
Attachment #340050 - Flags: superreview?(roc)
Attachment #340050 - Flags: review?(roc)
Attachment #333038 - Flags: superreview?(roc)
Attachment #333038 - Flags: review?(roc)
Two instances of this:

-  rect.ScaleRoundOut(1.0f / scale);
+  rect.width = NSToCoordCeil(rect.XMost() / scale);
+  rect.height = NSToCoordCeil(rect.YMost() / scale);
+  rect.width -= rect.x = NSToCoordFloor(rect.x / scale);
+  rect.height -= rect.y = NSToCoordFloor(rect.y / scale);

This is a little bit ugly. I think it's worth keeping ScaleRoundOutInverse for this.

-  return nsRect((nscoord)cocoaRect.origin.x,
-                (nscoord)(MenuBarScreenHeight() - (cocoaRect.origin.y + cocoaRect.size.height)),
-                (nscoord)cocoaRect.size.width,
-                (nscoord)cocoaRect.size.height);
+  return nsIntRect(NSToIntFloor(cocoaRect.origin.x),
+                   NSToIntFloor(MenuBarScreenHeight() - (cocoaRect.origin.y + cocoaRect.size.height)),
+                   NSToIntCeil(cocoaRect.size.width),
+                   NSToIntCeil(cocoaRect.size.height));

This is no doubt an improvement on what we had, but why not get it right and actually implement center-preserving rounding? What you've done here isn't geometrically satisfying.

Otherwise I think we're good to go. Please post a new patch and if you can, run reftests and mochitests ... those tests are finicky so you may need to run them once on trunk to identify any tests that just don't work on your machine. Thanks a ton!!!
Attachment #340050 - Flags: superreview?(roc)
Attachment #340050 - Flags: superreview+
Attachment #340050 - Flags: review?(roc)
Attachment #340050 - Flags: review+
I made those changes and pushed b3412569801f. Thanks Jeremy!!!
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I backed this out because of test failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1222387456.1222390757.21162.gz
*** 33791 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul |  bounding rect width - got 95, expected 93
*** 33793 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul |  bounding rect right - got 95, expected 93
*** 33809 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | innermenuitem bounding rect width - got 91, expected 89
*** 33811 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | innermenuitem bounding rect right - got 91, expected 89
*** 33827 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | outermenuitem bounding rect width - got 85, expected 83
*** 33829 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.xul | outermenuitem bounding rect right - got 85, expected 83
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch that was checked in (obsolete) (deleted) — — Splinter Review 
This is the patch I checked in. As well as the issues related to my review comments, it contains some build bustage fixes in nsIViewManager and accessibility/src/msaa that I landed after the initial checkin.
Attached patch Fix incorrect tests (obsolete) (deleted) — — Splinter Review 
Sorry about all the burning boxes :-(  Seems like I need to do some work on getting my build environments closer to tinderbox...  Already fixed the accessibility issues on Windows (turned it off when there were all the problems with using Express versions, oops), and downloaded a centos VM to use rather than my Ubuntu VM, to get whatever compiler caught that naughty stray semicolon...

Anyway, the attached patch fixes the mochitests for me on windows.  Simple mistakes in the tests. (The same tests that failed on the SVG app unit patch...  I could have sworn I tested them then, but I must have tested a build with just that patch and not this one)

Obviously though there was some change in rendering on windows.  I see a difference between the border of the first submenu in the XUL popup, which on 3.0.2 has a flat single pixel border and on my patched build has a beveled border.  The same effect can be seen on the normal menu bar, so I would think it is an intentional change on trunk.
Attachment #340527 - Flags: superreview?(roc)
Attachment #340527 - Flags: review?(roc)
Attachment #340527 - Flags: superreview?(roc)
Attachment #340527 - Flags: superreview+
Attachment #340527 - Flags: review?(roc)
Attachment #340527 - Flags: review+
Comment on attachment 340527 [details] [diff] [review]
Fix incorrect tests

nice catch
So we're good to reland now?
I think we are.
Pushed again as changeset 536b982929cd
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Backed out again. Test failures on Linux:

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/trunk_linux-8/build/layout/reftests/bugs/413292-1.html| Failed to load
*** 68412 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_popup_scaled.xul | anchored top position - got 59, expected 58

The first failure might be random. The second failure happened on both Linux test boxes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Looks like it might have also caused a 3%-5% pageload time regression on Windows.  See http://graphs.mozilla.org/graph.html#show=395008,395020,395048,912148,1431867&sel=1222531363,1222613112 for the Windows XP machines, at least.
Txul regression on Windows XP was more noticeable:
http://graphs.mozilla.org/graph.html#show=395002,395014,395042,912144,1431854&sel=1222531363,1222613112 but I'm still not seeing it on other platforms (although the Windows Vista perf test boxes may not have even cycled yet).
I think this patch was responsible for the vertical mispostioning of the text in selection boxes that can be seen in today's MAC and Linux nightlies when visiting http://crash-stats.mozilla.com/
(In reply to comment #24)
> I think this patch was responsible for the vertical mispostioning of the text
> in selection boxes that can be seen in today's MAC and Linux nightlies when
> visiting http://crash-stats.mozilla.com/

I have verified that the patch from this bug WAS the cause of the above regression.
Depends on: 457533
okay, I guess we won't be relanding this until Jeremy comes back.
Sorry, I've been so quiet on this...  I've finally got all my build infrastructure caught up, the patch updated, and moved to platforms closer to the reference platforms, standalone talos installed and working on windows (is Twinopen the same as Txul?), etc.  But I've not had any time to look at the three problems yet.
Yes, I think Twinopen is the same as Txul.
Attached patch Fix test failures (obsolete) (deleted) — — Splinter Review 
OK, finally found some time to look at this.  The disappearing select box text was a rookie mistake with misordering the constructor arguments to nsMargin in one place.  I'm surprised this wasn't caught by testing.  The popup thing was the introduction of some unwanted rounding in the code if the anchor element was not pixel aligned, which moved the popup by .2 of a pixel.  I've made some changes to nnMenuPopupFrame.cpp to use .ScaleRoundOut(), which now passes the tests.

In my testing Twinopen is 10% faster, over 100 test runs.  This is with VS2008, without PGO.  I haven't managed to get this into VTune, but a simple test run, replacing all of occurances of ScaleRoundOut(1.0/factor) with ScaleRoundOutInverse(factor) (which this patch effectively does) showed about a 3% increase in Twinopen.  So I think this (4 divs instead of 1 div and 4 mul) might be the problem...  But it's the right thing to do.

This passes all tests on Windows and adds no new failures on my Linux machine.  I can't get tests to run on the mac ;-(.  The mochitest fix is rolled in.
Attachment #340050 - Attachment is obsolete: true
Attachment #340485 - Attachment is obsolete: true
Attachment #340527 - Attachment is obsolete: true
Attachment #356050 - Flags: superreview?(roc)
Attachment #356050 - Flags: review?(roc)
Oh, just one more thing...  This patch will introduce a lot more warnings on windows (which is the only platform which complains), because of the change in NSAppUnitsToFloatPixels to take a float instead of an nscoord as the second argument.  I have left this, because I think the change is wrong...  but I think that NSAppUnitsToFloatPixels should also be returning a GfxFloat (i.e. double) rather than float, since it is used as a GfxFloat in all but one or two places.  This would require making sure that the the underlying functions are also double aware.  I think I might work on that next.
Fabulous!

I've submitted this to the try server, that should give us some Ts numbers.
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

If we're still seeing a Ts regression, then the best thing to do is probably to reimplement ScaleRoundOutInverse the "wrong way" as a divide and four multiplies and then check Ts again; if that does fix the regression, we should land it that way and worry about how to fix that later.
> In my testing Twinopen is 10% faster, over 100 test runs.

Did you mean 10% slower?
Attachment #356050 - Flags: superreview?(roc)
Attachment #356050 - Flags: superreview+
Attachment #356050 - Flags: review?(roc)
Attachment #356050 - Flags: review+
No.  10% faster, although I just realized that was on median values.  1.5% faster on mean (73.98 sec with patch, 75.22 seconds without).
OK

For those Windows warnings, there would only be 12 new warnings, right?
Unfortunately not, since the functions are inline, VC++ emits 4 warnings each time any of the nsRect::ToXxx functions are called.  That's probably a few hundred extra warnings.  The easy fix is to cast aAppUnitsPerPixel in these functions to float.
Yeah, let's do that
The try-server builds are showing no significant performance regressions.
So I think we can rev this to get rid of the Windows warnings and then reland it.
Attached patch Fix harmless warnings (deleted) — — Splinter Review 
OK, now with more castability...  And merged to tip (no functional changes).
Attachment #356050 - Attachment is obsolete: true
Let's do it. I'll be away for a couple of days but I can try landing this next week.
Keywords: checkin-needed
Whiteboard: [needs landing]
Updated for some last-minute bitrotting and pushed as 9813b594352a. Let's hope it sticks this time! Thanks!
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
No test failures, no noticeable performance impact. Thanks a ton!!!
Depends on: 473906
Depends on: 474187
Could this have caused bug 477236?
Why did we make ToNearest/Inside/OutsidePixels static instead of member functions? That seems dumb to me in retrospect.
I don't think it was a conscious decision...  I originally had them as straight functions, like a lot of the stuff in nsCoord.h.  But I can think of one case where a static function is right:

nsRect r = nsIntRect::ToAppUnits(nsIntRect(x,y,w,h),p2a);

is it possible to do:

nsRect r = nsIntRect(x,y,w,h).ToAppUnits(p2a);?

The C++ FAQ suggests this might work: http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.18.  We should open another bug though.  I've been thinking about exploring a ATL style template class for unit conversions on the *Context classes:

template<class T>
nsUnitConverter {
  gfxFloat AppUnitsToGfxUnits(nscoord aCoord) {
    return nsAppUnitsToFloatPixels(aCoord,
       static_cast<T*>(this)->AppUnitsPerDevPixel());
  }
}

nsDevContext : nsUnitConverter<nsDeviceContext>
{
  nscoord AppUnitsPerDevPixel() { return mAppUnitsPerDevPixel(); }
}

I'll open another bug on this idea too.  But I don't want to do to much work in this area if the plan is to move the conversion down into the cairo matrix...
(In reply to comment #46)
> is it possible to do:
> 
> nsRect r = nsIntRect(x,y,w,h).ToAppUnits(p2a);?

It certainly is. I've filed bug 489485 on fixing this.

> template<class T>
> nsUnitConverter {
>   gfxFloat AppUnitsToGfxUnits(nscoord aCoord) {
>     return nsAppUnitsToFloatPixels(aCoord,
>        static_cast<T*>(this)->AppUnitsPerDevPixel());
>   }
> }
> 
> nsDevContext : nsUnitConverter<nsDeviceContext>
> {
>   nscoord AppUnitsPerDevPixel() { return mAppUnitsPerDevPixel(); }
> }
> 
> I'll open another bug on this idea too.

I'm not sure if that's all that useful, we shouldn't have too many unit converters.

> But I don't want to do to much work in
> this area if the plan is to move the conversion down into the cairo matrix...

We'll still need to convert in a lot of places, for example when we deal with native widget and screen coordinates.
Depends on: 503814
Depends on: 504617
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: