move suiterunner help over to toolkit help viewer

RESOLVED FIXED

Status

defect
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: kairo, Assigned: kairo)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 10 obsolete attachments)

8.81 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.80 KB, patch
mark
: review+
Details | Diff | Splinter Review
10.77 KB, patch
neil
: review+
Details | Diff | Splinter Review
26.03 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.00 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Assignee

Description

13 years ago
For suiterunner, we should use the toolkit help viewer for the help window, and we should move over help content to suite/locales/

I'm working on a patch to achieve that, I should have a preliminary version very soon.
Assignee

Comment 1

13 years ago
Here's the way I want to go for this.
The patch may not apply cleanly to trunk as I've hacked the diff itself heavily to reflect a patch aginst what the tree will be looking like after a few pending cvs copies and checkins (my communcator chrome moves).
I'll attach a cvs copy file as well in a minute, basically all the help content needs to be copied over into suite/

After applying this work, still some cleanup needs to be done to get style working correctly and remove redundancy of the "using help" content.
Assignee

Comment 2

13 years ago
These are the cvs copies we need along with the poatch in this bug to get SeaMonkey help content into suite/locales/
Attachment #228818 - Flags: review?(neil)
xulrunner is a pain the backside. Why can't we have the chrome we want?

Can we just ignore toolkit and install all our help under communicator?
Assignee

Comment 4

13 years ago
(In reply to comment #3)
> Can we just ignore toolkit and install all our help under communicator?

We probably could, but what's wrong with using their help viewer? I think we should try to share as much as possible with them, so that the amount of code we need to care about gets to be the stuff that really makes a difference.

Additionally, splitting between chrome://help and chrome://suitehelp seems to me like a good idea because it splits help viewer from help content, which are two different things actually.
(In reply to comment #4)
>We probably could, but what's wrong with using their help viewer?
Because of all the other changes we need to get it to work?
Assignee

Comment 6

13 years ago
(In reply to comment #5)
> (In reply to comment #4)
> >We probably could, but what's wrong with using their help viewer?
> Because of all the other changes we need to get it to work?

Let's see... The configure.in would be needed in some way anyways, as we're (correctly) removing help from extensions/ dir. The jar.nm changes are needed in any case. The toolkit/components changes are something we want to do some time anyways, as we want/need to build toolkit without any ifdefs in the future. Adding the one pref is very trivial, and utilityOverlay stuff is only moving some part from a now uneeded overlay (as it's no extension any more) directly into that file. The suitehelp.rdf changes are only cosmetical anyways.

So I don't see "all the other changes" that are that strange to do... It wasn't as easy as I would have liked to find out what to do, but it's all stuff we need to do in any case or is trivial, IMO. The biggest thing we could discuss is if we'd want to keep that helpOverlay, but I think it's useless to keep it when we don't treat help as optional any more.
Comment on attachment 228818 [details]
cvs copy file for moving seamonkey help content over into suite/

Can't review this until I can apply attachment 228817 [details] [diff] [review].
Attachment #228818 - Flags: review?(neil)
Assignee

Comment 8

13 years ago
This patch is against clean trunk and should work... I'm still in the process of building with it, but it should work.
Attachment #228817 - Attachment is obsolete: true
Attachment #228941 - Flags: review?(neil)
Assignee

Updated

13 years ago
Attachment #228941 - Flags: superreview?(neil)
Assignee

Comment 9

13 years ago
Comment on attachment 228818 [details]
cvs copy file for moving seamonkey help content over into suite/

requesting reviews again now that there's a patch that can be applied to trunk
Attachment #228818 - Flags: superreview?(neil)
Attachment #228818 - Flags: review?(neil)
Assignee

Comment 10

13 years ago
Sorry, I had missed the jar.mn change when porting this over to normal trunk. As we're moving preprocessed code from helpOverlay to utilityOverlay, we now have to preprocess the utilityOverlay
Attachment #228941 - Attachment is obsolete: true
Attachment #229072 - Flags: superreview?(neil)
Attachment #229072 - Flags: review?(neil)
Attachment #228941 - Flags: superreview?(neil)
Attachment #228941 - Flags: review?(neil)
Comment on attachment 229072 [details] [diff] [review]
patch, v1.1: to use toolkit help viewer for suiterunner

>+  if test "$MOZ_XUL_APP"; then
>+    MOZ_EXTENSIONS_DEFAULT=" wallet xml-rpc p3p venkman inspector irc typeaheadfind spellcheck gnomevfs sroaming reporter"
>+  else
>+    MOZ_EXTENSIONS_DEFAULT=" wallet xml-rpc help p3p venkman inspector irc typeaheadfind spellcheck gnomevfs sroaming reporter"
>+  fi
This needs build review.

>+% content suitehelp %content/suitehelp/ xpcnativewrappers=yes
I am really not keen on the use of suitehelp as a package.
Can we not put the docs under chrome://communicator/locale/help/ ?

> 	typeaheadfind \
But you didn't disable extensions/typeaheadfind. Do the two not conflict? Or are you cheating by building toolkit chrome but overriding the component?

>+                oncommand="openHelp('welcome', 'chrome://suitehelp/locale/help.rdf');"/>
How are we going to handle all the existing openHelp calls?
Attachment #229072 - Flags: review?(neil)
Assignee

Comment 12

13 years ago
(In reply to comment #11)
> (From update of attachment 229072 [details] [diff] [review] [edit])
> >+  if test "$MOZ_XUL_APP"; then
> >+    MOZ_EXTENSIONS_DEFAULT=" wallet xml-rpc p3p venkman inspector irc typeaheadfind spellcheck gnomevfs sroaming reporter"
> >+  else
> >+    MOZ_EXTENSIONS_DEFAULT=" wallet xml-rpc help p3p venkman inspector irc typeaheadfind spellcheck gnomevfs sroaming reporter"
> >+  fi
> This needs build review.

Sure, will try to get that...

> >+% content suitehelp %content/suitehelp/ xpcnativewrappers=yes
> I am really not keen on the use of suitehelp as a package.
> Can we not put the docs under chrome://communicator/locale/help/ ?

Sure we can, I just though we might want to have it in a separate chrome package, but I'm OK with sticking it into communicator as well.

> > 	typeaheadfind \
> But you didn't disable extensions/typeaheadfind. Do the two not conflict? Or
> are you cheating by building toolkit chrome but overriding the component?

Apparently they either don't conflict or override each other in a way that everything works. We really should disable extensions/typeaheadfind, I guess, but I didn't dare to risk this in the first patch, as we still can clean it up afterwards.

> >+                oncommand="openHelp('welcome', 'chrome://suitehelp/locale/help.rdf');"/>
> How are we going to handle all the existing openHelp calls?

From what I've seen, we need to add the URI to all of them, as the toolkit viewer doesn't know where to get its contents from if we don't tell it the URI (which is logical as it's a generic help viewer component)
Comment on attachment 229072 [details] [diff] [review]
patch, v1.1: to use toolkit help viewer for suiterunner

(In reply to comment #12)
>>>+% content suitehelp %content/suitehelp/ xpcnativewrappers=yes
>>I am really not keen on the use of suitehelp as a package.
>>Can we not put the docs under chrome://communicator/locale/help/ ?
>Sure we can, I just though we might want to have it in a separate chrome
>package, but I'm OK with sticking it into communicator as well.
I don't see the point of creating a package just for some locale bits.

>>> 	typeaheadfind \
>But you didn't disable extensions/typeaheadfind. Do the two not conflict? Or
>> are you cheating by building toolkit chrome but overriding the component?
>Apparently they either don't conflict or override each other in a way that
>everything works. We really should disable extensions/typeaheadfind, I guess,
>but I didn't dare to risk this in the first patch, as we still can clean it up
>afterwards.
Well I double-checked and toolkit help requires toolkit nsITypeAheadFind which requires toolkit typeaheadfind. So for a start you will have to register it. Then you will need to disable the extensions version. And finally fix up all the other windows that use it...

>>>+                oncommand="openHelp('welcome', 'chrome://suitehelp/locale/help.rdf');"/>
>> How are we going to handle all the existing openHelp calls?
>From what I've seen, we need to add the URI to all of them, as the toolkit
>viewer doesn't know where to get its contents from if we don't tell it the URI
>(which is logical as it's a generic help viewer component)
Ok, but as this stands this will break xpfe help as this patch builds the docs under different URLs depending on MOZ_XUL_APP.
Attachment #229072 - Flags: superreview?(neil) → superreview-
You might find it easier to split the help docs move into a separate bug.
Assignee

Comment 15

13 years ago
(In reply to comment #14)
> You might find it easier to split the help docs move into a separate bug.

If you are OK with moving all the files listed in the cvs copy file here to suite/locales/en-US/common/help/ (translates to chrome://communicator/locale/help/ ) and leave them there as for-now-unused files that we can then patch and use when this bigger bug gets resolved, I'll just do a cvs copy bug and build upon those copied files afterwards right here in this bug.

About the typeaheadfind issue:
I wonder why both browser and help FAYT do work well with my build, but that may just be because suiterunner uses toolkit browser and tabbrowser right now.
What except browser is using XPFE typeaheadfind right now?

About openHelp():
I'm less concerned about breaking the openHelp() calls as I'd hope we can retire xpfe seamonkey ASAP - so either we break those calls until we flip the XOZ_XUL_APP switch or we fix them up only after we do that (and leave a bug open for that)
(In reply to comment #15)
>If you are OK with moving all the files listed in the cvs copy file here to
>suite/locales/en-US/common/help/
I am.

>I wonder why both browser and help FAYT do work well with my build, but that
>may just be because suiterunner uses toolkit browser and tabbrowser right now.
Even so toolkit help uses findBar uses toolkit fayt (via browser).
So any patch would be incomplete unless fayt is [already] flipped.

>What except browser is using XPFE typeaheadfind right now?
>I'm less concerned about breaking the openHelp() calls as I'd hope we can
>retire xpfe seamonkey ASAP
Before we have a working suiterunner?
Assignee

Updated

13 years ago
Depends on: 344611
Assignee

Comment 17

13 years ago
(In reply to comment #16)
> >If you are OK with moving all the files listed in the cvs copy file here to
> >suite/locales/en-US/common/help/
> I am.

OK, cvs copy bug filed for that, see 344611

> >I wonder why both browser and help FAYT do work well with my build, but that
> >may just be because suiterunner uses toolkit browser and tabbrowser right now.
> Even so toolkit help uses findBar uses toolkit fayt (via browser).
> So any patch would be incomplete unless fayt is [already] flipped.

Hmm, I guess I'll have to look into that then... Just need to find out where else extensions/typeaheadfind is still being used - if anywhere...

> >I'm less concerned about breaking the openHelp() calls as I'd hope we can
> >retire xpfe seamonkey ASAP
> Before we have a working suiterunner?

No, but we should try to get it working as soon as we can...

But I guess we may better be able to live with keeping an open bug on changing over the openHelp() calls, which will be dependent on the "flip the switch" bug 328887
(In reply to comment #17)
>But I guess we may better be able to live with keeping an open bug on changing
>over the openHelp() calls
So you're splitting this into two patches a) suiterunner should build toolkit help and typeaheadfind and b) switch all the openHelp calls?
Assignee

Comment 19

13 years ago
(In reply to comment #18)
> So you're splitting this into two patches a) suiterunner should build toolkit
> help and typeaheadfind and b) switch all the openHelp calls?

Yes, I think that's reasonable unless we want to introduce a big list of ifdefs or break xpfe SeaMonkey due to changed openHelp calls... we could introduce the second parameter as a no-op into the "old" openHelp() implementation though, in which case both SeaMonkey variants would work with the changed openHelp calls
(In reply to comment #19)
>we could introduce the second parameter as a no-op into the "old" openHelp()
It already has the second parameter. The difference is that it has a default.
Assignee

Comment 21

13 years ago
(In reply to comment #20)
> (In reply to comment #19)
> >we could introduce the second parameter as a no-op into the "old" openHelp()
> It already has the second parameter. The difference is that it has a default.

Oh... I actually haven't checked if the toolkit one somehow supports a default - browser/ useage suggests it doesn't while mail/ useage suggests it may support that... I think I'll need to dig deeper there to find out about that...
Assignee

Comment 22

13 years ago
This patch is using the chrome://communicator/locale/help/ location, includes changes to incorporate what bug 343395 has done for security help buttons, and excludes extensions/typeaheadfind from the build.

With this applied, help works, even the overlayed security help buttons do work (using setHelpFileURI to set the help RDF file for all thoise openHelp calls) - and FAYT work in help (even though it displays a find bar).
What I couldn't find out yet is how to get FAYT in browser again via toolkit's typeaheadfind, but I figured a patch for that will likely touch a different set of files and can easily be split from this one - and I'm not yet sure if I'm able to find out how to do it myself, as I know almost nothing about FAYT (not even how it's invoked).
Attachment #228818 - Attachment is obsolete: true
Attachment #229072 - Attachment is obsolete: true
Attachment #230120 - Flags: review?(neil)
Attachment #228818 - Flags: superreview?(neil)
Attachment #228818 - Flags: review?(neil)
Assignee

Updated

13 years ago
Depends on: 345526
Assignee

Comment 23

13 years ago
Following Benjamin's suggestion, this is a patch to rename the old typeaheadfind component used by SeaMonkey to suitetypeaheadfind so that we can use it in parallel to toolkit's typeaheadfind component.

This approach removes the need of excluding extensions/typeaheadfind in the other patch posted here, and makes us able to handle bug 345526 (basing SeaMonkey's FAYT on toolkit code) independently of this bug here, as the old FAYT implementation still does work as it used to.
Attachment #230250 - Flags: superreview?(neil)
Attachment #230250 - Flags: review?(benjamin)
Assignee

Comment 24

13 years ago
Comment on attachment 230120 [details] [diff] [review]
use toolkit help viewer, don't build xpfe typeaheadfind

For reviewing the main patch, there are two small changes I've done locally, but I don't want to upload another patch just for those two lines:

>Index: mozilla/configure.in
>===================================================================
>+  if test "$MOZ_XUL_APP"; then
>+    MOZ_EXTENSIONS_DEFAULT=" wallet xml-rpc p3p venkman inspector irc spellcheck gnomevfs sroaming reporter"

should now be
+    MOZ_EXTENSIONS_DEFAULT=" wallet xml-rpc p3p venkman inspector irc typeaheadfind spellcheck gnomevfs sroaming 

along with the suitetypeaheadfind patch, of course.

>Index: mozilla/xpfe/communicator/resources/content/utilityOverlay.xul
>===================================================================
>+#ifdef MOZ_XUL_APP
>     <menupopup id="helpPopup">

move the #ifdef inside the <menupopup>, so that non-suiterunner builds still work.
Attachment #230120 - Flags: superreview?(neil)

Updated

13 years ago
Attachment #230250 - Flags: superreview?(neil) → superreview+

Comment 25

13 years ago
Comment on attachment 230250 [details] [diff] [review]
rename old typeaheadfind to be able to use it in parallel to toolkit (checked in)

I didn't look through the other patches on this bug, but you'll need to fix the packaging files in xpinstall/packager to reflect the name change.
Attachment #230250 - Flags: review?(benjamin) → review+
Assignee

Comment 26

13 years ago
Thanks for catching that, Benjamin. This hould go in along with the renaming patch.
I probably also should poke Camino devs as mozilla/camino/Camino.xcode/project.pbxproj refers to that .xpt as well.
Attachment #230915 - Flags: superreview?(neil)
Attachment #230915 - Flags: review?(neil)
Assignee

Comment 27

13 years ago
This should be all Camino needs for that file name change (I hope)...
Attachment #230918 - Flags: review?
Assignee

Updated

13 years ago
Attachment #230918 - Flags: review? → review?(mark)
Comment on attachment 230915 [details] [diff] [review]
update packaging files for suitetypeheadfind.xpt

>-  deleteThisFile("Components", "typeaheadfind.xpt");
These lines are used during an in-place upgrade to remove obsolete or old versions of files, which you do want to do here. sr=me with this fixed.
Attachment #230915 - Flags: superreview?(neil)
Attachment #230915 - Flags: superreview+
Attachment #230915 - Flags: review?(neil)
Attachment #230915 - Flags: review+
Assignee

Comment 29

13 years ago
Attachment #230918 - Attachment is obsolete: true
Attachment #230923 - Flags: review?(mark)
Attachment #230918 - Flags: review?(mark)
Assignee

Comment 30

13 years ago
I missed the libs in the first version this, here they should be included as well.
Attachment #230915 - Attachment is obsolete: true
Attachment #230928 - Flags: superreview?(neil)
Attachment #230928 - Flags: review?(neil)

Updated

13 years ago
Attachment #230923 - Flags: review?(mark) → review+
Comment on attachment 230928 [details] [diff] [review]
update packaging files for suitetypeheadfind (include libs) (checked in)

r+sr=me with comment #28 changes as discussed on IRC.
Attachment #230928 - Flags: superreview?(neil)
Attachment #230928 - Flags: superreview+
Attachment #230928 - Flags: review?(neil)
Attachment #230928 - Flags: review+
Assignee

Comment 32

13 years ago
Comment on attachment 230250 [details] [diff] [review]
rename old typeaheadfind to be able to use it in parallel to toolkit (checked in)

checked in the bits to rename old typeaheadfind.
Attachment #230250 - Attachment description: rename old typeaheadfind to be able to use it in parallel to toolkit → rename old typeaheadfind to be able to use it in parallel to toolkit (checked in)
Assignee

Updated

13 years ago
Attachment #230923 - Attachment description: update Camino for suitetypeheadfind → update Camino for suitetypeheadfind (checked in)
Assignee

Updated

13 years ago
Attachment #230928 - Attachment description: update packaging files for suitetypeheadfind (include libs) → update packaging files for suitetypeheadfind (include libs) (checked in)
Assignee

Comment 33

13 years ago
This is an updated patch, corrected for the two changes I mentioned in comment #24, and taking into account that the help content files have already been cvs copied. Because of that, it includes correcting the stylesheet URL in the help content .xhtml files.
Attachment #230120 - Attachment is obsolete: true
Attachment #231147 - Flags: superreview?(neil)
Attachment #231147 - Flags: review?(neil)
Attachment #230120 - Flags: superreview?(neil)
Attachment #230120 - Flags: review?(neil)
Assignee

Comment 34

13 years ago
There's one hunk I missed, please consider this part of that last patch:

Index: mozilla/suite/locales/en-US/chrome/common/help/helpFileLayout.css
===================================================================
 a[href^="http://"]:after, a[href^="https://"]:after
-{ content: url("chrome://help/locale/images/web-links.png"); }
+{ content: url("chrome://communicator/locale/help/images/web-links.png"); }
Assignee

Comment 35

13 years ago
Sorry, look like I'm good at bitrotting my own patches ;-)

Here's a patch that should apply correctly on current trunk, this also includes some previously missed conversions for help/locale to communicatior/locale/help in the new help locale files.
Attachment #231147 - Attachment is obsolete: true
Attachment #231240 - Flags: superreview?(neil)
Attachment #231240 - Flags: review?(neil)
Attachment #231147 - Flags: superreview?(neil)
Attachment #231147 - Flags: review?(neil)
Comment on attachment 231240 [details] [diff] [review]
use toolkit help viewer, build toolkit typeaheadfind

>Index: mozilla/suite/locales/en-US/chrome/common/help/suitehelp.rdf
Did you have to rewrite this file? How am I supposed to see what you changed?

>-  <a href="chrome://help/locale/help_help.xhtml">Using the Help Window</a>.</p>
>+  <a href="chrome://communicator/locale/help/help_help.xhtml">Using the Help Window</a>.</p>
This should be a relative link. In fact the stylesheets should be too.

Also I'll double-check but for some reason my help index was doubled.
Assignee

Comment 37

13 years ago
(In reply to comment #36)
> (From update of attachment 231240 [details] [diff] [review] [edit])
> >Index: mozilla/suite/locales/en-US/chrome/common/help/suitehelp.rdf
> Did you have to rewrite this file? How am I supposed to see what you changed?

I did rewrite it based on Firefox's file, it's hard to say what has changed.

> >-  <a href="chrome://help/locale/help_help.xhtml">Using the Help Window</a>.</p>
> >+  <a href="chrome://communicator/locale/help/help_help.xhtml">Using the Help Window</a>.</p>
> This should be a relative link. In fact the stylesheets should be too.

Hmm, you're right. Probably even the suite-toc.rdf entries, right?

> Also I'll double-check but for some reason my help index was doubled.

Usually, only the "Using Help" entry should be doubled. We still need to figure out how to handle that best, but this should be no blocker fo the moment, I'll file a followup on that.
Assignee

Updated

13 years ago
Blocks: 346604
Assignee

Updated

13 years ago
Blocks: 346605

Comment 38

13 years ago
Hmm, this patch doesn't seem to work on my mac (10.3.9) suiterunner debug build. Feels like some things are missing:

1) I got no Tools/Window menu. Or rather, no windows menu but a tools menu without any menu title in the menubar.

2) If I try to open Help, I get a parsing error:

3) Opening the "Go" menu results in a js error:
JavaScript error: chrome://global/content/hiddenWindow.xul, line 1: updateGoMenu is not defined

4) Opening the bookmarks menu also gives me a js error:
JavaScript error: chrome://navigator/content/navigator.js, line 1015: gBrowser has no properties

5) Selecting "Manage Bookmarks..." in the bm menu gives me this:
Bm menu --> Manage bookmarks:
JavaScript error: chrome://navigator/content/navigator.js, line 1357: toOpenWindowByType is not defined

Note:
This is with patched and clean source (clobber) - checkout finish: Tue Aug 1 17:53:32 CEST 2006

With clean source (clobber) - checkout finish: Wed Aug 2 00:35:29 CEST 2006 everything seems fine

Comment 39

13 years ago
(In reply to comment #38)

> With clean source (clobber) - checkout finish: Wed Aug 2 00:35:29 CEST 2006
> everything seems fine
> 

Erm, non-patched (of course)
Assignee

Comment 40

13 years ago
(In reply to comment #38)
> Hmm, this patch doesn't seem to work on my mac (10.3.9) suiterunner debug
> build. Feels like some things are missing:

I had such problems with an earlier revision of this patch when utilityOverlay had problems, but I fixed all those problem I saw in the current version of the patch...
Assignee

Comment 41

13 years ago
This patch should hopefully work on Mac as well...
Attachment #231240 - Attachment is obsolete: true
Attachment #232979 - Flags: superreview?(neil)
Attachment #232979 - Flags: review?(stefanh)
Attachment #231240 - Flags: superreview?(neil)
Attachment #231240 - Flags: review?(neil)

Comment 42

13 years ago
Comment on attachment 232979 [details] [diff] [review]
build toolkit help viewer and typeaheadfind, hopefully work on Mac

Sorry, but I don't see any difference with this. Fwiw, the parsing error I get when I try to open help is:

XML Parsing Error: undefined entity
Location: chrome://help/content/help.xul
Line Number 65, Column 5:    <key key="&goBackCmd.commandkey;" command="Help:Back" modifiers="accel"/>
Attachment #232979 - Flags: review?(stefanh) → review-
Assignee

Comment 43

13 years ago
(In reply to comment #42)
> Sorry, but I don't see any difference with this.

I think you're not applying the configure.in patch to configure, and so you're ending up with duplicating help references (from suite/ and extensions/). So either you're patching configure or building with --enable-extensions=defaults,-help but else you can't accurately test this patch.

Comment 44

13 years ago
(In reply to comment #43)
> (In reply to comment #42)
> > Sorry, but I don't see any difference with this.
> 
> I think you're not applying the configure.in patch to configure, and so you're
> ending up with duplicating help references (from suite/ and extensions/). So
> either you're patching configure or building with
> --enable-extensions=defaults,-help but else you can't accurately test this
> patch.
> 
erm, you're right :-/

Updated

13 years ago
Attachment #232979 - Flags: review- → review?(stefanh)

Comment 45

13 years ago
Comment on attachment 232979 [details] [diff] [review]
build toolkit help viewer and typeaheadfind, hopefully work on Mac

This works fine now on mac, so r=me for the mac part. Help opens, no regressions in Keyboard shortcuts, and FAT works as well.

Some general issues that should apply on all platforms (either fixing them now or file follow-up bugs):

1) We now use Firefox/Pinstripe navigation/print buttons in the Help toolbar. The same goes for the toolkit welcome page - that page is now styled differently than ours. We should of course use our own toolbar buttons and have the welcome page styled in our own way (even if we use the toolkit welcome page).


2) Opening Help produces a js error:
"JavaScript strict warning: chrome://global/content/bindings/tree.xml, line 135: reference to undefined property this.columns[i]"

(As a side-note, I tried the previous patch with a patched configure and the keyboard shortcuts didn't work.)
Attachment #232979 - Flags: review?(stefanh) → review+
(In reply to comment #45)
>2) Opening Help produces a js error:
>"JavaScript strict warning: chrome://global/content/bindings/tree.xml, line
>135: reference to undefined property this.columns[i]"
That's bug 332797.
Assignee

Comment 47

13 years ago
(In reply to comment #45)
> 1) We now use Firefox/Pinstripe navigation/print buttons in the Help toolbar.
> The same goes for the toolkit welcome page - that page is now styled
> differently than ours. We should of course use our own toolbar buttons and have
> the welcome page styled in our own way (even if we use the toolkit welcome
> page).

We should do that in a followup, probably using overlays or such.

> (As a side-note, I tried the previous patch with a patched configure and the
> keyboard shortcuts didn't work.)

Yes, I think the current one is better, as can support both Mac keyboard shortcuts.

Comment 48

13 years ago
(In reply to comment #47)
> (In reply to comment #45)
> > 1) We now use Firefox/Pinstripe navigation/print buttons in the Help toolbar.
> > The same goes for the toolkit welcome page - that page is now styled
> > differently than ours. We should of course use our own toolbar buttons and have
> > the welcome page styled in our own way (even if we use the toolkit welcome
> > page).
> 
> We should do that in a followup, probably using overlays or such.
>

Wouldn't everything be easier if Help was in chrome://help/locale/ ?
Assignee

Comment 49

13 years ago
(In reply to comment #48)
> Wouldn't everything be easier if Help was in chrome://help/locale/ ?

Overriding isn't much more work, actually, and chrome://help/ is reserved for the toolkit part, so it's no option. We can't pack stuff into toolkit.jar if we're serious with wanting to move to XULRunner later on.
(In reply to comment #48)
>Wouldn't everything be easier if Help was in chrome://help/locale/ ?
In bug 346605 I'd like to move it all to chrome://communicator/locale/help although as yet I don't understand KaiRo's review comments...
Assignee

Comment 51

13 years ago
This version is updated to current trunk after the recent changes done by Neil, the rest is still same as in the last patch.
Attachment #232979 - Attachment is obsolete: true
Attachment #242167 - Flags: superreview?(neil)
Attachment #242167 - Flags: review?(neil)
Attachment #232979 - Flags: superreview?(neil)
Assignee

Comment 52

13 years ago
Thanks to Neil's testing, we found out it's probably a good idea to remove the old xpfe help from the extensions list when building suiterunner, as its overlays clash with new UI pieces. This patch should do just that :)
Attachment #242241 - Flags: superreview?(neil)
Attachment #242241 - Flags: review?(neil)
Comment on attachment 242167 [details] [diff] [review]
build toolkit help viewer and typeaheadfind, updated to current trunk

>+<!ENTITY % platformCommunicatorDTD SYSTEM "chrome://communicator-platform/locale/platformCommunicatorOverlay.dtd">
>+%platformCommunicatorDTD;
Unnecessary?

> 
>+  <commandset id="tasksCommands">
>+    <command id="cmd_openHelp"
>+             oncommand="openHelp('welcome', 'chrome://communicator/locale/help/suitehelp.rdf');"/>
>+  </commandset>
>+
Missing #ifdef MOZ_XUL_APP
Attachment #242167 - Flags: superreview?(neil)
Attachment #242167 - Flags: superreview+
Attachment #242167 - Flags: review?(neil)
Attachment #242167 - Flags: review+

Updated

13 years ago
Attachment #242241 - Flags: superreview?(neil) → superreview+

Comment 54

13 years ago
(In reply to comment #53)
> (From update of attachment 242167 [details] [diff] [review] [edit])
> >+<!ENTITY % platformCommunicatorDTD SYSTEM "chrome://communicator-platform/locale/platformCommunicatorOverlay.dtd">
> >+%platformCommunicatorDTD;
> Unnecessary?

No, because in suite/locales/en-US/chrome/common/mac/platformCommunicatorOverlay.dtd#27:

 27 <!-- Help viewer -->
 28 <!ENTITY openHelpCmd.label          "&brandShortName; Help">
 29 <!ENTITY openHelpCmd.accesskey      "H">
Assignee

Comment 55

13 years ago
Comment on attachment 242241 [details] [diff] [review]
better configure.in fix: remove old help from extensions list if building XUL_APP

bsmedberg, could you please review this part?
I hope this is the correct way to
1) Have different default extensions for xpfe- and toolkit-based suite
2) exclude help extension if it might be included in the extension list when building toolkit-based suite (it's incompatible UI-wise)
Attachment #242241 - Flags: review?(neil) → review?(benjamin)
Comment on attachment 242241 [details] [diff] [review]
better configure.in fix: remove old help from extensions list if building XUL_APP

how about:
  MOZ_EXTENSIONS_DEFAULT=" wallet xml-rpc p3p venkman inspector irc typeaheadfind gnomevfs sroaming reporter"
  if test -z  "$MOZ_XUL_APP"; then
    MOZ_EXTENSIONS_DEFAULT="$MOZ_EXTENSIONS_DEFAULT help"
  fi

Updated

13 years ago
Attachment #242241 - Flags: review?(benjamin) → review+
Assignee

Comment 57

13 years ago
Checked in.

This should finally be fixed now. Further work on getting toolkit help perform correctly and look good enough in suiterunner all over the place should be done in followup bugs.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Assignee

Updated

13 years ago
Blocks: 345526
No longer depends on: 345526
Assignee

Updated

13 years ago
Blocks: 360109
Comment on attachment 242167 [details] [diff] [review]
build toolkit help viewer and typeaheadfind, updated to current trunk

>+                    <Description nc:panelid="search"
>+                        nc:datasources="search-db.rdf"
Except no such file exists, so we log two lines in the Error Console...

Updated

12 years ago
Depends on: 365181

Updated

12 years ago
Depends on: 386200
Depends on: 445429
You need to log in before you can comment on or make changes to this bug.