Closed
Bug 105085
Opened 23 years ago
Closed 23 years ago
XPI download/install progress dialogs need C++ abstraction.
Categories
(Core Graveyard :: Installer: XPInstall Engine, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0
People
(Reporter: jud, Assigned: dveditz)
References
Details
(Keywords: topembed+)
Attachments
(1 file, 3 obsolete files)
225.50 KB,
patch
|
dveditz
:
review+
alecf
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
Currently the XPI download and install progress dialogs are built in XUL. Embeddors need C++ callbacks to be fired in order to override these dialogs natively. See danm's model for this override here http://www.mozilla.org/xpfe/embedding-dialogs.html . This abstraction may require another bug to be broken out to track two dialogs. Currently the same XUL dialog is leveraged for both download and install progress. I'm not sure this consolidation makes sense using C++ notification/throw/display type callbacks. We should use already frozen interfaces for the status/progress callbacks. See nsIWebProgressListener which is widely used for this kind of notification.
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Updated•23 years ago
|
Keywords: mozilla1.0+
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.0
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 years ago
|
||
Assignee | ||
Comment 4•23 years ago
|
||
Since this is the largest part of the embedding XPI change I'll stick my patches here, but it does fix other bugs as well such as bug 105083. Didn't want to put them in the meta bug 105144 because this patch doesn't include the directory service changes dprice is working on. This is a large patch, I'd appreciate multiple reviews before seeking sr= and a=. Added two interfaces for embedding: nsIXPIDialogService and nsIXPIProgressDialog Got rid of the old nsIXPIProgressDlg and nsPIXPIManagerCallbacks private and non-embeddable interfaces. This needs testing with both the default XUL dialogs, and with an embedding client replacement. I've whipped up a quick and dirty test component in mozilla/xpinstall/test/ which you can just drop into the components directory and register. It shows a basic confirm dialog, and for progress dumps stuff to the console in debug mode. Delete the component to get the original XUL UI back.
Reporter | ||
Comment 5•23 years ago
|
||
Comment on attachment 72186 [details] [diff] [review] Patch for review (-w to hide all the trailing whitespace fixes) >RCS file: xpinstall/public/nsIXPIDialogService.idl <snip> >+%{C++ >+#define NS_XPIDIALOGSERVICE_CONTRACTID "@mozilla.org/embedui/xpinstall-dialog-service;1" >+ >+// {9A5BEF68-3FDA-4926-9809-87A5A1CC8505} >+#define NS_XPIDIALOGSERVICE_CID \ >+{0x9a5bef68, 0x3fda, 0x4926, {0x98, 0x09, 0x87, 0xa5, 0xa1, 0xcc, 0x85, 0x5}} >+ >+#define XPI_PROGRESS_TOPIC "xpinstall-progress" >+%} Generally, "%{C++"... in public idl files implies we're leaking some impl specific data out into the public arena. Exposing CID's reveals too much implementation detail. Please bury the CID in private details/includes. Even exposing the contract ID in the idl file isn't necessary. General convention on the observer topic (of which there's been oodles of debate in the newsgroups and bugs :-) ) is that it's an implicit contract, so, we haven't been putting them in the IDL file either. Basically "%C++"... has been washed out of public (or soon to be public) header files. BTW - nice usage of the nsI*P*... notation! Seemingly no-one actually uses it. Can you please add to the .idl files you are pushing into the public arena, "@status UNDER_REVIEW"?
Attachment #72186 -
Flags: needs-work+
Assignee | ||
Comment 6•23 years ago
|
||
Updated w/Jud's comments. Sean Su wrote: > * can probably remove the following line: > > nsSoftwareUpdate.h: #include "nsIDOMWindowInternal.h" Done > * In nsJSInstall.cpp: change the following: > {"refreshPlugins", InstallRefreshPlugins, 0}, > to > {"refreshPlugins", InstallRefreshPlugins, 1}, Done. > * Why is it no longer needed to call SetParentDOMWindow()? Because it's really a DOMWindowInternal which we may not be able to get in the embedded case. The dialogs were somewhat re-arranged and we don't really need it anymore anyways. > * Why is it no longer necessary to call > nsLoggingProgressListener::OnInstallDone() in: > > nsInstall::InternalAbort() > nsInstall::FinalizeInstall() > > where FinalStatus() used to be called from? Is it because > OnInstallDone() closes the log stream? I've combined two notifications into one and centralized its calling point. The embedded dialog needed the status, but the old way could result in an unknown multiple number of status results if there were multiple init/perform blocks in a script. Trying to explain or account for this made the dialogs difficult. > * You did not declare: > > NS_IMETHODIMP ConfirmInstall(nsIDOMWindow *aParent, const > PRUnichar **aPackageList, PRUint32 aCount, PRBool *aRetval); > > in mozilla/xpinstall/src/nsXPInstallManager.h. That's part of the nsIXPIDialogService interface.
Reporter | ||
Updated•23 years ago
|
+NS_IMETHODIMP nsXPInstallManager::Observe( nsISupports *aSubject, + const char *aTopic, + const PRUnichar *aData ) +{ Check aTopic for non-NULL. Also, what if topic is not one of the values you check for? It might be good to return some error indication to the embeddor to tell them they must pass in a recognized topic. Returning NS_OK could be misleading. + if ( nsDependentCString( aTopic ).Equals( XPI_PROGRESS_TOPIC ) ) + { + //------------------------------------------------------ + // Communication from the XPInstall Progress Dialog + //------------------------------------------------------ + + nsDependentString data( aData ); + + if ( data.Equals( NS_LITERAL_STRING("open") ) ) + { + // -- The dialog has been opened + if (mDialogOpen) + return NS_OK; // We've already been opened, nothing more to do + + mDialogOpen = PR_TRUE; + nsresult rv; + nsCOMPtr<nsIXPIProgressDialog> dlg = do_QueryInterface(aSubject, &rv); + if (dlg) + { + // --- create and save a proxy for the dialog + nsCOMPtr<nsIProxyObjectManager> pmgr = + do_GetService(kProxyObjectManagerCID, &rv); + if (pmgr) + { + rv = pmgr->GetProxyForObject( NS_UI_THREAD_EVENTQ, + NS_GET_IID(nsIXPIProgressDialog), + dlg, + PROXY_SYNC | PROXY_ALWAYS, + getter_AddRefs(mDlg) ); + } + } - mParentWindow = do_QueryInterface(aWindow, &rv); + // -- get the ball rolling DownloadNext(); + } - return rv; + else if ( data.Equals( NS_LITERAL_STRING("cancel") ) ) + { + // -- The dialog/user wants us to cancel the download + mCancelled = PR_TRUE; + if ( !mDialogOpen ) + { + // if we've never been opened then we can shutdown right here, + // otherwise we need to let mCancelled get discovered elsewhere + Shutdown(); + } + } + } + + return NS_OK; }
Assignee | ||
Comment 9•23 years ago
|
||
Assignee | ||
Comment 10•23 years ago
|
||
Attachment #72186 -
Attachment is obsolete: true
Attachment #72187 -
Attachment is obsolete: true
Assignee | ||
Comment 11•23 years ago
|
||
Comment on attachment 75068 [details] [diff] [review] review comments incorporated argh, double submit
Attachment #75068 -
Attachment is obsolete: true
Assignee | ||
Comment 12•23 years ago
|
||
Comment on attachment 75069 [details] [diff] [review] review comments incorporated r=syd via IM
Attachment #75069 -
Flags: review+
Comment 14•23 years ago
|
||
Comment on attachment 75069 [details] [diff] [review] review comments incorporated an excellent patch. sr=alecf next time, when there are so many whitespace diffs, attaching a 2nd diff -bw would be helpful :)
Attachment #75069 -
Flags: superreview+
Assignee | ||
Comment 15•23 years ago
|
||
Will do. Did for the first version, forgot the second time. The patch includes fixes for a number of related bugs as well: 76424,83702,88757,97691,98458,105083,113149,124467,124470
Comment 16•23 years ago
|
||
Comment on attachment 75069 [details] [diff] [review] review comments incorporated a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #75069 -
Flags: approval+
Assignee | ||
Comment 17•23 years ago
|
||
Landed embeddable XPInstall
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 18•23 years ago
|
||
Build: 2002-03-28-06-trunk(WIN), 2002-03-28-08-trunk(MAC), 2002-03-28-08-trunk(LINUX) Definitely checked in. Marking Verified.
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•