76664 - (MozillaControl) [ActiveX] control needs proper profile/directory service support

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: ActiveX Wrapper, defect)

Description

[Adam Lock]

The ActiveX control needs to set a profile or provide a directory service 
provider in order for Gecko to function correctly.

Options include:

Use profiles, share with Mozilla
Pro: People can share profiles between Mozilla and the control
Con: Mozilla must default to a profile name such as "MozillaControl" because it 
cannot display a profile choose dialog when instantiated.
Con: Seeing Mozilla start with the profile manager because "MozillaControl" is 
there could be annoying.

Use profiles, don't share with Mozilla app
Pro: Profiles are maintained seperately from Mozilla
Con: People may want to share profiles between Mozilla and the control
Con: No way to manage profiles, e.g. create/delete them because control has no 
UI for that purpose

Use directory location provider.
Pro: Lighter, complete customisability
Con: Not compatible with Mozilla at all.
Con: Don't get prefs for free, must be managed manually.

Perhaps a sensible default from the above should be chosen with methods added to 
IMozControlBridge to allow the programmer to change the default 
profile name or mechanism.

Comment 1

[:deanis74]

Option 1 is definitely out. Just because I run an app that uses the ActiveX
control doesn't mean that I should start getting confused because there's now
two profiles. Not to mention really annoyed because I have to go through the
profile selection dialog each time I start Moz.

I like option 3.  From what I read in bug 76654, we don't need profiles, really,
other than because they provide easy access to a cache directory, etc.  I don't
see that it's not compatible with Moz as a con, I think it's a pro.  They should
be completely separate.  And the prefs should be handled by whatever app embeds
the control, not by the control itself.

Comment 2

[Adam Lock]

0.9 does option 1 because it was the quickest, but I'm open to suggestions 
(hence this bug)

Just note that the other options are less convenient since there is no easy way 
to change prefs or to add, delete, rename profiles short of writing a bunch of 
dialogs to do it.

I also imagine that some people *will* want their control to pick up their Moz 
settings while others won't. This means I may craft the profile setting code to 
default to one behaviour but allow it to be changed via IMozControlBridge.

Comment 3

[:deanis74]

"I also imagine that some people *will* want their control to pick up their Moz 
settings"

I don't think so.  People will want K-meleon to pick up their K-meleon settings, 
NeoPlanet to pick up their NeoPlanet settings, and Mozilla to pick up their 
Mozilla settings.

Comment 4

[Adam Lock]

Following patch introduces standalone profiles to Mozilla control. Based on work 
for bug 80932

Comment 5

[Adam Lock]

Attachment: Patch makes control use a directory provider object

Comment 6

[Benoit]

Is this patch still valid? 
The Winamp3 minibrowser still creates his own profile (mozillaControl) in the
mozilla profile directory, causing the profile manager to appear each time
Mozilla is launched. Or is it a Winamp3 bug? I haven't this problem with K-Meleon.

Comment 7

[Adam Lock]

I didn't even know WinAmp used my control :)

Yes, there's still a reason for this bug to be open and eventually I will try
and address it. Basically the choices still boil down to those I cited when I
opened it, either share profiles with Mozilla or write my own directory service
and store them somewhere else (like mfcEmbed). The latter stops "MozillaControl"
appearing in the picker when you start Moz, but is harder to get right.

Comment 8

[Adam Lock]

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

Comment 9

[Adam Lock]

Read bug 80932 for more info

Comment 10

[Conrad Carlen (not reading bugmail)]

With changes in bug 116435, it's much easier for an embedding app to do this. To
have your own profiles and registry, your provider just needs to support
NS_APP_APPLICATION_REGISTRY_DIR, NS_APP_APPLICATION_REGISTRY_FILE, and
NS_APP_USER_PROFILES_ROOT_DIR. Keys you don't support will fall back to the
application provider in xpcom. 

Comment 11

[Adam Lock]

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

Comment 12

[Adam Lock]

Attachment: Generic directory provider

Patch is work in progress. Patch represents a "first compiling" version of a
simple, generic directory service provider object. Embedders will be able to
create this object and configure it through a few properties on its
nsISimpleDirectoryProvider interface to hand out file locations.

Comment 13

[Glenn Chittenden Jr.]

Is this still being worked on?

Comment 14

[Adam Lock]

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

Comment 15

[Brian 'netdragon' Bober]

This bug depends on bug 80932, which is duped against this one.

Comment 16

[Adam Lock]

Attachment: Simple directory provider object

This patch implements a simple directory provider object. It holds onto three
values supplied by the creator, the application data root dir, the application
registry dir and the profile root dir. The embedder can create this object, set
those three things and then add it to the chain of providers.

Conrad, does this look okay to you?

Another patch follows which shows the MozillaControl changed to use it.

Comment 17

[Adam Lock]

Attachment: Patch for control

Patch changes control to create the simpl directory provider, initialise it
with the directories for stuff and add it to the provider chain.

Comment 18

[Conrad Carlen (not reading bugmail)]

+    rv = NS_InitEmbedding(binDir, nsnull);
+
+    nsCOMPtr<nsIDirectoryService> directoryService =
do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);
+    if (NS_FAILED(rv))
+    {
+        return E_FAIL;
+    }
+
+    nsCOMPtr<nsIDirectoryServiceProvider> directoryServiceProvider =
do_QueryInterface(directoryService);
+    // Create a directory provider object to point at the control's own app data
+    nsCOMPtr<nsISimpleDirectoryProvider> simpleProvider =
+        do_CreateInstance(NS_SIMPLEDIRECTORYPROVIDER_CONTRACTID, &rv);

  The provider is being created *after* XPCOM is initialized. It needs to be
created before and passed to NS_InitEmbedding. It may work right now doing it
afterwards, but that's not guaranteed. This app-level provider is done the way
that it is so that it can provide locations during XPCOM initialization. Thus,
it can't be gotten as a component.

Comment 19

[Adam Lock]

I didn't put it before because I can't see a way to create the normal directory
service manager before XPCOM is initialised. If I call do_GetService for the
provider, it indirectly calls NS_InitXPCOM and NS_InitEmbedding returns with an
error.

Would it be acceptable to create a NS_NewDefaultDirectoryProvider for this
purpose? It would work similarly to NS_NewLocalFile, being safe to call before
NS_InitXPCOM has been called. Perhaps even the 'simple' provider functionality
can even be merged into the default provider.

Comment 20

[Conrad Carlen (not reading bugmail)]

I don't see why we need a globally available "default." The amount of code
required to initialize such an object is about as much code as you need to write
in order to make your own provider, anyway. And, it's not as cleanly
encapsulated in an object. BTW, the impl in winEmbedFileLocProvider.cpp provides
just about *every* location, making it larger than it needs to be. It could be
reduced down to the 3 or so commonly overridden locations and commented that
only a subset are being provided but more are possible.

Comment 21

[Adam Lock]

Can I clarify - if I create the simple directory provider object and pass that
into NS_InitEmbedding and only implement 3 or so locations and return errors for
the rest, then XPCOM will fallback on the default provider for those files?

Comment 22

[Conrad Carlen (not reading bugmail)]

> then XPCOM will fallback on the default provider for those files

Yes. I need to change and comment the impls in the embedding samples to make
that more clear.

Comment 23

[Adam Lock]

Attachment: Patch

Giving up on the idea of a reusable simple provider object. This patch
implements the class directly in MozillaBrowser.cpp, an instance of which is
fed straight into NS_InitEmbedding. It is pretty simple and clean.

Reviews?

Comment 24

[Conrad Carlen (not reading bugmail)]

Comment on attachment 108387 [details] [diff] [review]
Patch

Nice and simple. r=ccarlen

Comment 25

[Adam Lock]

Comment on attachment 108387 [details] [diff] [review]
Patch

Chris can you sr this? It's an embedding API thing even though it's in the
control. Thanks.

Comment 26

[Christopher Blizzard (:blizzard)]

Comment on attachment 108387 [details] [diff] [review]
Patch

>+    NS_ENSURE_ARG_POINTER(prop);
>+    NS_ENSURE_ARG_POINTER(persistent);
>+    NS_ENSURE_ARG_POINTER(_retval);
>+
>+	*_retval = nsnull;
>+	*persistent = PR_TRUE;
>+

Got a couple of tabs in there?

Other than that, sr=blizzard.

Comment 27

[Markus Gerstel]

Comment on attachment 108387 [details] [diff] [review]
Patch

Can we get this into the trunk at monday ? This is urgently needed in the
1.0-branch for embedding.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 108387 [details] [diff] [review]
Patch

not a 1.3a blocker. please hold until we open the tree for 1.3beta. thanks.

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 108387 [details] [diff] [review]
Patch

change of heart. a=asa for checkin to 1.3a. this needs to land today if it's
gonna make 1.3a.

Comment 30

[Adam Lock]

Fix is checked into 1.3a. I have my doubts that it will go into the 1.0.x
branch. I'd mark this bug fixed1.3 if there were such a keyword.

Comment 31

[Adam Lock]

fixed

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 108387 [details] [diff] [review]
Patch

approved for 1.0 branch; add fixed1.0.2 to keywords when checked in.  Please
check in ASAP to make 1.0.2 cut.

Comment 33

[Adam Lock]

Checked into 1.0.x branch

Comment 34

[Markus Gerstel]

Verified. Many thanks for the quick fix, r, sr, a and checkin =)