309848 - Three minor bugs in help viewer's parsing of help content packs

Status: RESOLVED FIXED

(SeaMonkey :: Help Viewer, defect)

Description

[Jeff Walden (gone starting June 8)]

I ran across three small bugs in the help viewer while working on a port of bug
296012 to SeaMonkey's help viewer (see bug 309837); at least one was definitely
introduced when bug 296012 was fixed, and the other two were most likely
introduced then as well.  The bugs are:

== Customized text/link for no search results fail if nc:datasources="" ==
In the loop in which we run through all the defined panels in a content pack, we
bail early if a panel's datasources are "" as a short-circuit.  In the general
case this is fine, but if we're working with a search panel we never get to the
code that sets the search text and link to the ones the content pack defines. 
This is testable in current Firefox builds; search for "zx" and note that the
search string and link don't work as the Firefox content pack file says they
should.  A workaround to this problem would be to set nc:datasources to a
datasource which contains no assertions the help viewer will parse.

== Defining nc:datasources="rdf:null" probably fails ==
The Firefox help content pack doesn't do this, opting instead to use an empty
nc:datasources.  SeaMonkey does, however, and attempting
nsIRDFService.GetDataSourcesBlocking on "rdf:null" throws an XPCOM exception
that breaks the SeaMonkey help viewer.  (This fails in my partially-patched
SeaMonkey build; it works in vanilla SeaMonkey because search datasources are
stored in a string, and loadCompositeDS filters out empty string datasources and
rdf:null.)  There's no protection for this in toolkit help viewer code, so it
probably will fail there, too.  The workaround here is to use "" or an empty
datasource instead of "rdf:null" (although this is contrary to how the
datasources attribute in XUL is to be used).

== indexOf() doesn't guarantee the user's platform is in a platforms list ==
If the user's platform is a substring of a platform included in a node's
nc:platform, then the node will be incorrectly assumed to apply on the user's
platform.  For example, a user on "windows" will be able to view a node with
nc:platform="notwindows".  This problem is not present in the platform-specific
panel code because the panel list is split(/\s+/) and indexOf() will work
correctly on an array, but in node platform processing indexOf() works on a
string and can thus fail.  (Note that this is not a problem with the current
list of platforms we support, but it's a theoretical future problem.)

None of these are show-stopping bugs in what we want to claim to be able to do;
all are currently non-issues or have workarounds.  However, the bugs are
probably easily solvable with very minimal risk, so I plan to have a patch in
the next couple days or so to fix the problems before 1.5.

Comment 1

[Jeff Walden (gone starting June 8)]

Attachment: Patch

I explicitly replace the absence of an nc:datasources attribute with "rdf:null"
instead of the empty string because "rdf:null" has to be filtered out at some
time anyway if it's specified for an attribute, so we might as well limit the
number of filters to just one for "rdf:null".  I also think that "rdf:null" is
clearer than just an empty string.

Comment 2

[Jeff Walden (gone starting June 8)]

The bugs enumerated here are all minor, and all can be worked around.  However,
the workarounds aren't very pretty, and the code to fix the bugs is short and
simple, so I'd like to see this get on the branch.

Comment 3

[Asa Dotzler [:asa]]

not a blocker but we'd evaluate a fully reviewd patch.

Comment 4

[Jeff Walden (gone starting June 8)]

Comment on attachment 197514 [details] [diff] [review]
Patch

Neil says he won't have time to get to this.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 197514 [details] [diff] [review]
Patch

r=mano.

Comment 6

[Jeff Walden (gone starting June 8)]

Comment on attachment 197514 [details] [diff] [review]
Patch

This patch fixes three small bugs in how help content packs are parsed.  One of
the problems makes it annoying to specify a page to load when a search through
the available help docs returns no results, and the other two could result in
failure of the viewer to load properly on what have historically been valid
content packs.

There should be very little risk here for several reasons.  First, the
magnitude of the changes here is small.  Second, most of the code changes are
small tweaks to evaluation order which are easy to verify to be correct. 
Third, the changes that involve actual function are trivial one-method changes.
 Fourth, the help viewer isn't used all that much outside of Firefox help
documentation, so in the extremely unlikely event that any problems were to be
introduced (note that I do *not* think this will happen), they would have
little impact on the toolkit-using community in general.

I can check this in on trunk first if desired, but I'd prefer to check in to
both branch and trunk simultaneously to ensure things are synced.

Comment 7

[Jeff Walden (gone starting June 8)]

Patch checked into both branch and trunk.