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

RESOLVED FIXED in seamonkey2.15

Status

SeaMonkey
Passwords & Permissions
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Philip Chee, Assigned: ewong)

Tracking

({regression})

Trunk
seamonkey2.15
regression

SeaMonkey Tracking Flags

(seamonkey2.15 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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;"/>
(Assignee)

Comment 1

6 years ago
Created attachment 664339 [details] [diff] [review]
Remove cookiesDisplayImgCmd id as it was interferring with Image Manager. (v1)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #664339 - Flags: review?(mnyromyr)
(Reporter)

Comment 2

6 years ago
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);
(Assignee)

Comment 3

6 years ago
Created attachment 664819 [details] [diff] [review]
Ensure the id is not 'menuitem_cookieDisplay' before running CookieImageAction(). (v2)
Attachment #664339 - Attachment is obsolete: true
Attachment #664339 - Flags: review?(mnyromyr)
Attachment #664819 - Flags: review?(mnyromyr)
(Assignee)

Comment 4

6 years ago
Created attachment 664821 [details] [diff] [review]
Ensure the id is not 'menuitem_cookieDisplay' before running CookieImageAction(). (v2)
Attachment #664819 - Attachment is obsolete: true
Attachment #664819 - Flags: review?(mnyromyr)
Attachment #664821 - Flags: review?(mnyromyr)

Comment 5

6 years ago
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);
Attachment #664821 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 667294 [details] [diff] [review]
Check if the id is 'menuitem_cookieDisplay' before running CookieImageAction(). (v3)
Attachment #664821 - Attachment is obsolete: true
Attachment #667294 - Flags: review?(mnyromyr)
(Assignee)

Comment 7

6 years ago
(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

6 years ago
(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

6 years ago
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.
Attachment #667294 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 667768 [details] [diff] [review]
Check if the id is 'menuitem_cookieDisplay' before running CookieImageAction(). (v4)

Wrapped branch.
Attachment #667294 - Attachment is obsolete: true
Attachment #667768 - Flags: review+
(Assignee)

Comment 11

6 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3debe9b1eed2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Settings flags to make tracking of fixed bugs easier.
status-seamonkey2.15: --- → fixed
Target Milestone: --- → seamonkey2.15
You need to log in before you can comment on or make changes to this bug.