309837 - Port fix to bug 296012 to Suite's help viewer

Status: RESOLVED WONTFIX

(SeaMonkey :: Help Documentation, defect)

Description

[Jeff Walden (gone starting June 8)]

For purposes of content pack compatibility, I want to support nc:platform
attributes on items within Help datasources.  I hope to get the fix in the 1.8
branch as that's where we're getting SeaMonkey 1.0.  As the code is
SeaMonkey-only, I think there's a reasonable chance the patch will be approved
for the branch (especially as Firefox Help has been using and abusing the
functionality for the past couple months with no ill effects).

Comment 1

[Jeff Walden (gone starting June 8)]

Attachment: Most-of-the-way patch

This patch works most of the way, but it doesn't function properly on hidden
search datasources (datasources whose contents aren't displayed in UI but whose
information is reflected in searches).	You can test this using a testcase
extension I'll post in a second.  The testcase will be enough to show that
filtering works, but beyond that don't expect most of the help items in the
ocntent pack to actually work.

I'm pretty sure the problem is that I'm passing the hidden search datasource by
value instead of by reference, so filterDatasourceByPlatform filters the copy
it has and then throws it away when the function completes.  Logging at
appropriate places shows that the search datasource does get filtered as
expected, but the filtering doesn't actually stay.  I'm probably going to ask
for help with this on #developers, because I ran into exactly this same problem
when working on the patch for bug 296012 and didn't make any progress.	We
could use the hack I used there again (add a hidden tree to help.xul which is
used to hold the hidden search datasources), but I'm less happy about adding
hacks to code that I don't have a vested interest in keeping hack-free.

Comment 2

[Jeff Walden (gone starting June 8)]

Attachment: Testcase

This should work on both SeaMonkey and Firefox; use the created top-level menu
and contained menuitem to access the test content pack.  The ToC should contain
a "Popup Blocker *" item, where * is "Options" on win/os2 and "Preferences" on
unix/mac.  The glossary should contain a bunch of items, none of which should
be marked as not for the user's platform; ditto for index.  Searching for
"win", "mac", or "unix" should return items which don't say they're for another
platform.  This is where the problem is; anything prepended with "search: " is
from the hidden search datasources, and a platform search will show that these
datasources aren't filtered.  That's where my current problem lies.

Also, while working on the patch I discovered three minor bugs in my previous
patch.	They're filed as bug 309848, and this patch shouldn't introduce any of
them in SeaMonkey's help viewer.

Comment 3

[Jeff Walden (gone starting June 8)]

Attachment: Patch (includes an ugly hack due to time constraints)

This patch is complete and should work correctly.  It uses the hack I mentioned
earlier (putting a hidden search datasource in help.xul for use in storing
hidden search databases), however, because I've had no luck figuring out why
the problem I've run into previously even exists.  If this is to get in on the
branch, I need to get it reviewed *now*, and I'm not going to solve this
problem in time to do so.

There's already a bug filed to fix the hack in toolkit's help viewer, so I'll
just piggyback fixing the hack here onto that bug.  The fix should be pretty
much identical, so it's very little extra effort.

I'm not sure who I want to ask to review this (Neil has said he won't have time
for an even smaller patch, so I know he won't have time here).

Comment 4

[jag (Peter Annema)]

Comment on attachment 198893 [details] [diff] [review]
Patch (includes an ugly hack due to time constraints)

>Index: extensions/help/resources/content/help.js
>===================================================================
>@@ -281,41 +347,29 @@

<snip/>

>+  for (var i = 0; i < tocDSArray.length; i++) {
>+    var resource = RDF.GetResource(tocDSArray[i] + "#" + ID);
>+    var link = tocDS.GetTarget(resource, NC_LINK, true);
>+    if (!link) // no such rdf:ID found
>       continue;
>+    return link.QueryInterface(Components.interfaces.nsIRDFLiteral).Value;
>   }
>-  return null;
> }

You probably still want that |return null;| there, and put in the "Right
Way(tm)" code in there, but commented out, with a helpful comment like "that is
how we'd like to do it, but because of bug XXXXXX we need to do it this way for
now"

r= or sr=jag with that, whichever you need.

Comment 5

[Jeff Walden (gone starting June 8)]

Attachment: Patch with comments addressed

Comment 6

[Jeff Walden (gone starting June 8)]

Comment on attachment 198893 [details] [diff] [review]
Patch (includes an ugly hack due to time constraints)

Removing review request from obsoleted patch...

Comment 7

[[e-mail]]

Comment on attachment 199473 [details] [diff] [review]
Patch with comments addressed

This patch doubles the time it takes to open help :-(

Comment 8

[Jeff Walden (gone starting June 8)]

(In reply to comment #7)
> This patch doubles the time it takes to open help :-(

...which is no longer than it takes to run a search through a content pack.

Comment 9

[Serge Gautherie (:sgautherie)]

Jeff, alecf,
Are you still working on this ?

Comment 10

[Jeff Walden (gone starting June 8)]

No, not really; I really haven't been active in help stuff at all in the last couple years.

Comment 11

[Ian Neal]

As we have moved away from using extensions/help a while back now, this should probably be marked as INVALID now...

Comment 12

[Philip Chee]

As per comment 11 => WONTFIX.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 199473 [details] [diff] [review]
Patch with comments addressed

extensions/help is gone and this bug is marked as wontfix. Therefore clearing obsolete review request.