116435 - provider in nsAppFileLocationProvider.cpp should always be registered

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Conrad Carlen (not reading bugmail)]

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: patch to make it always registered

The patch makes it so it's always registered. In RegisterProvider, new
providers are always added so that the default one we added is searched last.

Comment 2

[Conrad Carlen (not reading bugmail)]

Oh - and some explaination might help

The problem now is that, if you supply your own provider, the default provider
in xpcom is not used - it's one or the other. Instead, I think the service
should always create and register the default provider. The providers are
searched in a known order. If the one they supply doesn't supply a location, the
service will go on to ask the default provider. This makes it easier for the
embeddor. Instead of having to supply *all* those locations just because they
want to override one, their provider can just provide the locations they want to
change.

This was from mail to rpotts after he ran into a problem which is now bug 116323.

Comment 3

[rpotts (gone)]

hey conrad,

can't we just insert the default provider when the directory service is created
and then append any other providers?

it seems like your code to always insert new providers after the default
provider will put them in the wrong order if there is more than one provider...

-- rick 

Comment 4

[rpotts (gone)]

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

Comment 5

[Conrad Carlen (not reading bugmail)]

> it seems like your code to always insert new providers after the default
They get inserted before the default which is what we want. Look at
InsertElementAt(). The enumeration of providers goes forward from the first elem
so we want the default provider at the end and providers added with
RegisterProvider in order before that.

Comment 6

[rpotts (gone)]

hey conrad,

i guess i'm still confused :-(  it seems like the following will happen if 2
providers are inserted into the list:

1. the default provider 'A' is added:

element:    A
index       0

2. the first provider 'B' is inserted at index 0

element:    B A
index       0 1

3. the second provider 'C' is inserted at index 1

element:    B C A
index       0 1 2

Isn't this the wrong order?  it seems like you want the order to be 'C B A'... 
Shouldn't the new elements *always* be inserted at index 0 to achieve this?

It seems like it would be easier to just call AppendElement(...) and then walk
the list from back to front when enumerating...

So in the above example, the list would look like 'A B C' and the enumerator
would just walk backwards from 'C' to 'A'...

what am i missing?
-- rick

Comment 7

[Conrad Carlen (not reading bugmail)]

3. the second provider 'C' is inserted at index 1

element:    B C A
index       0 1 2

This 'BCA' is what I wanted. Since C is registered after B, it should follow it
in the enumeration in order to match current behavior.

Maybe the current behavior is not that sacred. The way you suggest:

> just call AppendElement(...) and then walk the list from back to front when
> enumerating...

is much simpler and and the rule: providers will be searched from most recently
registered to least recently registered makes sense.

I'll make the change later today.

Comment 8

[Conrad Carlen (not reading bugmail)]

Sorry for the delay there - holidays.

I think I'll stick with the patch as-is. It keeps the search order the same as
it always has been: providers are searched in the order they're added. The
default app provider is always searched after these. I think that's what we want.

Comment 9

[rpotts (gone)]

The main thing that worries me about calling the providers in 
first-in-first-called order is that it prevents later providers from 'overriding 
' the behavior of earlier providers.

This means that there is *no* way to override the behavior of a provider... new 
providers can *only* support new behavior.

I'm not sure how big an issue this is :-)  But it seems like calling them 
first-in-last-called addresses this issue and has the added benifit of being 
simplier ;-)

I bet that switching to this calling order will not be an issue (at this point) 
because up until now we haven't had the notion of a 'default' provider...

Comment 10

[Conrad Carlen (not reading bugmail)]

Attachment: patch enumerate backwards

Yeah - since there has been no overlap between providers until this point,
changing the order of registered providers won't change anything. And, it is
simpler :-)

Rick, Doug - can you r/sr?

Comment 11

[Conrad Carlen (not reading bugmail)]

Can I get review on this and get it into 0.9.8?

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 63237 [details] [diff] [review]
patch enumerate backwards

r=dougt

Comment 13

[rpotts (gone)]

Comment on attachment 63237 [details] [diff] [review]
patch enumerate backwards

sr=[e-mail]

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 63237 [details] [diff] [review]
patch enumerate backwards

a=dbaron for 0.9.8

Comment 15

[Conrad Carlen (not reading bugmail)]

Fix checked in. Now if the embedding app's provider does not provide a location,
it can fall back to the one in nsAppFileLocationProvider.cpp

Now, to make the embedding samples even better samples, I'd like to diverge the
providers implemented by MfcEmbed & PPEmbed. One should be complete - support
all keys and implement nsIDirectoryServiceProvider2. The other should do the
bare minimum that's required to have its own app registry and profiles. Both
strategies (overriding all vs. overriding minimum) are probably of interest to
embeddors and they should have lxr links to one another.