793582 - Cannot open Image Manger via Tools->Image Manager->Manage Image Permissions (regression)

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Philip Chee]

Mon Sep 24 2012 12:55:10
Error: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPermissionManager.add]
Source file: chrome://navigator/content/navigator.js
Line: 826

Regression caused by Bug 175175 which added an ID here:
http://hg.mozilla.org/comm-central/rev/f6cce22f417f#l1.178

   1.178 -            <menuitem label="&cookieDisplayImagesCmd.label;"
   1.179 +            <menuitem id="menuitem_cookieDisplay"
   1.180 +                      label="&cookieDisplayImagesCmd.label;"
   1.181                        accesskey="&cookieDisplayImagesCmd.accesskey;"/>

Comment 1

[Edmund Wong (:ewong)]

Attachment: Remove cookiesDisplayImgCmd id as it was interferring with Image Manager. (v1)

Comment 2

[Philip Chee]

I was thinking something like this would work:

-              oncommand="if (event.target.id) CookieImageAction(event.target);
+              oncommand="if (event.target.id != 'menuitem_cookieDisplay')
+                           CookieImageAction(event.target);

or perhaps:

+              oncommand="if (event.target.id.startsWith('cookie_'))
+                           CookieImageAction(event.target);

Comment 3

[Edmund Wong (:ewong)]

Attachment: Ensure the id is not 'menuitem_cookieDisplay' before running CookieImageAction(). (v2)

Comment 4

[Edmund Wong (:ewong)]

Attachment: Ensure the id is not 'menuitem_cookieDisplay' before running CookieImageAction(). (v2)

Comment 5

[Karsten Düsterloh]

Comment on attachment 664821 [details] [diff] [review]
Ensure the id is not 'menuitem_cookieDisplay' before running CookieImageAction(). (v2)

>+              oncommand="if (event.target.id) && (event.target.id != 'menuitem_cookieDisplay')
>+                           CookieImageAction(event.target);
>                          else toDataManager('|permissions');">

It really is a good idea to actually *test* patches before submitting them, *including* looking at the Error Console for unwanted behaviour. This would have shown this very obvious syntax error right away. ;-)

Apart from this, only one out of the five menuitems is meant to call the Data Manager, hence using its id in a rejection condition is a bit weird.
Just do a plain
   if (event.target.id == 'menuitem_cookieDisplay')
     toDataManager('|permissions');
   else
     CookieImageAction(event.target);

Comment 6

[Edmund Wong (:ewong)]

Attachment: Check if the id is 'menuitem_cookieDisplay' before running CookieImageAction(). (v3)

Comment 7

[Edmund Wong (:ewong)]

(In reply to Karsten Düsterloh from comment #5)
> Comment on attachment 664821 [details] [diff] [review]
> Ensure the id is not 'menuitem_cookieDisplay' before running
> CookieImageAction(). (v2)
> 
> >+              oncommand="if (event.target.id) && (event.target.id != 'menuitem_cookieDisplay')
> >+                           CookieImageAction(event.target);
> >                          else toDataManager('|permissions');">
> 
> It really is a good idea to actually *test* patches before submitting them,
> *including* looking at the Error Console for unwanted behaviour. This would
> have shown this very obvious syntax error right away. ;-)

When running the above code, it didn't even get to the point of showing the
menu list.  It just hung there.  And Ctrl-Shift-J doesn't work in this
case.   Can you clarify why this doesn't work?:
   if (event.target.id) && (event.target.id != 'menuitem_cookieDisplay')
        CookieImageAction(event.target);
   else 
        toDataManager('|permissions');
  
Without the first "(event.target.id) &&" part, the patch works.  

My understanding is that it tests if event.target.id is non-null.  

(This is purely an academic query. I've already changed the patch to
reflect your comments.)

Comment 8

[Karsten Düsterloh]

(In reply to Edmund Wong (:ewong) from comment #7)
> Can you clarify why this doesn't work?:
>    if (event.target.id) && (event.target.id != 'menuitem_cookieDisplay')
>         CookieImageAction(event.target);
>    else 
>         toDataManager('|permissions');
>   
> Without the first "(event.target.id) &&" part, the patch works.  

JavaScript, like C++ and others, expects this syntax:
  if (expression)
Even if the expression itself consists of several other expressions in parantheses, you still need the outer brackets:
  if ((expr) && (expr))
Everything else will result in a syntax error!
  
> My understanding is that it tests if event.target.id is non-null.  

It does; you don't fail for it. With outer brackets, it'd work, but it still would be unneeded. ;-)

Comment 9

[Karsten Düsterloh]

Comment on attachment 667294 [details] [diff] [review]
Check if the id is 'menuitem_cookieDisplay' before running CookieImageAction(). (v3)

>+              oncommand="if (event.target.id == 'menuitem_cookieDisplay')
>+                           toDataManager('|permissions');
>+                         else CookieImageAction(event.target);">

Either wrap both branches or none.

r=me with that.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Check if the id is 'menuitem_cookieDisplay' before running CookieImageAction(). (v4)

Wrapped branch.

Comment 11

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3debe9b1eed2

Comment 12

[Frank Wein [:mcsmurf]]

Settings flags to make tracking of fixed bugs easier.