105085 - XPI download/install progress dialogs need C++ abstraction.

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Judson Valeski]

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.

Comment 1

[Daniel Veditz [:dveditz] out until Nov 10]

taking, nominating, setting target milestone

Comment 2

[Daniel Veditz [:dveditz] out until Nov 10]

Attachment: Patch for review (-w to hide all the trailing whitespace fixes)

Comment 3

[Daniel Veditz [:dveditz] out until Nov 10]

Attachment: Patch to apply for testing (includes whitespace-only changes)

Comment 4

[Daniel Veditz [:dveditz] out until Nov 10]

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.

Comment 5

[Judson Valeski]

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

Comment 6

[Daniel Veditz [:dveditz] out until Nov 10]

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.

Comment 7

[Syd Logan]

+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;
 }
 

Comment 8

[Syd Logan]

Fix my suggestion and r=syd

Comment 9

[Daniel Veditz [:dveditz] out until Nov 10]

Attachment: review comments incorporated

Comment 10

[Daniel Veditz [:dveditz] out until Nov 10]

Attachment: review comments incorporated

Comment 11

[Daniel Veditz [:dveditz] out until Nov 10]

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

argh, double submit

Comment 12

[Daniel Veditz [:dveditz] out until Nov 10]

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

r=syd via IM

Comment 13

[Syd Logan]

nsbeta1- per adt traige (important only to embedding)

Comment 14

[Alec Flett]

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

Comment 15

[Daniel Veditz [:dveditz] out until Nov 10]

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

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 17

[Daniel Veditz [:dveditz] out until Nov 10]

Landed embeddable XPInstall

Comment 18

[Jimmy Lee]

Build: 2002-03-28-06-trunk(WIN), 2002-03-28-08-trunk(MAC),
2002-03-28-08-trunk(LINUX)

Definitely checked in.  Marking Verified.