The JS function for "BuildRecentMenu" does too much. It should be split into two separate functions. The saving of the prefs should be a separate routine since sometimes (like when a document is opened or saved with a new name or given a new title) the menu doesn't need to be rebuilt and only the preferences need to be updated. BuildRecentMenu should do just that (and only that). Perhaps the new function could be called something like "SaveRecentPrefs" or something clearer than being a flag for the BuildRecentMenu function.

Mine

Went over this patch with brade in #composer, we took out the StripPassword, because the password is always stripped when going into the prefs, and we return if the curUrl is about:blank, because we won't be changing anything.

Oh, this patch is untested, can somebody please test?

Comment on attachment 100424 [details] [diff] [review] Split SaveRecentFilesPrefs from BuildRecentMenu r=cmanske Thanks! Note that this patch has to be merged with the one on bug 169029, so the change at block @@ -3841,8 +3841,9 @@ becomes: @@ -326,9 +326,11 @@ // Call EditorSetDefaultPrefsAndDoctype first so it gets the default author before initing toolbars EditorSetDefaultPrefsAndDoctype(); EditorInitToolbars(); - BuildRecentMenu(true); // Build the recent files menu and save to prefs + + // Set window title and build "Recent Files" menu + UpdateWindowTitle(); We don't have to save the recent file items to prefs when a new editor has just been created. I'll check this work in with fix to bug 169029.

uggh! of course we have to build the RecentFiles menu: + // Set window title and build "Recent Files" menu + UpdateWindowTitle(); + BuildRecentMenu();

Comment on attachment 100424 [details] [diff] [review] Split SaveRecentFilesPrefs from BuildRecentMenu Actually you should enable/disable the recent files option (as part of EditorInitFileMenu?) when you open the file menu so that you only need to call BuildRecentMenu when opening the recent menu.

Comment on attachment 100424 [details] [diff] [review] Split SaveRecentFilesPrefs from BuildRecentMenu Actually, I'm not sure I agree with Neil but I'd have to do some testing to be sure. The scenario I am thinking of goes something like this: one file in history: foo.html open new page open recent menu (1) save as foo.html open recent menu (2) The first time I open the recent menu, I expect it to show me one item. The 2nd time, I'd expect the menu to be disabled. Related might be when the user changes their preferences from showing 0 to 3 (or whatever) or from 3 to 0?

Comment on attachment 100424 [details] [diff] [review] Split SaveRecentFilesPrefs from BuildRecentMenu OK, so brade has convinced me that enabling/disabling (as opposed to rebuilding) the whole recent menu on open or save is enough.

Comment on attachment 100424 [details] [diff] [review] Split SaveRecentFilesPrefs from BuildRecentMenu reviewers comments were addressed.

I don't suppose you can slip in another fix I spotted: Currently AppendRecentMenuitem adds an oncommand attribute to each recent item. Instead, there should be a bubbling oncommand attribute set on the menupopup: <menupopup id="menupopup_RecentFiles" oncommand="editPage(event.target.getAttribute('value'), window, false);"/>

Comment on attachment 100424 [details] [diff] [review] Split SaveRecentFilesPrefs from BuildRecentMenu This all looks fine except for the savePrefFile() - I'd really prefer that you don't save prefs every time someone opens a document...let prefs figure out when the best time to save is) sr=alecf without that line.

This was checked in with fix to bug 169029

Fixes: 1. Use GetDocumentTitle 2. Really don't save about:blank 3. Don't save data: if you're not going to show it 4. If no prefs, disable recent menu 5. Fix brade's edge case: save about:blank over sole recent url

Reopening for consideration.

I fixed some of these issues in recent checkin for bug 169029. I forgot about this patch -- I'll review it and see how to update as suggested.

This patch updates for bitrot and puts the enabling in the "onPopupShowing" method of the Files menu. This is better since after a user opens a 2nd file in a different window, the first window's "Recent Pages" will be enabled correctly. The actual menu is constructed from prefs *only* when menu is opened. Saving the recently-opened filenames and titles to prefs is done only when absolutely necessary. I also removed the "cmd_buildRecentMenu" command since it is no longer used (it used to be used by editorShell.)

Comment on attachment 107965 [details] [diff] [review] Fix v3 >-function BuildRecentMenu() >+function BuildRecentPagesMenu() What's this doing here? You haven't renamed it in editorOverlay. >+ if (historyCount && !IsUrlAboutBlank(curUrl)) >+ { >+ titleArray.push(GetDocumentTitle()); >+ urlArray.push(curUrl); >+ } I was trying to avoid saving data urls in history... >+ // Enable recent pages submenu if there are any history entries in prefs >+ var historyUrl = GetUnicharPref("editor.history_url_0"); >+ >+ // See if there's more if current file is only entry in history list >+ if (historyUrl && historyUrl == docUrl) >+ historyUrl = GetUnicharPref("editor.history_url_1"); >+ SetElementEnabledById("menu_RecentFiles", historyUrl != ""); Not quite accurate - you don't check the historyCount, or for data: urls.

Re: Neil's comments: (From update of attachment 107965 [details] [diff] [review]) >-function BuildRecentMenu() >+function BuildRecentPagesMenu() What's this doing here? You haven't renamed it in editorOverlay. cmanske: Oops! It's changed in my tree. Forgot to update patch. >+ if (historyCount && !IsUrlAboutBlank(curUrl)) >+ { >+ titleArray.push(GetDocumentTitle()); >+ urlArray.push(curUrl); >+ } I was trying to avoid saving data urls in history... cmanske: I'll fix that >+ // Enable recent pages submenu if there are any history entries in prefs >+ var historyUrl = GetUnicharPref("editor.history_url_0"); >+ >+ // See if there's more if current file is only entry in history list >+ if (historyUrl && historyUrl == docUrl) >+ historyUrl = GetUnicharPref("editor.history_url_1"); >+ SetElementEnabledById("menu_RecentFiles", historyUrl != ""); Not quite accurate - you don't check the historyCount, or for data: urls. cmanske: Good point. It would be a silly user who set their pref to "0", imho!

This fixes all issues noted by Neil. I kept the check for "data" urls in SaveRecentFilesPrefs() even though I was tempted to remove it, since if we check when we add the URL for the current page, we shouldn't have to check again. But Neil pointed out that there may be data urls from previous versions. This allows us to not check for data url in the enable code in method InitFileMenu, to make that more efficient.

Same as v4, but includes change to editorOverlay.xul for "BuildRecentPagesMenu()"

Comment on attachment 108123 [details] [diff] [review] fix v5 You have a long line in the try { historyCount thing. Otherwise, it seems to address all the issues. r=akkana

Comment on attachment 108123 [details] [diff] [review] fix v5 >+ var historyCount; Nit: Should be = 10 by default as in other places >+ try { historyCount = gPrefs.getIntPref("editor.history.url_maximum"); } catch(e) {} Nit: separate lines for try and catch >+ historyUrl = GetUnicharPref("editor.history_url_1"); Possible check for historyCount > 1? >+ onpopupshowing="BuildRecentPagesMenu()"> Nit: Should add ;

Ok, issues noted by Neil in last comment fixed.

Comment on attachment 108123 [details] [diff] [review] fix v5 sr=alecf

Comment on attachment 108123 [details] [diff] [review] fix v5 a=asa for checkin to 1.3a

checked into 1.3a trunk