117026 - bookmark properties dialog very slow to load

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Jeremy M. Dolan]

Right click a bookmark in BM manager, click Properties.

Actual Results: Wait 6 seconds, see properties.
Expected Results: Wait <2 seconds, see properties.

New window open is down to 5.5 seconds here...BM Manager itself opens in 3, and
I have a lot of bookmarks. 6 seconds for the tiny properties dialog seems very
excessive.

Comment 1

[Peter Trudelle]

mass reassign of pchen bookmark bugs to ben

Comment 2

[André Dahlqvist]

Jeremy, what kind of a machine do you have? It takes less than two seconds for
me on a PIII with 256 megs of RAM. Does this happen only on a large bookmark file?

Comment 3

[Peter Trudelle]

nsbeta1- per nav triage team, but we'd consider a patch that removed the
schedule and notify panes, which should make the dialog faster.  helpwanted.

Comment 4

[André Dahlqvist]

trudelle, so you just want the schedule and notify stuff to be removed from the
bm-props.xul file? How about the accompanying javascript and dtd stuff?

Comment 5

[Jeremy M. Dolan]

Err... I originally had XUL cache disabled. With it back on, nothing seems much
faster, except for this. Props now open in 1.5 seconds. Pretty severe perf
penalty (400%) for props w/o cache.

Comment 6

[[not reading bugmail]]

I'm seeing a similar on window close perf problem bug 131130.

Comment 7

[Jeremy M. Dolan]

Upcoming patch

Comment 8

[Jeremy M. Dolan]

Attachment: remove tabs

Speeds up dialog load from 1.5s to near instantaneous (actually pops up before
the dialog menu is destroyed sometimes). Reviews?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 93669 [details] [diff] [review]
remove tabs

sr=bzbarsky.  Make sure a UI-ish person (blake or jag) reviews...

Comment 10

[Jeremy M. Dolan]

Did a quick search, and it seems some people are (attempting) to use the
scheduling features. But it does seem fairly broken:

bug 44282  bookmark scheduled update fails to trigger
bug 56096  bookmark scheduler settings prevents 23pm to 8am
Bug 90148  bookmark scheduler doesn't finish
bug 94530  bookmark schedule/notification breaks on '?' in URL
bug 110044 Scheduled bookmark notification triggers multiple times
Bug 107268 bookmark scheduler wont do anything
     ^-- this is what I saw when I tried it. No notifications ever.
Bug 94530  bookmark schedule/notification breaks on '?' in URL

Feature incomplete:

Bug 130344 Can't view the schedules as I can in IE
Bug 74627  'schedule' and 'notify' bookmarks globally
Bug 75330  [RFE] Do bookmark checking if scheduled time has passed

And terribly in need of UI love:

bug 132380 Merge "Schedule" and "Notify" tabs in Bookmark Properties
     ^-- this is terribly needed
Bug 89059  Schedule in properties of bookmark should only talk about [...]


So... do we want this in, in it's half working form, or out, and make bookmark
props load in a reasonable time?

It would require a lot of work to make notification robust enough to where it
should be exposed to users, IMO. (Note my patch was made in responce to
trudelle's call for one)

Comment 11

[André Dahlqvist]

*** Bug 174212 has been marked as a duplicate of this bug. ***

Comment 12

[Alfred Kayser]

Attachment: patched file: communicator\bookmarks\bm-props.xul

This is a replacement (cannot diff at this moment)
for the bookmarks properties dialog box, fixing the following:
* this bug (slow loading): 
   - removed tabs, and lots of extra spacers...
   - removed descriptions and icons (non-info)
* bug 132380: merge schedule and notification panels
* bug 179058: make dialogbox wider, so that most urls fit in editbox.

So, whilst not removing the scheduling features, this speeds up the opening
of this dialogbox, and shows all info in one box, instead of hiding bits and
pieces.

Comment 13

[Alfred Kayser]

Attachment: Corresponding file: en-US\communicator\bookmarks\bm-props.dtd

Removed the entries not used anymore

Comment 14

[Alfred Kayser]

Ben Goodger, it seems that you are the owner, can
you progress this bug through the r, sr, etc stages?
Note, that my patch also makes schedule.gif and notification.gif in the themes
dead meat.

Comment 15

[Alfred Kayser]

Attachment: Screenshot of new and improved bookmark properties

Comment 16

[Alfred Kayser]

Attachment: Screenshot with Classic

Comment 17

[Alfred Kayser]

Attachment: Screenshot with Modern

Other one is really the 'Classic' shot

Comment 18

[:deanis74]

Comment on attachment 110814 [details]
Corresponding file: en-US\communicator\bookmarks\bm-props.dtd

><!ENTITY checkforupdates.legend.label "Check this location for updates:">
><!ENTITY notifications.legend.label   "Notification:">

These are group box labels, and thus shouldn't have colons.

Comment 19

[:deanis74]

Please note bug 194553, which is about adding access keys to this dialog.

Comment 20

[timeless]

=>patch author

Comment 21

[Alfred Kayser]

Attachment: Patch for in xpfe\components\bookmarks\resources

Complete patch, to merge all tabs into one dialogbox (see screenshots).
Does also fix bug 132380, bug 179058, and bug 194553.
(Merge Schedule/Notify, Prop. too narrow, resp. Add access keys).

Fully tested with current builds, and safe change as it is only dtd and xul
change (long live XUL!)

Comment 22

[Alfred Kayser]

Comment on attachment 121896 [details] [diff] [review]
Patch for in xpfe\components\bookmarks\resources

Note, also includes bug 132380, bug 179058, and bug 194553.

Comment 23

[:deanis74]

Comment on attachment 121896 [details] [diff] [review]
Patch for in xpfe\components\bookmarks\resources

>+  <groupbox><caption label="&generalInfo.label;" />

These should be on separate lines.

>-                <label value="&bookmarks.name.label;" control="name"/>
>+        <label value="&bookmarks.name.label;" accesskey="&bookmarks.name.accesskey;" control="name"/>

I assume your indentation is proper, and this looks weird only because it's a
diff -w?

>-              <row id="descriptionrow">
>-                <label value="&bookmarks.description.label;" control="description"/>
>+      <row>
>+        <label value="&bookmarks.description.label;" accesskey="&bookmarks.description.accesskey;" control="description"/>

Without a row with id of "descriptionrow", doesn't line 200 throw a js error?

  document.getElementById("descriptionrow").setAttribute("hidden", "true");

>+</groupbox>
>+<hbox>
>+  <groupbox><caption label="&checkforupdates.legend.label;"/>

These lines should be intended two spaces more, shouldn't they?

>-          <image id="notification-icon"/>

Should we get rid of this image, then, from bookmarksWindow.css (and eventually
from cvs)?

> <!ENTITY bookmarks.name.label         "Name:">
>+<!ENTITY bookmarks.name.accesskey     "a">

'N' is available now, why not use that?

Other than that, looks good to me.  Can you attach a new patch that addresses
my questions and comments?

Comment 24

[Alfred Kayser]

Attachment: Patch (diff -uw format)

Fixed comments from Dean:
* caption on separate line
* indendation fixed
* re-added 'descriptionrow'
* filed separate bug for removal of 'notification-icon' and 'schedule-icon'
* accesskey for Name is now 'N'.
* fixed hiding of scheduling groupboxes for non-http bookmarks.

Comment 25

[:deanis74]

Comment on attachment 121992 [details] [diff] [review]
Patch (diff -uw format)

>-    document.getElementById("ScheduleTab").setAttribute("hidden", "true");
>-    document.getElementById("NotifyTab").setAttribute("hidden", "true");
>+    document.getElementById("scheduling").setAttribute("hidden", "true");
>   }

Are we not hiding the notification section anymore, just scheduling?

>+  <hbox id="scheduling">

Does this have a closing tag somewhere that doesn't show up in the diff -w?

Comment 26

[Alfred Kayser]

Ad 1. No, the 'scheduling' id refers to the hbox containing
both notification and scheduling groupboxes.

Ad 2. There is a closing hbox, only the diff -uw doesn't show it.
The diff -u is not very useable, as the indenting of almost everything
has changed (due to the removal of the tabs).

Comment 27

[:deanis74]

Comment on attachment 121992 [details] [diff] [review]
Patch (diff -uw format)

That clears up my questions.  r=me

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121992 [details] [diff] [review]
Patch (diff -uw format)

sr=me, but why do you need the <hbox id="scheduling">?	Couldn't you put the id
on the groupbox?

Comment 29

[Alfred Kayser]

Attachment: patch for xpfe\components\bookmarks (diff -u)

diff -u to be applied to cvs

Note the 'id=scheduling' is to the hbox, so that both
groupbox's contained in the hbox are hidden for bookmark
items (such as folders) are hidden by this flag.
Furthermore the hbox is required to put the groupbox's
horizontally next to each other. (see screenshots)

Comment 30

[Darin Fisher]

this patch has merge conflicts in bm-props.xul.  alfred: it's not obvious to me
how to resolve the conflicts.  if you can post an updated patch, i'll be more
than happy to check it in for you.  thx!

Comment 31

[Alfred Kayser]

Attachment: Updated diff -u patch

Comment 32

[Alfred Kayser]

Note, if this is checked in, the following bugs can also be 
closed bug 132380, bug 179058, and bug 194553.
(Merge Schedule/Notify, Prop. too narrow, resp. Add access keys).

Comment 33

[Alfred Kayser]

Attachment: More cleaner patch

This is the last and final patch.
Also removed the redundant showDescription function
and renamed the global var Bookmarks to gBookmarks.

Comment 34

[Darin Fisher]

fixed-on-trunk

Comment 35

[Darin Fisher]

adding bugs blocked by this one per comment #32

Comment 36

[Pierre Chanial]

It would have been better to have this patch reviewed by a person familiar with
the bookmarks code (Jan or me which appear to be the default assignee for the
bookmark component).
For instance, the renaming of bookmarks into gBookmarks makes no sense since
there is already a global var |BMDS| to that effect that should have been used.
Also, descriptions from bookmarks.properties should be removed. The indentation
is wrong in some places, also.
Please avoid that in the future.

Comment 37

[Darin Fisher]

reopening bug

alfred: can you please create a followup patch to address p_ch's review
comments.  thx!

Comment 38

[Alfred Kayser]

Attachment: fixed followup patch for the additional review comments

> It would have been better to have this patch reviewed by a person familiar
with
> the bookmarks code (Jan or me which appear to be the default assignee for the

> bookmark component).
Sorry about that, will look better and harder for the right reviewers...

> For instance, the renaming of bookmarks into gBookmarks makes no sense since
> there is already a global var |BMDS| to that effect that should have been
used.
See patch

> Also, descriptions from bookmarks.properties should be removed. 
See patch

> The indentation is wrong in some places, also.
If you take a look at:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bm-props.xul

you can see it is correct. 
The patch looks like a mess, because the removal of the tab/tabpanel levels.

Comment 39

[Pierre Chanial]

Comment on attachment 132987 [details] [diff] [review]
fixed followup patch for the additional review comments

please, don't forget to re-indent the file.

Comment 40

[Pierre Chanial]

> If you take a look at .../bm-props.xul
> you can see it is correct. The patch looks like a mess, because the removal 
> of the tab/tabpanel levels.

I was referring to
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bm-props.js
indentation is not correct.

Comment 41

[Alfred Kayser]

Comment on attachment 132987 [details] [diff] [review]
fixed followup patch for the additional review comments

Index: resources/bm-props.js
===================================================================
RCS file: /cvsroot/mozilla/xpfe/components/bookmarks/resources/bm-props.js,v
retrieving revision 1.31
diff -u -r1.31 bm-props.js
--- resources/bm-props.js	9 Oct 2003 21:23:27 -0000	1.31
+++ resources/bm-props.js	20 Oct 2003 09:43:49 -0000
@@ -42,9 +42,6 @@
 // that they are associated with.
 var gProperties;

-// All the bookmarks datasource
-var gBookmarks;
-
 // The ID of the current shown bookmark
 var gBookmarkID;

@@ -63,7 +60,6 @@
		  NC_NS + "ShortcutURL",
		  NC_NS + "Description"];

-  gBookmarks = RDF.GetDataSource("rdf:bookmarks");
   gBookmarkID = window.arguments[0];

   var i;
@@ -74,7 +70,7 @@
   for (i = 0; i < gFields.length; ++i) {
     var field = document.getElementById(gFields[i]);

-    var value = gBookmarks.GetTarget(resource,
RDF.GetResource(gProperties[i]), true);
+    var value = BMDS.GetTarget(resource, RDF.GetResource(gProperties[i]),
true);

     if (value)
       value = value.QueryInterface(Components.interfaces.nsIRDFLiteral).Value;
@@ -90,7 +86,7 @@
   propsWindow.setAttribute("title", title);

   // if its a container, disable some things
-  var isContainerFlag = RDFCU.IsContainer(gBookmarks, resource);
+  var isContainerFlag = RDFCU.IsContainer(BMDS, resource);
   if (!isContainerFlag) {
     // XXX To do: the "RDFCU.IsContainer" call above only works for RDF
sequences;
     // 	   if its not a RDF sequence, we should to more checking to see
if
@@ -110,7 +106,7 @@
   }

   var showScheduling = false;
-  var url = gBookmarks.GetTarget(resource, RDF.GetResource(gProperties[1]),
true);
+  var url = BMDS.GetTarget(resource, RDF.GetResource(gProperties[1]), true);
   if (url) {
     url = url.QueryInterface(Components.interfaces.nsIRDFLiteral).Value;
     if (url.substr(0, 7).toLowerCase() == "http://" ||
@@ -125,7 +121,7 @@
   } else {
     // check bookmark schedule
     var scheduleArc =
RDF.GetResource("http://home.netscape.com/WEB-rdf#Schedule");
-    value = gBookmarks.GetTarget(resource, scheduleArc, true);
+    value = BMDS.GetTarget(resource, scheduleArc, true);

     if (value) {
       value = value.QueryInterface(Components.interfaces.nsIRDFLiteral).Value;
@@ -222,9 +218,8 @@
       // Get the new value as a literal, using 'null' if the value is empty.
       var newvalue = field.value;

-      var oldvalue = gBookmarks.GetTarget(RDF.GetResource(gBookmarkID),
-					  RDF.GetResource(gProperties[i]),
-					  true);
+      var oldvalue = BMDS.GetTarget(RDF.GetResource(gBookmarkID),
+				     RDF.GetResource(gProperties[i]), true);

       if (oldvalue)
	 oldvalue =
oldvalue.QueryInterface(Components.interfaces.nsIRDFLiteral);
@@ -255,8 +250,8 @@
   var schedulingHidden = scheduling.getAttribute("hidden");
   if (schedulingHidden != "true") {
     var scheduleRes = "http://home.netscape.com/WEB-rdf#Schedule";
-    oldvalue = gBookmarks.GetTarget(RDF.GetResource(gBookmarkID),
-				    RDF.GetResource(scheduleRes), true);
+    oldvalue = BMDS.GetTarget(RDF.GetResource(gBookmarkID),
+			       RDF.GetResource(scheduleRes), true);
     newvalue = "";
     var dayRangeNode = document.getElementById("dayRange");
     var dayRange = dayRangeNode.selectedItem.getAttribute("value");
@@ -308,7 +303,7 @@
   }

   if (changed) {
-    var remote =
gBookmarks.QueryInterface(Components.interfaces.nsIRDFRemoteDataSource);
+    var remote =
BMDS.QueryInterface(Components.interfaces.nsIRDFRemoteDataSource);
     if (remote)
       remote.Flush();
   }
@@ -324,21 +319,16 @@
   if (prop && (oldvalue || newvalue) && oldvalue != newvalue) {

     if (oldvalue && !newvalue) {
-      gBookmarks.Unassert(RDF.GetResource(gBookmarkID),
-			  RDF.GetResource(prop),
-			  oldvalue);
+      BMDS.Unassert(RDF.GetResource(gBookmarkID), 
+		     RDF.GetResource(prop), oldvalue);
     }
     else if (!oldvalue && newvalue) {
-      gBookmarks.Assert(RDF.GetResource(gBookmarkID),
-			RDF.GetResource(prop),
-			newvalue,
-			true);
+      BMDS.Assert(RDF.GetResource(gBookmarkID),
+		   RDF.GetResource(prop), newvalue, true);
     }
     else /* if (oldvalue && newvalue) */ {
-      gBookmarks.Change(RDF.GetResource(gBookmarkID),
-			RDF.GetResource(prop),
-			oldvalue,
-			newvalue);
+      BMDS.Change(RDF.GetResource(gBookmarkID), 
+		   RDF.GetResource(prop), oldvalue, newvalue);
     }

     changed = true;
Index: resources/locale/en-US/bookmarks.properties
===================================================================
RCS file:
/cvsroot/mozilla/xpfe/components/bookmarks/resources/locale/en-US/bookmarks.pro
perties,v
retrieving revision 1.4
diff -u -r1.4 bookmarks.properties
--- resources/locale/en-US/bookmarks.properties 5 Oct 2003 08:21:25 -0000      
1.4
+++ resources/locale/en-US/bookmarks.properties 20 Oct 2003 09:43:49 -0000
@@ -18,15 +18,6 @@
 # Contributor(s): 
 #

-description_Bookmark = %brandShortName% can remember web page locations for
you. Type the page name and location in the fields below, then click OK. Select
the page from the Bookmarks menu or your Bookmarks Sidebar tab to visit the
page.
-description_Folder = %brandShortName% can organize your bookmarks into
specific folders. Type the folder name and an optional comment below, then
click OK.
-description_FolderGroup = %brandShortName% can bookmark a group of web pages.
Type a group name and an optional comment in the field below, then click OK.
Select the group from the Bookmark menu or your Bookmarks Sidebar tab to open
each page in a separate Navigator tab.
-description_PersonalToolbarFolder = Folders and bookmarks in this folder
appear on the Personal Toolbar.
-description_NewBookmarkFolder = Newly created bookmarks will be stored here by
default.
-description_NewSearchFolder = Search results will be stored here by default.
-description_NewBookmarkAndSearchFolder = Newly created bookmarks and search
results will be stored here by default.
-description_BookmarkSeparator = %brandShortName% can organize your bookmarks
by putting separators between them. Type an optional separator name below, then
click OK.
-
 cmd_bm_open = Open
 cmd_bm_expandfolder = Expand
 cmd_bm_collapsefolder = Collapse

Comment 42

[Alfred Kayser]

Attachment: Final patch to fix the last items (indenting, BMDS, and descriptions)

Final, follow-up patch, to fix the review comments of p_ch.

Comment 43

[Alfred Kayser]

Comment on attachment 133865 [details] [diff] [review]
Final patch to fix the last items (indenting, BMDS, and descriptions)

Very minor cleanup, but still nice to include in 1.6

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133865 [details] [diff] [review]
Final patch to fix the last items (indenting, BMDS, and descriptions)

sr=bzbarsky, though this does not address whatever indent issue pch
mentioned... you may want to address that at checkin time.

Comment 45

[Asa Dotzler [:asa]]

Comment on attachment 133865 [details] [diff] [review]
Final patch to fix the last items (indenting, BMDS, and descriptions)

a=asa (on behalf of drivers) for checkin to 1.6alpha

Comment 46

[Alfred Kayser]

It should also address the indenting issues, as they are
related to the 'bookmarks' to 'gBookmarks' to 'BMDS' changes,
which are included in this last patch.

Comment 47

[Darin Fisher]

patch checked in on the trunk for 1.6 alpha.

marking FIXED