385065 - protocol handling dialog

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This bug is a first cut at the simplified content-handling dialog (bug 377782), initially just scoped to work with the external protocol handler.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

It's crude and early still, but comments greatly appreciated.

Comment 2

[cmtalbert]

Comment on attachment 270649 [details] [diff] [review]
v0.1


>Index: toolkit/mozapps/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/jar.mn,v
>retrieving revision 1.34
>diff -u -8 -p -r1.34 jar.mn
>--- toolkit/mozapps/jar.mn	14 Mar 2006 20:44:33 -0000	1.34
>+++ toolkit/mozapps/jar.mn	3 Jul 2007 00:22:32 -0000
>@@ -23,8 +23,12 @@ toolkit.jar:
> * content/mozapps/shared/richview.xml                           (shared/content/richview.xml)
>   content/mozapps/plugins/pluginInstallerWizard.xul             (plugins/content/pluginInstallerWizard.xul)
>   content/mozapps/plugins/pluginInstallerWizard.js              (plugins/content/pluginInstallerWizard.js)
>   content/mozapps/plugins/pluginInstallerWizard.css             (plugins/content/pluginInstallerWizard.css)
>   content/mozapps/plugins/pluginInstallerDatasource.js          (plugins/content/pluginInstallerDatasource.js)
>   content/mozapps/plugins/pluginInstallerService.js             (plugins/content/pluginInstallerService.js)
>   content/mozapps/plugins/missingPlugin.xml                     (plugins/content/missingPlugin.xml)
>   content/mozapps/plugins/missingPluginBinding.css              (plugins/content/missingPluginBinding.css)
>+  content/mozapps/handling/handling.css                         (handling/content/handling.css)
>+  content/mozapps/handling/handling.xml                         (handling/content/handling.xml)
These ^^ two lines need to refer to handler.css and handler.xml, respectively.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v0.2

There are several XXX's in this patch that I'm not exactly sure how to address.  Feedback/input needed.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Note that in populateList, "preferredApplicationHandler" is the correct spelling of that attribute.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

More random stuff I noticed:

* the id attribute on the bindings element in the XBL is spelled incorrectly

* in nsExternalHelperAppService::LoadURI there are two += that seem to have lots their =

* the control flow just before the chooser is created is wrong: if the pref lookup succeeded, and no warning is necessary, it simply returns rather than continuing the load

* I think before landing this, it'd be good to have discussion about the fact that the warning is going away.  Can you announce the change to m.d.platform, m.d.a.firefox, and m.d.security, and set Followups to m.d.platform?

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.0 non-toolkit

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: Screenshot of first opening

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: Screenshot of selected app and checkbox

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 273646 [details]
Screenshot of first opening

The following strings should be changed ..:

Dialog Title: "Launch Application"

Text: "This Web site would like to send information to an application./nSend to:"

Listbox options:
- remove the "OS Default" string, it's not needed
- s/"Choose an Application"/"Another application"

Checkbox label: "Remember my choice for this type of information."

Also, the styling is a little off; you're not using the default fonts for dialogs.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 273647 [details]
Screenshot of selected app and checkbox

same comments as above

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: Screenshot of first opening

It helps to grab the global stylesheet...

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: v1.0 toolkit changes

short of any changes beltzner will have me make of course

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 273673 [details]
Screenshot of first opening

I had several string changes in comment 9, two of which seemed to make it into this update ;)

Do the other two and you can assume ui-r+

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273489 [details] [diff] [review]
v1.0 non-toolkit

Looks good; just a few minor things!  sr=dmose with the following addressed:

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>
> [...]
>
>   // allowLoad is now true. See whether we have to ask the user
>   nsCAutoString warningPref(kExternalWarningPrefPrefix);
>   warningPref += scheme;
>   PRBool warn = PR_TRUE;
>   rv = prefs->GetBoolPref(warningPref.get(), &warn);
>   if (NS_FAILED(rv))
>   {
>     // no scheme-specific value, check the default
>     rv = prefs->GetBoolPref(kExternalWarningDefaultPref, &warn);
>   }

It used to be the case that this value of rv was checked for the case where we were missing the default warning pref and force a prompt.  That appears to no longer be true.  Should be easy to fix.

>Index: uriloader/exthandler/nsExternalProtocolHandler.cpp
> 
>     // get an nsIPrompt from the channel if we can
>-    nsCOMPtr<nsIPrompt> prompt;
>-    NS_QueryNotificationCallbacks(mCallbacks, mLoadGroup, prompt);
>-    rv = extProtService->LoadURI(mUrl, prompt);
>+    rv = extProtService->LoadURI(mUrl, nsnull);

The comment above the call to LoadURI has become obsolete.

>+interface nsIContentDispatchChooser : nsISupports {
>+ /**
>+  * This request is passed to the helper app dialog because Gecko can not
>+  * handle content of this type.
>+  */
>+  const unsigned long REASON_CANNOT_HANDLE = 0;
>+
>+ /**
>+  * The server requested external handling.
>+  */
>+  const unsigned long REASON_SERVER_REQUEST = 1;

As far as I know, the reason is a content-only thing (triggered by e.g. HTTP's Content-Disposition header) and will never be needed or used for protocol handlers.  If that's true, it's probably worth a comment in the IDL.

>Index: uriloader/exthandler/nsIExternalProtocolService.idl
>
>+   * @param aWindow The window to open the dialog in.
>    */
>-  void loadURI(in nsIURI aURI, in nsIPrompt aPrompt);
>+  void loadURI(in nsIURI aURI, [optional] in nsIDOMWindow aWindow);

This is really the window to which the dialog should be parented, so I'd suggest changing the variable name to aParentWindow or something like that and tweaking the doxygen stuff appropriately.

>Index: uriloader/exthandler/nsMIMEInfoImpl.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v
>retrieving revision 1.56.4.9
>diff -u -8 -p -r1.56.4.9 nsMIMEInfoImpl.cpp
>--- uriloader/exthandler/nsMIMEInfoImpl.cpp	23 Jul 2007 21:52:25 -0000	1.56.4.9
>+++ uriloader/exthandler/nsMIMEInfoImpl.cpp	23 Jul 2007 23:35:19 -0000
>@@ -361,17 +361,18 @@ nsMIMEInfoBase::LaunchWithURI(nsIURI* aU
> 
>     rv = GetLocalFileFromURI(aURI, getter_AddRefs(docToLoad));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     return LaunchWithIProcess(executable, docToLoad);
>   }
>   else if (mPreferredAction == useSystemDefault) {
>     rv = GetLocalFileFromURI(aURI, getter_AddRefs(docToLoad));
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    if (NS_FAILED(rv))
>+        return LoadUriInternal(aURI);

A comment explaining the above call would be helpful here.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v1.1 non-toolkit

(In reply to comment #13)
Oops, can't believe I missed that.  Fixed locally except for the description since you didn't get back with me about that (regarding the string if they type it in the location bar and not that they clicked a link).

(In reply to comment #14)
> It used to be the case that this value of rv was checked for the case where we
> were missing the default warning pref and force a prompt.  That appears to no
> longer be true.  Should be easy to fix.
True, but if it fails, we'll still warn like before.  I removed the assignment since we no longer care about the return value.

> The comment above the call to LoadURI has become obsolete.
Oops. Fixed.

> As far as I know, the reason is a content-only thing (triggered by e.g. HTTP's
> Content-Disposition header) and will never be needed or used for protocol
> handlers.  If that's true, it's probably worth a comment in the IDL.
So, I think this was originally designed with content handlers in mind as well, but I think we should drop those constants until we are ready to hook them up.  If you disagree, let me know.

> This is really the window to which the dialog should be parented, so I'd
> suggest changing the variable name to aParentWindow or something like that and
> tweaking the doxygen stuff appropriately.
Fixed.

> A comment explaining the above call would be helpful here.
Done

Also neglected to include the change to docshell in the last version...

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #15)
> True, but if it fails, we'll still warn like before.  I removed the assignment
> since we no longer care about the return value.

Sounds good.
 
> So, I think this was originally designed with content handlers in mind as well,
> but I think we should drop those constants until we are ready to hook them up. 
> If you disagree, let me know.

Seems like a good plan; less confusing.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273489 [details] [diff] [review]
v1.0 non-toolkit

Something tickled the back of my brain to go poke around in lxr a bit more, and it turns out there are a bunch of JS callers of nsIExternalProtocolService.loadUrl.  Rather than trying to chase these down now and risk regressions; I'd suggest putting loadUrl back and spin its demise off to another bug.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273489 [details] [diff] [review]
v1.0 non-toolkit

I shouldn't have set this to sr-, since there's no real need for me to re-review the fix I suggested.  sr+ with that issue fixed so that landing doesn't burninate the tree.

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: v1.2 non-toolkit

Removed that change, added a comment about it being depreciated, and filed a bug for later removal.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 273675 [details] [diff] [review]
v1.0 toolkit changes

>Index: toolkit/mozapps/handling/content/dialog.js
>===================================================================
...
>+ * window.arguments[7]:
>+ *   This is the URI that we are being brought up for in the first place.

uri or uri spec?


>+const STRING_BUNDLE_URI =
>+  "chrome://mozapps/content/handling/handling.properties";
>+

any reason not to use a <stringbundle/> element?

>+////////////////////////////////////////////////////////////////////////////////
>+//// Initialization/Destruction
>+
>+window.addEventListener("load", ProtocolHandlerDialog_initialize, false);

Could just do <dialog onload=""> here.

>+
>+function ProtocolHandlerDialog_initialize()
>+{
>+  dialog = new ProtocolHandlerDialog();
>+  dialog.initialize();
>+}
>+
>+////////////////////////////////////////////////////////////////////////////////
>+//// ProtocolHandlerDialog class
>+
>+function ProtocolHandlerDialog()
>+{
>+  this.mHandlerInfo = window.arguments[6].QueryInterface(Ci.nsIHandlerInfo);
>+  this.mURI         = window.arguments[7].QueryInterface(Ci.nsIURI);
>+  this.mItemChoose  = document.getElementById("item-choose");
>+}
>+
>+ProtocolHandlerDialog.prototype =

Since you don't need multiple instances of this object, you can simlify this to

var gProtocolHandlerDialog = {

>+  //////////////////////////////////////////////////////////////////////////////
>+  //// Member Variables
>+
>+  mHandlerInfo: null,
>+  mItemChoose: null,
>+  mURI: null,
>+

nit: toolkit & browser convenstion is a underscore prefix for private variables in js objects.

>+    checkbox.text.textContent    = window.arguments[5];
>+
>+    // Hide stuff that needs to be hidden
>+    if ("" == checkbox.desc.label)

this is the same as if (checkbox.desc.label)

>+
>+    if (this.mHandlerInfo.hasDefaultHandler) {
>+      let elm = document.createElement("handler");

Please make this use <richlistitem type="handler">, then you get all richlistitem style rules applied as well.

>+      elm.setAttribute("id", "os-default-handler");

nit: elm.id

>+      elm.setAttribute("name", this.mHandlerInfo.defaultDescription);
>+    
>+      document.getElementById("items").insertBefore(elm, this.mItemChoose);
>+    }
>+  },
>+  
>+ /**
>+  * Brings up a filepicker and allows a user to choose an application.
>+  */
>+  chooseApplication: function chooseApplication()
>+  {
>+    let sbs = Cc["@mozilla.org/intl/stringbundle;1"].
>+              getService(Ci.nsIStringBundleService);
>+    let bundle = sbs.createBundle(STRING_BUNDLE_URI);

prefer a <stringbundle/> element. 

>+    let title = bundle.GetStringFromName("choose.application.title");
>+

nit: trailing spaces.


>+
>+ /**
>+  * Function called when the OK button is pressed.
>+  */
>+  onAccept: function onAccept()
>+  {
>+    let checkbox = document.getElementById("remember");
>+    if (!checkbox.hidden) {
>+      // We need to make sure that the default is properly set now
>+      if (this.selectedItem.obj) {
>+        // default OS handler doesn't have this property
>+        this.mHandlerInfo.preferredAction = Ci.nsIHandlerInfo.useHelperApp;
>+        this.mHandlerInfo.preferredApplicationHandler = this.selectedItem.obj;
>+      } else {

please avoid } else { code style.

>+        this.mHandlerInfo.preferredAction = Ci.nsIHandlerInfo.useSystemDefault;
>+      }

and braces around single-line blocks.

>Index: toolkit/mozapps/handling/content/dialog.xul
>===================================================================

>+<?xul-overlay href="chrome://global/content/globalOverlay.xul"?>

what's this for?

>+  <script src="chrome://mozapps/content/handling/dialog.js" type="text/javascript"/>

nit: application/javascript

>+  <hbox>
>+    <image id="description-image"/>
>+    <description id="description-text"/>
>+  </hbox>
>+
>+  <vbox flex="1">
>+    <description id="item-action-text"/>
>+    <richlistbox id="items" flex="1"
>+                 onselect="if (dialog) dialog.updateOKButton();">
>+      <richlistitem id="item-choose" orient="horizontal">
>+        <description value="&ChooseApp.description;"/>

make this a <label> so <richlistitem>.label could expose it to screen readers.

>+        <spacer flex="1"/>

couldn't you set the flex on the label element?

>+        <button oncommand="if (dialog) dialog.chooseApplication();"
>+                label="&ChooseApp.label;"/>

missing accesskey.

>Index: toolkit/mozapps/handling/content/handler.xml
>===================================================================
>+
>+<bindings id="hanlder-bindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+          xmlns:xbl="http://www.mozilla.org/xbl">
>+
>+  <binding id="handler"
>+           extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+
>+    <content>
>+      <xul:hbox flex="1">

the hbox wouldn't be necessary once you use <richlistitem type="handler">, i think.

>+        <xul:vbox pack="center">
>+          <xul:image xbl:inherits="src=image" height="32" width="32"/>

hrm, you seem to set the dimensions both here and in the style sheets.

>+        </xul:vbox>
>+        <xul:vbox flex="1">
>+          <xul:description xbl:inherits="value=name"/>
>+          <xul:description xbl:inherits="value=description"/>

These should be <label/> elements.

>Index: toolkit/mozapps/handling/content/handling.dtd
>===================================================================
>+<!ENTITY window.width "320">

unused.

>Index: toolkit/mozapps/handling/content/handling.properties
>===================================================================
>+protocol.choices.label=What would you like to do with this?
>+protocol.checkbox.label=Remember my choice for this type of information.

missing accesskey.

>+protocol.checkbox.extra=This can be changed in %S's preferences.

browser/ dependency? ;)

>Index: toolkit/mozapps/handling/src/nsContentDispatchChooser.js
>===================================================================

>+    let ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>+             getService(Ci.nsIWindowWatcher);
>+    ww.openWindow(window,
>+                  CONTENT_HANDLING_URL,
>+                  null,
>+                  "chrome,dialog=yes,resizable,centerscreen",

hrm, shouldn't it be modal?


>Index: toolkit/themes/*stripe/mozapps/handling/handling.css
>===================================================================

>+dialog {
>+  width: 320px;
>+}
>+

what's this for?

>+#description-image:not([src]) {
>+  height: 32px;
>+  width: 32px;
>+}
>+
>+richlistitem, handler {
>+  height: 36px; /* Don't forget to update the richlistbox height! */
>+  padding: 1px 2px 1px 2px;
>+}
>+
>+richlistitem[selected="true"], handler[selected="true"] {
>+  background-color: Highlight;
>+  color: HighlightText;
>+}
>+

richlistbox.css applies a less buggy version of these rules for you once you do type="handler".

>+richlistbox {
>+  /* 3 items high, pluse 4px for top and bottom margins, less 2px for border */

plus*

Comment 21

[Shawn Wilsher :sdwilsh]

Attachment: v1.1 toolkit changes

(In reply to comment #20)
> uri or uri spec?
nsIURI - updated comment

> any reason not to use a <stringbundle/> element?
No, not really - using it now.

> Could just do <dialog onload=""> here.
Done.

> Since you don't need multiple instances of this object, you can simlify this to ...
actually, that broke things in ways I could not understand (suddenly getting errors with the dialog's xbl).  Since we are trying to get this into M7, I put it back the way things were.

> Please make this use <richlistitem type="handler">, then you get all
> richlistitem style rules applied as well.
Done.

> >+<?xul-overlay href="chrome://global/content/globalOverlay.xul"?>
> what's this for?
um, not sure - just kinda always tossed it in there, but it looks like I don't need it...

> make this a <label> so <richlistitem>.label could expose it to screen readers.
Done.

> couldn't you set the flex on the label element?
Yes - fixed.

> missing accesskey.
Oh neat!  Oops!

> the hbox wouldn't be necessary once you use <richlistitem type="handler">, i
> think.
That is correct

> hrm, you seem to set the dimensions both here and in the style sheets.
No, that is the image for the whole dialog, which we currently don't have anything for (in the CSS)

> >+protocol.checkbox.extra=This can be changed in %S's preferences.
> browser/ dependency? ;)
Is it?  Do we have apps that do not have preferences?  Beltzner - alternate strings?

> hrm, shouldn't it be modal?
No, it's supposed to be just like the download box, which is also not modal.

> >+dialog {
> >+  width: 320px;
> >+}
> >+
> what's this for?
Keeping the window at a sane width - otherwise it just expands to the width of the longest string (eww).

> >+richlistitem[selected="true"], handler[selected="true"] {
> >+  background-color: Highlight;
> >+  color: HighlightText;
> >+}
> >+
> richlistbox.css applies a less buggy version of these rules for you once you do
> type="handler".
Except that is uses the -moz-dialog color for some strange reason, and it looks really bad.

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: Screenshot of selected app and checkbox

Comment 23

[Asaf Romano (gone)]

(In reply to comment #21)

> 
> > Could just do <dialog onload=""> here.
> Done.
> 
> > Since you don't need multiple instances of this object, you can simlify this to ...
> actually, that broke things in ways I could not understand (suddenly getting
> errors with the dialog's xbl).  Since we are trying to get this into M7, I put
> it back the way things were.
> 

what errors?

> > Please make this use <richlistitem type="handler">, then you get all
> > richlistitem style rules applied as well.
> Done.
> 
> > >+<?xul-overlay href="chrome://global/content/globalOverlay.xul"?>
> > what's this for?
> um, not sure - just kinda always tossed it in there, but it looks like I don't
> need it...
>

ew, anything that's in tree? This file is obsolete.
 



> > hrm, you seem to set the dimensions both here and in the style sheets.
> No, that is the image for the whole dialog, which we currently don't have
> anything for (in the CSS)

OK.
 
> > >+protocol.checkbox.extra=This can be changed in %S's preferences.
> > browser/ dependency? ;)
> Is it?  Do we have apps that do not have preferences?  Beltzner - alternate
> strings?
>

Clearly this was just to annoy you!
 
> > >+dialog {
> > >+  width: 320px;
> > >+}
> > >+
> > what's this for?
> Keeping the window at a sane width - otherwise it just expands to the width of
> the longest string (eww).
>

Yes, but it should be set in dialog.xml in a locale-dependent maaner.

 
> > >+richlistitem[selected="true"], handler[selected="true"] {
> > >+  background-color: Highlight;
> > >+  color: HighlightText;
> > >+}
> > >+
> > richlistbox.css applies a less buggy version of these rules for you once you do
> > type="handler".
> Except that is uses the -moz-dialog color for some strange reason, and it looks
> really bad.
> 
When the richlistbox is focused, it uses the highlight colors, otherwise -moz-Dialog. This is the correct behavior on mac and windows AFAICT.

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: v1.3 non-toolkit

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273874 [details] [diff] [review]
v1.3 non-toolkit

>+ /**
>+  * Asks the user what to do with the content.
>+  *
>+  * @param aHander
>+  *        The handler for this piece of content.
>+  * @param aWindowContext
>+  *        The parent context to show this (can be null, some implementations
>+  *        may not care about it).

The naming seems odd.  Maybe just aCallbacks, and with more commentary about how it's actually used.  e.g. that the default impl 

sr=dmose with that change

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

e.g. that the default impl requests nsIDOMWindow which it will use to parent the opened dialog.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 273874 [details] [diff] [review]
v1.3 non-toolkit

nsExtProtocolChannel::OpenURL() should pass a context; you can use mCallbacks

uriloader/exthandler/nsIExternalProtocolService.idl  
   * @depreciated Use LoadURI instead (See Bug 389565 for removal)

The word here is deprecated; depreciated means something else (and is not a
valid doxygen command)


uriloader/exthandler/nsMIMEInfoImpl.cpp  
     rv = GetLocalFileFromURI(aURI, getter_AddRefs(docToLoad));
-    NS_ENSURE_SUCCESS(rv, rv);
+    if (NS_FAILED(rv)) {
+      // There is no local file for this uri, so we need to load it internally
+      return LoadUriInternal(aURI);
+    }

Hm... I'm not really sure that this is correct. Shouldn't this be more like, if
(mClass == eProtocolInfo) { return LoadURInternal(); } else {
GetLocalFileFromURI(); return LaunchDefaultWithFile(); } ?

Similar for Mac. Mac also has a (void) cast. Why did you add a
GetHasDefaultHandler?

nsMIMEInfoUnix.h contains a class called nsMIMEInfoWin


I would prefer if you protocol prefs in its own patch, instead of doing it in a
stealthy way as part of this bug.




Why did you implement LoadUriInternal on nsMIMEInfoImpl instead of just leaving
it abstract?

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

Once you're done, please file a bug on Camino to implement this interface, so that it doesn't catch them by surprise

Comment 29

[Shawn Wilsher :sdwilsh]

Attachment: v1.2 toolkit changes

Addresses comments here and on irc.  Follow up bugs will come tomorrow.

Comment 30

[Shawn Wilsher :sdwilsh]

Attachment: v1.3 toolkit changes

now with friendly packaging!

Comment 31

[Asaf Romano (gone)]

Comment on attachment 273891 [details] [diff] [review]
v1.3 toolkit changes

let r=mano.

File a follow-up on the width issue please.

Comment 32

[Shawn Wilsher :sdwilsh]

Attachment: v1.4 non-toolkit

(In reply to comment #27)
> Hm... I'm not really sure that this is correct. Shouldn't this be more like
Yes, did that.  This isn't correct though when we have web handlers so that will have to be fixed then.

> Similar for Mac. Mac also has a (void) cast. Why did you add a
> GetHasDefaultHandler?
It fixes a bug on mac where it says it doesn't have a default handler when it really did.

> nsMIMEInfoUnix.h contains a class called nsMIMEInfoWin
WOW - oops.  Fixed.

> I would prefer if you protocol prefs in its own patch, instead of doing it in a
> stealthy way as part of this bug.
I'll make a newsgroup post tomorrow as we discussed (it's on my whiteboard of things to do)

> Why did you implement LoadUriInternal on nsMIMEInfoImpl instead of just leaving
> it abstract?
That was baggage from before Bug 388701 was fixed.  It's pure virtual now.

Comment 33

[Shawn Wilsher :sdwilsh]

Any remaining issues get to go into follow-up bugs - please.

Checking in toolkit/mozapps/Makefile.in;
new revision: 1.14; previous revision: 1.13
Checking in toolkit/mozapps/jar.mn;
new revision: 1.35; previous revision: 1.34
Checking in toolkit/mozapps/handling/Makefile.in;
initial revision: 1.1
Checking in toolkit/mozapps/handling/content/dialog.js;
initial revision: 1.1
Checking in toolkit/mozapps/handling/content/dialog.xul;
initial revision: 1.1
Checking in toolkit/mozapps/handling/content/handler.css;
initial revision: 1.1
Checking in toolkit/mozapps/handling/content/handler.xml;
initial revision: 1.1
Checking in toolkit/mozapps/handling/content/handling.dtd;
initial revision: 1.1
Checking in toolkit/mozapps/handling/content/handling.properties;
initial revision: 1.1
Checking in toolkit/mozapps/handling/src/Makefile.in;
initial revision: 1.1
Checking in toolkit/mozapps/handling/src/nsContentDispatchChooser.js;
initial revision: 1.1
Checking in toolkit/themes/pinstripe/mozapps/jar.mn;
new revision: 1.20; previous revision: 1.19
Checking in toolkit/themes/pinstripe/mozapps/handling/handling.css;
initial revision: 1.1
Checking in toolkit/themes/winstripe/mozapps/jar.mn;
new revision: 1.18; previous revision: 1.17
Checking in toolkit/themes/winstripe/mozapps/handling/handling.css;
initial revision: 1.1
Checking in browser/installer/unix/packages-static;
new revision: 1.122; previous revision: 1.121
Checking in browser/installer/windows/packages-static;
new revision: 1.133; previous revision: 1.132
Checking in uriloader/exthandler/Makefile.in;
new revision: 1.67; previous revision: 1.66
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.327; previous revision: 1.326
Checking in uriloader/exthandler/nsExternalHelperAppService.h;
new revision: 1.84; previous revision: 1.83
Checking in uriloader/exthandler/nsExternalProtocolHandler.cpp;
new revision: 1.42; previous revision: 1.41
Checking in uriloader/exthandler/nsIContentDispatchChooser.idl;
initial revision: 1.1
Checking in uriloader/exthandler/nsIExternalProtocolService.idl;
new revision: 1.11; previous revision: 1.10
Checking in uriloader/exthandler/nsMIMEInfoImpl.cpp;
new revision: 1.62; previous revision: 1.61
Checking in uriloader/exthandler/nsMIMEInfoImpl.h;
new revision: 1.33; previous revision: 1.32
Checking in uriloader/exthandler/beos/nsMIMEInfoBeOS.cpp;
new revision: 1.3; previous revision: 1.2
Checking in uriloader/exthandler/beos/nsMIMEInfoBeOS.h;
new revision: 1.7; previous revision: 1.6
Checking in uriloader/exthandler/beos/nsOSHelperAppService.cpp;
new revision: 1.22; previous revision: 1.21
Checking in uriloader/exthandler/mac/nsMIMEInfoMac.cpp;
new revision: 1.8; previous revision: 1.7
Checking in uriloader/exthandler/mac/nsMIMEInfoMac.h;
new revision: 1.8; previous revision: 1.7
Checking in uriloader/exthandler/mac/nsOSHelperAppService.cpp;
new revision: 1.52; previous revision: 1.51
Checking in uriloader/exthandler/os2/nsMIMEInfoOS2.cpp;
new revision: 1.7; previous revision: 1.6
Checking in uriloader/exthandler/os2/nsMIMEInfoOS2.h;
new revision: 1.8; previous revision: 1.7
Checking in uriloader/exthandler/os2/nsOSHelperAppService.cpp;
new revision: 1.44; previous revision: 1.43
Checking in uriloader/exthandler/os2/nsOSHelperAppService.h;
new revision: 1.19; previous revision: 1.18
Checking in uriloader/exthandler/unix/nsMIMEInfoUnix.cpp;
initial revision: 1.1
Checking in uriloader/exthandler/unix/nsMIMEInfoUnix.h;
initial revision: 1.1
Checking in uriloader/exthandler/unix/nsOSHelperAppService.cpp;
new revision: 1.67; previous revision: 1.66
Checking in uriloader/exthandler/win/nsMIMEInfoWin.cpp;
new revision: 1.6; previous revision: 1.5
Checking in uriloader/exthandler/win/nsMIMEInfoWin.h;
new revision: 1.9; previous revision: 1.8
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
new revision: 1.72; previous revision: 1.71
Checking in mail/installer/windows/packages-static;
new revision: 1.67; previous revision: 1.66
Checking in suite/installer/windows/packages;
new revision: 1.23; previous revision: 1.22

Comment 34

[Shawn Wilsher :sdwilsh]

Attachment: v1.4.1 non-toolkit

I neglected to address two comments.

Comment 35

[Shawn Wilsher :sdwilsh]

Sorry about that.

Checking in uriloader/exthandler/nsExternalProtocolHandler.cpp;
new revision: 1.43; previous revision: 1.42
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.328; previous revision: 1.327

Comment 36

[Walter Meinl]

Shawn, was the change for
new revision: 1.44; previous revision: 1.43
Checking in uriloader/exthandler/os2/nsOSHelperAppService.h;
-  nsresult LoadUriInternal(nsIURI * aURL);
intentional? Asking, because this line is still present in the nsOSHelperAppService.h files from win, unix and beos

Comment 37

[Shawn Wilsher :sdwilsh]

Yes, as a matter of fact, it was intentional.  Those are supposed to be gone from all platforms...

Comment 38

[Shawn Wilsher :sdwilsh]

Attachment: v1.4.2 non-toolkit

Removes the lines from the headers (not sure how this got missed in the first place...)

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273982 [details] [diff] [review]
v1.4.2 non-toolkit

sr=dmose

Comment 40

[Shawn Wilsher :sdwilsh]

Attachment: v1.4.3 non-toolkit

I managed to backout patches for Bug 389580 and Bug 389106 :(

This should restore that code.

Comment 41

[(not reading, please use [e-mail] instead)]

shawn / dmose:  please see bug #389705: tighten up the "Launch Application" dialog (uses too much vertical space)

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 274001 [details] [diff] [review]
v1.4.3 non-toolkit

oh, sorry, what was checked in didn't include the apostrophe (per dveditz's review comment), so please remove that.

Comment 43

[Shawn Wilsher :sdwilsh]

Checking in uriloader/exthandler/beos/nsOSHelperAppService.h;
new revision: 1.13; previous revision: 1.12
Checking in uriloader/exthandler/mac/nsOSHelperAppService.h;
new revision: 1.18; previous revision: 1.17
Checking in uriloader/exthandler/unix/nsOSHelperAppService.h;
new revision: 1.23; previous revision: 1.22
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.330; previous revision: 1.329

Comment 44

[Peter Weilbacher]

Shawn, could you please clearly indicate which of your patches really got checked in and when? Your comments with cvs ci logs are pretty useless if you don't say which attachment this was about. Investigating the OS/2 build breaks that many of your patches bring would be easier with such info. CCing me before you check in OS/2 stuff might also help...

Something is again broken, supposedly by checkin(s) from this bug. The LoadUriInternal that you made up from bits copied from elsewhere doesn't work.

Comment 45

[Shawn Wilsher :sdwilsh]

(In reply to comment #44)
All non-obsolete patches got checked in.  Since OS/2 is not a tier one platform, please don't reopen bugs - file a new bug and set it blocking this bug.

Comment 46

[Ginn Chen]

In toolkit/mozapps/handling/src/Makefile.in
We don't have nsContentDispatchChooser.js.in
So we don't want to GARBAGE += nsContentDispatchChooser.js , right?

otherwise, we'll have problem with make clobber.

Comment 47

[Shawn Wilsher :sdwilsh]

Hrm - yeah.  I think I started with a .in file, which is why I had that there...

Comment 48

[Shawn Wilsher :sdwilsh]

Attachment: v1.3.1 toolkit changes

Removes the GARBAGE line since we don't want that

Comment 49

[Shawn Wilsher :sdwilsh]

Checking in toolkit/mozapps/handling/src/Makefile.in;
new revision: 1.2; previous revision: 1.1

Comment 51

[cmtalbert]

This dialog is covered in litmus:
https://litmus.mozilla.org/show_test.cgi?searchType=by_category&product_id=1&branch_id=15&testgroup_id=55&subgroup_id=884
https://litmus.mozilla.org/show_test.cgi?searchType=by_category&product_id=1&branch_id=15&testgroup_id=56&subgroup_id=820

Verifying on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre