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

VERIFIED FIXED in mozilla1.0

Status

Core Graveyard
Installer: XPInstall Engine
VERIFIED FIXED
17 years ago
3 years ago

People

(Reporter: Judson Valeski, Assigned: dveditz)

Tracking

({topembed+})

Trunk
mozilla1.0
x86
Windows 2000
topembed+
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

17 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.
(Reporter)

Updated

17 years ago
Blocks: 65233
(Reporter)

Updated

17 years ago
Blocks: 105144

Updated

17 years ago
Target Milestone: --- → M1

Updated

17 years ago
Target Milestone: M1 → Future
(Assignee)

Comment 1

17 years ago
taking, nominating, setting target milestone
Assignee: syd → dveditz
Keywords: nsbeta1
Target Milestone: Future → mozilla0.9.8
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Updated

17 years ago
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

17 years ago
Blocks: 83702
(Assignee)

Updated

17 years ago
Depends on: 124467
(Assignee)

Updated

17 years ago
Depends on: 124470
(Reporter)

Updated

17 years ago
Keywords: topembed

Updated

17 years ago
Keywords: mozilla1.0+
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0
(Assignee)

Comment 2

17 years ago
Created attachment 72186 [details] [diff] [review]
Patch for review (-w to hide all the trailing whitespace fixes)
(Assignee)

Comment 3

17 years ago
Created attachment 72187 [details] [diff] [review]
Patch to apply for testing (includes whitespace-only changes)
(Assignee)

Comment 4

17 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

17 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

17 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

17 years ago
Keywords: topembed → topembed+

Comment 7

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

Comment 8

17 years ago
Fix my suggestion and r=syd
(Assignee)

Comment 9

17 years ago
Created attachment 75068 [details] [diff] [review]
review comments incorporated
(Assignee)

Comment 10

17 years ago
Created attachment 75069 [details] [diff] [review]
review comments incorporated
Attachment #72186 - Attachment is obsolete: true
Attachment #72187 - Attachment is obsolete: true
(Assignee)

Comment 11

17 years ago
Comment on attachment 75068 [details] [diff] [review]
review comments incorporated

argh, double submit
Attachment #75068 - Attachment is obsolete: true
(Assignee)

Comment 12

17 years ago
Comment on attachment 75069 [details] [diff] [review]
review comments incorporated

r=syd via IM
Attachment #75069 - Flags: review+

Comment 13

17 years ago
nsbeta1- per adt traige (important only to embedding)
Keywords: nsbeta1+ → nsbeta1-

Comment 14

17 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

17 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

17 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

17 years ago
Landed embeddable XPInstall
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 18

17 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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.