Closed Bug 402252 Opened 13 years ago Closed 12 years ago

application details should be accessible from prefs UI

Categories

(Firefox :: File Handling, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3 beta3

People

(Reporter: dmose, Assigned: florian)

References

(Depends on 1 open bug)

Details

(Keywords: late-l10n, Whiteboard: [proto])

Attachments

(2 files, 2 obsolete files)

At the security/design review, it was observed that it's not possible to see the hostname or URI of a web-based protocol handler from the preferences UI, nor is it possible to edit the title.  I just noticed that it's also not possible to see the path of a local application either.

Nominating as blocking-firefox? because it might actually be wanted-firefox3.
Flags: blocking-firefox3?
Whiteboard: [proto]
Why would the use want to edit the title?  They can't do this for local apps.  I don't think the path to local applications will mean much to mainstream users, but we should provide the path for Web applications due to security.
Not sure why we believe that adding the path would do anything other than clutter the display; people who know what the difference is between online and client-side apps will know them by their brand names, people who don't won't be helped by adding a URL.

The trust decision should be made at install-time.

That said, if we wanted to segment Web Apps out into their own section of the drop-down, I'd support that, I guess. Prism confuses this, of course, as it could be a client-side app that's actually a web-app. :)
Flags: blocking-firefox3? → blocking-firefox3-
>The trust decision should be made at install-time.

yeah, I agree.
(In reply to comment #1)
> Why would the use want to edit the title?  They can't do this for local apps. 

In case sites give insufficiently unique titles in their registration ("RSS Reader").

> I don't think the path to local applications will mean much to mainstream
> users

It is possible to have an identically named app in two different directories.  Without a way to see the path, you won't be able to tell which is which.  This is, of course, quite edge-casy.

(In reply to comment #2)
> Not sure why we believe that adding the path would do anything other than
> clutter the display;

I don't know that they need to be in primary UI, a tooltip or something might suffice.

> The trust decision should be made at install-time.

Absolutely.  But when people are using this pref UI, they're potentially re-evaluating that trust decision, perhaps in the context of what other apps they now have installed or new information or education.  But now they no longer have all the information that was available to them when they initially made the decision.


We pretty much decided over in bug 402245 not to make this trust decision at install time.  As a result, I think we need this for Fx3.  Renominating for reconsideration.
Flags: blocking-firefox3- → blocking-firefox3?
Note that the tentative plan for bug 402620 (per faaborg) is to do the same thing (provide ancillary info, in particular at least the hostname) in the dialog as well to prevent web-based handlers from being able to spoof local handlers.
Assignee: dmose → florian
(In reply to comment #5)
> We pretty much decided over in bug 402245 not to make this trust decision at
> install time.  As a result, I think we need this for Fx3.  Renominating for
> reconsideration.

I don't think that's actually the case, though I've been using the terminology loosely.

The user makes the trust decision at the time they click on the link asking to install the application handler, and then again trusts them when they choose the application as a handler for a specific file.

What I said was that if we know that handler shouldn't be trusted, we should caution users; but they've already *made* their trust decision then, and will again.

That said, I'd support some way of verifying that "Gmail" is actually from "www.google.com" when you're running. I think that bug 406620 is the blocker here, and this one is just wanted until we figure out how to do it in a way that doesn't make the design ugly.

Ideas for that would be:

 [Y] Yahoo! Mail (www.yahoo.com)
 [G] GMail  (www.gmail.com)

with the URL part in smaller text in the drop-down.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Keywords: uiwanted
Keywords: uiwanted
Whiteboard: [proto] → [proto][needs ui-decision, patch]
Pasting here the content of an email from beltzner:


I think we need to make the following changes:

- switch the "Applications" column header to say "Action"
- for files and protocol handlers, always order the list as follows:

  Always ask
  [HandleWithFirefoxOption]
  -----------------------
  [Use System Default]
  Use [app1]
  Use [app2]
  Use other...
  Application details...

 where [HandleWithFirefoxOption] is "Save File" or "Add Live Bookmark" or nothing, depending

- selecting "Application details..." would open a dialog that looks like

 You have indicated that the following applications can be used
 to handle the itms protocol:

  .-------------------------------------------.
  |=App1==(web)============================== | (Remove)
  | App2                                      |
  | App3                                      |
  |                                           |
  '-------------------------------------------'
        App1 is a web application hosted at
        http://foo.bar.baz/biff/baff/boff

                                      ( Cancel ) (( OK ))
Keywords: uiwanted
Whiteboard: [proto][needs ui-decision, patch] → [proto][needs patch]
Following the UI mockup pasted in the previous comment would also fix bug 402153.
Blocks: 402153
(In reply to comment #7)

> [...] this one is just wanted until we figure out how to do it in a way
> that doesn't make the design ugly.
> 

We have the UI design now.  Renominating, though if blocking it would probably be P4 or P5.
Flags: blocking-firefox3- → blocking-firefox3?
Attached patch patch v1 (obsolete) — Splinter Review
This was ui-reviewed by beltzner over IRC.
Attachment #299332 - Flags: review?(mano)
Status: NEW → ASSIGNED
Whiteboard: [proto][needs patch] → [proto][needs review Mano]
Comment on attachment 299332 [details] [diff] [review]
patch v1

nits, mostly.

>Index: browser/components/preferences/applicationManager.js
>===================================================================
>+# Portions created by the Initial Developer are Copyright (C) 2005

that cannot be right.

>+    var defaultApp = this.handlerInfo.preferredApplicationHandler;
>+    var handlers = this.handlerInfo.possibleApplicationHandlers;
>+    for (var i = 0; i < this._removed.length; ++i) {
>+      var app = this._removed[i];
>+      if (defaultApp && app.equals(defaultApp)) {
>+        // If the app we remove was the default app, we must make sure
>+        // it won't be used anymore
>+        this.handlerInfo.alwaysAskBeforeHandling = true;
>+        this.handlerInfo.preferredApplicationHandler = null;

Please file a follow up to automate this.

>+  remove: function appManager_remove() {
>+    var list = document.getElementById("appList");
>+    this._removed.push(list.selectedItem.app);
>+    var index = list.selectedIndex;
>+    list.removeItemAt(index);
>+    if (!list.getRowCount()) {
>+      // The list is now empty, make the bottom part disappear
>+      document.getElementById("appType").hidden = true;
>+      document.getElementById("appLocation").hidden = true;

maybe put the "bottom part" in a box?

>Index: browser/components/preferences/applicationManager.xul
>===================================================================
>+<?xml-stylesheet href="chrome://global/skin/"?>
>+
>+<!DOCTYPE dialog SYSTEM "chrome://browser/locale/preferences/applicationManager.dtd">
>+
>+<dialog id="appManager"
...
>+        windowtype="Browser:AppManager">

this is a modal dialog...

>+  <script type="application/x-javascript"
>+          src="chrome://browser/content/preferences/applicationManager.js"/>
>+  <script type="application/x-javascript"
>+          src="chrome://browser/content/preferences/applications.js"/>
>+

application/javascript

>+  <commandset id="appManagerCommandSet">
>+    <command id="cmd_remove"
>+             oncommand="gAppManagerDialog.remove();"
>+             disabled="true"/>
>+  </commandset>
>+
>+  <keyset id="appManagerKeyset">
>+    <key id="delete" keycode="VK_DELETE" command="cmd_remove"/>
>+  </keyset>

Any chance you could implement a controller for cmd_delete instead? We should also [try to] support backspace on OS X.


>+    </vbox>
>+  </hbox>
>+  <separator class="thin"/>
>+  <description id="appType"/>

label, i think.

>Index: browser/components/preferences/applications.js
>=================================================================== 
>+    // Create a menu item for saving to disk.
>+    // Note: this option isn't available to protocol types, since we don't know
>+    // what it means to save a URL having a certain scheme to disk, nor is it
>+    // available to feeds, since the feed code doesn't implement the capability.
>+    if ((handlerInfo.wrappedHandlerInfo instanceof Ci.nsIMIMEInfo) &&
>+        handlerInfo.type != TYPE_MAYBE_FEED) {
>+      var saveMenuItem = document.createElementNS(kXULNS, "menuitem");

just createElement, feel free to fix other places in this file :)

ditto for other instances in this patch, feel free to fix other places in this file :

* the textNode->textContent change.
Attachment #299332 - Flags: review?(mano) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #299332 - Attachment is obsolete: true
(In reply to comment #12)

> >+    var defaultApp = this.handlerInfo.preferredApplicationHandler;
> >+    var handlers = this.handlerInfo.possibleApplicationHandlers;
> >+    for (var i = 0; i < this._removed.length; ++i) {
> >+      var app = this._removed[i];
> >+      if (defaultApp && app.equals(defaultApp)) {
> >+        // If the app we remove was the default app, we must make sure
> >+        // it won't be used anymore
> >+        this.handlerInfo.alwaysAskBeforeHandling = true;
> >+        this.handlerInfo.preferredApplicationHandler = null;
> 
> Please file a follow up to automate this.
> 

I added a removePossibleApplicationHandler method to HandlerInfoWrapper in applications.js

> >+  <commandset id="appManagerCommandSet">
> >+    <command id="cmd_remove"
> >+             oncommand="gAppManagerDialog.remove();"
> >+             disabled="true"/>
> >+  </commandset>
> >+
> >+  <keyset id="appManagerKeyset">
> >+    <key id="delete" keycode="VK_DELETE" command="cmd_remove"/>
> >+  </keyset>
> 
> Any chance you could implement a controller for cmd_delete instead? We should
> also [try to] support backspace on OS X.

I would prefer doing this in a follow up bug.

Fixed the other nits.
Attachment #299666 - Flags: review?(mano)
Comment on attachment 299666 [details] [diff] [review]
patch v2

I don't like the vanishing details area much. I think it should include the separator if it says.

>+  onOK: function appManager_onOK() {
>+    for (var i = 0; i < this._removed.length; ++i)
>+      this.handlerInfo.removePossibleApplicationHandler(this._removed[i]);
>+
>+    this.handlerInfo.store();

<florian>	I should probably add back the if(!this._removed.lenght) return; at the begining of onOK

>+  onCancel: function appManager_onCancel() {
>+  },
>+

Just remove it (or add a "be gentle, do nothing..

>+  remove: function appManager_remove() {
>+    var list = document.getElementById("appList");
>+    this._removed.push(list.selectedItem.app);
>+    var index = list.selectedIndex;
>+    list.removeItemAt(index);
>+    if (!list.getRowCount()) {

I prefer a |== 0| check here.

>Index: browser/locales/en-US/chrome/browser/preferences/applicationManager.dtd
>===================================================================

>+<!ENTITY appManager.title     "Application details">

Application Details.

r=mano otherwise.
Attachment #299666 - Flags: review?(mano) → review+
Attached patch patch v3Splinter Review
Attachment #299666 - Attachment is obsolete: true
Keywords: late-l10n
Whiteboard: [proto][needs review Mano] → [proto]
Attachment #299693 - Flags: approval1.9?
Comment on attachment 299693 [details] [diff] [review]
patch v3

a1.9+=damons
Attachment #299693 - Flags: approval1.9? → approval1.9+
Fixed the bitrot from bug 398627:
  saveMenuItem.setAttribute("image", ICON_URL_SAVE);
replaced by:
  saveMenuItem.setAttribute(APP_ICON_ATTR_NAME, "save");

and checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Backed out the applications.js changes to fix unit test failure on tinderbox until I recover the lost string changes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Parts of the patch that I missed in the previous diff...
Attachment #300046 - Flags: ui-review+
Attachment #300046 - Flags: review+
Attachment #300046 - Flags: approval1.9+
Relanded the changes in applications.js, along with attachment 300046 [details] [diff] [review].
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Localizer rant follows.

Quoting applicationManager.properties:
> descriptionApplications=The following applications can be used to handle %S.
> 
> handleProtocol=%S links
> handleWebFeeds=Web Feeds
> handleFile=%S content

Please avoid dynamically composing sentences as often as you can. Too often they become a headache for localizers. Thanks! :)
If I open the application details for Web-Feed, I get the following two errors.

Fehler: redeclaration of const Cc
Quelldatei: chrome://browser/content/preferences/applications.js
Zeile: 11

Fehler: gApplicationsPane is not defined
Quelldatei: chrome://browser/content/preferences/applicationManager.js
Zeile: 34


It seems that applicationManager.js is loaded before applications.js, because it defines the constants Ci and Cc. And because of this redeclaration-error, the 2nd error occurs causing gAppManagerDialog.init to fail.

After commenting out the definitions of const Ci and const Cc applicationManager.js everything is ok.

My OS is Windows XP. I guess Ci and Cc are not needed at all in applicationManager.js or it needs an ifdef XP_MACOSX, so that they are only defined, if they are not in applications.js (note the #ifndef XP_MACOSX there).
Flags: blocking-firefox3? → blocking-firefox3+
Depends on: 416471
Depends on: 416899
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3 ID:2008020511.

The Application Details dialog is shown but doesn't list any of the available handlers with the above version. This is fixed (in another bug) when running a latest nightly build e.g. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008021804 Minefield/3.0b4pre ID:2008021804
Status: RESOLVED → VERIFIED
Target Milestone: --- → Firefox 3 beta3
You need to log in before you can comment on or make changes to this bug.