296012 - Support an optional nc:platform attribute on items in Help content pack trees

Status: RESOLVED FIXED

(SeaMonkey :: Help Viewer, enhancement)

Description

[Jeff Walden (gone starting June 8)]

Bug 254992 was supposed to make platform-specific Help content easy.  If you're
willing to split up your content into separate files and you have minimal
platform-specific information, this can work.  However, for an app like Firefox
whose functionality differs between platforms too much, this doesn't work (see
bug 289776 comment 1).  It also doesn't work for item ordering: if you have a
platform-specific node in between generic nodes, the only way I can see to
represent it is to have three separate URIs (and possibly three separate files -
I don't remember if passing an RDF resource as URL+ID works here or not).

The solution is the one that's been proposed before: add an nc:platform
attribute that describes on which platforms a node applies.  Before now, it
wasn't happening because I didn't understand the code well enough to write a
patch and no one else cared enough to write one for me.  Now, tho, I think I'm
getting somewhere, so I'm refiling the bug, and this time for nc:platform
specifically.  My current patch is tested and works at processing the current
Help stuff (which has no nc:platform attributes); I'm working on a testcase to
see whether or not the new functionality works as well.  The other issue I'm
sorting through now is what syntax is needed to implement a node whose children
are generic but whose nc:title should vary depending on platform.

This is big for finally fully platform-izing Firefox Help, which is approaching
a l10n freeze within the next few weeks, so I plan to get the patch done in time
to do that.  An equivalent patch for extensions/help to maintain content pack
compatibility may come afterward, but with time being more critical now it's not
likely to be much of a priority until after l10n freeze.

Comment 1

[Jeff Walden (gone starting June 8)]

So, for most practical uses I'm 100% finished with the patch.  However, most
practical uses don't cover working correctly with additional, hidden search
datasources.  If you eliminate that edge case everything works.

I'll attach the patch in a second, along with the testcase I've been using ever
since I got my work to a state of near-completion.  I don't think it should be
difficult to take what I've done and extend it that little extra bit to make it
work completely, but I'm honestly not sure how much longer that will take.  I
think I know what the problem is, but I don't know how to fix it without what
would amount to a rewrite of it (and resorting to other tricks I don't want to
do.  Consequently, I'm posting it and asking for a little help (tho naturally
I'll keep working at it as well).

Comment 2

[Jeff Walden (gone starting June 8)]

Attachment: The not-quite-complete patch

Here's how the patch works:

The input from the user for which datasources to use comes in the form of a
whitespace-separated string of URIs (one for each panel definition).  Before,
we'd count on the builder to rebuild based on a change in the datasources
attribute on the tree.	Now, we do it ourselves because we're ignoring the
datasources attribute entirely and working with properties of the tree itself.

First, we take our list of URIs for the current panel and from it create an
array of nsIRDFDataSources corresponding to the URIs in the list.  In the
general case we then use tree.database.AddDataSource to add each retrieved
datasource to the tree.  If we were to manually force a tree rebuild at this
point, we'd be shown exactly the same thing we'd see with what exists
currently.  However, tree.database is raw data possibly full of
platform-specific information that has to be removed.  Therefore, we have to
filter it, and only then can we rebuild it and go to the next panel definition.


Changing from an attribute-based system to a property-based system was
surprisingly easy.  I had to rewrite GetLink to work with database instead of
datasources, but it was actually pretty trivial.  Given the ID, we create an
nsIRDFResource from it and use GetTarget on the contents datasource to get the
link value, which is then returned.  This should be pretty easy to understand
if you have some experience with Mozilla's RDF interfaces.  I also got to
eliminate some code which relied on a datasources attribute being around when a
tree hadn't been displayed (because consequently its database hadn't been built
yet).

The fun part came in implementing the filtering.  Filtering is implemented by
three separate functions: filterDatasource, filterNode, and filterSeq. 
filterDatasource is the "public" function, and the top-level things happen
through it.  filterNode and filterSeq are used underneath filterDatasource to
go through an RDF graph.  filterNode acts upon individual rdf:Description
elements which have been predetermined to contain nc:subheadings and then calls
filterSeq on the rdf:Seq's found inside.  (This is safe because filterSeq
checks before it calls filterNode, and the only other time occurs when we're
processing the topmost node beneath urn:root - and we can require there to be
nc:subheadings there.)

filterSeq is where most of the work happens.  We get a local enumerator from an
nsIRDFContainer created for the rdf:Seq.  Then we iterate through, testing each
item for an nc:platform that indicates the item's for other platforms.	If the
item's for another platform, we remove it from the nsIRDFContainer and move on
to the next item.  If it isn't, we test for nc:subheadings, calling filterNode
if there is one and continuing if there isn't.

Anyway, that about covers how the patch works.	The problem is that databases
associated with trees get filtered properly while the database for search
datasources that's stored as a global JS object does not.  More correctly, when
we call filterDatasource on each tree's database, the database is mutated. 
When we call it on searchDS, however, I think we're only mutating the arguments
passed to filterDatasource, but the arguments aren't bound to the same location
as searchDS is, so when filterDatasource completes we've got a filtered
database and the original, unmutated JS object.  As a result, the search
datasources aren't filtered.  I really don't know how to fix this short of
rewriting filterDatasource to create and return a newly created datasource
(which is memory-wasteful and probably unnecessary), so any help here is
appreciated.

Comment 3

[Jeff Walden (gone starting June 8)]

Attachment: Testcase

You'll have to use the JavaScript console to use this testcase.  I never
defined any external UI or keyboard shortcuts and instead relied on my
lightning-fast paste-fu (cutting wasn't a problem because my editor of choice,
nano, doesn't touch the clipboard unless I intentionally use the GNOME terminal
shortcut to do so) to quickly open and close the Help viewer with the testcase
content pack loaded.  You'll need to enter the following code in the evaluation
textbox (it does *not* have to be in a single line, interestingly):


const
p=Components.classes["@mozilla.org/embedcomp/dialogparam;1"].createInstance(
Components.interfaces.nsIDialogParamBlock);p.SetNumberStrings(2);p.SetString(
0,'chrome://foobar/locale/firebirdhelp.rdf');p.SetString(1,'welcome');
Components.classes["@mozilla.org/embedcomp/window-watcher;1"].getService(
Components.interfaces.nsIWindowWatcher).openWindow(null,
"chrome://help/content/help.xul","_blank","chrome,all,alwaysRaised,dialog=no"
,p);


You'll probably want to take a look at the code behind it as well.  All the
relevant files are in predictable places.  To test that added search
datasources don't work, just try searching for "only" - you'll get a whole
bunch of (undisplayable) results that claim to be for some combination of
platforms *ONLY*.  Testing that regular datasources work is a little more
tricky, but I think a search for "pref" should work.  (Don't quote - I
developed the patch entirely on Linux and everything's sort of biased toward
being testable in a Linux environment.)

Anyway, help is greatly, greatly appreciated, especially from RDF gurus.  :-) 
I still want to see this in time to use it for 1.1 l10n work, and time's
quickly running out.

Comment 4

[Jeff Walden (gone starting June 8)]

Comment on attachment 185132 [details]
Testcase

Incidentally, don't expect displayable content for most of the items in the
trees, because I didn't care enough to give items working links.  I know things
still work because the error pages indicate the proper page gets loaded, and
that was all I cared about.

Comment 5

[Mike Connor [:mconnor]]

this is going to be fun to merge, maybe :)

maybe this is out-of-scope, but in the Help Viewer UI whacking, the
glossary/index trees are hidden, and I'd like to kill that entirely and just
hook into the datasource directly.  Is this going to hurt that?

Comment 6

[Jeff Walden (gone starting June 8)]

(In reply to comment #5)
> maybe this is out-of-scope, but in the Help Viewer UI whacking, the
> glossary/index trees are hidden, and I'd like to kill that entirely and just
> hook into the datasource directly.  Is this going to hurt that?

The setup as it currently stands relies on there being a <tree/> for
index/glossary.  The tree doesn't actually have to be displayed, tho, so as a
short-term hack we could always just keep the trees in source with display:none
and still be able to use them pretty much as they're used now.  (Come to think
of it, we could do this for the recalcitrant search datasources as well.)  We'd
simply use a tree.database as in this patch for the toc and then lump all the
other datasources together and add them to a hidden tree the exact same way. 
The method for modifying and accessing each database would be exactly the same,
so theoretically there wouldn't be any bugs.

The only question is how much hackishness we're willing to allow for 1.1.  The
idea in the last paragraph is definitely a hack, and if I had a better idea how
to fix the problems I'd do so.  However, I don't, so unless someone figures out
the problem I'm all for using hidden <tree/> elements for the datasources.  I
know I'm going to be around for the forseeable future, so I'm not concerned
about a small bit of hackishness like this because I know it'll get fixed. 
Anyway, what's important is that these are merely implementation details; these
changes will not affect what toolkit users will see when writing content packs.

Comment 7

[Jeff Walden (gone starting June 8)]

Comment on attachment 185132 [details]
Testcase

Erhmm...this breaks on the built-in Help pack, it seems.  I'll probably
investigate tomorrow.

Comment 8

[Jeff Walden (gone starting June 8)]

Attachment: Fixes existing help content

This is the patch, fixed up to work with existing Firefox Help content.  The
main problem I found was that apparently "#rdfid" will not get the correct
resource to load a topic based on its ID, so I had to go back to iterating
through the list of toc datasources and getting the first one that returned
something useful.  My previous testing method hadn't caught this because
getLink() wasn't being exercised with that execution path.  I also got rid of
the definition of LoadCompositeDS(), which was unnecessary, and probably fixed
up a few other things I've forgotten in the meantime.  Use the patch viewer to
compare this patch and the old one to see exactly what I've changed in full
detail.

I also tried testing it with the two other Help content packs that I know exist
(both are at <http://firebirdhelp.mozdev.org/hce/>.  The first, Creating
Applications with Mozilla, failed with error -239 (chromereg error) and so was
untestable.  The Calendar Help extension didn't even install, so I went to the
project site at <http://calendarhelp.mozdev.org/installation.html> and tried
out the latest code.  It also didn't work.  I did some logging at various
points in the code and eventually figured out that the problem occurs when you
call what's effectively the following line of code:

Components.classes["@mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService).GetDataSourceBlocking("chrome://calendarhelp/locale/index.rdf");


Try loading the latest Calendar Help extension and running that in the JS
Console.  Every time I try, I get an error.  Now, run the same thing but remove
"Blocking" from the command.  For me, it inexplicably works, even tho both
calls just call the exact same method with only one difference in parameters
(true/false), and that one difference only applies in one place within the code
<http://lxr.mozilla.org/mozilla/source/rdf/base/src/nsRDFService.cpp#1564>. 
The odd thing is it's an NS_ERROR_UNEXPECTED, which looks to be at line 1559,
except that code hasn't been touched in any substantial way since before
Firefox 1.0, yet both GetDataSource and its blocking companion work correctly
there.	I don't know what the problem is.

Anyway, tho, these are the only two Help content packs I know of that were
designed to be compatible with Firefox Help.  I'm not particularly worried
about the incompatibilities, because the documentation state for Help content
packs right now makes it almost certain that such problems will exist.	Once
bug 295817 is fixed, I'll revisit this and retool the patch to fit with the
source code then.  Maybe we'll pick up calendarhelp then.  (For Calendar Help,
tho, the key is to get toolkit Help fully app-independent, because right now
they use some sort of hybrid of the toolkit viewer and a forked copy of it, and
if we get them to use the toolkit one it'll be a lot more likely they'll try to
keep things compatible in the long run.  It'll be in Sunbird and Firefox, and
the content pack should be compatible with Seamonkey still, so it's unlikely
they'll  keep forking their copy.)

I didn't touch the extra search datasource problems I was still fixing with the
last version, because the changes for bug 295817 will make them possibly
unneeded and probably a waste of time.	Once that's in I'll get back to that
issue, which hopefully can be easily avoided for the time being by just using a
tree.database for them.

Comment 9

[Jeff Walden (gone starting June 8)]

Comment on attachment 185443 [details] [diff] [review]
Fixes existing help content

If anyone feels like testing this patch, remove the semicolon at the end of the
line that initializes datasourceArray - things will almost certainly silently
fail otherwise.  I apparently edited the file after testing the code and didn't
edit it back -- oops.

Comment 10

[Jeff Walden (gone starting June 8)]

I need a working fix for this so that I can fix bug 289776, which must be fixed
before the 1.1 l10n freeze.  Consequently, I need this to block 1.8b3 so that I
can get bug 289776 in time for Firefox 1.1.

Comment 11

[Axel Hecht]

Comment on attachment 185443 [details] [diff] [review]
Fixes existing help content

+    var subheadings = aDatasource.GetTarget(currentTarget, NC_SUBHEADINGS,
true);
+    if (NS_RDF_NO_VALUE == subheadings)   // no subheadings, it's a leaf

This won't work, you need to check Components.lastResult.

Comment 12

[Benjamin Smedberg]

You've got a week for 1.8b4, or bust.

Comment 13

[Jeff Walden (gone starting June 8)]

Attachment: Final patch against trunk

This patch takes advantage of the hidden tree hack mentioned in comment 6. 
These trees were added for bug 295817 as a short-term hack because we don't
want too much code churn before 1.8 if it can be helped.  In addition to using
these hidden trees (glossary and index trees already existed), I added an
additional tree for hidden search databases.  It works extremely well for me,
and the previous problems I hit when attempting to store the composite search
datasource in a global variable are completely eliminated.  (This is, of
course, *not* to say this is a good long-term method; in the short-term,
however, it works well enough and reduces possible risk.  I intend to go back
and fix this pretty early in the 1.9 cycle.)

I removed a bunch of template code from within the hidden tree elements; the
patch works fine without it, and it probably slows down content pack loading
somewhat.  It might be possible to remove more of the code there, but I didn't
think it was a good idea to go too far there.  I only removed stuff I was
absolutely sure wouldn't affect functionality.

Please make sure to look at the line which uses |Components.lastResult|.  I'm
pretty sure what I do there is right, but I want to be certain I understood
comment 11 correctly.

All in all, tho, the patch is essentially the same as what's been posted
already.  Barring a few little stumbles, it didn't take a whole lot of work to
update to trunk, post-bug 295817.

I still hope there's a very small chance this could get in for 1.8, so I want
to wrap this up reasonably quickly, if at all possible.

Comment 14

[Jeff Walden (gone starting June 8)]

Attachment: Testcase (updated)

This is the final version of the testcase I've been using.  (I've been updating
it as I use it, mostly to make it easier to use.)  I prefixed the name of every
entry in any of the datasources with something like "index: " or "toc: ", so
it's clear what type of datasource supplies each entry.  You can use searches
for "win", "mac", or "unix" to test that things apply only where they're
supposed to apply.

Once again, use the following line in the JS console to test:

const p =
Components.classes["@mozilla.org/embedcomp/dialogparam;1"].createInstance(Components.interfaces.nsIDialogParamBlock);
p.SetNumberStrings(2);p.SetString(0,'chrome://foobar/locale/firebirdhelp.rdf');p.SetString(1,'welcome');Components.classes["@mozilla.org/embedcomp/window-watcher;1"].getService(Components.interfaces.nsIWindowWatcher).openWindow(null,
"chrome://help/content/help.xul", "_blank",
"chrome,all,alwaysRaised,dialog=no", p);

Comment 15

[[e-mail]]

Comment on attachment 188627 [details] [diff] [review]
Final patch against trunk

AFAIK a .database is just a composite data source so you should be able to
create one directly.

>+    // try to get the subheadings underneath this rdf:Seq
>+    var subheadings = aDatasource.GetTarget(currentTarget, NC_SUBHEADINGS, true);
>+    if (NS_RDF_NO_VALUE == Components.lastResult)   // no subheadings, it's a leaf
>+      continue;
>+    else  // it has subheadings
>+      filterNode(aDatasource, currentTarget, aCurrentLevel+1);
filterNode already checks for subheadings.

>+      link = link.QueryInterface(Components.interfaces.nsIRDFLiteral);
>+      if (link)
QueryInterface never returns null (it might throw though).

Comment 16

[Jeff Walden (gone starting June 8)]

Attachment: Patch with issues addressed

I removed the unneeded check mentioned in comment 15, and I got rid of the
null-check mentioned there as well.  Unless someone is deliberately making a
content pack that specifically does not set nc:link to be a URI in text format
(which I think would require using the non-attribute format, too), there
shouldn't be a problem here with any thrown exceptions.

I also got rid of a little debug cruft that had been accidentally left in the
previous patch, and I removed a couple other items that were unused in the
current code.  The patch is essentially the same, tho, as the one before.

Back when I was first working on this I attempted to use a global variable
containing the datasource just as Neil suggests.  However, something was going
wrong when I did so, because the datasource would invariably not be filtered. 
I suspect it's a problem related to how JavaScript determines whether to pass
by value or pass by reference, and I wasn't having a lot of success solving it.
 It still would be more code churn than we want, and I don't think the changes
required are worthwhile right now.

Comment 17

[Benjamin Smedberg]

What is the l10n impact of this change?

Comment 18

[Jeff Walden (gone starting June 8)]

(In reply to comment #17)
> What is the l10n impact of this change?

None whatsoever.  All existing content should continue to work correctly, so no
changes are needed.

I do intend to get some work done on bug 289776 and bug 299976 which will have
l10n impact and will depend on the functionality introduced here, but those
changes are completely separate and can be reviewed for l10n impact separately
from what's here when the time comes.

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 188709 [details] [diff] [review]
Patch with issues addressed

> const NSRESULT_RDF_SYNTAX_ERROR = 0x804e03f7;
>+const NS_RDF_NO_VALUE = 0x004f0002;

Should use NSRESULT_RDF_NO_VALUE here to match the other.

>+          datasourceArray.forEach(helpSearchPanel.database.AddDataSource);

I'd like to see an explicit this-obj parameter there, though in this case
the XPConnect setup probably saves us:

  datasourceArray.forEach(helpSearchPanel.database.AddDataSource,
			  helpSearchPanel.database);

>+        // cache toc datasources list for use in getLink()
>+        if (panelID == "toc") {
>+          gTocDSList += " " + datasources;
>         }

House style looks like it would omit the braces for this single-line
then clause.

>+        // add each datasource to the current tree
>+        datasourceArray.forEach(tree.database.AddDataSource);

Explicit this-obj again, please.  (I almost want another syntax for a
method reference that binds its this from the left-side object.)

>+# Remove statements for other platforms from a datasource.
>+function filterDatasource(aDatasource) {
>+# Remove statements for other platforms from the provided datasource.
>+function filterNode(aDatasource, aCurrentResource, aCurrentLevel) {
>+# filterSeq
>+# Go through the children of aNode, if any, removing statements applicable
>+# only on other platforms.

I like the function decomposition here, but I think I'd prefer to see the
functions named in a way that indicates what they're filtering.  (Until I
got to filterSeq, I was looking for a predicate that was passed as a
parameter.)  filterDatasourceByPlatform, etc.?

>+    if (!ID) return null;
>+
>+    var tocDS = document.getElementById("help-toc-panel").database;
>+    if (!tocDS) return null;

Separate lines for the returns.

r=shaver with those nits picked.

Comment 20

[Jeff Walden (gone starting June 8)]

Attachment: Patch for checkin

(In reply to comment #19)
> Should use NSRESULT_RDF_NO_VALUE here to match the other.

I removed it because it's no longer needed in the patch.

> >+	    // cache toc datasources list for use in getLink()
> >+	    if (panelID == "toc") {
> >+	      gTocDSList += " " + datasources;
> >	    }
> 
> House style looks like it would omit the braces for this single-line
> then clause.

Just for kicks I did a count through most of the current version and found at
least 10 instances with braces, a roughly equal number without, and a couple
instances of ifs with the then clause on the same line.  "house style" indeed! 
That said, I made the style changes because they fit perfectly -- an(y)
arbitrary choice of style is *exactly* how we do things.  :-)  Until bug 265687
is fixed, I declare help viewer house style to be "hodgepodge".

> filterDatasourceByPlatform, etc.?

Sure, that naming works with me.

Comment 21

[Jeff Walden (gone starting June 8)]

The changes in help.xul and help.js have been checked into source, so this is
now FIXED.