Use nsQueryFrame in menu/popup code

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

9 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

7 years ago
Posted patch patchSplinter Review
Attachment #645750 - Flags: review?(neil)
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+
https://hg.mozilla.org/mozilla-central/rev/1ca7e8b00fbb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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

7 years ago
I checked in a change for that missed part. The GetParentMenu method isn't being used so I just removed it.
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.