BuildRecentMenu is inefficient; should be split up

RESOLVED FIXED in mozilla1.3alpha

Status

SeaMonkey
Composer
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: Kathleen Brade, Assigned: Charles Manske)

Tracking

({perf})

Trunk
mozilla1.3alpha

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Updated

15 years ago
Keywords: perf

Comment 1

15 years ago
Mine

Comment 2

15 years ago
Created attachment 100424 [details] [diff] [review]
Split SaveRecentFilesPrefs from BuildRecentMenu

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.

Comment 3

15 years ago
Oh, this patch is untested, can somebody please test?
(Assignee)

Comment 4

15 years ago
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.
Attachment #100424 - Flags: review+
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
Whiteboard: [FIX IN HAND]need sr=
Target Milestone: --- → mozilla1.2beta
(Assignee)

Comment 5

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

Comment 6

15 years ago
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.
Attachment #100424 - Flags: needs-work+
(Reporter)

Comment 7

15 years ago
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 8

15 years ago
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.
(Assignee)

Comment 9

15 years ago
Comment on attachment 100424 [details] [diff] [review]
Split SaveRecentFilesPrefs from BuildRecentMenu

reviewers comments were addressed.
Attachment #100424 - Flags: needs-work+

Comment 10

15 years ago
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 11

15 years ago
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.
Attachment #100424 - Flags: superreview+
(Assignee)

Comment 12

15 years ago
This was checked in with fix to bug 169029
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Whiteboard: [FIX IN HAND]need sr=

Comment 13

15 years ago
Created attachment 101767 [details] [diff] [review]
Resolves some possible errors

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

Comment 14

15 years ago
Reopening for consideration.
Status: RESOLVED → REOPENED
Keywords: patch, review
Resolution: FIXED → ---
(Assignee)

Comment 15

15 years ago
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.
Status: REOPENED → ASSIGNED
Whiteboard: [FIX IN HAND] need r=,sr=
Target Milestone: mozilla1.2beta → mozilla1.3alpha

Updated

15 years ago
Attachment #101767 - Flags: review?(cmanske)
(Assignee)

Comment 16

15 years ago
Created attachment 107965 [details] [diff] [review]
Fix v3

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.)
Attachment #100424 - Attachment is obsolete: true
Attachment #101767 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #107965 - Flags: superreview?(alecf)
Attachment #107965 - Flags: review?(neil)

Comment 17

15 years ago
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.
Attachment #107965 - Flags: review?(neil) → review-
(Assignee)

Comment 18

15 years ago
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!
(Assignee)

Comment 19

15 years ago
Created attachment 108116 [details] [diff] [review]
fix v4

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.
Attachment #107965 - Attachment is obsolete: true
(Assignee)

Comment 20

15 years ago
Created attachment 108123 [details] [diff] [review]
fix v5

Same as v4, but includes change to editorOverlay.xul for
"BuildRecentPagesMenu()"
Attachment #108116 - Attachment is obsolete: true

Comment 21

15 years ago
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
Attachment #108123 - Flags: review+
(Assignee)

Updated

15 years ago
Attachment #107965 - Flags: superreview?(alecf)
Attachment #107965 - Flags: review-
(Assignee)

Updated

15 years ago
Attachment #100424 - Flags: superreview+
Attachment #100424 - Flags: review+
(Assignee)

Updated

15 years ago
Attachment #108123 - Flags: superreview?(alecf)

Comment 22

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

Comment 23

15 years ago
Ok, issues noted by Neil in last comment fixed.
Whiteboard: [FIX IN HAND] need r=,sr= → [FIX IN HAND] need sr=

Comment 24

15 years ago
Comment on attachment 108123 [details] [diff] [review]
fix v5

sr=alecf
Attachment #108123 - Flags: superreview?(alecf) → superreview+
(Assignee)

Updated

15 years ago
Attachment #108123 - Flags: approval1.3a?

Comment 25

15 years ago
Comment on attachment 108123 [details] [diff] [review]
fix v5

a=asa for checkin to 1.3a
Attachment #108123 - Flags: approval1.3a? → approval1.3a+
(Assignee)

Comment 26

15 years ago
checked into 1.3a trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
Whiteboard: [FIX IN HAND] need sr=

Updated

15 years ago
Attachment #101767 - Flags: review?(cmanske)
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.