325416 - Move typeaheadfind out of TB ifdefs

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Jeff Walden [:Waldo]]

I've been sitting on a patch to do this for a month while waiting for review in bug 320628; after seeing bug 239136 comment 5 I've decided to just go ahead with filing this and not worry about that bug quite yet.  (I was waiting because I wanted the typeaheadfind change to be part of the patch to make the help viewer fully functional in Thunderbird [and presumably all xulrunner apps].)

The patch is coming within the next day.  It works for me on one platform with one linker/compiler set, but I'm not sure whether some parts of it are actually correct; those can be addressed in review.

(Also, I'll note that typeaheadfind seems perfectly functional in my testing in Thunderbird in a patch help viewer, even down to link/text handling based on prefs; I'm not sure why it hasn't been built previously, as I expected there to be some sort of problems that made it unusable in Thunderbird.)

Comment 1

[Jeff Walden (gone starting June 8)]

Attachment: Build typeaheadfind, use it with find bar in View Source

In toolkit/components/build/Makefile.in, is |EXTRA_DSO_LIBS = gkgfx| needed for typeaheadfind?  I assume the line was there for a reason but don't know.

The change to all.js is required because the findbar blindly assumes the pref is present and will throw an exception if it isn't.  It's the only findbar-specific pref treated this way; there are some other typeaheadfind-generic prefs already in all.js whose presence prevents other exceptions.

The few findbar files that are added (findBar.css, find.png, notfound.png, wrap.png) are/will be direct copies from Winstripe; I expect Thunderbird people will want to change these eventually themselves.

This patch also starts building help in Thunderbird; help's not totally ready yet (mostly miscellaneous skinning issues in addition to bug 320628), but given that it's the only other change I have in that file right now I'd like to get the modification out of my cvsco.logs.

Comment 2

[Benjamin Smedberg]

Comment on attachment 210369 [details] [diff] [review]
Build typeaheadfind, use it with find bar in View Source

You're going to need to put typeaheadfind in the MOZ_XUL ifdef (because it requires nsIDOMXULDocument), or perhaps just remove the #include "nsIDOMXULDocument.h" which appears at first glance to be unneeded.

Comment 3

[Jeff Walden (gone starting June 8)]

Attachment: Remove

Both Firefox and Thunderbird still build correctly with the #include "nsIDOMXULDocument.h" removed, at least on my computer; it does indeed appear unneeded.  Anything else?

Comment 4

[Benjamin Smedberg]

Comment on attachment 210382 [details] [diff] [review]
Remove 

Nope :-) /me is very happy

This should get moa=mscott if you haven't already gotten his approval.

Comment 5

[Jeff Walden (gone starting June 8)]

Attachment: Trivially unbitrotted

Comment 6

[Scott MacGregor]

Attachment: updated patch

I had to make a couple minor tweaks including adding a few extra images and css rules to make this actually work for me. Here's an updated patch. 

Carrying forward Benjamin's review as I didn't change anything signficant.

I've also implemented the thunderbird front end hooks to actually use typeahead find and put those changes in Bug 325416.

Comment 7

[Jeff Walden (gone starting June 8)]

(In reply to comment #6)
> I had to make a couple minor tweaks including adding a few extra images and
> css rules to make this actually work for me.

The addition of the necessary images actually was in the patch, but there's no way to diff a binary file for inclusion in the patch; I mentioned that these files would be copied from winstripe in comment 1.  As for the added image close.png, you don't need it if you use Close.gif (Qute's analog to close.png), which is what the previous patch used.

Finally, you removed the change to modules/libpref/src/init/all.js to add the pref "accessibility.typeaheadfind.flashBar" with value 1; this will cause an error to be thrown when the find bar is used in Thunderbird, because the only place where that value is used doesn't check for the pref's existence before attempting to get its value.  (This might even stop that JavaScript from running, leaving the find bar in only a half-working state.)

http://lxr.mozilla.org/mozilla/source/toolkit/components/typeaheadfind/content/findBar.js#125

Comment 8

[Scott MacGregor]

(In reply to comment #7)
> (In reply to comment #6)
> > I had to make a couple minor tweaks including adding a few extra images and
> > css rules to make this actually work for me.
> 
> The addition of the necessary images actually was in the patch, but there's no
> way to diff a binary file for inclusion in the patch; I mentioned that these
> files would be copied from winstripe in comment 1.  As for the added image
> close.png, you don't need it if you use Close.gif (Qute's analog to close.png),
> which is what the previous patch used.

we should be using close.png here and I fixed that in my patch.
> 
> Finally, you removed the change to modules/libpref/src/init/all.js to add the
> pref "accessibility.typeaheadfind.flashBar" with value 1; this will cause an
> error to be thrown when the find bar is used in Thunderbird, because the only
> place where that value is used doesn't check for the pref's existence before
> attempting to get its value.  (This might even stop that JavaScript from
> running, leaving the find bar in only a half-working state.)

That pref along with the other typeahead prefs are in all-thunderbird.js.

Thanks.

Comment 9

[Jeff Walden (gone starting June 8)]

(In reply to comment #8)
> That pref along with the other typeahead prefs are in all-thunderbird.js.

According to LXR that specific preference is currently not in all-thunderbird.js.  Is it in a patch posted to a separate bug?

http://lxr.mozilla.org/seamonkey/search?string=flashbar

Comment 10

[Scott MacGregor]

yes, they are in the patch I linked to here. See the blocking bug. 

Comment 11

[Scott MacGregor]

Jeff, did you have any other comments or concerns? Otherwise I'll go ahead and start landing this. 

Comment 12

[Scott MacGregor]

Comment on attachment 213393 [details] [diff] [review]
updated patch

I'd eventually like to take this on the 1.8.1 branch as well. Removal of ifdefs gets thunderbird closer to playing well with xul runner, and I'd like to have thunderbird 2 using the find bar instead of the old find dialog anyway.

Comment 13

[Jeff Walden (gone starting June 8)]

(In reply to comment #11)
> Jeff, did you have any other comments or concerns?

Nope, I think that covered it.

Comment 14

[Scott MacGregor]

fixed branch and trunk, although the merge wasn't pretty on the branch.