Bug 422607: avoid unneeded pixel/twip conversions to fix bookmarks menu aligment issues, r=enndeakin, sr=roc

layout/generic/nsFrame.cpp

@@ -3477,7 +3477,20 @@ nsIntRect nsIFrame::GetScreenRectExternal() const
 
 nsIntRect nsIFrame::GetScreenRect() const
 {
-  nsIntRect retval(0,0,0,0);
+  nsRect r = GetScreenRectInAppUnits().ScaleRoundOutInverse(PresContext()->AppUnitsPerDevPixel());
+  // nsRect and nsIntRect are not necessarily the same
+  return nsIntRect(r.x, r.y, r.width, r.height);
+}
+
+// virtual
+nsRect nsIFrame::GetScreenRectInAppUnitsExternal() const
+{
+  return GetScreenRectInAppUnits();
+}
+
+nsRect nsIFrame::GetScreenRectInAppUnits() const
+{
+  nsRect retval(0,0,0,0);
   nsPoint toViewOffset(0,0);
   nsIView* view = GetClosestView(&toViewOffset);
 
@@ -3486,14 +3499,14 @@ nsIntRect nsIFrame::GetScreenRect() const
     nsIWidget* widget = view->GetNearestWidget(&toWidgetOffset);
 
     if (widget) {
-      nsRect ourRect = mRect;
-      ourRect.MoveTo(toViewOffset + toWidgetOffset);
-      ourRect.ScaleRoundOut(1.0f / PresContext()->AppUnitsPerDevPixel());
-      // Is it safe to pass the same rect for both args of WidgetToScreen?
-      // It's not clear, so let's not...
-      nsIntRect ourPxRect(ourRect.x, ourRect.y, ourRect.width, ourRect.height);
-      
-      widget->WidgetToScreen(ourPxRect, retval);
+      // WidgetToScreen really should take nsIntRect, not nsRect
+      nsIntRect localRect(0,0,0,0), screenRect;
+      widget->WidgetToScreen(localRect, screenRect);
+
+      retval = mRect;
+      retval.MoveTo(toViewOffset + toWidgetOffset);
+      retval.x += PresContext()->DevPixelsToAppUnits(screenRect.x);
+      retval.y += PresContext()->DevPixelsToAppUnits(screenRect.y);
     }
   }
 

layout/generic/nsIFrame.h

@@ -1509,12 +1509,19 @@ public:
   virtual nsPoint GetOffsetToExternal(const nsIFrame* aOther) const;
 
   /**
-   * Get the screen rect of the frame.
+   * Get the screen rect of the frame in pixels.
    * @return the pixel rect of the frame in screen coordinates.
    */
   nsIntRect GetScreenRect() const;
   virtual nsIntRect GetScreenRectExternal() const;
 
+  /**
+   * Get the screen rect of the frame in app units.
+   * @return the app unit rect of the frame in screen coordinates.
+   */
+  nsRect GetScreenRectInAppUnits() const;
+  virtual nsRect GetScreenRectInAppUnitsExternal() const;
+
   /**
    * Returns the offset from this frame to the closest geometric parent that
    * has a view. Also returns the containing view or null in case of error

layout/xul/base/src/nsMenuPopupFrame.cpp

@@ -950,7 +950,7 @@ nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
 
   // the screen rectangle of the anchor, or if null, the root frame, in dev pixels.
   nsRect anchorScreenRect;
-  nsRect rootScreenRect = rootFrame->GetScreenRect();
+  nsRect rootScreenRect = rootFrame->GetScreenRectInAppUnits();
 
   nsIDeviceContext* devContext = PresContext()->DeviceContext();
   nscoord offsetForContextMenu = 0;
@@ -961,9 +961,11 @@ nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
     // the most room. The combination of anchor and alignment dictate if we readjust 
     // above/below or to the left/right.
     if (mAnchorContent) {
-      anchorScreenRect = aAnchorFrame->GetScreenRect();
-      xpos = presContext->DevPixelsToAppUnits(anchorScreenRect.x - rootScreenRect.x);
-      ypos = presContext->DevPixelsToAppUnits(anchorScreenRect.y - rootScreenRect.y);
+      anchorScreenRect = aAnchorFrame->GetScreenRectInAppUnits();
+      // adjust for differences in the anchor frame's scaling
+      anchorScreenRect.ScaleRoundOut(adj);
+      xpos = anchorScreenRect.x - rootScreenRect.x;
+      ypos = anchorScreenRect.y - rootScreenRect.y;
 
       // move the popup according to the anchor and alignment. This will also tell us
       // which axis the popup is flush against in case we have to move it around later.
@@ -981,8 +983,8 @@ nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
     xpos += presContext->CSSPixelsToAppUnits(mXPos);
     ypos += presContext->CSSPixelsToAppUnits(mYPos);
 
-    screenViewLocX = presContext->DevPixelsToAppUnits(rootScreenRect.x) + xpos;
-    screenViewLocY = presContext->DevPixelsToAppUnits(rootScreenRect.y) + ypos;
+    screenViewLocX = rootScreenRect.x + xpos;
+    screenViewLocY = rootScreenRect.y + ypos;
   }
   else {
     // the popup is positioned at a screen coordinate.
@@ -1009,8 +1011,8 @@ nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
 
     // determine the x and y position by subtracting the desired screen
     // position from the screen position of the root frame.
-    xpos = screenViewLocX - presContext->DevPixelsToAppUnits(rootScreenRect.x);
-    ypos = screenViewLocY - presContext->DevPixelsToAppUnits(rootScreenRect.y);
+    xpos = screenViewLocX - rootScreenRect.x;
+    ypos = screenViewLocY - rootScreenRect.y;
   }
 
   // Compute info about the screen dimensions. Because of multiple monitor systems,
@@ -1030,7 +1032,6 @@ nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
 
   // for content shells, clip to the client area rather than the screen area
   if (mInContentShell) {
-    rootScreenRect.ScaleRoundIn(presContext->AppUnitsPerDevPixel());
     rect.IntersectRect(rect, rootScreenRect);
   }
 
@@ -1076,7 +1077,6 @@ nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
            screenViewLocX < screenLeftTwips ||
           (screenViewLocY + mRect.height) > screenBottomTwips ) {
       nsRect screenParentFrameRect(anchorScreenRect);
-      screenParentFrameRect.ScaleRoundOut(PresContext()->AppUnitsPerDevPixel());
 
       // figure out which side of the parent has the most free space so we can move/resize
       // the popup there. This should still work if the parent frame is partially screen.