306613 - Can't add pref via about:config / about:config contextual menu missing

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect, P3)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

In Fx, you can ctrl-click on an about:config entry and get a contextual menu,
which allows you to create a new pref (for instance, to add
permissions.default.script, which does not exist by default), to lock a given
setting, to reset a setting, to modify a setting, to toggle a setting, copy name
or copy value.  (The latter four seem somewhat superfluous, esp. as double-click
already toggles or modifies, depending on the pref type.)  Camino shows the page
contextual menu instead.

These are useful (esp for QA!) and allow modifications and additions without the
quit-edit-restart merry-go-round we currently have in Camino for some prefs.

Comment 1

[Amir Caspi]

This bug is still present in v1.5 ... and it really should be fixed.  The only way to add prefs right now is via manual editing of prefs.js.  Firefox allows on-the-fly adding via the contextual menu; Camino should adopt this menu as well.

Comment 2

[Stuart Morgan]

Please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html before commenting in bugs.

Comment 3

[Amir Caspi]

Stuart, apologies if it sounds like I violated rule #2, that was unintentional.  My intent was merely to report that the bug was still present in the current build (which is IMHO a valid comment).

Comment 4

[Chris Lawson (gone)]

Commenting that "this bug is still a bug" when the bug is not RESOLVED FIXED is less than useful. That's why Bugzilla has resolutions in the first place -- so that people can see when a bug is resolved. If the bug is not marked RESOLVED, it's understood that it's still a problem.

Attempting to justify a failure to follow Bugzilla etiquette by further violating it is not helpful.

cl

Comment 5

[Chris Lawson (gone)]

I've been looking into this tonight, and I noticed we don't show the page menu at all any more. I think the reason for that is the checkin for bug 298458, since the page menu disappeared around the time that was checked in (to within about 48 hours).
Simon, is there any simple way to fix this without re-breaking JavaScript context menus? I assume about:config has its own oncontextmenu handler (or some analogous handler as far as nsIContextMenuListener is concerned) in the XUL that's causing this problem.

cl

Comment 6

[Chris Lawson (gone)]

Pretty sure Pink isn't working on this (if he ever was, which I also doubt). But I'd be happy to take it since I'm already working on it.

Comment 7

[Chris Lawson (gone)]

Attachment: one possible approach

OK, I can get us to show a menu on the page again by special-casing the about:config URI.

Before I go a lot further on this, can someone tell me if that's a legitimate approach? This patch outlines (roughly) what I'm planning. My tree is dirty and it probably won't apply, but it's the quickest and easiest way I could think of to explain my plans.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: 1.9a context menu

FWIW, this is what you get in Minefield/GranPardiso these days.

Comment 9

[Simon Fraser [no longer active]]

The about:config url checking is a nasty hack. Chris, you should figure out why we don't get an OnContextMenu callback for XUL content; maybe they fire some other kind of event we should look for?

Comment 10

[Håkan Waara]

I think I found the bug. 

1) When you context-click in the contentview, nsIDocShellTreeOwner is responsible for notifying embeddors, and that works fine. (http://mxr.mozilla.org/mozilla/source/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp#1656)

2) But when a XUL widget gets the context-click - which is very unusual in our case, since we try to avoid any contact with XUL interfaces, except for the about:config case - then there's some other code that should notify embeddors.

I think the problem is that it doesn't, at all - AFAICS. :-). See http://mxr.mozilla.org/mozilla/source/content/xul/content/src/nsXULPopupListener.cpp#210

Hopefully, the fix is simple - just add an equivalent notification in case #2. This would have to wait until after Enn is done in bug 279703

Comment 11

[Chris Lawson (gone)]

Håkan -- now that the patch in 279703 landed, can you maybe take another look at this sometime soon?

Comment 12

[Håkan Waara]

Enn, is there some nsIContextMenuListener equivalent for XUL popups?

Right now we're notified of context-clicks in the content view for web pages, but not for XUL documents (e.g. about:config) because they go through another code flow which doesn't notify embeddors (see comment 10).

Any suggestions?

Comment 13

[Neil Deakin]

Aren't you loading about:config in a content window? Both 1 and 2 use a contextmenu event, and this event should fire regardless. Do you need to just respond to the context menu event, or handle it after checking the xul context attribute and a xul menupopup has been found?

Comment 14

[Chris Lawson (gone)]

I'm certainly not working on this at the moment, and I sort of doubt Håkan is either, unfortunately.

Comment 15

[Christopher Henderson]

Attachment: Fix v1.3

OK, here we go. Adds a listener for the xul popup event. Adds functionality to BWC's context menu method to parse a <popup> node. The parser creates XULMenuItems, which is an NSMenuItem subclass that encapsulates each menu node. Each XULMenuItem handles its own command event, which what allows us to get the functionality of the items (new pref, reset, etc).

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 412096 [details] [diff] [review]
Fix v1.3

cl is supposed to be reviewing this.

Comment 17

[Chris Lawson (gone)]

Subject to bug 528379, which means we can't create new BOOL prefs yet, this works as expected. There's one instance of whitespace on a blank line in the existing patch, which hendy has already fixed locally. Insofar as I'm qualified to review this mostly-Gecko code, it looks good to me.

Comment 18

[Chris Lawson (gone)]

Comment on attachment 412096 [details] [diff] [review]
Fix v1.3

r+ subject to the whitespace fix (forthcoming patch v1.4)

Comment 19

[Christopher Henderson]

Attachment: Fix v1.4

Fixes the whitespace issue.

Comment 20

[Stuart Morgan]

Comment on attachment 416472 [details] [diff] [review]
Fix v1.4

>+@interface XULMenuItem : NSMenuItem

Give the new class its own files, rather than adding it here.

>+    nsAutoString nodeName;
>+    if (NS_FAILED(mDataOwner->mContextMenuNode->GetNodeName(nodeName)))
>+      return nil;

Dying out of the whole method seems wrong. Isn't the only actualy effect of failing here that we can't then do the subsequent XUL node check?

>+    XULMenuItem* theXULItem = [[[XULMenuItem alloc] initWithMenuItem:nodeElement] autorelease];

You may as well create a convenience autoreleased creation method for this.

>+      [theMenu insertItem:theXULItem atIndex:[theMenu numberOfItems]];

This will fail if there's a duplicate title. Is potentially losing entries a problem?

>+    NSMenu* subMenu = nil;
>+    if (nodeNameString.Equals(NS_LITERAL_STRING("menu"))) {
>+      subMenu = [self menuFromNode:node];
>+    }
>+    if (subMenu)
>+      [theMenu setSubmenu:subMenu forItem:theXULItem];

Seems like this would be more straightforward as:
  if (...) {
    NSMenu* subMenu = ...
    if (submenu)
      ...
  }


General note about the whole menuFromNode: method: it has nothing to do with BWC, so it would probably be better as a category on NSMenu (maybe in the same file as XULMenuItem?).
    
>+  if ( (self = [super init]) ) {

Remove the extra spaces in the parens.

>+    mMenuItem = anItem;
>+    NS_IF_ADDREF(mMenuItem);

Shouldn't you just autorelease and return nil if mMenuItem is NULL?

>+    [self setKeyEquivalent:@""];

Why? This should be the default state.

> ... [[self command] isEqualToString:@""] ...

This is the only use I can find of command; why a public, string-returning method that's only ever really used as a boolean value, internally?

>+    [self setAction:shouldEnableItem ? @selector(performCommand) : NULL];

I find complex obj-c method arguments like ?: blocks to be clearer if you put parens around the whole thing. Although since NULL is presumably the default, this could just be a simple set conditioned on shouldEnableItem.

>+  if (mMenuItem && NS_SUCCEEDED(mMenuItem->GetAttribute(NS_LITERAL_STRING("hidden"), nodeHidden)))
>+    result = [[NSString stringWith_nsAString:nodeHidden] isEqualToString:@"true"];
...
>+  if (mMenuItem && NS_SUCCEEDED(mMenuItem->GetAttribute(NS_LITERAL_STRING("disabled"), nodeDisabled))) {
>+    result = ![[NSString stringWith_nsAString:nodeDisabled] isEqualToString:@"true"];

This code is identical except for the attribute name; it's begging for a little helper method that takes an attribute name and returns a BOOL.

>+  if (!mMenuItem)
>+    return;
>+ 
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mMenuItem)); 
>+  if (!content)
>+    return;

You have several pairs like this that can be made much shorter; it's safe to d_QI a null pointer (you get null back), and you don't care about the intermediate value, so you can skip the check on it.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

P3 on the 2.1 list.

Comment 22

[Christopher Henderson]

Attachment: Fix v1.5

Patch addressing the sr comments. I don't think we need to worry about duplicate titles.

Comment 23

[Stuart Morgan]

Comment on attachment 427443 [details] [diff] [review]
Fix v1.5

Sorry, one more round of comments.

>+ * The Original Code is mozilla.org code.

I think we prefer "is Camino code" for new files.

>+//
>+// XULMenuItem
>+//
>+// NSMenuItem subclass for wrapping a XUL <menuitem> element.
>+//
>+//

At most one trailing blank comment line. FYI, if you like this style (empty lines around the comment, repeating the class/method name in the comment) you can use it, but for new files it's not at all required; it's certainly not universal in the code (and I personally dislike it ;) ).

>+  nsIDOMElement* mMenuItem;   // strong

Having a menu item member of a menu item class is a bit confusing (same with all the method names below). How about mMenuItemElement?

>++ (id)itemWithMenuItem:(nsIDOMElement*)anItem;
>+- (id)initWithMenuItem:(nsIDOMElement*)anItem;
>+- (nsIDOMElement*)menuItem;
>+- (BOOL)isHidden;
>+- (void)performCommand;

These should all have comments here. However, menuItem is never used, and should thus be removed, and performCommand can be private since it's not intended to be called except by the menu item itself.

In fact... the only reason anything but the implementation of menuFromNode: needs to know about this class is to call isHidden, which is part of NSMenu in 10.5+. So I would suggest calling this new file NSMenu+Gecko instead, and making a note in the class comment that once Camino is 10.5+ the declaration of XULMenuItem can be moved into the implementation file.


>+// NSMenu(XULMenu)
>+//
>+// NSMenu category that provides a method to create a menu from a XUL <popup>
>+// element.

I'd suggest commenting the method, rather than the category.

>+- (BOOL)hasAttribute:(NSString*)anAttr;
>+- (NSString*)valueForAttribute:(NSString*)anAttr;

Need comments.

>+    mMenuItem = anItem;
>+    if (mMenuItem) {
>+      NS_ADDREF(mMenuItem);
>+    }
>+    else {
>+      [self autorelease];
>+      return nil;
>+    }

It's a bit odd to have the early return stuck between this logic and the rest of the logic. Probably better to have the very first thing be:
if (!anItem) {
  <autorelease and return>
}
<do everything else>

>+  if (mMenuItem)
>+    mMenuItem->HasAttribute(attrName, &result);

It's impossible to have an object where mMenuItem is null, so there's no need to check.

>+  if (mMenuItem && NS_SUCCEEDED(mMenuItem->GetAttribute(attrName, attrValue)))

Same here.


>+  nsCOMPtr<nsIDOMEvent> event;
>+  docEvent->CreateEvent(NS_LITERAL_STRING("xulcommandevent"), getter_AddRefs(event));
>+  if (!event)
>+    return;
>+
>+  nsCOMPtr<nsIDOMXULCommandEvent> xulCommand = do_QueryInterface(event);
>+  if (!xulCommand)
>+    return;

Since nsIDOMXULCommandEvent is a descendent class of nsIDOMEvent you should be able to collapse these, and use xulCommand directly in the event sending call (which would also read better, since the current code looks like you set up xulCommand but ignore it and send something else).


>+  PRBool hasChildren;
>+  nsresult rv = aNode->HasChildNodes(&hasChildren);
>+  if (NS_FAILED(rv))
>+    return nil;

Shouldn't that be "if (NS_FAILED(rv) || !hasChildren)"?

>+  if (NS_FAILED(rv))
>+    return nil;
>+
>+  if (!children)
>+    return nil;

Make this a compound if, rather than two blocks.

>+  for (PRUint32 childIndex = 0; childIndex < numChildren; childIndex++) {
>+    rv = children->Item(childIndex, getter_AddRefs(node));
>+    if (NS_FAILED(rv))
>+      return nil;
>+
>+    nsAutoString nodeNameString;
>+    rv = node->GetNodeName(nodeNameString);
>+    if (NS_FAILED(rv))
>+      return nil;

Why "return nil" instead of "continue"?

>+    XULMenuItem* theXULItem = [XULMenuItem itemWithMenuItem:nodeElement];

You need to nil-check theXULItem

>+  nsCOMPtr<nsIDOMNSEvent> nsEvent (do_QueryInterface(inEvent, &rv));

No space between the variable name and the constructor arguments. And unless you need to know exactly why a do_QI fails, use the single-argument form and null-check the pointer, as you do in the XULMenuItem code.

>+  PRBool eventIsTrusted = PR_FALSE;
>+  nsEvent->GetIsTrusted(&eventIsTrusted);
>+  if (!eventIsTrusted)
>+    return NS_OK;

Why are you skipping the return value check of the call in just this case?

>+  nsCOMPtr<nsIDOMNode> domNodeSendingEvent = do_QueryInterface(eventTarget, &rv);
>+  if (NS_FAILED(rv))
>+    return NS_OK;

Again, single-argument do_QI.

Comment 24

[Christopher Henderson]

Attachment: Fix v1.6

Updated patch with review comments addressed.

>Sorry, one more round of comments.

No apology needed.

>Why are you skipping the return value check of the call in just this case?

Because if the function fails, eventIsTrusted will still be PR_FALSE, so we'll return anyway. eventIsTrusted will be PR_TRUE iff the function succeeds.

Comment 25

[Stuart Morgan]

Comment on attachment 427990 [details] [diff] [review]
Fix v1.6

>+  if (anItem)
>+    return [[[XULMenuItem alloc] initWithMenuItem:anItem] autorelease];
>+  else
>+    return nil;

Since initWithMenuItem: will already do the right thing, and having a slight perf hit in a case that's never supposed to happen doesn't matter, just make this a direct call through to initWithMenuItem: with no check.

>+    if (!theXULItem)
>+      return nil;

continue, rather than return.

>+    if (![theXULItem isHidden])
>+      [theMenu insertItem:theXULItem atIndex:[theMenu numberOfItems]];

It would be better to change this (combining with the previous check) to:
    if (!theXULItem || [theXULItem isHidden])
      continue;

    [theMenu insertItem:theXULItem atIndex:[theMenu numberOfItems]];
    ...
since there's no point in building the submenu for an item you aren't adding.

sr=smorgan with those changes.

Comment 26

[Stuart Morgan]

Comment on attachment 427990 [details] [diff] [review]
Fix v1.6

Oops, I missed these twice:

>+  nsCOMPtr<nsIDOMElement> contextMenuElement = do_QueryInterface(aNode, &rv);
>+  if (NS_FAILED(rv))
>+    return nil;

One-arg do_QI (and null-check the pointer)

>+    nsCOMPtr<nsIDOMElement> nodeElement = do_QueryInterface(node, &rv);

One-arg do_QI

Comment 27

[Christopher Henderson]

Attachment: Fix v1.7, for checkin

Fix with sr comments addressed, for checkin.

Comment 28

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk.

For those of you playing along at home, you won't be able to create new boolean prefs until bug 528379 has landed, but everything else works.

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Stuart, what do you think about taking this (and of course bug 528379) for 2.0.3?

2.0.3 will be the first 2.0.x release since this patch has been available, and 2.0.x is really the place where the patch would do the most good, since a) that's where the users are, b) for 2.0, we switched our hidden prefs docs to use about:config, and c) the patch actually works there.

Comment 30

[Stuart Morgan]

Seems a bit odd to take in a bugfix update, but since it's so specific in scope it doesn't really seems like there's much capacity for it to cause problems, so I'm fine with landing it given (b).

Comment 31

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on the CAMINO_2_0_BRANCH.