563878 - allow documents in the same view manager hierarchy to have different zoom

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Timothy Nikkel (:tnikkel)]

Layout is currently broken if two document in the same view manager hierarchy have different zoom values. When we link the chrome and content view manager hierarchies in bug 130078 we will need to support this.

Comment 1

[Timothy Nikkel (:tnikkel)]

I'm going to post some _very_ early patches that are disgusting, contain tons of junk that they shouldn't, and don't deserve to see the light of day.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: change APIs

view/ is only partially converted, and somewhat badly at that, some key determinations have yet to be made.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: displayzoom

This implements a nsDisplayZoom display item.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: fix various call sites

The actual fixes for places that assume constant-zoom are outnumbered by the general fixes I have come across reading the many many places that need to be audited. Those will have to be split out at some point.

Comment 5

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

+  //xxx this should remove opaque regions from avisibleregion, actually who cares?

it probably does matter. We want to be able to cull stuff that's hidden behind a zoomed document.

Comment 6

[Timothy Nikkel (:tnikkel)]

Oops, I added the "who cares" yesterday after not having touched that code for a while thinking it was talking about the scrolling analysis stuff with a visible region before move and so on, which you are reworking.

Comment 7

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

Instead of adding boolean parameters I think it's better to add a flags word, even if it's just one flag, because then we can use meaningful flag names in callers instead of PR_TRUE/PR_FALSE.

+  for (nsIFrame* f = aFrame; f;
+       f = aCrossDoc ?
+             nsLayoutUtils::GetCrossDocParentFrame(f) :
+             f->GetParent()) {

Seems like we should have an nsLayoutUtils::GetParent(nsIFrame* aFrame, PRUint32 aFlags) method.

+    //xxx we should abstract out a function to do this

yeah!

-  if (aRegion)
-    rc->SetClipRegion(*aRegion, nsClipCombine_kReplace);
+  if (aRegion) {
+    // Convert aRegion from CSS pixels to dev pixels
+    nsCOMPtr<nsIRegion> region =
+      do_CreateInstance("@mozilla.org/gfx/region/nsThebes;1");
+
+    nsRegionRectSet *rects = nsnull;
+    nsresult rv = aRegion->GetRects(&rects);
+    if (NS_SUCCEEDED(rv) && rects) {
+      for (PRUint32 i = 0; i < rects->mNumRects; i++) {
+        region->Union(pc->CSSPixelsToDevPixels((PRInt32)rects->mRects[i].x),
+                      pc->CSSPixelsToDevPixels((PRInt32)rects->mRects[i].y),
+                      pc->CSSPixelsToDevPixels((PRInt32)rects->mRects[i].width),
+                      pc->CSSPixelsToDevPixels((PRInt32)rects->mRects[i].height));
+      }
+
+      aRegion->FreeRects(rects);

Ick, factor this out. Better still, convert code from nsIRegion to nsRegion and add methods to nsRegion if necessary.

Comment 8

[Timothy Nikkel (:tnikkel)]

> Ick, factor this out. Better still, convert code from nsIRegion to nsRegion and
> add methods to nsRegion if necessary.

That is one of the changes will has nothing to do with this bug and will need to be split out. If I didn't draw the line somewhere with unrelated code cleanup that I come across in the course of this bug I would never finish.

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Misc cleanup.

The ClipListToRange change does not actually change the behaviour. We never descended before, but that was due to a lucky accident in using CompareNodeToRange incorrectly. The change makes us use CompareNodeToRange correctly and makes it clear that we don't descend into subdocuments.

The nsIRegion in PaintRangePaintInfo comes from a scriptable interface so it needs to be the scriptable nsIRegion. As for putting the conversion from CSS pixels to dev pixels code into ns(I)Region, that would require somehow passing the prescontext function CSSPixelsToDevPixels to the region, or duplicating that code.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. View cleanup.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Fix FindViewContaining.

FindViewContaining seems broken to me. Not really sure why it hasn't bit us.

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: Part 4. Add AppUnitsPerDevPixel convenience function to viewmanager.

I make more use of this function in later parts.

Comment 13

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

Would you like to investigate what it would take to remove the use of nsIRegion from this code altogether?

Comment 14

[Timothy Nikkel (:tnikkel)]

My patch queue for this bug is available at http://hg.mozilla.org/users/tnikkel_gmail.com/zoom-patches

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1a. Make nsThebesRegion hold a nsIntRegion like it should.

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1b. Make nsIPresShell::RenderNode and nsIRenderingContext::SetClipRegion take an nsIntRegion.

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1c. Misc cleanup.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 448443 [details] [diff] [review]
Part 1a. Make nsThebesRegion hold a nsIntRegion like it should.

In gfx/src/thebes/nsThebesRegion.cpp
> +nsIntRegion nsThebesRegion::GetUnderlyingRegion() const

nit: the style in this file has a space before (

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 448445 [details] [diff] [review]
Part 1b. Make nsIPresShell::RenderNode and nsIRenderingContext::SetClipRegion take an nsIntRegion.

In gfx/src/nsRegion.cpp
> +nsRegion nsIntRegion::ToAppUnits(nscoord aAppUnitsPerPixel) const {

nit: space before ( and move { to its own line

> +  const nsIntRect *currentRect;

nit: * goes with the type in this file

If you can, please also fix nsRegion::ToOutsidePixels
in the same way.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 448446 [details] [diff] [review]
Part 1c. Misc cleanup.

In layout/base/nsDisplayList.h
-   * @return the frame that the point is considered over, or nsnull if
-   * this is not over any frame
+   * @returns in aOutFrames the frame(s) that the rect is considered over, it
+   * may be empty if the rect is not over any frame

Would it be even more precise to say it *appends* frames and qualify
"is not over any frame" as "is not over any frame in this display item"?


In layout/base/nsPresShell.cpp
> +static PRBool gDumpRangePaintList = PR_TRUE;

I assume this debug stuff shouldn't be part of the patch.

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 448224 [details] [diff] [review]
Part 2. View cleanup.

In view/src/nsViewManager.cpp:
> nsViewManager::HandleEvent(nsView* aView, nsGUIEvent* aEvent) {

nit: move the { to the next line while you're here

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 448226 [details] [diff] [review]
Part 4. Add AppUnitsPerDevPixel convenience function to viewmanager.

In view/src/nsViewManager.h
> +#include "nsIDeviceContext.h"

Please remove the same in the .cpp file
 
+  inline PRInt32 AppUnitsPerDevPixel() const { return mContext->AppUnitsPerDevPixel(); }

I don't think we use explicit 'inline' in simple cases like this.
Also, 80 columns please.

r=mats

Comment 23

[Timothy Nikkel (:tnikkel)]

(In reply to comment #20)
> In layout/base/nsPresShell.cpp
> > +static PRBool gDumpRangePaintList = PR_TRUE;
> 
> I assume this debug stuff shouldn't be part of the patch.

I actually intended the debug stuff to be part of the patch, but not turned on of course. Is it okay if I keep the debug stuff but make gDumpRangePaintList = PR_FALSE?

Comment 24

[Mats Palmgren (inactive)]

Ok, r=mats on that part too then, with PR_FALSE.

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to comment #20)
> (From update of attachment 448446 [details] [diff] [review])
> In layout/base/nsDisplayList.h
> -   * @return the frame that the point is considered over, or nsnull if
> -   * this is not over any frame
> +   * @returns in aOutFrames the frame(s) that the rect is considered over, it
> +   * may be empty if the rect is not over any frame
> 
> Would it be even more precise to say it *appends* frames and qualify
> "is not over any frame" as "is not over any frame in this display item"?

I changed it to:
   * @param aOutFrames each item appends the frame(s) in this display item that
   * the rect is considered over (if any) to aOutFrames.

Comment 26

[Timothy Nikkel (:tnikkel)]

Made those changes and pushed parts 1a-c, 2, 3, and 4.
http://hg.mozilla.org/mozilla-central/rev/f2991bf41c20
http://hg.mozilla.org/mozilla-central/rev/d5ca465f4238
http://hg.mozilla.org/mozilla-central/rev/2c4a36b7e9ea
http://hg.mozilla.org/mozilla-central/rev/5702bf7ea7eb
http://hg.mozilla.org/mozilla-central/rev/83d0cd161be2
http://hg.mozilla.org/mozilla-central/rev/fdb1e4bc853d

Comment 27

[Timothy Nikkel (:tnikkel)]

Attachment: Part 5. Rename nsIFrame::GetWindow(Offset) to GetNearestWidget.

Rename this function to describe what is actually does. There are a couple of call sites that are confused about what this actually does. Those are fixed in later patches. This is just a straight forward rename patch.

Comment 28

[Timothy Nikkel (:tnikkel)]

Attachment: Part 6. Simplify ViewToWidget.

There is only one place that calls ViewToWidget, so we can simplify it.

The changes making sure we use the right view manager for a given view (and the assertions to that effect) will be important with bug 130078 when two viewmanagers in the same hierarchy have different appunits per devpixel ratios. And this does not fix all places where we need to do that.

Comment 29

[Timothy Nikkel (:tnikkel)]

Attachment: Part 7. Change the main frame, point, rect, and region APIs that we will need.

Comment 30

[Timothy Nikkel (:tnikkel)]

Attachment: Part 8. Remove workaround from ViewToWidgetOffset because it is not needed anymore.

This code was added in bug 408913. But I don't think it's needed anymore (and removing it simplifies later patches). ViewToWidgetOffset is only called on views with widgets, but now there are no views with widgets that are also rootviews with parent views. So I think this code never gets executed.

Comment 31

[Timothy Nikkel (:tnikkel)]

Attachment: Part 9. Remove nsIView::GetScreenPosition.

There are no users in the tree of nsIView::GetScreenPosition. Lets get rid of it.

This should have been part of the earlier view cleanup patch but I overlooked it.

Comment 32

[Timothy Nikkel (:tnikkel)]

Attachment: Part 10. Overhaul generic parts of view/ to handle non-constant zoom view manager hierarchies.

Comment 33

[Timothy Nikkel (:tnikkel)]

Attachment: Part 11. Rework the synth mouse move code to handle non-constant zoom.

Comment 34

[Timothy Nikkel (:tnikkel)]

Attachment: Part 12. Change the view observer interface and overhaul painting in view/.

Having both aDirtyRegion and aIntDirtyRegion as arguments on Paint seems a little much, but we compute both in view code, so we may as well pass them along instead of trying to reverse-engineer one to get the other.

Comment 35

[Timothy Nikkel (:tnikkel)]

Attachment: Part 13. Implement zoom display list item to handle document hierarchies where zoom is not constant.

Comment 36

[Mats Palmgren (inactive)]

Comment on attachment 451364 [details] [diff] [review]
Part 5. Rename nsIFrame::GetWindow(Offset) to GetNearestWidget.

>layout/generic/nsIFrame.h
>-   * Returns the window that contains this frame. If this frame has a
>+   * Returns the nearest widget containing this frame. If this frame has a
>    * view and the view has a window, then this frames window is

Might as well fix s/frames/frame's/ on the next line.

Comment 37

[Mats Palmgren (inactive)]

Comment on attachment 451369 [details] [diff] [review]
Part 6. Simplify ViewToWidget.

>view/src/nsViewManager.cpp
>+nsIntRect nsViewManager::ViewToWidget(...
>   // intersect aRect with bounds of aWidgetView, to prevent generating any illegal rectangles.

s/aWidgetView/aView/

I think we should document in nsViewManager.h that aRect is clipped
to the view rect before transforming it.  It seems like an
essential part of what ViewToWidget does.

Comment 38

[Mats Palmgren (inactive)]

Comment on attachment 451384 [details] [diff] [review]
Part 7. Change the main frame, point, rect, and region APIs that we will need.

>gfx/public/nsPoint.h
>+nsPoint::ConvertAppUnits(PRInt32 aFromAPP, PRInt32 aToAPP) {
>+  if (aFromAPP != aToAPP) {
>+    x = NSToCoordRound(float(x * aToAPP)/float(aFromAPP));
>+    y = NSToCoordRound(float(y * aToAPP)/float(aFromAPP));
>+  }
>+  return *this;

Should it be (float(x) * aToAPP) to avoid integer overflow?

>gfx/public/nsRect.h
>+  inline nsRect& ConvertAppUnits(PRInt32 aFromAPP, PRInt32 aToAPP,
>+                                 PRBool aRoundIn = PR_FALSE) {

Nit: remove the explicit "inline"

I'd prefer an enum instead of the PRBool to make the call sites
clear what they do without having to lookup the documentation
to see what true/false means. E.g.
  r.ConvertAppUnits(a, b, nsRect::eScaleOut);
(not having a default is better in this case I think)

>+    if (aRoundIn) {
>+      return ScaleRoundIn(float(aToAPP)/float(aFromAPP));
>+    } else {
>+      return ScaleRoundOut(float(aToAPP)/float(aFromAPP));
>+    }

Do we risk losing precision here?  It's different from how
nsPoint/nsSize::ConvertAppUnits do it.  How about:
float NSCoordScale(nscoord aCoord, PRInt32 aToAPP, PRInt32 aFromAPP)
{
  return (float(aCoord) * aToAPP) / aFromAPP;
}
and use that throughout for consistency?

>layout/generic/nsFrame.cpp
>+
>+  NS_ASSERTION(PresContext() == aOther->PresContext(),
>+               "GetOffsetTo called on frames in different documents");
>+
>+  //XXX sometime in the near future once we are confident that all GetOffsetTo
>+  // callers pass frames that are really in the same doc we can get rid of this
>+  // check.

I see a massive amount of these assertions. Please comment it out
until it's less noisy.

>+nsPoint nsIFrame::GetOffsetToCrossDoc(const nsIFrame* aOther, const PRInt32 aAPD) const

80 columns.

>+        docOffset.x = docOffset.y = 0;

I don't think "docOffset = nsPoint(0,0)" is any less efficient.

>+    if (f == aOther) {
>+      offset += docOffset.ConvertAppUnits(currAPD, aAPD);
>+      docOffset.x = docOffset.y = 0;
>+    }

I think you can move this block outside the |while| loop.
(the edge case of this==aOther will have docOffset=0,0)

>+  if (f != aOther) {

Make this an |else| in that case.


r=mats on part 5,6,7 with the above taken into consideration.

Comment 39

[Timothy Nikkel (:tnikkel)]

Attachment: Part 14. Fix GetCrossDocParentFrame callsites.

This comes from my audit of all GetCrossDocParentFrame call sites. In all cases but one it seemed basically okay that the call sites could cross content/chrome and zoom-change boundaries with bug 130078. The one case is PresShell::PageMove which currently doesn't handle crossing any document boundaries correctly.

The change in PresShell::GetCurrentItemAndPositionForElement means we might be using coordinates from a document with a different zoom, but that is fixed in the next part.

Comment 40

[Timothy Nikkel (:tnikkel)]

Attachment: Part 15. Fix GetOffsetTo callsites.

Comment 41

[Timothy Nikkel (:tnikkel)]

That is the last patch I have to post.

Comment 42

[Mats Palmgren (inactive)]

Comment on attachment 451423 [details] [diff] [review]
Part 10. Overhaul generic parts of view/ to handle non-constant zoom view manager hierarchies.

>view/public/nsIView.h
>-  virtual nsIWidget* GetNearestWidget(nsPoint* aOffset) const;
>+  virtual nsIWidget* GetNearestWidget(nsPoint* aOffset);

Why drop 'const' ?

>view/src/nsView.cpp
>+nsView::GetBoundsInParentUnits() const
>+{
>+  nsView* parent = GetParent();
>+  if (!mViewManager || this != mViewManager->GetRootView() || !parent ||
>+      !parent->mViewManager) {

I think 'mViewManager' is guarranteed to be non-null for the lifetime
of the view, so no need for those null-checks.
Use the GetViewManager() accessor?

>+nsView::ConvertFromParentCoords(nsPoint aPt) const
>+{
>+  nsViewManager* VM;
>+  nsViewManager* parentVM;
>+  PRInt32 APD, parentAPD;
>+  if (mParent &&
>+      (VM = GetViewManager()) != (parentVM = mParent->GetViewManager()) &&
>+      (APD = VM->AppUnitsPerDevPixel()) !=
>+        (parentAPD = parentVM->AppUnitsPerDevPixel())) {
>+    aPt.ConvertAppUnits(parentAPD, APD);
>+  }

Same here.  Also, why check VM != parentVM ? or APD != parentAPD ?
(ConvertAppUnits does the latter inline)
I think you can just write this as:
  if (mParent)
    aPt.ConvertAppUnits(mParent->GetViewManager()->AppUnitsPerDevPixel(),
                        GetViewManager()->AppUnitsPerDevPixel());

>view/src/nsView.h
>-   * or to the left of its origin position.
>+   * or to the left of its origin position. The term 'dimensions' indicates it
>+   * is relative to this.

s/this/|this|/ or s/this/this view/ would make it clearer.

>+  nsPoint GetOffsetTo(const nsView* aOther, PRInt32* aAPD = nsnull) const;
>+  nsIWidget* GetNearestWidget(nsPoint* aOffset, PRInt32* aAPD = nsnull);

Why not use "const PRInt32 aAPD" for both of those?  And add
nsPoint nsView::GetOffsetTo(const nsView* aOther) const
{
  return GetOffsetTo(aOther, GetViewManager()->AppUnitsPerDevPixel());
}
for convenience.

>view/src/nsViewManager.cpp
> NS_IMETHODIMP nsViewManager::ResizeView(...
>+  nsRect oldBounds = view->GetBoundsInParentUnits();

You can move this to the top of the |else| that uses it, like so:

    } else {
      nsRect oldBounds = view->GetBoundsInParentUnits();
      view->SetDimensions(aRect, PR_TRUE);
      if (!aRepaintExposedAreaOnly) {
        //Invalidate the union of the old and new size
        UpdateView(view, aRect, NS_VMREFRESH_NO_SYNC);
        UpdateView(parentView, oldBounds, NS_VMREFRESH_NO_SYNC);
      } else {
        InvalidateRectDifference(...

Comment 43

[Mats Palmgren (inactive)]

Comment on attachment 451443 [details] [diff] [review]
Part 13. Implement zoom display list item to handle document hierarchies where zoom is not constant.

>+  if (aRect.width == 1 && aRect.height == 1) {
>+    rect.MoveTo(aRect.TopLeft().ConvertAppUnits(mParentAPD, mAPD));
>+    rect.width = rect.height = 1;

Please explain why this case is special.

>+class nsDisplayZoom : public nsDisplayWrapList {
>+public:
>+  /**
>+   * aFrame is the root frame of the subdocument. aList contains the display
>+   * items for the subdocument. aAPD is the app units per dev pixel ratio of
>+   * the subdocument. aParentAPD is the app units per dev pixel ratio of the
>+   * parent document.
>+   */

Start a new line for each sentence and prefix them with @param.

Comment 44

[Timothy Nikkel (:tnikkel)]

(In reply to comment #36)
> (From update of attachment 451364 [details] [diff] [review])
> >layout/generic/nsIFrame.h
> >-   * Returns the window that contains this frame. If this frame has a
> >+   * Returns the nearest widget containing this frame. If this frame has a
> >    * view and the view has a window, then this frames window is
> 
> Might as well fix s/frames/frame's/ on the next line.

Done. I also did s/window/widget/ in this comment.

Comment 45

[Timothy Nikkel (:tnikkel)]

(In reply to comment #37)
> (From update of attachment 451369 [details] [diff] [review])
> >view/src/nsViewManager.cpp
> >+nsIntRect nsViewManager::ViewToWidget(...
> >   // intersect aRect with bounds of aWidgetView, to prevent generating any illegal rectangles.
> 
> s/aWidgetView/aView/
> 
> I think we should document in nsViewManager.h that aRect is clipped
> to the view rect before transforming it.  It seems like an
> essential part of what ViewToWidget does.

Done.

Comment 46

[Timothy Nikkel (:tnikkel)]

(In reply to comment #38)
> Should it be (float(x) * aToAPP) to avoid integer overflow?

Switched to this.

> >gfx/public/nsRect.h
> >+  inline nsRect& ConvertAppUnits(PRInt32 aFromAPP, PRInt32 aToAPP,
> >+                                 PRBool aRoundIn = PR_FALSE) {
> 
> Nit: remove the explicit "inline"

Done.

> I'd prefer an enum instead of the PRBool to make the call sites
> clear what they do without having to lookup the documentation
> to see what true/false means. E.g.
>   r.ConvertAppUnits(a, b, nsRect::eScaleOut);
> (not having a default is better in this case I think)

Switched to an enum. I left the default as round out, because there is only one use of rounding in in all my patches. If you'd still prefer no default argument there I can remove it.

> >+    if (aRoundIn) {
> >+      return ScaleRoundIn(float(aToAPP)/float(aFromAPP));
> >+    } else {
> >+      return ScaleRoundOut(float(aToAPP)/float(aFromAPP));
> >+    }
> 
> Do we risk losing precision here?  It's different from how
> nsPoint/nsSize::ConvertAppUnits do it.  How about:
> float NSCoordScale(nscoord aCoord, PRInt32 aToAPP, PRInt32 aFromAPP)
> {
>   return (float(aCoord) * aToAPP) / aFromAPP;
> }
> and use that throughout for consistency?

Done. But note that I made the order "PRInt32 aFromAPP, PRInt32 aToAPP" for consistency with all the other functions.

> I see a massive amount of these assertions. Please comment it out
> until it's less noisy.

Sorry. In my mind these patches were part of one operation. I commented the assertion out in this part and enabled it in the last part.

> >+nsPoint nsIFrame::GetOffsetToCrossDoc(const nsIFrame* aOther, const PRInt32 aAPD) const
> 
> 80 columns.

Done.

> >+        docOffset.x = docOffset.y = 0;
> 
> I don't think "docOffset = nsPoint(0,0)" is any less efficient.

Done.

> >+    if (f == aOther) {
> >+      offset += docOffset.ConvertAppUnits(currAPD, aAPD);
> >+      docOffset.x = docOffset.y = 0;
> >+    }
> 
> I think you can move this block outside the |while| loop.
> (the edge case of this==aOther will have docOffset=0,0)
> 
> >+  if (f != aOther) {
> 
> Make this an |else| in that case.

Done and done.

Comment 47

[Timothy Nikkel (:tnikkel)]

Attachment: Part 7 v2. Change the main frame, point, rect, and region APIs that we will need.

Comment 48

[Timothy Nikkel (:tnikkel)]

Attachment: Part 16. Make nsMenuPopupFrame use the new API to convert app units.

Just noticed this existing code that converts between app units. It can use the new API instead.

Comment 49

[Timothy Nikkel (:tnikkel)]

(In reply to comment #42)
> Why drop 'const' ?

It was left over from some churn and I forgot to remove it.

> I think 'mViewManager' is guarranteed to be non-null for the lifetime
> of the view, so no need for those null-checks.
> Use the GetViewManager() accessor?

Done.

> Same here.  Also, why check VM != parentVM ? or APD != parentAPD ?
> (ConvertAppUnits does the latter inline)
> I think you can just write this as:
>   if (mParent)
>     aPt.ConvertAppUnits(mParent->GetViewManager()->AppUnitsPerDevPixel(),
>                         GetViewManager()->AppUnitsPerDevPixel());

Done.

> s/this/|this|/ or s/this/this view/ would make it clearer.

Done.

> Why not use "const PRInt32 aAPD" for both of those?  And add
> nsPoint nsView::GetOffsetTo(const nsView* aOther) const
> {
>   return GetOffsetTo(aOther, GetViewManager()->AppUnitsPerDevPixel());
> }
> for convenience.

Done.

> You can move this to the top of the |else| that uses it, like so:
> 
>     } else {
>       nsRect oldBounds = view->GetBoundsInParentUnits();
>       view->SetDimensions(aRect, PR_TRUE);
>       if (!aRepaintExposedAreaOnly) {
>         //Invalidate the union of the old and new size
>         UpdateView(view, aRect, NS_VMREFRESH_NO_SYNC);
>         UpdateView(parentView, oldBounds, NS_VMREFRESH_NO_SYNC);
>       } else {
>         InvalidateRectDifference(...

Done.

Comment 50

[Timothy Nikkel (:tnikkel)]

Attachment: Part 10 v2. Overhaul generic parts of view/ to handle non-constant zoom view manager hierarchies.

Comment 51

[Timothy Nikkel (:tnikkel)]

(In reply to comment #43)
> (From update of attachment 451443 [details] [diff] [review])
> >+  if (aRect.width == 1 && aRect.height == 1) {
> >+    rect.MoveTo(aRect.TopLeft().ConvertAppUnits(mParentAPD, mAPD));
> >+    rect.width = rect.height = 1;
> 
> Please explain why this case is special.

I added this comment:

  // A 1x1 rect indicates we are just hit testing a point, so pass down a 1x1
  // rect as well instead of possibly rounding the width or height to zero.

> Start a new line for each sentence and prefix them with @param.

Done.

Comment 52

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

Comment on attachment 451439 [details] [diff] [review]
Part 12. Change the view observer interface and overhaul painting in view/.

Looks fine but will require updating after patches in bug 564991 land

Comment 53

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

Comment on attachment 452614 [details] [diff] [review]
Part 7 v2. Change the main frame, point, rect, and region APIs that we will need.

Lets make ConvertAppUnits functional, like the To... methods.

+  nsRect& ConvertAppUnits(PRInt32 aFromAPP, PRInt32 aToAPP,
+                          RectRounding aRound = eRoundOut);

Don't use a default parameter here. Let's make it clear in the caller what's happening.

But why not just have two methods here?

Comment 54

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

Comment on attachment 451443 [details] [diff] [review]
Part 13. Implement zoom display list item to handle document hierarchies where zoom is not constant.

+  subdocBoundsInParentUnits.ConvertAppUnits(subdocAPD, parentAPD);
+
   // Get the bounds of subdocView relative to the reference frame.
-  nsRect shellBounds = subdocView->GetBounds() +
-                       mInnerView->GetPosition() +
-                       GetOffsetTo(aBuilder->ReferenceFrame());
+  subdocBoundsInParentUnits = subdocBoundsInParentUnits +
+                              mInnerView->GetPosition() +
+                              GetOffsetToCrossDoc(aBuilder->ReferenceFrame());

So, subdocBoundsInParentUnits is in the appunits of the parent document but is relative to the builder's reference frame, whose appunits might not be the same as this parent document's?

Comment 55

[Timothy Nikkel (:tnikkel)]

(In reply to comment #54)
> So, subdocBoundsInParentUnits is in the appunits of the parent document but is
> relative to the builder's reference frame, whose appunits might not be the same
> as this parent document's?

Correct. That is how everything works in painting, coordinates are in the appunits of the local document, but relative to the reference frame (which might be in a different document and have different zoom). In this case it's a little more complicated in that we might have three zoom's involved, but the principle is the same.

Comment 56

[Damon Sicore (:damons)]

Doesn't block anything for the Firefox team for the first beta.  Moving to beta 2.

Comment 57

[Mats Palmgren (inactive)]

(In reply to comment #46)
> I commented the assertion out in this part and enabled it in the last part.

Ok, I didn't know there were more patches coming that would fix it.
With the third batch of patches the assertion is mostly silent, so we can
land it enabled.  Note that it still occurs though, so we'll need a follow-up
bug on that.

Comment 58

[Mats Palmgren (inactive)]

Comment on attachment 452107 [details] [diff] [review]
Part 14. Fix GetCrossDocParentFrame callsites.

I have a feeling that if you're asking for a frame to scroll but
limiting the ancestor walk to the same document, then you're probably
doing something wrong.  As far as I can tell you only use this for
in PresShell::PageMove?  Can we make nsFrameSelection::CommonPageMove
work across document boundaries instead?

r=mats as there's nothing wrong with the code per se, but I would be
happier if we could avoid changing the API's here.

Comment 59

[Mats Palmgren (inactive)]

Comment on attachment 452131 [details] [diff] [review]
Part 15. Fix GetOffsetTo callsites.

oops, wasn't quite done with this review yet...

Comment 60

[Mats Palmgren (inactive)]

Comment on attachment 452620 [details] [diff] [review]
Part 10 v2. Overhaul generic parts of view/ to handle non-constant zoom view manager hierarchies.

Looks great, thanks.

Comment 61

[Mats Palmgren (inactive)]

Comment on attachment 452131 [details] [diff] [review]
Part 15. Fix GetOffsetTo callsites.

> dom/base/nsDOMWindowUtils.h
+  // widget returned by GetWidget..

Double dot typo?

Comment 62

[Timothy Nikkel (:tnikkel)]

(In reply to comment #57)
> Ok, I didn't know there were more patches coming that would fix it.
> With the third batch of patches the assertion is mostly silent, so we can
> land it enabled.  Note that it still occurs though, so we'll need a follow-up
> bug on that.

That is surprising. Can you get a stack of when it happens? I did go through every GetOffsetTo call. I don't want to land this with known asserts happening.

Comment 63

[Timothy Nikkel (:tnikkel)]

(In reply to comment #58)
> I have a feeling that if you're asking for a frame to scroll but
> limiting the ancestor walk to the same document, then you're probably
> doing something wrong.  As far as I can tell you only use this for
> in PresShell::PageMove?

Correct.

> Can we make nsFrameSelection::CommonPageMove
> work across document boundaries instead?
> 
> r=mats as there's nothing wrong with the code per se, but I would be
> happier if we could avoid changing the API's here.

Okay. If you agree, here is what I think we should do. Scrap this patch and file a separate bug for fixing nsFrameSelection::CommonPageMove. Bug 130078 would only make this bug "worse" in the unlikely case of a PageMove happening in unscrollable content that gets propagated to a scrollable in chrome.

Comment 64

[Timothy Nikkel (:tnikkel)]

Attachment: Part 7 v3. Change the main frame, point, rect, and region APIs that we will need.

Updated to comments.

Comment 65

[Timothy Nikkel (:tnikkel)]

Attachment: Part 17. Use nsIFrame::GetNearestWidget to simplify some code.

I just happened to come across this.

Comment 66

[Mats Palmgren (inactive)]

Comment on attachment 453247 [details] [diff] [review]
Part 17. Use nsIFrame::GetNearestWidget to simplify some code.

r=mats

Comment 67

[Mats Palmgren (inactive)]

Comment on attachment 453246 [details] [diff] [review]
Part 7 v3. Change the main frame, point, rect, and region APIs that we will need.

>layout/generic/nsFrame.cpp
>+  if (f == aOther) {
>+    offset += docOffset.ConvertAppUnits(currAPD, aAPD);
>+    docOffset = nsPoint(0, 0);
>+  } else {

The assignment to docOffset above is unnecessary.

Comment 68

[Mats Palmgren (inactive)]

(In reply to comment #62)
> That is surprising. Can you get a stack of when it happens?

Nevermind, I can't reproduce it anymore.

Comment 69

[Mats Palmgren (inactive)]

(In reply to comment #63)
> Scrap this patch and
> file a separate bug for fixing nsFrameSelection::CommonPageMove.

Yeah, let's do that.

Comment 70

[Timothy Nikkel (:tnikkel)]

Landed part 8 and part 9
http://hg.mozilla.org/mozilla-central/rev/2b45b05d003e
http://hg.mozilla.org/mozilla-central/rev/474d481c0465

Comment 71

[Timothy Nikkel (:tnikkel)]

Landed part 5 and part 6
http://hg.mozilla.org/mozilla-central/rev/f8c796eea626
http://hg.mozilla.org/mozilla-central/rev/62b8319818b1

Comment 72

[Timothy Nikkel (:tnikkel)]

Landed parts 7, 10, 11, 12, 13, 15, 16, 17
http://hg.mozilla.org/mozilla-central/rev/9c9ee87d42fa
http://hg.mozilla.org/mozilla-central/rev/712cbadfd661
http://hg.mozilla.org/mozilla-central/rev/a85ea14c92bf
http://hg.mozilla.org/mozilla-central/rev/8f2143a0e3d0
http://hg.mozilla.org/mozilla-central/rev/890fb263cf64
http://hg.mozilla.org/mozilla-central/rev/ea97d39a1036
http://hg.mozilla.org/mozilla-central/rev/67ca71d33ed2
http://hg.mozilla.org/mozilla-central/rev/3c910a1eadf9

Comment 73

[Boris Zbarsky [:bzbarsky]]

The push from comment 72 caused a 5% regression on Dromaeo CSS on Win7 and XP.  Was it a particular test that got slower, or just across the board?  Any idea why things got slower at all?

Comment 74

[Timothy Nikkel (:tnikkel)]

Not sure why this regressed.

Numbers from before and after
dojo: 1617 -> 1396
ext: 5025 -> 4714
jquery 2645 -> 2601
mootools: 2177 -> 2180
prototype: 1312 -> 1150
yui: 1152 -> 1126

So they all regressed except mootools had a very slight improvement.

Comment 75

[Boris Zbarsky [:bzbarsky]]

What about the subtests under those?  Let's start by looking at dojo, since that's the one with the biggest slowdown, looks like...

Comment 76

[Boris Zbarsky [:bzbarsky]]

We may want a bug tracking the regression.

Comment 77

[Timothy Nikkel (:tnikkel)]

The bug tracking the regression is bug 579963.