282469 - Remove unnecessary menutobedisplayed

Status: RESOLVED WONTFIX

(Core :: XUL, defect)

Description

[Aaron Leventhal]

Follow up from bug 282438:

In nsPopupSetFame.cpp, pass mIsOpen to SyncViewWithFrame to avoid having touse
the menutobedisplayed attribute.

While we're in there it might be a good idea look at the possibility of removing
mShouldRollup and EnableRollup where they aren't used or necessary.

Comment 1

[Joshua Welderson]

Attachment: Fix

Taking and requesting review.

Comment 2

[[e-mail]]

Comment on attachment 200307 [details] [diff] [review]
Fix

>   nsresult SyncViewWithFrame(nsPresContext* aPresContext, const nsString& aPopupAnchor,
>                              const nsString& aPopupAlign,
>-                             nsIFrame* aFrame, PRInt32 aXPos, PRInt32 aYPos);
>+                             nsIFrame* aFrame, PRInt32 aXPos, PRInt32 aYPos,
>+                             PRBool shouldDisplay);
Might want to make shouldDisplay default to PR_FALSE (see below).

>-    // When we sync the popup view with the frame, we'll show the popup if |menutobedisplayed|
>+    // When we sync the popup view with the frame, we'll show the popup if mIsOpen
>     // is set by setting the |menuactive| attribute. This used to trip css into showing the menu
>     // but now we do it ourselves. 
>-    if (aActivateFlag)
>-      // XXXben hook in |width| and |height| usage here? 
>-      aEntry->mPopupContent->SetAttr(kNameSpaceID_None, nsXULAtoms::menutobedisplayed, NS_LITERAL_STRING("true"), PR_TRUE);
>-    else {
>+    if (!aActivateFlag) {
>       aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None, nsXULAtoms::menuactive, PR_TRUE);
>-      aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None, nsXULAtoms::menutobedisplayed, PR_TRUE);
What I don't understand is why we don't just set/unset menuactive here
directly, which is why I'm chickening out of the review.

>-      menuPopup->SyncViewWithFrame(mPresContext, popupAnchor, popupAlign, this, -1, -1);
>+      menuPopup->SyncViewWithFrame(mPresContext, popupAnchor, popupAlign, this, -1, -1, IsOpen());
>-    menuPopup->SyncViewWithFrame(presContext, popupAnchor, popupAlign, this, -1, -1);
>+    menuPopup->SyncViewWithFrame(presContext, popupAnchor, popupAlign, this, -1, -1, IsOpen());
As far as I can tell, the menu will already be active at this point, so it
makes no difference what you pass to the shouldDisplay argument.

Comment 3

[Joshua Welderson]

I have a new patch with some other stuff fixed (mShouldRollup has been removed,
although EnableRollUp doesn't seem to be unnecessary), as well as some of the
things you said.  But I'd like clarification on this point:

(In reply to comment #2)
> >+    if (!aActivateFlag) {
> >       aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None,
nsXULAtoms::menuactive, PR_TRUE);
> >-      aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None,
nsXULAtoms::menutobedisplayed, PR_TRUE);
> What I don't understand is why we don't just set/unset menuactive here
> directly, which is why I'm chickening out of the review.
Do you mean you don't understand why menuactive isn't being set on the aEntry,
or why it's not being set regardless of aActivateFlag, or something else?  I'm
not terribly familiar this part of the codebase, but I think all the present
code expects menuactive to be set on the menupopup, so I presume that's not what
you meant...

Comment 4

[[e-mail]]

(In reply to comment #3)
>>>+    if (!aActivateFlag) {
>>>       aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None,
nsXULAtoms::menuactive, PR_TRUE);
>>>-      aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None,
nsXULAtoms::menutobedisplayed, PR_TRUE);
>>What I don't understand is why we don't just set/unset menuactive here
>>directly, which is why I'm chickening out of the review.
>Do you mean you don't understand why menuactive isn't being set on the aEntry,
>or why it's not being set regardless of aActivateFlag, or something else?
I mean why does the code wait until the sync before setting the menuactive.

Comment 5

[Joshua Welderson]

Attachment: Fixes + mShouldRollUp

Fix addressing the most of Roc's comments and removing mShouldRollup. 
Hopefully Brian can clear up Neil's point.  I couldn't find any unnecessary
instances of EnableRollup, though, so that's not included in the patch.

Comment 6

[Joshua Welderson]

Er, Neil.  I'm bad with names.

Comment 7

[Joshua Welderson]

Brian, would you please take a look or so at this?

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 200381 [details] [diff] [review]
Fixes + mShouldRollUp

I don't know this code well enough to review, and I don't think I'll have time to come up to speed on it.

Comment 9

[Neil Deakin]

Popup changed make this bug obsolete