Last Comment Bug 361901 - Save Page menuitem label missing from File menu when DM is open
: Save Page menuitem label missing from File menu when DM is open
Status: RESOLVED FIXED
: fixed-seamonkey1.1
Product: SeaMonkey
Classification: Client Software
Component: Download & File Handling (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: seamonkey1.1final
Assigned To: Stefan [:stefanh]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-11-26 14:25 PST by Stefan [:stefanh]
Modified: 2006-12-02 08:43 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
possible fix (1.90 KB, patch)
2006-11-29 13:22 PST, Stefan [:stefanh]
mnyromyr: review+
neil: superreview+
Details | Diff | Splinter Review
Updated patch that addresses review comments (and not using strres.js) (1.66 KB, patch)
2006-12-02 07:36 PST, Stefan [:stefanh]
stefanh: review+
stefanh: superreview+
csthomas: approval‑seamonkey1.1+
Details | Diff | Splinter Review

Description Stefan [:stefanh] 2006-11-26 14:25:55 PST
SeaMonkey trunk/branch, Mac.

1) Open SeaMonkey browser
2) Open DM
3) Open the File menu

--> A menuitem with no label (but with keys Cmd+S) appearing in the menu.

Neil, I guess the label never gets set - do you have any suggestion on how to fix this easily? There are lots of enabled menuitems that doesn't work, but I'm not sure time permits any major fixes here. I was thinking that we could just show it on branch even if it doesn't work - it looks rather strange having no label on the menuitem... :-/
Comment 1 neil@parkwaycc.co.uk 2006-11-26 15:12:49 PST
Well, you seem to have two options: you could make it so that you call updateSavePageItems or its equivalent, or you could alter the XUL to give the savepage menuitem a default label of one of the two strings in question.
Comment 2 Stefan [:stefanh] 2006-11-29 10:56:07 PST
<menupopup id="menu_FilePopup" onpopupshowing="updateCloseItems();getContentAreaFrameCount();updateSavePageItems();updateFileUploadItem();">


looks like there are two problems here:

1) The mac dlmanagermenuoverlay.xul doesn't have the strings used in updateCloseItems(), so the function fails --> the rest of the functions fail
2) We don't init the prefs necessary for updateSavePageItems()

So, we need to do some things when we load... 

Comment 3 Stefan [:stefanh] 2006-11-29 13:22:08 PST
Created attachment 246976 [details] [diff] [review]
possible fix

This is one way of doing it. This also makes File --> Open File work. The other way would be to add a label to the menuitem in navigatorOverlay.xul. Note here that we could fix more menuitems by forking stuff from hiddenWindow.xul and ripping some more js from hiddenWindowStartup() in navigator.js. We shouldn't disable all the items we do in hiddenWindowStartup(), though. And since this is ment to go on branch it might be an overkill... I also noticed that it's quite easy to "enable" the close command (you can't close DM from the menu or the keyboard atm) by doing some checks in BrowserCloseTabOrWindow() and BrowserCloseWindow().
Comment 4 Stefan [:stefanh] 2006-11-30 23:22:24 PST
Comment on attachment 246976 [details] [diff] [review]
possible fix

I think I like to go with this one, rather than altering the XUL
Comment 5 neil@parkwaycc.co.uk 2006-12-01 01:49:42 PST
Comment on attachment 246976 [details] [diff] [review]
possible fix

>+  <script type="application/x-javascript" src="chrome://global/content/strres.js"/>
We're still using strres.js? That sucks :-/

>-  <stringbundleset id="stringbundleset"/>
Heh, that line was pointless anyway :-)

>+    function dmOverlayStartup() {
>+
>+      var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                              .getService(Components.interfaces.nsIPrefService);
>+      pref = prefService.getBranch(null);
>+
>+      gNavigatorBundle = document.getElementById("bundle_navigator");
>+
>+    }
No need to double-space this. Also the indent for .getService is wrong. You could even avoid the temp var and write pref = Components...getBranch(null);

>+  <stringbundleset id="stringbundleset">
>+    <stringbundle id="bundle_navigator" src="chrome://navigator/locale/navigator.properties"/>
>+    </stringbundleset>
Wrong indentation on the </stringbundleset>

sr=me with these fixed.
Comment 6 Karsten Düsterloh 2006-12-01 17:46:23 PST
Comment on attachment 246976 [details] [diff] [review]
possible fix

>+    function dmOverlayStartup() {

Strip the excessive amount of empty lines inside this function. In fact, with Neil's comments, you don't need any.

r=me with that.
Comment 7 Stefan [:stefanh] 2006-12-02 07:29:55 PST
(In reply to comment #5)
> (From update of attachment 246976 [details] [diff] [review] [edit])
> >+  <script type="application/x-javascript" src="chrome://global/content/strres.js"/>
> We're still using strres.js? That sucks :-/

Oops, my mistake - I assumed we used it since it's included in hiddenWindow.xul. Apparantly we're not using strres.js - I've also tested without and everything works as expected. I suppose this has been fixed in bug 56680 (it seems to deal with this - I haven't checked the logs though). Anyway, will post an updated patch in a sec.
Comment 8 Stefan [:stefanh] 2006-12-02 07:36:19 PST
Created attachment 247269 [details] [diff] [review]
Updated patch that addresses review comments (and not using strres.js)

This fixes the review comments and is without strres.js. I think we should do this for trunk as well - I'll try to fix the rest of the issues in the 1.9.

Anyway, this fixes a highly visible issue - the Save Page menuitem label is missing in the file menu when you have the DM open on mac. This is also safe - the patch only alters code in the mac-specific dlmanagermenuoverlay.xul.
Comment 9 Robert Kaiser 2006-12-02 07:56:18 PST
Comment on attachment 247269 [details] [diff] [review]
Updated patch that addresses review comments (and not using strres.js)

first-a=me for 1.1, need one more to go...
Comment 10 Stefan [:stefanh] 2006-12-02 08:43:14 PST
Landed on trunk and branch by ajschult (thanks!)

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