441077 - Fork and clean up SearchDialog.js

Status: RESOLVED WORKSFORME

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: patch v1

The current SearchDialog files make me want to cry.  SearchDialog.xul includes almost every javascript file that the main window does.  Also, there's this really annoying problem that while SearchDialog.xul is forked, SearchDialog.js is not.  This makes it incredibly difficult to try to de-rdfify or simply clean up SearchDialog.xul, because the javascript is shared with another xul file.  Thus, as painful as a fork is, I think it's the right call here.

The attached patch is an mq patch based on a tree where the shared SearchDialog.js was copied into mail/, so that I can show what I intended to change about the javascript file.  The rest of the patch is a normal patch.

As a result of this patch, the global scope of the Search dialog has been reduced by 60% (from 701 to 270 items).  In my rough profiling, the time to open the search dialog (that has already been opened before) has dropped by 10-15% (from ~93ms to ~82ms).  The time to open the dialog after rebuilding has dropped over 50% (from ~980ms to ~410ms).

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
> The current SearchDialog files make me want to cry.  SearchDialog.xul includes
> almost every javascript file that the main window does.  Also, there's this
> really annoying problem that while SearchDialog.xul is forked, SearchDialog.js
> is not.  This makes it incredibly difficult to try to de-rdfify or simply clean
> up SearchDialog.xul, because the javascript is shared with another xul file. 
> Thus, as painful as a fork is, I think it's the right call here.

So from what I can see the differences between the two SearchDialog.xul files is not that large, I expect only a couple of bugs need to be ported - and even if we end up not merging the two xul files, just making them even should enable us to keep the js files the same. Have you tried asking the SeaMonkey team (either the council or moz.dev.apps.seamonkey) if someone is prepared to do this within, say, the next month?

IMHO the pain caused by forking a file (especially one such as this) is far greater than waiting a few weeks for a merge. I've been feeling this in various places - LDAP autocomplete is one, if I change the APIs, I currently need to change the code in 3 places. This costs me more and everyone afterwards.

Obviously if we're getting near to final or if it is really blocking work, and there is no action on the SeaMonkey front, then yes, we should fork it - but can we at least ask the question first?

Comment 2

[Joey Minta]

The changes here aren't just based on the SearchDialog.xul differences, but on all the differences between each of files that were previously included.  It's one thing for me to check to what extent SearchDialog.js depends on Thunderbird's mailWindowOverlay.js, msgMail3PaneWindow.js, etc, but to also track Seamonkey's copies' dependencies adds a whole magnitude of additional work.  Are there plans to un-fork those files I've un-included soon?

Comment 3

[Karsten Düsterloh]

Up to now, we tried to keep as much code as possible in sync, even if it wasn't shared, so that eg. extension authors mustn't care much about how to port their stuff. And, more importantly, so that developers didn't need to hunt down fine details in both apps just for making a backend change.

I'm not sure that hasty forking does any good here.
I am sure that your actual changes are for the better. ;-)

The differences in how this dialog is used/"fed" with data shouldn't be too big.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #2)
> The changes here aren't just based on the SearchDialog.xul differences, but on
> all the differences between each of files that were previously included.  It's
> one thing for me to check to what extent SearchDialog.js depends on
> Thunderbird's mailWindowOverlay.js, msgMail3PaneWindow.js, etc, but to also
> track Seamonkey's copies' dependencies adds a whole magnitude of additional
> work.

I'm not saying you need to track SeaMonkey's dependencies as well. All I'm really asking for is giving the SeaMonkey folk a chance to bring the SeaMonkey code "up to date", so that this change can be made without forking.

Just taking a quick look at the main differences between the two xul files:

SeaMonkey has: bug 83968 (Allow local searches even when online)
SeaMonkey has: help button - but Thunderbird wouldn't want this at the moment
Thunderbird's version includes a copy of the xul code for the thread pane (SeaMonkey includes threadPane.xul.
Thunderbird has: Bug 413781 XBLify folder-selection menus

So the main bug for SeaMonkey to port asap would be bug 413781, and I believe work has already started on moving the xbl code for that. If the changes on the search dialog are done first, then I think this wouldn't take too long to do.

Your patch can then be re-adjusted to work with the currently core SearchDialog.js, then the SeaMonkey team tests it out and depending on length of time to make changes, we either land and break SeaMonkey temporarily (i.e. with know regressions), or give them a couple of extra days to prepare fixes.


Having just said all that, bug 413781 doesn't seem to have actually affected SearchDialog.js, which would mean these dialogs are actually quite close... So maybe all we need to do is just to get someone to try out applying this to the SearchDialog.js/xul on SeaMonkey and see if there are any regressions.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Having just said all that, bug 413781 doesn't seem to have actually affected
> SearchDialog.js, which would mean these dialogs are actually quite close... So
> maybe all we need to do is just to get someone to try out applying this to the
> SearchDialog.js/xul on SeaMonkey and see if there are any regressions.

I think I didn't make this clear enough earlier - probably because I changed my mind at the end of comment 4, but left the information in just for reference.

What I'm trying to say as the main point, is that we should be working with the SeaMonkey team in situations like this to land the patch on the core code.

If someone working on one application doesn't want to do the work for the other application, that's fine, but please give the other application a chance to try the patch and do appropriate changes before deciding to fork a file. If the other application takes on the patch, then regressions in their application on landing will be their responsibility. Forking has, and still is costing us. We need to minimise it wherever possible.

If I seem to be trying to dictate/preach, I apologise - working with both applications and seeing the missed opportunities because of forking means I care a lot about what both sides do in this area.

Comment 6

[David :Bienvenu]

I'd rather not do hasty forking either.

Comment 7

[David :Bienvenu]

Comment on attachment 326142 [details] [diff] [review]
patch v1

I'm going to minus for now, and hope that we can work this out with SeaMonkey fairly quickly.

Comment 8

[Joey Minta]

Attachment: patch v2

This is the patch applied to the current SearchDialog.js, with a rough port to SeaMonkey's SearchDialog.xul as well.  I have no idea what it does there, because, as I said, I don't know the interactions among the various other includes/overlays that SeaMonkey has in its dialog.

I also resent the claim that this fork was in any way "hasty."  Filing this bug was not a spur of the moment decision on my part, but something I thought hard about. The continued fork of the xul files while sharing the js remains absurd in my mind. I've been struggling with the search dialog for the last few months in trying to de-rdf Thunderbird, and it's broken several of my patches. In general, the codebase is plagued by shared files that make assumptions about the forked files they depend on.  The search dialog files remain the most obvious offender, but threadpane.js and messengerdnd.js are other significant examples.  A shared base for mailnews is fine and good, but a base that depends on its consumers is not a base.  It's just a nightmare.

Comment 9

[David :Bienvenu]

Sorry, jminta, didn't know you'd been working on the search dialog for the last few months.

I agree that forking the xul and sharing the js isn't going to work very well in general, and we're going to need to figure out how to share more xul or share js differently.

I'll try this patch in the next day or so - it would be great if a Seamonkey person could try it as well.

Comment 10

[David :Bienvenu]

Comment on attachment 326391 [details] [diff] [review]
patch v2

r/sr=me, with some nits. I ran TB with this patch and things seemed to work. I don't know about SM...

+  if (destUri.length == 0) { 
+    destUri = destFolder.getAttribute('file-uri')
+  }

no need for the extra braces here.

+  return rdfService.GetResource(uri).QueryInterface(Components.interfaces.nsIMsgFolder);
+}
\ No newline at end of file

missing a newline.

+    if (!uri || uri[0] != 'n') {
+        return false;
+    }
+    else {
+        return ((uri.substring(0,6) == "news:/") || (uri.substring(0,14) == "news-message:/"));
+    }

don't need braces here either - could do this in one statement using ? operator, if you wanted to...

Comment 11

[Philip Chee]

Caching the stop button using gSearchStopButton seems a useful optimization. Any particular performance reason for uncaching it?

Comment 12

[Philip Chee]

(In reply to Comment #8)
> Created an attachment (id=326391)

> diff -r 5fd4720bd717 mailnews/base/search/resources/content/SearchDialog.xul
-<?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>

Just doing some preliminary code inspection. There seems to be at least two reasons why SeaMonkey mailnews references utilityOverlay.xul

1. There is an offline status indicator in the mailnews search dialog statusbar that depends on utilityOverlay.{xul|js}. If we remove the offline statusbarpanel then it looks like we can remove this dependency.

2. We have a help button that calls openHelp(). This currently resides in the toolkit contextHelp.js. We can reference this script directly.

Comment 13

[Joey Minta]

(In reply to comment #11)
> Caching the stop button using gSearchStopButton seems a useful optimization.
> Any particular performance reason for uncaching it?
(1) document.getElementById() is really fast, fast enough that there's virtually no difference between the performance impact of calling it, and the performance impact of adding another item to the global scope against which may js variable/funtion resolutions will occur. (2) It's a useless optimization, since afaict, it's not a significant signature in any performance measurements in these functions. If you so a performance analysis of some tbird function where getElementById shows up as significant, I'd be *really* curious to see it.

(In reply to comment #12)
> Just doing some preliminary code inspection. There seems to be at least two
> reasons why SeaMonkey mailnews references utilityOverlay.xul
OK, Seamonkey people need to tell me quickly what they want me to do.  Right now I see two options. (A) I can land this patch as is, and you can fix what, if anything breaks there. (B) I can land this patch without any changes to the includes/overlays of Seamonkey's xul file, and you can fix what breaks as a result of the then double-declarations of a variety of functions and variables within this scope.

Comment 14

[[e-mail]]

Comment on attachment 326391 [details] [diff] [review]
patch v2

>diff -r 5fd4720bd717 mail/base/content/msgMail3PaneWindow.js
>diff -r 5fd4720bd717 mailnews/base/resources/content/msgMail3PaneWindow.js
>diff -r 5fd4720bd717 mailnews/base/resources/content/threadPane.js
Bits of other patches?

>-        gSearchStopButton.setAttribute("label", gSearchBundle.getString("labelForSearchButton"));
>-        gSearchStopButton.setAttribute("accesskey", gSearchBundle.getString("labelForSearchButton.accesskey"));
>+        var stopButton = document.getElementById("search-button");
>+        stopButton.setAttribute("label", gSearchBundle.getString("labelForSearchButton"));
>+        stopButton.setAttribute("accesskey", gSearchBundle.getString("labelForSearchButton.accesskey"));
Maybe make the stop button a member variable?

>+    const VIRTUAL =  Components.interfaces.nsMsgFolderFlags.Virtual;
>+    if (!folder || !folder.server.canSearchMessages || (folder.flags & VIRTUAL)) {
You didn't make this a constant anywhere else, which seems inconsistent.
(Also you had two spaces after =)

> function updateSearchFolderPicker(folderURI) 
> { 
>-    SetFolderPicker(folderURI, gFolderPicker.id);
>+    SetFolderPicker(folderURI, "searchableFolders");
>+    var rdfService = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                               .getService(Components.interfaces.nsIRDFService);
> 
>     // use the URI to get the real folder
>-    gCurrentFolder =
>-        RDF.GetResource(folderURI).QueryInterface(nsIMsgFolder);
>+    gCurrentFolder = rdfService.GetResource(folderURI)
>+                               .QueryInterface(Components.interfaces.nsIMsgFolder);
Make use of GetMsgFolderFromUri perhaps?

>+    var mailSession = Components.classes["@mozilla.org/messenger/services/session;1"]
>+                                .getService(Components.interfaces.nsIMsgMailSession);
>     var nsIFolderListener = Components.interfaces.nsIFolderListener;
>     var notifyFlags = nsIFolderListener.event;
>-    gMailSession.AddFolderListener(gFolderListener, notifyFlags);
>+    mailSession.AddFolderListener(gFolderListener, notifyFlags);
You didn't bother with the temporary variable when removing the listener.

>-function IsThreadAndMessagePaneSplitterCollapsed()
Oops, how long has that been lying dormant :-[

>+  var rdfService = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                             .getService(Components.interfaces.nsIRDFService);
> 
>+  var destResource = RDF.GetResource(destUri);
> 
>+  var destMsgFolder = destResource.QueryInterface(Components.interfaces.nsIMsgFolder);
GetMsgFolderFromUri again!

> function BeginDragThreadPane(event)
> {
>     // no search pane dnd yet
>     return false;
> }
Ah, I see why this lets you remove messengerdnd.js

>+function GetSelectedIndices(dbView) {
>+  var indices = {};
>+  dbView.getIndicesForSelection(indices, {});
>+  return indices.value;
>+}
Ooh, this is just asking to be turned into a retval!

>+  view.getURIsForSelection(messageArray,length);
>+  if (length.value)
>+    return messageArray.value;
>+  else
>+    return null;
As is this!

>+//Ported from mailWindowOverlay.js
So this isn't a copy? I'm just not sure of the benefit of duplicating code...

>+// end commandglue copying
>+
>+var gStatusFeedback = {
...
>+// So we don't have to include widgetglue.js
So where's gStatusFeedback copied from?

>diff -r 5fd4720bd717 mailnews/base/search/resources/content/SearchDialog.xul
>-<?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
As long as SearchDialog.xul remains forked, suite needs this line.

Comment 15

[Joey Minta]

(In reply to comment #14)
> Bits of other patches?
No, that's me moving the global gThreadTree to the file that actually uses it.

> Maybe make the stop button a member variable?
We could, but I'm still really skeptical that caching DOM nodes is at all useful except in the most extreme of circumstances.

> 
> >+    const VIRTUAL =  Components.interfaces.nsMsgFolderFlags.Virtual;
> >+    if (!folder || !folder.server.canSearchMessages || (folder.flags & VIRTUAL)) {
> You didn't make this a constant anywhere else, which seems inconsistent.
I did it here to try to help with the 80char limit.

> (Also you had two spaces after =)
Doh.

> Make use of GetMsgFolderFromUri perhaps?
Then I can't remove widgetglue.js

> 
> >+//Ported from mailWindowOverlay.js
> So this isn't a copy? I'm just not sure of the benefit of duplicating code...
It's a 95% copy, I removed other dependencies in these functions (such as gPrefBranch).  The point is that it's better to just copy 1 function, rather than include a 3000line javascript file.  If SearchDialog.js wanted 10 functions from the file, then we should probably include it (that's why threadpane.js stays), but here, it's not worth the performance hit of parsing the huge file.  (see timing numbers above)

> So where's gStatusFeedback copied from?
It's not.  It's a slimmed down version of nsMsgStatusFeedback in mailWindow.js

> 
> >diff -r 5fd4720bd717 mailnews/base/search/resources/content/SearchDialog.xul
> >-<?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
> As long as SearchDialog.xul remains forked, suite needs this line.
OK, I'll restore at least that one on checkin.

Still waiting on a report of how the suite checkin should be structured...

Comment 16

[Karsten Düsterloh]

I'm going to look into unforking the XUL in 441340...

Comment 17

[[e-mail]]

(In reply to comment #15)
> (In reply to comment #14)
> > Bits of other patches?
> No, that's me moving the global gThreadTree to the file that actually uses it.
Sure, but this bug isn't called "Move the global gThreadTree to the file that actually uses it", nor did any of the previous comments mention it.

> > >+    const VIRTUAL =  Components.interfaces.nsMsgFolderFlags.Virtual;
> > >+    if (!folder || !folder.server.canSearchMessages || (folder.flags & VIRTUAL)) {
> > You didn't make this a constant anywhere else, which seems inconsistent.
> I did it here to try to help with the 80char limit.
You could just wrap it onto a second line... that's acceptable, you know ;-)

> > Make use of GetMsgFolderFromUri perhaps?
> Then I can't remove widgetglue.js
I mean the one you copied, of course...

> The point is that it's better to just copy 1 function
Remember, two functions are twice as hard to maintain.
Would it be better to make a new file shared between search and 3pane?

> > So where's gStatusFeedback copied from?
> It's not.  It's a slimmed down version of nsMsgStatusFeedback in mailWindow.js
IMHO a comment to that effect would be useful.

Comment 18

[[e-mail]]

(In reply to comment #14)
>(From update of attachment 326391 [details] [diff] [review])
>>+function GetSelectedIndices(dbView) {
>>+  var indices = {};
>>+  dbView.getIndicesForSelection(indices, {});
>>+  return indices.value;
>>+}
>Ooh, this is just asking to be turned into a retval!
Fixed in bug 442256.

Comment 19

[Karsten Düsterloh]

Like Philip already mentioned, SM's statusbar and help depend on other code. But even without the removals to SM's SearchDialog.xul, its status bar will be  broken courtesy to the SearchDialog.js changes:

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "document.getElementById("statusbar-progresspanel") is null" {file: "chrome://messenger/content/SearchDialog.js" line: 894}]' when calling method: [nsIMsgSearchNotify::onNewSearch]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://messenger/content/SearchDialog.js :: onSearch :: line 418"  data: yes]

Furthermore, certain functionality will be broken:
- "Open" button defunct
- File As" defunct
  "JavaScript error: chrome://messenger/content/SearchDialog.js, 
  line 719: RDF is not defined"
- Save Search comes up, but doesn't saves anything

And, apart from that, duplicating markup (back then when forking this) and now a gazillion methods in code is *really* a very bad idea on its own, even if you wouldn't break innocent bystanders.
It would have been much more useful to first file a bug, discuss what to do and then fix it according to the results of that discussion...

Comment 20

[Joey Minta]

I think comment #19 reaffirms the benefits of a fork, by demonstrating that the underlying forked files that the dialog depends on are just too different to be reliably used.  With the exception of the File As error, which is a typo in the patch, all of the other functions work fine in my build of Thunderbird.  Notwithstanding that, my current plan of action is to therefore check in this patch without any changes to SeaMonkey's copy of the xul file.  This gives Thunderbird the performance win, while hopefully not breaking SeaMonkey as severely.

(In reply to comment #19)
> And, apart from that, duplicating markup (back then when forking this) and now
> a gazillion methods in code is *really* a very bad idea on its own, even if you
> wouldn't break innocent bystanders.
Let's be clear.  I'm copying 8 functions from 4 different files.  In no case am I copying more than 3 functions from any particular file.  Including files with literally hundreds of functions in order to use 2-3 of them is just wrong, and the benefits to the user (increased performance) will always outweigh any increases in developer complexity. Even with these duplicated functions, I still argue that this actually makes the overall codebase *easier* to maintain, since no longer do you have to guess what changes in these no-longer-included files might break in the search dialog. When trying to do things like de-rdf, which involves excising RDF-dependent functions, this is a non-trivial benefit to the hacker.

> It would have been much more useful to first file a bug, discuss what to do and
> then fix it according to the results of that discussion...
> 
I don't understand what there is/was to discuss. The SearchDialog contains a demonstrable inefficiency/performance loss in that it has an overly complex set of functions/variables in its global scope.  I wanted to fix that, and the patches here do exactly that.  If I had filed this bug first, what different approach would you have suggested? (Wait for someone to potentially unfork the xul, which would then make this solution impossible and the bug permanent, since seamonkey has proven different requirements?)

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #20)
> Let's be clear.  I'm copying 8 functions from 4 different files.  In no case am
> I copying more than 3 functions from any particular file.  Including files with

(Would putting these common functions a separate shared file(s) work ?)

Comment 22

[Joey Minta]

(In reply to comment #21)
> (Would putting these common functions a separate shared file(s) work ?)
> 
It depends on what you mean by "work."  If you mean would the code still all run, yes.  Architecturally, though, it strikes me as plain wrong.  This would be a file with no modularity/purpose/area of functionality, but instead one simply defined by the needs of today as "all functions that are needed in both xul files."  In a front-end that doesn't have namespace-like global variables (or ES4 actual namespaces), files seem to be the only structural component capable of defining a module, and thereby creating some code independence.  I'd prefer not to create files on an ad-hoc basis to solve problems like this, absent the duplication of a lot more code. (No, don't ask me for a specific threshold, I don't have one, but I'm pretty confident this is on the small side of the line.) Creating an ad-hoc defined file like this undermines any attempt at creating that modularity within front-end code. 

Comment 23

[David :Bienvenu]

I'm OK with the minor duplication of js methods. I think the de-forking process and the de-rdfication processes are a bit at odds, but there's a lot more energy and urgency behind the de-rdfication process, so the de-forking needs to take a bit of a back seat.

Comment 24

[David :Bienvenu]

Just to be clear, I think de-forking is a huge win for SM and TB, and I'm all for it. It might be easier, though, once de-rdfication is done.

Comment 25

[Phil Ringnalda (:philor)]

So, where are we here? I've been ignoring two (undoubtedly rotten by now) reviews that depend on this for two months now, and I'm getting rather tired of seeing them in my queue.

Comment 26

[Magnus Melin [:mkmelin]]

searchdialog.js is forked nowadays, and the most stuff suggested here looks fixed/obsoleted. -> WFM

Comment 27

[Wayne Mery (:wsmwk)]

(In reply to Magnus Melin from comment #26)
> searchdialog.js is forked nowadays, and the most stuff suggested here looks
> fixed/obsoleted. -> WFM

Magnus, Thanks for the cleanup!

What does this suggest about bug 441340?  Should it be wontfix, or is there really still good reason to defork at some point? (Also, it currently has no open blockers)