119923 - [minimo]implement minimal chrome registry

Status: RESOLVED WONTFIX

(Minimo Graveyard :: General, defect, P1)

Description

[Chris Waterson]

We could probably break the dependency of embedding on RDF (and perhaps a few
other modules) if we implemented a `minimal' chrome registry. Such an
implementation might be a hard-coded list of lookups for scrollbars, etc.

Comment 1

[Alec Flett]

I hope I can get to this for moz 1.0 :)

Comment 2

[David Hyatt]

This is an issue for Chimera. Hopefully no one minds if I adorn the status 
whiteboard to indicate that.

Let me know if you need any help/support with this.  IMO you just have to 
make an in-memory table mapping for the packages, skins, and locales 
listed in installed-chrome.txt in embedding/config.

We should probably also do something like patch NS_InitEmbedding 
(which presumably isn't called by the main app?) to register the simpler 
chrome registry automatically.

Another note.  Make sure not to eliminate the user stylesheet support.  
That should carry over to a simpler chrome registry.

Comment 3

[Marek Z. Jeziorek]

This is not related to a top priority embedding project at this point.  Minusing
to topembed- and adding embed.

Comment 4

[Alec Flett]

nsIChromeRegistry is huge. I might cut it up to make implementation simpler (and
just force some existing consumers to QI())

Comment 5

[Alec Flett]

Attachment: Split nsIChromeRegistry into 3 interfaces

ok, looking for an sr=hyatt here :)
I've broken out some of the routines from nsIChromeRegistry into 3 interfaces:
- nsIChromeInstaller - contains all the runtime-install stuff that xpinstall
uses, such as installSkin(), etc
- nsIChromePackageRegistry - contains all the per-package stuff, like
selectSkinForPackage(), isLocaleSelectedForPackage(), etc
- nsIChromeRegistry - everything else

While I was there, I also did some big cleanups to switch lots of internal
methods from nsCString& to nsACString&, which allowed us to remove a ton of
nsCAutoStrings, and use NS_LITERAL_CSTRING instead (which should be faster)

None of this has any real effect on the existing chrome registry
implementation.

I'd like to land this so that the embedding-only registry can simply implement
nsIChromeRegistry - that seems to be all we need to have an embedding registry.


reviews?

Comment 6

[David Hyatt]

I don't really see the necessity of splitting up nsIChromeInstaller and 
nsIChromePackageRegistry.  Both deal with the installation and selection 
of skins, and IMO conceptually they belong together.  "Package" is also 
the incorrect term to use here, since the interface contains skin and locale 
methods.

I'd prefer to just see two interfaces:

(1) nsIXULChromeRegistry (which contains all the methods of your 
nsIChromeInstaller and nsIChromePackageRegistry)
(2) nsIChromeRegistry (the base interface)

nsIXULChromeRegistry should *inherit* from nsIChromeRegistry so that 
JS code that wishes to use the base chrome registry methods doesn't 
have to QI.

Be aware that there is JS code that uses nsIChromeRegistry, so you'll 
need to patch it to use nsIXULChromeRegistry instead.  (The code that 
selects skins dynamically probably does this.  Although it may be 
commented out, it should still be patched.)

Comment 7

[David Hyatt]

Also, the code for agent sheets and user sheets needs to be in the 
minimal chrome registry.  That is not part of the XUL chrome registry.  

Ultimately this code should be broken out into a completely different 
object (that isn't a chrome registry at all), but that can wait.  The point is 
that it is used by embedding and is part of a minimal registry.

Comment 8

[Alec Flett]

Ok, thanks for the suggestions, I'll reduce this to 2 interfaces.

do you have any other suggestions for what goes in nsIXULChromeRegistry? I had a
few questionable routines:

* boolean allowScriptsForSkin(in nsIURI url);
is scriptablity something XUL-specific?

* void reloadChrome();
if chrome isn't XUL does this method make any sense? Maybe it does in case
chimera or embedding wants a call to reload chrome?

* void getAgentSheets(in nsIDocShell docShell, out nsISupportsArray styleSheets);
the implementation is very XUL-specific

* nsISimpleEnumerator getOverlays(in nsIURI aChromeURL);
overlays also seem to be xul-specific.

In fact, I'm wondering if any of the "skin" or "locale" related stuff even makes
sense in nsIChromeRegistry... if that's the case, it looks like all the basic
chrome registry stuff does is deal with chrome URLs. Thoughts?

Comment 9

[David Hyatt]

getAgentSheets *is* relevant for embedding and would be part of a 
minimal chrome registry, since you need to (at the very least) supply the 
scrollbar sheet for embedding.  User sheets should be part of the base 
registry as well.

Everything dealing with skins and locales should just be part of 
nsIXULChromeRegistry IMHO.  Of the methods you listed, all belong in 
nsIXULChromeRegistry except for getAgentSheets.

Comment 10

[David Hyatt]

You should sanity-check all the locale stuff.  They may have code that 
asks for the current locale in embedding.  I'm not sure.

Comment 11

[Alec Flett]

Attachment: Split nsIChromeRegistry into 2 interfaces

ok, I've taken hyatt's suggestions and implemented them... nsIChromeRegistry is
now split into nsIChromeRegistry and nsIXULChromeRegistry, and I've updated all
the C++ and JS I could find (via lxr) that would affect this.

hyatt, mind taking a look, and possibly sr'ing?

Comment 12

[Alec Flett]

Attachment: the resulting nsIChromeRegistry.idl

the resulting .idl file, to make things clearer

Comment 13

[David Hyatt]

Yeah, this split looks reasonable.

Comment 14

[David Hyatt]

Comment on attachment 77717 [details] [diff] [review]
Split nsIChromeRegistry into 2 interfaces

sr=hyatt

Comment 15

[Chris Waterson]

Lose this, and r=waterson.

@@ -1188,6 +1187,10 @@
 static nsresult main1(int argc, char* argv[], nsISupports *nativeApp )
 {
   nsresult rv;
+
+  printf("sizeof(nsStr) = %d\n", sizeof(nsStr));
+  printf("sizeof(nsString) = %d\n", sizeof(nsString));
+  

Comment 16

[Alec Flett]

Comment on attachment 77717 [details] [diff] [review]
Split nsIChromeRegistry into 2 interfaces

oops! 
printf's removed. I'm adding the has-review flag to make it easier for drivers
to see that the patch is ready
(also grepped the rest of the patch for printfs, just to be sure!)

Comment 17

[Alec Flett]

ok, with a little trouble, this patch has landed on the trunk. tomorrow's builds
will have this, so hopefully all will be well. I tested everything I could think
of...

Comment 18

[scottputterman]

Adding adt1.0.0-.  We can ship Mach V beta without this and this is topembed-. 
If this is needed for Mozilla1.0, please land after Mach V branches.

Comment 19

[Vidar Haarr (not reading bugmail)]

mark fixed ?

Comment 20

[Alec Flett]

I'll do that when I actually fix the bug.

Comment 21

[Alec Flett]

*** Bug 68832 has been marked as a duplicate of this bug. ***

Comment 22

[Alec Flett]

moving bugs to mozilla 1.2alpha that missed 1.1beta

Comment 23

[Alec Flett]

Attachment: convert CID to ContractID in all consumers

This converts all consumers of the chrome registry over to using a ContractID,
so that I can do a drop-in replacement by just registering under the
contractid. It also makes the CID private, so it can only be referenced from
the chrome DLL. 

I'll have a patch for the commercial tree momentarily. Can I get reviews?

Comment 24

[Alec Flett]

Attachment: convert CID to ContractID in commercial tree

here's the commercial patch.

Comment 25

[Alec Flett]

Comment on attachment 90978 [details] [diff] [review]
convert CID to ContractID in commercial tree

ignore the fact that we're switching from nsIChromeRegistry to
nsIXULChromeRegistry - the code is #if 0'ed, and we will need to use
nsIXULChromeRegistry if this code is ever turned on.

Comment 26

[Brian Nesse (gone)]

Comment on attachment 90977 [details] [diff] [review]
convert CID to ContractID in all consumers

please be consistent in your use of NS_CHROMEREGISTRY_CONTRACTID vs.
"@mozilla.org/chrome/chrome-registry;1" in your do_GetService calls. with
that... r=bnesse.

Comment 27

[Brian Nesse (gone)]

Comment on attachment 90978 [details] [diff] [review]
convert CID to ContractID in commercial tree

r=bnesse.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 90978 [details] [diff] [review]
convert CID to ContractID in commercial tree

sr=dveditz

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 90977 [details] [diff] [review]
convert CID to ContractID in all consumers

>Index: rdf/chrome/public/nsIChromeRegistry.idl
>===================================================================
>+#define NS_CHROMEREGISTRY_CONTRACTID \
>+  "@mozilla.org/chrome/chrome-registry;1"
> %}

When I added the embeddable interfaces for XPInstall I got slapped
during super-review for including my contractid define in the .idl file
("the contract id refers to a COMPONENT, the .idl describes an
INTERFACE"). I had to include the contract ID as a comment instead.

Bah. This way makes more sense to me than having all the consumers define
it themselves (or include yet another header file just for that) and I
prefer it, but thought I should mention the other line of thought.

>+                 do_GetService("@mozilla.org/chrome/chrome-registry;1", &rv);

You're changing these to the #define, right?

sr=dveditz

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 90977 [details] [diff] [review]
convert CID to ContractID in all consumers

a=asa (on behalf of drivers) for checkin to 1.1

Comment 31

[Alec Flett]

moving some stuff out to mozilla1.2beta that didn't make the mozilla1.2alpha train

Comment 32

[Alec Flett]

unfortunately RDF is too ingrained in the chrome registry right now to be able
to fix this for 1.2. Moving out to 1.3, which will require a change to the
chrome registry format, probably to some sort of "contents.txt" solution.

Comment 33

[Alec Flett]

ok, I've made some signifigant progress here. 

Here's how I'm going to tackle this:
- regchrome is going to have a new option -s to generated a "simplified" chrome
registry given an existing chrome.rdf file. This will basically read the current
chrome.rdf and turn it into a chrome.properties which will be a sort of "flat
file" version of the chrome registry
- a simplified chrome registry implementation will read chrome.properties and
will be able to answer all the same questions to get chrome URLs mapped to real
URLs.
- when you go to build the embedding client, the build system will generate
embed.jar, and then sic regchrome on it to generate a chrome.properties for the
file. When you distribute the embedding client, all you need to distribute is
the chrome.properties file

This means we'll still need RDF at BUILD time in order to process all the
contents.rdf files that exist in embed.jar, but once they are processed, we
won't need rdf.dll at runtime!

The other nice thing this means is that the only changes are:
- regchrome.cpp - to add this new capability (mostly complete)
- build system - to support generation of this new file

so no changes are necessary to the core gecko product.

Comment 34

[Alec Flett]

Attachment: ability to output to a .properties file

ok, this patch adds a switch "-p" to regchrome which allows someone to specify
a flat-file .properties file with all the data in the chrome registry. without
the -p, the behavior is the same. I'll attach a sample embed-chrome.properties
that I generated from the embedding build.

Comment 35

[Alec Flett]

Attachment: sample .properties file

here is a .properties file that could be read by a bare-bones chrome registry.

Comment 36

[Alec Flett]

Comment on attachment 121347 [details] [diff] [review]
ability to output to a .properties file

requesting reviews from dougt and bryner - this will allow us to have a
lightweight chrome registry (which could be used by embedding and/or camino...)

Comment 37

[Brian Ryner (not reading)]

Comment on attachment 121347 [details] [diff] [review]
ability to output to a .properties file

>+        
>+        // remove the prefix, the provider type, and the trailing ':'
>+        // (the compiler will optimize out the -1 + 1

close the parentheses.

Looks good.  sr=bryner.

Comment 38

[Alec Flett]

Attachment: ability to output to a .properties v1.1

ok, I was missing selectedSkin and selectedLocale because they followed
resources, not literals.

sorry I'll need reviews again :(

Comment 39

[Alec Flett]

Comment on attachment 121528 [details] [diff] [review]
ability to output to a .properties v1.1

hyatt/bryner - sorry I need a review on this again - some extra tweaks to
handle selectedSkin/selectedLocale

Comment 40

[Alec Flett]

I've also been hacking on nsEmbedChromeRegistry and am almost done implementing
nsIChromeRegistry such that it can auto-resolve URLs. the only other trick may
be looking at calls to nsIXULChromeRegistry::GetSelectedLocale() to see if they
should be converted to something more generic.

Comment 41

[Brian Ryner (not reading)]

Comment on attachment 121528 [details] [diff] [review]
ability to output to a .properties v1.1

sr=me

Comment 42

[David Hyatt]

Comment on attachment 121528 [details] [diff] [review]
ability to output to a .properties v1.1

r=hyatt

Comment 43

[Stephen Donner [:stephend] Not actively reading bugmail]

clearing target milestone, since 1.4 is long gone.

Comment 44

[Alec Flett]

just for my own reference, all of the above patches have been checked in.

next step, a chrome registry that can read .properties files.

Comment 45

[Doug Turner (:dougt)]

moving minimo bugs to the new bugzilla product.

Comment 46

[Doug Turner (:dougt)]

ben, any interest in this direction?  alec, any continued interest?

Comment 47

[Doug Turner (:dougt)]

marking as wont fix.  benjamin has done a great job on mozilla/chrome.  that is
what minimo will use.