219120 - help.js needs cleaning.

Status: VERIFIED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[R.J. Keller]

Help.js could use some cleaning. It's pretty dirty.

Comment 1

[R.J. Keller]

Attachment: Cleans up help.js contextHelp.js

Comment 2

[R.J. Keller]

Comment on attachment 131393 [details] [diff] [review]
Cleans up help.js contextHelp.js

Neil, can you review?

Comment 3

[[e-mail]]

Comment on attachment 131393 [details] [diff] [review]
Cleans up help.js contextHelp.js

[Not sure why you would want to change a bunch of var to const.]

>-const NSRESULT_RDF_SYNTAX_ERROR = 0x804e03f7; 
>+const NSRESULT_RDF_SYNTAX_ERROR = 0x804e03f7;
[Pity this isn't in Components.results :-( Oh well.]

>+  // Load help RDF.
>   loadHelpRDF();
Silly.

>-  // move to right end of screen
>-  var width = document.documentElement.getAttribute("width");
>-  var height = document.documentElement.getAttribute("height");
>-  window.moveTo(screen.availWidth-width, (screen.availHeight-height)/2);
>+  // Move to Center of Screen
>+  const width = document.documentElement.getAttribute("width");
>+  const height = document.documentElement.getAttribute("height");
>+  window.moveTo((screen.availWidth - width) / 2, (screen.availHeight - height) / 2);
Why the change in help position? [In fact, why move it at all?]

Also, review flags messed up somehow, so clearing.

Comment 4

[R.J. Keller]

Neil: Why would you want them as variables? It just seems easier from a
readability standpoint that these variables values do not change and that you
can assume that they do not change. If a new person were to read help.js, they
might not assume that those variables values do not change.

Also, removing BrowserBack() and BrowserForward() will not break things once bug
214678 is checked in. This patch assumes that that is checked in.

Comment 5

[R.J. Keller]

Attachment: Patch with Neils comments

Comment 6

[[e-mail]]

Comment on attachment 131398 [details] [diff] [review]
Patch with Neils comments

OK, so a whitespace nit: some of the wrapped lines need indenting when you
change the var to const.

Comment 7

[R.J. Keller]

Comment on attachment 131398 [details] [diff] [review]
Patch with Neils comments

jag, could you sr?

Comment 8

[[e-mail] (gone - use [e-mail] instead)]

+  // Try to find previosly opened help for content pack.

spelling.              ^

Comment 9

[R.J. Keller]

Attachment: Patch with more cleanup

This patch removes BrowserReload and BrowserReloadWithFlags. They are no longer
needed once the right-click menu is changed.

Also fixed the typo that Stephen pointed out.

Comment 10

[Michel Bougon]

Help About crashes:

XML Parsing Error: undefined entity Location:
jar:resource:///chrome/en-US.jar!/locale/en-US/global/about.xhtml Line Number
95, Column 15:<li>Copyright &copy; 1998-2003 by <a
href="about:credits">Contributors</a> to --------------^

Comment 11

[Daniel Veditz [:dveditz]]

I'm not following mbougon's comment 10 -- is that a random addition to the wrong
bug or are you claiming you crash after applying this patch to your own tree?
This has not been checked in yet.

Comment 12

[[e-mail]]

No, comment #10 is a (now fixed) bug which affected some installer builds.

Comment 13

[[e-mail]]

Comment on attachment 131434 [details] [diff] [review]
Patch with more cleanup

What about Page Info and View Source, aren't they obsolete too?

Comment 14

[R.J. Keller]

Are they in help.js? I didn't think so.

Comment 15

[Alec Flett]

Comment on attachment 131434 [details] [diff] [review]
Patch with more cleanup

looks good. sr=alecf

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 131434 [details] [diff] [review]
Patch with more cleanup

> // Call this function to display a help topic.
> // uri: [chrome uri of rdf help file][?topic]
> function openHelp(topic) {
>+  // Try to find previously opened help for content pack.
>   var topWindow = locateHelpWindow(helpFileURI);
>+
>   if ( topWindow ) {
>+    // Open topic in existing window.
>     topWindow.focus();
>     topWindow.displayTopic(topic);
>   } else {
>-    var params = Components.classes["@mozilla.org/embedcomp/dialogparam;1"]
>+    // Open topic in new window.
>+    const params = Components.classes["@mozilla.org/embedcomp/dialogparam;1"]
>                            .createInstance(Components.interfaces.nsIDialogParamBlock);


It would be nice to continue to line up the . operator on the two above lines
as they were before.


>     params.SetNumberStrings(2);
>     params.SetString(0, helpFileURI);
>     params.SetString(1, topic);
>-    var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>+    const ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>                        .getService(Components.interfaces.nsIWindowWatcher);


Here, too.


>-    ww.openWindow(null, MOZ_HELP_URI, "_blank", "chrome,all,alwaysRaised,dialog=no", params);
>+    ww.openWindow(null, "chrome://help/content/help.xul", "_blank", "chrome,all,dialog=no", params);
>   }
> }
> 
> function setHelpFileURI(rdfURI) {
>-  helpFileURI = rdfURI; 
>+  helpFileURI = rdfURI;
> }
> 
>-// Locate mozilla:help window (if any) opened for this help file uri.
>+// Locate existing help window and loads helpFileURI.
> function locateHelpWindow(helpFileURI) {
>-  var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>+  const windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>   var windowManagerInterface = windowManager.QueryInterface( Components.interfaces.nsIWindowMediator);


Why not make the above const if you are changing other similar things?	Even
better, why not just get rid of the var line and just move the QI into the
getService() call?


>-  var iterator = windowManagerInterface.getEnumerator( "mozilla:help");
>+  const iterator = windowManagerInterface.getEnumerator( "mozilla:help");
>   var topWindow = null;
>+
>+  // Loop through the help windows looking for the one with the
>+  // current Help URI loaded.
>   while (iterator.hasMoreElements()) {
>     var aWindow = iterator.getNext();
>     if (aWindow.getHelpFileURI() == helpFileURI) {
>       topWindow = aWindow;
>-      break;  
>-    }  
>+      break;
>+    }
>   }
>   return topWindow;
> }
>Index: resources/content/help.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/help/resources/content/help.js,v
>retrieving revision 1.44
>diff -u -r1.44 help.js
>--- resources/content/help.js	11 Jun 2003 17:45:30 -0000	1.44
>+++ resources/content/help.js	14 Sep 2003 13:52:09 -0000
>@@ -39,25 +39,25 @@
> 
> // Resources
> var RDF = Components.classes["@mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService);


Is there a good reason you decided to not make the above a const, but did for
everything else?


>-var RDF_ROOT = RDF.GetResource("urn:root");
>-var NC_PANELLIST = RDF.GetResource(NC + "panellist");
>-var NC_PANELID = RDF.GetResource(NC + "panelid");
>-var NC_EMPTY_SEARCH_TEXT = RDF.GetResource(NC + "emptysearchtext");
>-var NC_EMPTY_SEARCH_LINK = RDF.GetResource(NC + "emptysearchlink");
>-var NC_DATASOURCES = RDF.GetResource(NC + "datasources");
>-var NC_SUBHEADINGS = RDF.GetResource(NC + "subheadings");
>-var NC_NAME = RDF.GetResource(NC + "name");
>-var NC_CHILD = RDF.GetResource(NC + "child");
>-var NC_LINK = RDF.GetResource(NC + "link");
>-var NC_TITLE = RDF.GetResource(NC + "title");
>-var NC_BASE = RDF.GetResource(NC + "base"); 
>-var NC_DEFAULTTOPIC = RDF.GetResource(NC + "defaulttopic"); 
>+const RDF_ROOT = RDF.GetResource("urn:root");
>+const NC_PANELLIST = RDF.GetResource(NC + "panellist");
>+const NC_PANELID = RDF.GetResource(NC + "panelid");
>+const NC_EMPTY_SEARCH_TEXT = RDF.GetResource(NC + "emptysearchtext");
>+const NC_EMPTY_SEARCH_LINK = RDF.GetResource(NC + "emptysearchlink");
>+const NC_DATASOURCES = RDF.GetResource(NC + "datasources");
>+const NC_SUBHEADINGS = RDF.GetResource(NC + "subheadings");
>+const NC_NAME = RDF.GetResource(NC + "name");
>+const NC_CHILD = RDF.GetResource(NC + "child");
>+const NC_LINK = RDF.GetResource(NC + "link");
>+const NC_TITLE = RDF.GetResource(NC + "title");
>+const NC_BASE = RDF.GetResource(NC + "base");
>+const NC_DEFAULTTOPIC = RDF.GetResource(NC + "defaulttopic");
> 
>-var RDFCUtils = Components.classes["@mozilla.org/rdf/container-utils;1"].getService().
>+const RDFCUtils = Components.classes["@mozilla.org/rdf/container-utils;1"].getService().
>    QueryInterface(Components.interfaces.nsIRDFContainerUtils);


Bonus points: move the QI into the getService() call.


>-var RDFContainer = Components.classes["@mozilla.org/rdf/container;1"].getService(Components.interfaces.nsIRDFContainer);
>-var CONSOLE_SERVICE = Components.classes['@mozilla.org/consoleservice;1'].getService(Components.interfaces.nsIConsoleService);
>-            
>+const RDFContainer = Components.classes["@mozilla.org/rdf/container;1"].getService(Components.interfaces.nsIRDFContainer);
>+const CONSOLE_SERVICE = Components.classes['@mozilla.org/consoleservice;1'].getService(Components.interfaces.nsIConsoleService);
>+
> var urnID = 0;
> var RE;
> 
>@@ -68,21 +68,28 @@
> var helpBaseURI;
> 
> const defaultHelpFile = "chrome://help/locale/mozillahelp.rdf";
>-// Set from nc:defaulttopic. It is used when the requested uri has no topic specified. 
>-var defaultTopic = "welcome"; 
>+// Set from nc:defaulttopic. It is used when the requested uri has no topic specified.
>+const defaultTopic = "welcome";
> var searchDatasources = "rdf:null";
> var searchDS = null;
> 
>-const NSRESULT_RDF_SYNTAX_ERROR = 0x804e03f7; 
>+const NSRESULT_RDF_SYNTAX_ERROR = 0x804e03f7;
> 
> // This function is called by dialogs/windows that want to display context-sensitive help
> // These dialogs/windows should include the script chrome://help/content/contextHelp.js
> function displayTopic(topic) {
>+  // Use default topic if topic is not specified.
>   if (!topic)
>     topic = defaultTopic;
>+
>+  // Get the page to open.
>   var uri = getLink(topic);
>-  if (!uri) // Topic not found - revert to default.
>-      uri = getLink(defaultTopic); 
>+
>+  // Use default topic if specified topic is not found.
>+  if (!uri)
>+      uri = getLink(defaultTopic);


Note: you are touching this line to fix whitespace, but you are still leaving
broken whitespace.  This file claims and uses c-basic-offset: 2 but you leave
it as c-basic-offset: 4


>+
>+  // Load the page.
>   loadURI(uri);
> }
> 
>@@ -96,6 +103,7 @@
>   helpGlossaryPanel = document.getElementById("help-glossary-tree");
>   helpBrowser = document.getElementById("help-content");
> 
>+  // Get the help content pack, base URL, and help topic
>   var helpTopic = defaultTopic;
>   if ("arguments" in window && window.arguments[0] instanceof Components.interfaces.nsIDialogParamBlock) {
>     helpFileURI = window.arguments[0].GetString(0);
>@@ -107,19 +115,15 @@
> 
>   displayTopic(helpTopic);
> 
>-  // move to right end of screen
>-  var width = document.documentElement.getAttribute("width");
>-  var height = document.documentElement.getAttribute("height");
>-  window.moveTo(screen.availWidth-width, (screen.availHeight-height)/2);
>-
>-  var sessionHistory =  Components.classes["@mozilla.org/browser/shistory;1"]
>+  // Initalize history.
>+  getWebNavigation().sessionHistory = Components.classes["@mozilla.org/browser/shistory;1"]
>                   .createInstance(Components.interfaces.nsISHistory);


And finally, more inconsistent indentation caused by your changes.

Comment 17

[R.J. Keller]

> Even
> better, why not just get rid of the var line and just move the QI into the
> getService() call?

A getService() call? Do you mean like a global getService() call? I don't think
I understand what you're saying.

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

I mean instead of:

  var foo = Components.classes["foo"].getService().
                       QueryInterface(Components.interfaces.nsIFoo);

do:

  var foo = Components.classes["foo"].getService(Components.interfaces.nsIFoo);

Comment 19

[Daniel Wang]

Search is broken in 2003100609 build:

  Error: tree has no properties
  Source File: chrome://help/content/help.js
  Line: 547

Please investigate

Comment 20

[R.J. Keller]

Attachment: Patch with even more cleanup!

This patch removes BrowserViewSource(), BrowserPageInfo() and related
functions. Also applies caillon's comments.

Comment 21

[R.J. Keller]

Comment on attachment 133039 [details] [diff] [review]
Patch with even more cleanup!

caillon, can you review this? As owner of the Help module, I'm giving you
permission to review this (brenden said I could do that, so I am!). You've done
such a good job reviewing before...

also asking alecf for sr.

Comment 22

[R.J. Keller]

BTW, this patch fixes bug 221632.

Comment 23

[[e-mail]]

Comment on attachment 133039 [details] [diff] [review]
Patch with even more cleanup!

>-// Locate mozilla:help window (if any) opened for this help file uri.
>+// Locate existing help window (if any) and loads the helpFileURI parameter.
The "old" comment was more accurate...

Comment 24

[[e-mail]]

Comment on attachment 133039 [details] [diff] [review]
Patch with even more cleanup!

>+ *    Original author: [e-mail]
Hmm... this should probably say [e-mail] (Original Author)

Comment 25

[R.J. Keller]

sure, I'll do that when I check in.

(actually, it should probably be changed to [e-mail], his working
email)

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 133039 [details] [diff] [review]
Patch with even more cleanup!

>Index: help.js


>@@ -39,24 +41,23 @@
> 
> // Resources
> var RDF = Components.classes["@mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService);


Why is this still not const?


>-var RDF_ROOT = RDF.GetResource("urn:root");
>-var NC_PANELLIST = RDF.GetResource(NC + "panellist");
>-var NC_PANELID = RDF.GetResource(NC + "panelid");
>-var NC_EMPTY_SEARCH_TEXT = RDF.GetResource(NC + "emptysearchtext");
>-var NC_EMPTY_SEARCH_LINK = RDF.GetResource(NC + "emptysearchlink");
>-var NC_DATASOURCES = RDF.GetResource(NC + "datasources");
>-var NC_SUBHEADINGS = RDF.GetResource(NC + "subheadings");
>-var NC_NAME = RDF.GetResource(NC + "name");
>-var NC_CHILD = RDF.GetResource(NC + "child");
>-var NC_LINK = RDF.GetResource(NC + "link");
>-var NC_TITLE = RDF.GetResource(NC + "title");
>-var NC_BASE = RDF.GetResource(NC + "base"); 
>-var NC_DEFAULTTOPIC = RDF.GetResource(NC + "defaulttopic"); 
>-
>-var RDFCUtils = Components.classes["@mozilla.org/rdf/container-utils;1"].getService().
>-   QueryInterface(Components.interfaces.nsIRDFContainerUtils);
>-var RDFContainer = Components.classes["@mozilla.org/rdf/container;1"].getService(Components.interfaces.nsIRDFContainer);
>-var CONSOLE_SERVICE = Components.classes['@mozilla.org/consoleservice;1'].getService(Components.interfaces.nsIConsoleService);
>+const RDF_ROOT = RDF.GetResource("urn:root");
>+const NC_PANELLIST = RDF.GetResource(NC + "panellist");
>+const NC_PANELID = RDF.GetResource(NC + "panelid");
>+const NC_EMPTY_SEARCH_TEXT = RDF.GetResource(NC + "emptysearchtext");
>+const NC_EMPTY_SEARCH_LINK = RDF.GetResource(NC + "emptysearchlink");
>+const NC_DATASOURCES = RDF.GetResource(NC + "datasources");
>+const NC_SUBHEADINGS = RDF.GetResource(NC + "subheadings");
>+const NC_NAME = RDF.GetResource(NC + "name");
>+const NC_CHILD = RDF.GetResource(NC + "child");
>+const NC_LINK = RDF.GetResource(NC + "link");
>+const NC_TITLE = RDF.GetResource(NC + "title");
>+const NC_BASE = RDF.GetResource(NC + "base"); 
>+const NC_DEFAULTTOPIC = RDF.GetResource(NC + "defaulttopic"); 
>+
>+const RDFCUtils = Components.classes["@mozilla.org/rdf/container-utils;1"].getService(Components.interfaces.nsIRDFContainerUtils);
>+const RDFContainer = Components.classes["@mozilla.org/rdf/container;1"].getService(Components.interfaces.nsIRDFContainer);
>+const CONSOLE_SERVICE = Components.classes['@mozilla.org/consoleservice;1'].getService(Components.interfaces.nsIConsoleService);
>             


>Index: contextHelp.js
>===================================================================

>@@ -45,12 +48,14 @@
>   helpFileURI = rdfURI; 
> }
> 
>-// Locate mozilla:help window (if any) opened for this help file uri.
>+// Locate existing help window (if any) and loads the helpFileURI parameter.
> function locateHelpWindow(helpFileURI) {
>-  var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>-  var windowManagerInterface = windowManager.QueryInterface( Components.interfaces.nsIWindowMediator);
>-  var iterator = windowManagerInterface.getEnumerator( "mozilla:help");
>+  const windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator);  const windowManagerInterface = windowManager.QueryInterface( Components.interfaces.nsIWindowMediator);

Really long lines suck.  Wrap at 80, please and give each statement its own
line.  Fix the space on the second QI?	(see my next comment for details)


>+  var iterator = windowManager.getEnumerator( "mozilla:help");


Care to fix the whitespace here, too?  I think the file's style is foo("bar")
but I can't decide between that and foo( "bar" ).  Either way, there's a space
too many or a space too few.  I'd lean toward the former.


>   var topWindow = null;
>+
>+  // Loop through the help windows looking for one with the
>+  // current Help URI loaded.
>   while (iterator.hasMoreElements()) {
>     var aWindow = iterator.getNext();
>     if (aWindow.getHelpFileURI() == helpFileURI) {

Comment 27

[[e-mail]]

Comment on attachment 133039 [details] [diff] [review]
Patch with even more cleanup!

Final comments:

>-  // move to right end of screen
>-  var width = document.documentElement.getAttribute("width");
>-  var height = document.documentElement.getAttribute("height");
>-  window.moveTo(screen.availWidth-width, (screen.availHeight-height)/2);
Why is this being removed? Hmmm... it doesn't work too well on Linux, does it
:-/

>+  const windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator);  const windowManagerInterface = windowManager.QueryInterface( Components.interfaces.nsIWindowMediator);
I don't think you're using windowManaagerInterface any more. I expect you must
have joined the lines by mistake.

Comment 28

[R.J. Keller]

Attachment: Patch with Neil and Caillon's comments.

> Why is this being removed? Hmmm... it doesn't work too well on Linux, does it

> :-/

well, you said in comment #3, why move it at all. I agree that Help shouldn't
really be moved. The code does have problems on Linux, but that's not why I
removed it.

> I don't think you're using windowManaagerInterface any more.

yeah, I'm going to start using Kate on Linux for editing. Notepad has angered
me for the last time.


BTW, caillon, I kept the long lines in help.js because it looked better. I did
get rid of the long line you  noted in contextHelp.js.

Comment 29

[R.J. Keller]

Comment on attachment 133146 [details] [diff] [review]
Patch with Neil and Caillon's comments.

Even though I'm asking Neil to review this, I'd like caillon to look at this as
well. This is a big change and I'd like to get as many eyes on it as possible.

Comment 30

[R.J. Keller]

Attachment: Patch - Fixes error

This fixes a comment error.

Comment 31

[Alec Flett]

Comment on attachment 133151 [details] [diff] [review]
Patch - Fixes error

sr=alecf

Comment 32

[R.J. Keller]

Fix checked in.

Comment 33

[Malcolm Rowe]

> // Set from nc:defaulttopic. It is used when the requested uri has no topic 
specified. 
> -var defaultTopic = "welcome"; 
> +const defaultTopic = "welcome"; 

Did you really mean this?  At line 143, we have:
> defaultTopic = getAttribute(helpFileDS, RDF_ROOT, NC_DEFAULTTOPIC, "welcome");

See 
http://lxr.mozilla.org/seamonkey/source/extensions/help/resources/content/help.j
s#143

Comment 34

[[e-mail]]

Also I've noticed why you didn't use const RDF before - there's a conflicting
var RDF definition in bookmarks.js (why is that getting included?)

Comment 35

[[e-mail]]

Attachment: More cleanup

Comment 36

[R.J. Keller]

Comment on attachment 133224 [details] [diff] [review]
More cleanup

Code looks good.

Built with the patch just to be sure and have seen no JS errors and everything
works fine.

r=[e-mail]

Comment 37

[Daniel Wang]

-const defaultTopic = "welcome"; 
+var defaultTopic = "welcome"; 

Why the change?

Comment 38

[[e-mail]]

Attachment: Fixed patch

Good catch...

Comment 39

[[e-mail]]

Comment on attachment 133224 [details] [diff] [review]
More cleanup

Brain fart :-(

Comment 40

[[e-mail]]

Comment on attachment 133271 [details] [diff] [review]
Fixed patch

Comment 33 answers the question.

Comment 41

[Alec Flett]

Comment on attachment 133224 [details] [diff] [review]
More cleanup

why change defaultTopic to a var from const? Seems like it was perfectly good
with a const

sr=alecf with that changed back or with a good explanation :)

Comment 42

[[e-mail]]

alecf, the explanation is that we do theoretically assign to it...

Quoting comment 33:

defaultTopic = getAttribute(helpFileDS, RDF_ROOT, NC_DEFAULTTOPIC, "welcome");

Comment 43

[Daniel Wang]

v