Last Comment Bug 582719 - Use nsQueryFrame in menu/popup code
: Use nsQueryFrame in menu/popup code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Neil Deakin (away until Oct 3)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-28 13:03 PDT by Neil Deakin (away until Oct 3)
Modified: 2012-08-24 20:03 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (42.69 KB, patch)
2012-07-25 06:49 PDT, Neil Deakin (away until Oct 3)
neil: review+
Details | Diff | Splinter Review

Description Neil Deakin (away until Oct 3) 2010-07-28 13:03:33 PDT
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 (away until Oct 3) 2012-07-25 06:49:45 PDT
Created attachment 645750 [details] [diff] [review]
patch
Comment 2 neil@parkwaycc.co.uk 2012-07-25 14:31:36 PDT
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] 2012-07-31 06:10:17 PDT
https://hg.mozilla.org/mozilla-central/rev/1ca7e8b00fbb
Comment 4 neil@parkwaycc.co.uk 2012-08-01 13:20:03 PDT
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 (away until Oct 3) 2012-08-24 05:51:37 PDT
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] 2012-08-24 20:03:23 PDT
https://hg.mozilla.org/mozilla-central/rev/0363ed04a870

Note You need to log in before you can comment on or make changes to this bug.