Closed
Bug 113538
Opened 23 years ago
Closed 23 years ago
embedding depends on wallet
Categories
(Core :: DOM: Navigation, defect, P3)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla0.9.9
People
(Reporter: waterson, Assigned: ccarlen)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
morse
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
Embedding requires wallet to compiled for it to build properly. (This is a
follow-on from bug 113515.)
Reporter | ||
Updated•23 years ago
|
Blocks: 18352
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla0.9.7
Reporter | ||
Comment 1•23 years ago
|
||
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•23 years ago
|
||
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.
Reporter | ||
Comment 3•23 years ago
|
||
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.
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.
Maybe the wallet interface should seperated from the wallet implementation?
After all, an embedder might want to implement their own.
Assignee | ||
Comment 6•23 years ago
|
||
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.
Let's move it into networking, where all the other auth stuff lives.
Comment 8•23 years ago
|
||
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•23 years ago
|
||
*** Bug 100198 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 10•23 years ago
|
||
Sounds reasonable. I'll pick this up next milestone.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Reporter | ||
Comment 11•23 years ago
|
||
'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.
Assignee: waterson → alecf
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.8 → ---
I'm on this case.
Assignee: alecf → shaver
Comment 13•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 60436 [details] [diff] [review]
probably controversial fix
Ignore that patch.
Attachment #60436 -
Attachment is obsolete: true
Attachment #60436 -
Flags: needs-work+
Comment 16•23 years ago
|
||
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.
Keywords: patch
Assignee | ||
Comment 17•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Oops, I meant "move nsISingleSignonPrompt into
netwerk/base/public/nsIAuthPrompt.idl".
/be
Comment 20•23 years ago
|
||
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.
Assignee | ||
Comment 21•23 years ago
|
||
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•23 years ago
|
||
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?
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•23 years ago
|
||
shaver: thanks for clearing that up for me... yeah, i like nsIAuthPromptOverride
better too.
Assignee | ||
Comment 25•23 years ago
|
||
Override is OK. Really, nsISingleSignonPrompt is a filter that's placed over the
nsIAuthPrompt impl. How about "Filter" as a modifier?
Comment 26•23 years ago
|
||
override, filter... either seems fine. they are almost equivalent in this
context i think.
Comment 27•23 years ago
|
||
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•23 years ago
|
||
ooh.. yeah, i like wrapper even better ;-)
Assignee | ||
Comment 29•23 years ago
|
||
I'll take it. nsIAuthPromptWrapper it is. If it needs to be in 0.9.9, let me know.
Assignee: shaver → ccarlen
Target Milestone: --- → mozilla1.0
Comment 30•23 years ago
|
||
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
Keywords: mozilla0.9.9
Assignee | ||
Comment 31•23 years ago
|
||
I'm on it
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0 → mozilla0.9.9
Assignee | ||
Comment 32•23 years ago
|
||
Two patches - one from embedding/components and another from extensions/wallet.
Need r=.
Comment 33•23 years ago
|
||
Comment on attachment 68197 [details] [diff] [review]
nsISingleSignon renamed and relocated
sr=darin
Attachment #68197 -
Flags: superreview+
Comment 34•23 years ago
|
||
Comment on attachment 68197 [details] [diff] [review]
nsISingleSignon renamed and relocated
r=morse
Attachment #68197 -
Flags: review+
Assignee | ||
Comment 35•23 years ago
|
||
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.
Assignee | ||
Comment 36•23 years ago
|
||
Checked in. Thanks for quick reviews.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•