113538 - embedding depends on wallet

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Chris Waterson]

Embedding requires wallet to compiled for it to build properly. (This is a
follow-on from bug 113515.)

Comment 1

[Chris Waterson]

Attachment: probably controversial fix

This would make the things in the mozilla/embedding directory not depend on
wallet anymore. This change may be controversial because it adds `-D' flags to
the command line, one per extension (e.g., -DMOZILLA_EXTENSION_wallet). I see
no other way to deal with the problem in
mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp. The other
change is to simply remove nsNonPersistentAuthPrompt, which doesn't appear to
be used by anyone.

Comment 2

[hacker formerly known as [e-mail]]

The extra defines seem bad as we then have functionality in the "core" that is
dependent upon whether or not we compiled with a certain extension.  Also, the
extra DEFINES should be set in configure.in as we don't properly track defines
set in config.mk.

Comment 3

[Chris Waterson]

Yeah, I know. dbaron suggested that we could do this more conservatively, just
adding defines for ``known non-extension extensions'' as opposed to _all_
extensions. (But I was _so_ proud of my gmake hackery! :-))

Would that be more palatable? I could use this approach to quickly eliminate the
DOM dependency on transformiix and xml-extras until jst or peterv gets around to
fixing bug 92377, e.g.

Comment 4

[timeless]

not that anyone cares, i think i've seen cases where the command line failed 
because it was too long. adding lots of long -D things would increase the 
chances of this happening.

Comment 5

[Adam Lock]

Maybe the wallet interface should seperated from the wallet implementation?
After all, an embedder might want to implement their own.

Comment 6

[Conrad Carlen (not reading bugmail)]

Separating the interface would be good. Embedding (nsPrompt.cpp) is not
dependent on the whole wallet iface - only nsISingleSignonPrompt. Looking at
that iface, it's pretty tiny and generic.

I think nsISingleSignonPrompt could be renamed nsIAuthPromptPersist (that's
really what it does (provides an optional persistence layer for nsIAuthPrompt)
and moved into embed components.

Comment 7

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

Let's move it into networking, where all the other auth stuff lives.

Comment 8

[Alec Flett]

I am VERY VERY much against using #ifdefs, and valeski and I have discussed that
before... the only acceptable #ifdef is for ripping XUL out of layout, and that
doesn't even work last I heard :)

Anyhow, I am much more in favor of conrad's approach. I had a bug on this bug I
didn't know how to tackle it.

Comment 9

[Alec Flett]

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

Comment 10

[Chris Waterson]

Sounds reasonable. I'll pick this up next milestone.

Comment 11

[Chris Waterson]

'Tis the season. Gifting bugs! alecf, if you don't want to finish this, reassign
to me and I'll get to it after my sabbatical.

Comment 12

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

I'm on this case.

Comment 13

[Brendan Eich [:brendan]]

We're past the 1st.  What's the status?  I'm asking anyone who may be working on
this, I suspect shaver has been busy elsewhere.

/be

Comment 14

[Stephen P. Morse]

I removed all the wallet dependencies mentioned in bug 113540.  That involved 
changes to the wallet module as well as to the mailnews module.

The patch here involves changes in embedding files but none in the wallet 
module.  Can someone in the embedding area take a look at it?

Comment 15

[hacker formerly known as [e-mail]]

Comment on attachment 60436 [details] [diff] [review]
probably controversial fix

Ignore that patch.

Comment 16

[hacker formerly known as [e-mail]]

I just attempted what Conrad suggested and it looked relatively easy to move
nsISingleSignonPrompt from ns*WalletService* to ns*PromptService*.  Then I hit a
snag.  It looks as though nsISingleSignonPrompt uses a number of helper
functions from singsign.* which looks like it's littered with wallet ifdefs. 
Back to you, Morse.

Comment 17

[Conrad Carlen (not reading bugmail)]

I wouldn't change the implementation of single-signon, only the location of its
iface. Right now, embedding has a compile-time dependency on the wallet module
because of where nsISingleSignonPrompt is declared. The runtime dep is not a
hard dep. The code in embedding will use a single-signon impl if it's available,
otherwise hook up a non-persistent prompt without failure.

Comment 18

[Brendan Eich [:brendan]]

Why can't we move nsISingleSignonPrompt into netwerk/base/public/nsIPrompt.idl,
or even eliminate it by putting its one method in nsIAuthPrompt?  Cc'ing darin.

/be

Comment 19

[Brendan Eich [:brendan]]

Oops, I meant "move nsISingleSignonPrompt into
netwerk/base/public/nsIAuthPrompt.idl".

/be

Comment 20

[Darin Fisher]

i don't really understand what it means to "provide an optional persistence
layer for nsIAuthPrompt", so it's difficult for me to understand where this
interface might best fit... would help if nsISingleSignonPrompt had some
documentation ;-)

peeked at the source, and it looks like this is just about giving a
nsIAuthPrompt some nsIPrompt interface to use.  that seems generic enough, or am
i missing something?  if it is indeed common that a nsIAuthPrompt may be
implemented in terms of a nsIPrompt, then i'd vote for brendan's suggestion to
expand nsIAuthPrompt.  on the other hand, if supporting this method would cause
trouble for implementors of nsIAuthPrompt, then it probably does belong on
another interface -- maybe they don't want to allow some other nsIPrompt to come
in and take over.  do we want to allow NS_ERROR_NOT_IMPLEMENTED as a valid
return code?  probably not...

if embedding is the only customer of this interface, then moving it into
embedding as conrad suggests in comment #6 makes sense to me.

Comment 21

[Conrad Carlen (not reading bugmail)]

Darin, this is what makes it optional:
http://lxr.mozilla.org/mozilla/source/embedding/components/windowwatcher/src/nsPrompt.cpp#68
See what happens when a single-signon can't be created? Still works and is
invisible to the consumer.
This function is the only place where nsISingleSignonPrompt is used.

Comment 22

[Darin Fisher]

so it seems to me that this should be an interface defined by the windowwatcher.
 in other words, the windowwatcher can say: "provide an impl of _this_
interface, and i'll use it in place of my nsIAuthPrompt impl."

something like, nsIAuthPromptOverride... or is this what "persist" is meant to
indicate?  should it therefore be nsIAuthPromptPersist?

conrad: what is your opinion on this?

Comment 23

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

Persist refers to the fact that the data is stored between sessions, I believe,
which is to say that it's an implementation detail of the object seeking to
interpose itself between the windowwatcher/promptservice and the user.

I like Override better, in case that matters.

Comment 24

[Darin Fisher]

shaver: thanks for clearing that up for me... yeah, i like nsIAuthPromptOverride
better too.

Comment 25

[Conrad Carlen (not reading bugmail)]

Override is OK. Really, nsISingleSignonPrompt is a filter that's placed over the
nsIAuthPrompt impl. How about "Filter" as a modifier?

Comment 26

[Darin Fisher]

override, filter... either seems fine.  they are almost equivalent in this
context i think.

Comment 27

[Brendan Eich [:brendan]]

Filter and override imply mutation, but an implementation could just observe,
right?  Maybe wrapper is best.  But don't let me slow you down.

Conrad, can you take this bug?

/be

Comment 28

[Darin Fisher]

ooh.. yeah, i like wrapper even better ;-)

Comment 29

[Conrad Carlen (not reading bugmail)]

I'll take it. nsIAuthPromptWrapper it is. If it needs to be in 0.9.9, let me know. 

Comment 30

[Brendan Eich [:brendan]]

Conrad, we need this last week.  That was the deadline for decoupling extensions
from the default --with-extensions=none build.  We're past it, and I'm so far
avoiding taking drastic steps.  This seems like an easy fix, can you Just Do It
(tm)?  Thanks,

/be

Comment 31

[Conrad Carlen (not reading bugmail)]

I'm on it

Comment 32

[Conrad Carlen (not reading bugmail)]

Attachment: nsISingleSignon renamed and relocated

Two patches - one from embedding/components and another from extensions/wallet.
Need r=.

Comment 33

[Darin Fisher]

Comment on attachment 68197 [details] [diff] [review]
nsISingleSignon renamed and relocated

sr=darin

Comment 34

[Stephen P. Morse]

Comment on attachment 68197 [details] [diff] [review]
nsISingleSignon renamed and relocated

r=morse

Comment 35

[Conrad Carlen (not reading bugmail)]

Rats - I was about to make the checkin this morning and the clock struck 8:00.
In doing full builds (3 platforms) I found a file which is not used and needs to
be removed: embedding/browser/webBrowser/nsNonPersistAuthPrompt.cpp/.h. I'll
remove those with the rest of this.

Comment 36

[Conrad Carlen (not reading bugmail)]

Checked in. Thanks for quick reviews.