295475 - Addons aren't found anymore after upgrading from 1.8b1 to current trunk [overlayinfo unused]

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Karsten Düsterloh]

After upgrading Mozilla from 1.8b1 to current trunk, all (profile or app)
installed addons are gone (see s.t.r. below) - most probably because the old
overlayinfo data isn't recognized by current nightly builds anymore.

This is very unfriendly behaviour, IMO.

Neil suggested to do the following:
If the old style files exist, 
  then use those, 
otherwise 
  use the new style files


Steps to reproduce:
1. Create a profile with an old build, eg. 1.8b1
2. Install addons into application or profile chrome
3. Start the profile with a current nightly build, eg. 2005-05-24
=> all addons are gone

Comment 1

[Karsten Düsterloh]

Btw: if use the profile with an old style build again, the addons are still
there and working...

Comment 2

[Karsten Düsterloh]

Attachment: Watch out for profile overlayinfo/, v1

On startup, let the chrome registry check and remember the existence of an old
profile overlayinfo directory structure. If one is found, use it, else use the
new flat files.

Comment 3

[[e-mail]]

Comment on attachment 184580 [details] [diff] [review]
Watch out for profile overlayinfo/, v1

Let me count the errors:
1. Not dealing with profile switches
2. Might be checking before any profile exists
3. Adds overlayinfo for global chrome

If you could come up with a solution that puts new installations into
overlays.rdf then that would be great :-)

Comment 4

[Karsten Düsterloh]

Attachment: profile switch aware version, v2

Addressed Neil's comments: the overlayinfo status is checked
- after each profile change
- only if a profile could be loaded
- only for profiles
I see no point in registering new profile addons in a new style overlayinfo
structure parallel to an existing old style one. If we find an old one, we
should use that - splitting the profile overlayinfo handling would be more
costly than the gain (whatever benefit that might be, anyway).

Comment 5

[Karsten Düsterloh]

Attachment: missed an init, otherwise unchanged; v3

Comment 6

[[e-mail]]

Comment on attachment 184642 [details] [diff] [review]
missed an init, otherwise unchanged; v3

>+  nsCAutoString overlayFile(aIsOverlay ? "overlays.rdf" : "stylesheets.rdf"); 
>+  if (aUseProfile && mLegacyOverlayinfo)
>+  {
>+    nsCAutoString prefix(NS_LITERAL_CSTRING("overlayinfo/") + package);
>+    prefix += aIsOverlay ? "/content/" : "/skin/";
>+    overlayFile.Insert(prefix, 0);
>+  }
Here, I think I'd prefer if you started with an empty string, then used
AppendLiteral to build it up.

>+        PRBool isLegacyOverlayinfo;
>+        rv = overlayinfoDir->Exists(&isLegacyOverlayinfo);
>+        if (NS_SUCCEEDED(rv) && isLegacyOverlayinfo)
>+        {
>+          rv = overlayinfoDir->IsDirectory(&isLegacyOverlayinfo);
>+          mLegacyOverlayinfo = NS_SUCCEEDED(rv) && isLegacyOverlayinfo;
>+        }
Personally I'd just call IsDirectory(&mLegacyOverlayinfo); but I suppose you
can't rely on it setting the value to true if the folder does not exist. What I
think you should be able to rely on is that Exists always succeeds, and if it
returns true, then IsDirectory always succeeds.

Comment 7

[Karsten Düsterloh]

Attachment: comments fixed; v4

Taking over Neil's r+, but I'm unsure who'll do sr? 
It's SeaMonkey-only, but maybe darin or jag should give a third eye?

Comment 8

[[e-mail]]

Comment on attachment 184735 [details] [diff] [review]
comments fixed; v4

Sorry, you can't use ?: inside AppendLiteral...

Please try Darin next time ;-)

Comment 9

[Karsten Düsterloh]

Attachment: no ?: in AppendLiteral call; v5

Changed ?: to if-else, demanded by Neil ;-), even though I'm not quite happy
with that. The usage of ?: shouldn't make any difference wrt the type of thingy
passed to AppendLiteral, AFAICS. And even if, I'd rather use AppendASCII with
?: than the stuff now found in this patch, since the strings are passed to
AppendASCII anyway and the if-else will lead to unnecessarily doubled code with
unoptimized compilers...
Darin, can you sr?

Comment 10

[Darin Fisher]

Comment on attachment 184818 [details] [diff] [review]
no ?: in AppendLiteral call; v5

>Index: chrome/src/nsChromeRegistry.cpp

>+        overlayinfoDir->Exists(&mLegacyOverlayinfo); // all known implementations return NS_OK

coding to all known implementations is bad engineering practice.
will someone who changes nsLocalFileWin.cpp know to also change
this code?  you should always handle errors.


>Index: chrome/src/nsChromeRegistry.h

>   // make sure we only look once for the JAR override
>   PRPackedBool mSearchedForOverride;
>+  
>+  // if we find an old profile overlayinfo/ directory structure, use it
>+  // else use the new flat files
>+  PRBool mLegacyOverlayinfo;
> };

use PRPackedBool instead.

Comment 11

[Karsten Düsterloh]

Attachment: final version; v6

Reverting the changes Darin didn't like, thus carrying over sr+.
Also killing the unnecessary Exist check, r=Neil over IRC for that.
Waiting for the tree to open... :)

Comment 12

[Karsten Düsterloh]

Comment on attachment 184970 [details] [diff] [review]
final version; v6

SeaMonkey Chrome Registry.

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 184970 [details] [diff] [review]
final version; v6

a=shaver

Comment 14

[Karsten Düsterloh]

v6 checked in.

Comment 15

[Karsten Düsterloh]

JFTR: the bug that introduced the new style overlayinfo in SM was bug 191309.