Judson Valeski Reporter
	Description • 23 years ago

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.

	Daniel Veditz [:dveditz] Assignee
	Comment 1 • 23 years ago

taking, nominating, setting target milestone

	Daniel Veditz [:dveditz] 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.

	Judson Valeski 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"?

	Daniel Veditz [:dveditz] 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.

	Syd Logan
	Comment 7 • 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;
 }
 

	Syd Logan
	Comment 8 • 23 years ago

Fix my suggestion and r=syd

	Daniel Veditz [:dveditz] Assignee
	Comment 11 • 23 years ago

Comment on attachment 75068 [details] [diff] [review]
review comments incorporated

argh, double submit

	Daniel Veditz [:dveditz] Assignee
	Comment 12 • 23 years ago

Comment on attachment 75069 [details] [diff] [review]
review comments incorporated

r=syd via IM

	Syd Logan
	Comment 13 • 23 years ago

nsbeta1- per adt traige (important only to embedding)

	Alec Flett
	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 :)

	Daniel Veditz [:dveditz] 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

	Asa Dotzler [:asa]
	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

	Daniel Veditz [:dveditz] Assignee
	Comment 17 • 23 years ago

Landed embeddable XPInstall

	Jimmy Lee
	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.