Closed Bug 385065 Opened 12 years ago Closed 12 years ago

protocol handling dialog

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9alpha8

People

(Reporter: dmose, Assigned: sdwilsh)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 11 obsolete files)

28.47 KB, image/png
beltzner
: review-
Details
29.32 KB, image/png
Details
35.20 KB, patch
mano
: review+
Details | Diff | Splinter Review
65.71 KB, patch
Details | Diff | Splinter Review
2.03 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
4.07 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
2.64 KB, patch
Biesinger
: review+
Biesinger
: superreview+
Details | Diff | Splinter Review
683 bytes, patch
Gavin
: review+
Details | Diff | Splinter Review
This bug is a first cut at the simplified content-handling dialog (bug 377782), initially just scoped to work with the external protocol handler.
Flags: blocking1.9?
Blocks: 380415
No longer blocks: 372441
Target Milestone: --- → mozilla1.9alpha6
Depends on: 386078
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Attached patch v0.1 (obsolete) — Splinter Review
It's crude and early still, but comments greatly appreciated.
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.
Attached patch v0.2 (obsolete) — Splinter Review
There are several XXX's in this patch that I'm not exactly sure how to address.  Feedback/input needed.
Attachment #270649 - Attachment is obsolete: true
Note that in populateList, "preferredApplicationHandler" is the correct spelling of that attribute.
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?
Depends on: 387930
Depends on: 388701
Depends on: 388388
Attached patch v1.0 non-toolkit (obsolete) — Splinter Review
Attachment #271566 - Attachment is obsolete: true
Attachment #273489 - Flags: superreview?(dmose)
Depends on: 389323
Depends on: 389446
Attached image Screenshot of first opening (obsolete) —
Attached image Screenshot of selected app and checkbox (obsolete) —
Attachment #273646 - Flags: review?(beltzner)
Attachment #273647 - Flags: review?(beltzner)
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.
Attachment #273646 - Flags: review?(beltzner) → review-
Comment on attachment 273647 [details]
Screenshot of selected app and checkbox

same comments as above
Attachment #273647 - Flags: review?(beltzner) → review-
It helps to grab the global stylesheet...
Attachment #273646 - Attachment is obsolete: true
Attachment #273673 - Flags: review?(beltzner)
Attached patch v1.0 toolkit changes (obsolete) — Splinter Review
short of any changes beltzner will have me make of course
Attachment #273675 - Flags: review?(mano)
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+
Attachment #273673 - Flags: review?(beltzner) → review-
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.
Attachment #273489 - Flags: superreview?(dmose) → superreview+
Attached patch v1.1 non-toolkit (obsolete) — Splinter Review
(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...
Attachment #273489 - Attachment is obsolete: true
Attachment #273710 - Flags: review?(cbiesinger)
(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 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.
Attachment #273489 - Flags: superreview+ → superreview-
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.
Attachment #273489 - Flags: superreview- → superreview+
Attached patch v1.2 non-toolkit (obsolete) — Splinter Review
Removed that change, added a comment about it being depreciated, and filed a bug for later removal.
Attachment #273710 - Attachment is obsolete: true
Attachment #273798 - Flags: review?(cbiesinger)
Attachment #273710 - Flags: review?(cbiesinger)
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*
Attachment #273675 - Flags: review?(mano) → review-
Attached patch v1.1 toolkit changes (obsolete) — Splinter Review
(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.
Attachment #273675 - Attachment is obsolete: true
Attachment #273869 - Flags: review?(mano)
Attachment #273647 - Attachment is obsolete: true
Attachment #273870 - Flags: review?(beltzner)
(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.
Attached patch v1.3 non-toolkit (obsolete) — Splinter Review
Attachment #273798 - Attachment is obsolete: true
Attachment #273874 - Flags: superreview?
Attachment #273874 - Flags: review?(cbiesinger)
Attachment #273798 - Flags: review?(cbiesinger)
Attachment #273874 - Flags: superreview? → superreview?(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
Attachment #273874 - Flags: superreview?(dmose) → superreview+
e.g. that the default impl requests nsIDOMWindow which it will use to parent the opened dialog.
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?
Attachment #273874 - Flags: review?(cbiesinger) → review+
Once you're done, please file a bug on Camino to implement this interface, so that it doesn't catch them by surprise
Attached patch v1.2 toolkit changes (obsolete) — Splinter Review
Addresses comments here and on irc.  Follow up bugs will come tomorrow.
Attachment #273869 - Attachment is obsolete: true
Attachment #273888 - Flags: review?(mano)
Attachment #273869 - Flags: review?(mano)
now with friendly packaging!
Attachment #273888 - Attachment is obsolete: true
Attachment #273891 - Flags: review?(mano)
Attachment #273888 - Flags: review?(mano)
Comment on attachment 273891 [details] [diff] [review]
v1.3 toolkit changes

let r=mano.

File a follow-up on the width issue please.
Attachment #273891 - Flags: review?(mano) → review+
Attached patch v1.4 non-toolkitSplinter Review
(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.
Attachment #273874 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Closed: 12 years ago
Priority: -- → P1
Resolution: --- → FIXED
Version: unspecified → Trunk
I neglected to address two comments.
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
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
Depends on: 388909
Yes, as a matter of fact, it was intentional.  Those are supposed to be gone from all platforms...
Removes the lines from the headers (not sure how this got missed in the first place...)
Attachment #273982 - Flags: superreview?(dmose)
Attachment #273982 - Flags: review?(cbiesinger)
Comment on attachment 273982 [details] [diff] [review]
v1.4.2 non-toolkit

sr=dmose
Attachment #273982 - Flags: superreview?(dmose) → superreview+
Blocks: 389687
Blocks: 389689
Attachment #273982 - Flags: review?(cbiesinger) → review+
I managed to backout patches for Bug 389580 and Bug 389106 :(

This should restore that code.
Attachment #274001 - Flags: superreview?(cbiesinger)
Attachment #274001 - Flags: review?(cbiesinger)
shawn / dmose:  please see bug #389705: tighten up the "Launch Application" dialog (uses too much vertical space)
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.
Attachment #274001 - Flags: superreview?(cbiesinger)
Attachment #274001 - Flags: superreview+
Attachment #274001 - Flags: review?(cbiesinger)
Attachment #274001 - Flags: review+
Depends on: 389732
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
No longer blocks: 389705
Depends on: 389705
No longer blocks: 389689
Depends on: 389689
Depends on: 389758
Depends on: 389766
Depends on: 389870
Depends on: 389891
Depends on: 389899
Attachment #273870 - Flags: review?(beltzner)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Hrm - yeah.  I think I started with a .in file, which is why I had that there...
Depends on: 390075
Removes the GARBAGE line since we don't want that
Attachment #274474 - Flags: review?(gavin.sharp)
Attachment #274474 - Flags: review?(gavin.sharp) → review+
Checking in toolkit/mozapps/handling/src/Makefile.in;
new revision: 1.2; previous revision: 1.1
Blocks: 390075
No longer depends on: 390075
Blocks: 389705
No longer depends on: 389705
Blocks: 389732
No longer depends on: 389732
Blocks: 389766
No longer depends on: 389766
Depends on: 391640
Blocks: 391640
No longer depends on: 391640
Flags: in-litmus?
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
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Flags: blocking1.9?
Depends on: CVE-2016-1937
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.