Use nsQueryFrame in menu/popup code

RESOLVED FIXED in mozilla17

Status

()

Core
XP Toolkit/Widgets: XUL
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

42.69 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
The menu/popup code uses patterns like the following to convert an nsIFrame into a specific frame type:

if (frame && frame->GetType() == nsGkAtoms::menuPopupFrame) {
   nsMenuPopupFrame* popupFrame = static_cast<nsMenuPopupFrame *>(frame);
   ...
} 

Convert to use do_QueryFrame instead.
(Assignee)

Comment 1

5 years ago
Created attachment 645750 [details] [diff] [review]
patch
Attachment #645750 - Flags: review?(neil)

Comment 2

5 years ago
Comment on attachment 645750 [details] [diff] [review]
patch

>diff --git a/layout/xul/base/src/nsMenuBarFrame.cpp b/layout/xul/base/src/nsMenuBarFrame.cpp

>   if (foundMenu) {
>-    return (foundMenu->GetType() == nsGkAtoms::menuFrame) ?
>-           static_cast<nsMenuFrame *>(foundMenu) : nsnull;
>+    nsMenuFrame* menuFrame = do_QueryFrame(foundMenu);
>+    return menuFrame;
>   }
Later on you started using return do_QueryFrame, which looks as if it should work here too.

>diff --git a/layout/xul/base/src/nsMenuFrame.cpp b/layout/xul/base/src/nsMenuFrame.cpp

> void
> nsMenuFrame::InitMenuParent(nsIFrame* aParent)
> {
>   while (aParent) {
>+    nsMenuPopupFrame* popup = do_QueryFrame(aParent);
>+    if (popup) {
>+      mMenuParent = popup;
>       break;
>     }
>+    else {
>+      nsMenuBarFrame* menubar = do_QueryFrame(aParent);
>+      if (menubar) {
>+        mMenuParent = menubar;
>+        break;
>+      }
>     }
>     aParent = aParent->GetParent();
>   }
> }
Don't use else after break; r=me with that fixed.

I guess making do_QueryFrame work for nsMenuParent frames wasn't worthwhile just for this one loop.
void nsMenuFrame::InitMenuParent(nsIFrame* aParent)
{
  while (aParent) {
    mMenuParent = do_QueryFrame(aParent);
    if (mMenuParent)
      break;
    aParent = aParent->GetParent();
  }
}

>diff --git a/layout/xul/base/src/nsPopupBoxObject.cpp b/layout/xul/base/src/nsPopupBoxObject.cpp

>-  nsMenuPopupFrame* GetMenuPopupFrame()
>-  {
>-    nsIFrame* frame = GetFrame(false);
>-    if (frame && frame->GetType() == nsGkAtoms::menuPopupFrame)
>-      return static_cast<nsMenuPopupFrame*>(frame);
>-    return nsnull;
>-  }
I was looking into the considerations for whether or not to inline GetParentMenu, GetMenuPopupFrame or GetMenuFrameForContent.
GetMenuFrameForContent only has two callers, so it seems reasonable to remove it.
Both GetParentMenu and GetMenuPopupFrame have eight callers. However I guess you chose to keep GetParentMenu because it's part of the API for nsMenuPopupFrame?
Attachment #645750 - Flags: review?(neil) → review+

Comment 3

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1ca7e8b00fbb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 4

5 years ago
Comment on attachment 645750 [details] [diff] [review]
patch

>diff --git a/layout/xul/base/src/nsMenuPopupFrame.h b/layout/xul/base/src/nsMenuPopupFrame.h
>--- a/layout/xul/base/src/nsMenuPopupFrame.h
>+++ b/layout/xul/base/src/nsMenuPopupFrame.h
>@@ -206,21 +206,17 @@ public:
>   nsPopupType PopupType() const { return mPopupType; }
>   bool IsMenu() { return mPopupType == ePopupTypeMenu; }
>   bool IsOpen() { return mPopupState == ePopupOpen || mPopupState == ePopupOpenAndVisible; }
> 
>   bool IsDragPopup() { return mIsDragPopup; }
> 
>   // returns the parent menupopup, if any
>   nsMenuFrame* GetParentMenu() {
>-    nsIFrame* parent = GetParent();
>-    if (parent && parent->GetType() == nsGkAtoms::menuFrame) {
>-      return static_cast<nsMenuFrame *>(parent);
>-    }
>-    return nsnull;
>+    return do_QueryFrame(GetParent());
>   }
This change didn't land for some reason?
(Assignee)

Comment 5

5 years ago
I checked in a change for that missed part. The GetParentMenu method isn't being used so I just removed it.
https://hg.mozilla.org/mozilla-central/rev/0363ed04a870
You need to log in before you can comment on or make changes to this bug.