582719 - Use nsQueryFrame in menu/popup code

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

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.

Comment 1

[Neil Deakin]

Attachment: patch

Comment 2

[[e-mail]]

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?

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1ca7e8b00fbb

Comment 4

[[e-mail]]

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?

Comment 5

[Neil Deakin]

I checked in a change for that missed part. The GetParentMenu method isn't being used so I just removed it.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0363ed04a870

Comment 7

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336