protocol handling dialog

VERIFIED FIXED in mozilla1.9alpha8

Status

Core Graveyard
File Handling
P1
normal
VERIFIED FIXED
10 years ago
a year ago

People

(Reporter: dmose, Assigned: sdwilsh)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9alpha8
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 11 obsolete attachments)

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

Description

10 years ago
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?
(Reporter)

Updated

10 years ago
Blocks: 380415
No longer blocks: 372441
(Reporter)

Updated

10 years ago
Target Milestone: --- → mozilla1.9alpha6
(Assignee)

Updated

10 years ago
Depends on: 386078
(Assignee)

Updated

10 years ago
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
(Assignee)

Comment 1

10 years ago
Created attachment 270649 [details] [diff] [review]
v0.1

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

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
Created attachment 271566 [details] [diff] [review]
v0.2

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

Comment 4

10 years ago
Note that in populateList, "preferredApplicationHandler" is the correct spelling of that attribute.
(Reporter)

Comment 5

10 years ago
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?
(Assignee)

Updated

10 years ago
Depends on: 387930
(Assignee)

Updated

10 years ago
Depends on: 388701
(Assignee)

Updated

10 years ago
Depends on: 388388
(Assignee)

Comment 6

10 years ago
Created attachment 273489 [details] [diff] [review]
v1.0 non-toolkit
Attachment #271566 - Attachment is obsolete: true
Attachment #273489 - Flags: superreview?(dmose)
Depends on: 389323
(Reporter)

Updated

10 years ago
Depends on: 389446
(Assignee)

Comment 7

10 years ago
Created attachment 273646 [details]
Screenshot of first opening
(Assignee)

Comment 8

10 years ago
Created attachment 273647 [details]
Screenshot of selected app and checkbox
(Assignee)

Updated

10 years ago
Attachment #273646 - Flags: review?(beltzner)
(Assignee)

Updated

10 years ago
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-
(Assignee)

Comment 11

10 years ago
Created attachment 273673 [details]
Screenshot of first opening

It helps to grab the global stylesheet...
Attachment #273646 - Attachment is obsolete: true
Attachment #273673 - Flags: review?(beltzner)
(Assignee)

Comment 12

10 years ago
Created attachment 273675 [details] [diff] [review]
v1.0 toolkit changes

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

Comment 14

10 years ago
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+
(Assignee)

Comment 15

10 years ago
Created attachment 273710 [details] [diff] [review]
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...
Attachment #273489 - Attachment is obsolete: true
Attachment #273710 - Flags: review?(cbiesinger)
(Reporter)

Comment 16

10 years ago
(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.
(Reporter)

Comment 17

10 years ago
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-
(Reporter)

Comment 18

10 years ago
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+
(Assignee)

Comment 19

10 years ago
Created attachment 273798 [details] [diff] [review]
v1.2 non-toolkit

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-
(Assignee)

Comment 21

10 years ago
Created attachment 273869 [details] [diff] [review]
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.
Attachment #273675 - Attachment is obsolete: true
Attachment #273869 - Flags: review?(mano)
(Assignee)

Comment 22

10 years ago
Created attachment 273870 [details]
Screenshot of selected app and checkbox
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.
(Assignee)

Comment 24

10 years ago
Created attachment 273874 [details] [diff] [review]
v1.3 non-toolkit
Attachment #273798 - Attachment is obsolete: true
Attachment #273874 - Flags: superreview?
Attachment #273874 - Flags: review?(cbiesinger)
Attachment #273798 - Flags: review?(cbiesinger)
(Assignee)

Updated

10 years ago
Attachment #273874 - Flags: superreview? → superreview?(dmose)
(Reporter)

Comment 25

10 years ago
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+
(Reporter)

Comment 26

10 years ago
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
(Assignee)

Comment 29

10 years ago
Created attachment 273888 [details] [diff] [review]
v1.2 toolkit changes

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)
(Assignee)

Comment 30

10 years ago
Created attachment 273891 [details] [diff] [review]
v1.3 toolkit changes

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+
(Assignee)

Comment 32

10 years ago
Created attachment 273903 [details] [diff] [review]
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.
Attachment #273874 - Attachment is obsolete: true
(Assignee)

Comment 33

10 years ago
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
Last Resolved: 10 years ago
Priority: -- → P1
Resolution: --- → FIXED
Version: unspecified → Trunk
(Assignee)

Comment 34

10 years ago
Created attachment 273915 [details] [diff] [review]
v1.4.1 non-toolkit

I neglected to address two comments.
Attachment #273915 - Flags: review+
(Assignee)

Comment 35

10 years ago
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

10 years ago
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

Updated

10 years ago
Depends on: 388909
(Assignee)

Comment 37

10 years ago
Yes, as a matter of fact, it was intentional.  Those are supposed to be gone from all platforms...
(Assignee)

Comment 38

10 years ago
Created attachment 273982 [details] [diff] [review]
v1.4.2 non-toolkit

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

Comment 39

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

sr=dmose
Attachment #273982 - Flags: superreview?(dmose) → superreview+
(Assignee)

Updated

10 years ago
Blocks: 389687
(Assignee)

Updated

10 years ago
Blocks: 389689
Attachment #273982 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 40

10 years ago
Created attachment 274001 [details] [diff] [review]
v1.4.3 non-toolkit

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)
Blocks: 389705
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+
(Reporter)

Updated

10 years ago
Depends on: 389732
(Assignee)

Comment 43

10 years ago
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
(Reporter)

Updated

10 years ago
No longer blocks: 389705
Depends on: 389705
(Reporter)

Updated

10 years ago
No longer blocks: 389689
Depends on: 389689
(Reporter)

Updated

10 years ago
Depends on: 389758
(Reporter)

Updated

10 years ago
Depends on: 389766
(Assignee)

Updated

10 years ago
Depends on: 389870
(Assignee)

Updated

10 years ago
Depends on: 389891
(Assignee)

Updated

10 years ago
Depends on: 389899
(Assignee)

Updated

10 years ago
Attachment #273870 - Flags: review?(beltzner)

Comment 44

10 years ago
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 → ---
(Assignee)

Comment 45

10 years ago
(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
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 46

10 years ago
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.
(Assignee)

Comment 47

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

Updated

10 years ago
Depends on: 390075
(Assignee)

Comment 48

10 years ago
Created attachment 274474 [details] [diff] [review]
v1.3.1 toolkit changes

Removes the GARBAGE line since we don't want that
Attachment #274474 - Flags: review?(gavin.sharp)
Attachment #274474 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 49

10 years ago
Checking in toolkit/mozapps/handling/src/Makefile.in;
new revision: 1.2; previous revision: 1.1
(Reporter)

Updated

10 years ago
Blocks: 390075
No longer depends on: 390075
(Reporter)

Updated

10 years ago
Blocks: 389705
No longer depends on: 389705
(Reporter)

Updated

10 years ago
Blocks: 389732
No longer depends on: 389732
(Reporter)

Updated

10 years ago
Blocks: 389766
No longer depends on: 389766
(Assignee)

Updated

10 years ago
Depends on: 391640
(Assignee)

Updated

10 years ago
Blocks: 391640
No longer depends on: 391640
Duplicate of this bug: 167473
Flags: in-litmus?

Comment 51

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