165626 - Package type ahead find in default install

Status: VERIFIED FIXED

(SeaMonkey :: Find In Page, defect, P2)

Description

[Aaron Leventhal]

Type ahead find is becoming mature enough to get packaged with the default
installer. This is the bug for that.

Comment 1

[Aaron Leventhal]

Attachment: This should work, shouldn't it! ?

Yeek, for some reason, typeaheadfind won't work in resulting installed Mozilla.
What's wrong with this thing?

Comment 2

[Aaron Leventhal]

It works after I manually run regxpcom in the installation directory.
I don't know why that's not happening automatically.

Comment 3

[Aaron Leventhal]

This does work after all, yay!

I was running into bug 162593.

Seeking r=

Comment 4

[Aaron Leventhal]

Asa really wants this for 1.1a, so hopefully we can get this r=/sr=/a='d in the
next day or two. Curt?

Comment 5

[Curt Patrick (gone)]

Comment on attachment 97278 [details] [diff] [review]
This should work, shouldn't it! ?

r=curt

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 97278 [details] [diff] [review]
This should work, shouldn't it! ?

As mentioned on IM I think it's overkill to make this a separate component,
but if you want to go that way it looks pretty good. This will not get
installed on a commercial build, btw, you'd have to duplicate nearly everything
but the package list changes in the ns tree.

Once moved into browser.xpi (just move lines around in the package lists
and add the chrome registration call to the browser.jst files) you should
remove the typeaheadfind.js default prefs and merge into all.js to
avoid slowing down startup. You've said you'll file a bug on those
improvements.

>Index: packager/mac/typeaheadfind.jst
>===================================================================

>+var gVersion = "0.1";

Since this is a binary component it's probably mozilla-version-specific.
Probably better to simply use the overall browser version. But that'll
happen automatically when this gets moved into browser.xpi so you can
leave this for now.

>+err = addDirectory("", gVersion, "bin", fProgram, "", true);

On the Mac this has to be "viewer" instead of 'bin' to match the
build directory (as seen in the package lists).

>+registerChrome(LOCALE | DELAYED_CHROME, getFolder("Chrome","typeaheadfind.jar"), "locale/en-US/typeaheadfind/");

In the region-pack scripts Tao registers a placeholder CONTENT contents.rdf,
is this required here to make this work? You said you had one in your build,
too, but it's not registering one here. On the other hand you said you tested
without it and it worked, so I'm good with that.

>+err = addDirectory("", gVersion, "bin", fProgram, "", true);

You could simplify this to 
   err = addDirectory("", "bin", fProgram, "");
if you wanted. Doesn't matter to me though

>+if (getLastError() == SUCCESS) {
>+  err = performInstall(); 
>+  logComment("performInstall: " + err);
>+} else {
>+  cancelInstall(err);
>+}

If you get an error from the registerChrome() call you will return
the addDirectory error, which very well might be success. But if
addDirectory chrome registration will *definitely* fail with a
file-not-found error, which is probably not as useful as knowing
why we couldn't write the file out (permissions? out of space?)

at the very least do

   err = getLastError();
   if (err == SUCCESS) ... etc

but consider checking err before bothering with the registerChrome() call

Comment 7

[timeless]

um, given the various regressions and bugs that typeahead has caused recently, i
don't think it's ready :-)

as for getting this into 1.1a, you're a few months late :-).
if you want lots of feedback from 1.2a that's a different story, but let's be
sure we don't know of any major bugs in it before we decide to flip the switch.

Comment 8

[Aaron Leventhal]

Sorry, I mean 1.2a.

Dan, I have these changes - do you want to see a new patch?

Comment 9

[Daniel Veditz [:dveditz]]

sr=dveditz given to modified patch reviewed on Aaron's machine. This is a
short-term solution though, this should get rolled into the main install (bug
166712).

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 97278 [details] [diff] [review]
This should work, shouldn't it! ?

doesn't make a lot of sense to have it in the zips and not in the installers.
a=asa (on behalf of drivers) for checkin to 1.2a

Comment 11

[Aaron Leventhal]

Comment on attachment 97278 [details] [diff] [review]
This should work, shouldn't it! ?

Has sr=dveditz, a=asa

Comment 12

[Aaron Leventhal]

Should show up in tomorrows Linux and Windows installers.

Comment 13

[Aaron Leventhal]

Marking fixed.

Comment 14

[Akkana Peck]

The .xpi is part of the linux sea installer on linux, but it doesn't actually
install components/libtypeaheadfind.so.  Even after I point it at the xpi and it
reports no errors, the library isn't there.  I do have
defaults/pref/typeaheadfind.js.

Should I reopen this bug, or is there another bug tracking the issue?

If I unzip the xpi, I see:    creating: bin/
   creating: bin/components/
  inflating: bin/components/libtypeaheadfind.so  
  inflating: bin/components/typeaheadfind.xpt  
   creating: bin/chrome/
  inflating: bin/chrome/typeaheadfind.jar  
   creating: bin/defaults/
   creating: bin/defaults/pref/
  inflating: bin/defaults/pref/typeaheadfind.js  
  inflating: install.js              

Comment 15

[Aaron Leventhal]

Aha, it's getting typeaheadfind.js from browser.xpi, somehow it got in there too.

For some reason, when setup.exe runs this line in xpi.c:
        hrResult = pfnXpiInstall(szArchive, "", 0xFFFF);

for typeaheadfind.xpi, it comes back with a successful result but nothing was
unzipped.

How do the bin\ directories get changed to the right name as things get unzipped?

Comment 16

[Aaron Leventhal]

Awww... man ....

I managed to screw up and not checkin the new typeaheadfind.jst files.
It was substituting some other install.js for mine, but there was no error.

Comment 17

[Aaron Leventhal]

Ah, I noticed a bug with the installer, we package all the .js prefs files
twice, once in browser.xpi and once in the specific package xpi.

Doesn't seem to hurt anything.

Comment 18

[Daniel Veditz [:dveditz]]

It hurts performance, because a browser-only install then has to parse extra
pref files at startup. But then supposedly profile migration doesn't work right
if the mailnews pref defaults aren't present, and then if the user later wants
to add mail there is no way to migrate mail. Sounds weak to me, but that was the
excuse at the time.

Comment 19

[(not reading, please use [e-mail] instead)]

> It hurts performance, because a browser-only install then has to parse extra
> pref files at startup. 

by how much?

> But then supposedly profile migration doesn't work right
> if the mailnews pref defaults aren't present, and then if the user later wants
> to add mail there is no way to migrate mail. Sounds weak to me, but that was 
> the excuse at the time.

if it is really a perf hit, here's an idea:  at installer time, if I install 
browser and mail, cat all.js mailnews.js editor.js > foo.js

and now you've only got one file to parse at startup.

but we need the mailnews defaults.  if you migrate your 4.x profile, we need to 
have the defaults in mailnews.js or as dan says, running mail later will fail.

common scenario:

1) install mozilla 1.0 browser only, migrate 4.x profile
2) decide you like it, so when 1.1 is out, you get browser and mail.

another alternative is to fix when / how we do migration.  but our resources 
are better spent elsewhere.

Comment 20

[(not reading, please use [e-mail] instead)]

sorry, I mean:

"if it is really a perf hit, here's an idea:  at installer time, if I install 
browser and mail, cat all.js mailnews.js editor.js > foo.js"

in the tree have seperate js files.  but in the installed default/prefs dir, 
have one.  so we pay for the hit at install time, not at startup each time.

Comment 21

[(not reading, please use [e-mail] instead)]

I've taken the perf issue to bug 168515, and assigned to dveditz.

Comment 22

[Grace Bush]

lob-
Sarah, are you familiar with this- enough to verify?

Comment 23

[sairuh (rarely reading bugmail)]

find as you type (aka, type ahead find) is now part of mozilla builds. tested
with 2003.01.31.08 mozilla bits on linux rh8.0.