Closed Bug 402620 Opened 14 years ago Closed 14 years ago

protocol handling dialog should display icons and hostnames for web applications

Categories

(Firefox :: File Handling, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta2

People

(Reporter: dmose, Assigned: florian)

References

(Depends on 1 open bug)

Details

(Whiteboard: [proto])

Attachments

(2 files, 3 obsolete files)

Both web apps and local handler apps.
Flags: blocking-firefox3?
I suspect this wants to block Firefox3, as it looks somewhat goofy now.
Whiteboard: [proto]
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M10
Priority: -- → P3
Assignee: nobody → f.qu
Probably also want to give an icon for web apps to the dropdown in Applications pref pane at the same time (local handler apps already have them there).
(In reply to comment #2)
> Probably also want to give an icon for web apps to the dropdown in Applications
> pref pane at the same time (local handler apps already have them there).
> 

In my gmail mailto: handler I already see the gmail favicon so I suppose your are talking about a default icon for handlers without favicon?
Priority: P3 → P2
Yeah, I just added a comment to bug 391576 about coming up with an icon for that case.
For web application, we need to display the hostname because otherwise it would be easy to register a protocol handler spoofing a local application.
Status: NEW → ASSIGNED
Summary: protocol handling dialog should have icons for applications → protocol handling dialog should display icons and hostnames for web applications
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #288790 - Flags: ui-review?(beltzner)
Hmm, shouldn't we be using TLD like the download manager does?  This could be subject to some form of fraud:
http://gmail.com.some.super.malicious.site.that.wants.to.steal.your.login....
It'd be cut off, but the user would probably see gmail.com and think it'd be OK.

And good call changing the try catch to instanceof.  I thought I had filed a bug on that, but clearly I didn't (I should have done it right in the first place though).
(In reply to comment #8)
> Hmm, shouldn't we be using TLD like the download manager does?  This could be
> subject to some form of fraud:
> http://gmail.com.some.super.malicious.site.that.wants.to.steal.your.login....
> It'd be cut off, but the user would probably see gmail.com and think it'd be
> OK.
> 

Hmm, I was more thinking about a fraud like http://mozilla.thunderbird.blahblahblahblahblah.evil.com and that's why I wanted to display the scheme at the beginning. Also I would probably not use a mailto handler which is not over https.

Maybe I should add something like crop="center".
Well, we could display a lock icon (probably a bad idea there) for secure ones, but will everything be over http?  I mean, (dmose correct me if I'm wrong since you are more familiar with the spec), couldn't it dispatch to another protocol if it wanted to?

Also, shouldn't we be using the favicon service to get the favorite icon?  I'm about 90% sure that's toolkit code.
(In reply to comment #10)

> Also, shouldn't we be using the favicon service to get the favorite icon?  I'm
> about 90% sure that's toolkit code.
> 

According to the comment here we can't:
http://mxr.mozilla.org/seamonkey/source/browser/components/preferences/applications.js#1657

I assumed this comment was right so I haven't tried.
I would ask some places devs about that to be sure (maybe that should change?).
cc'ing Johnathan since he's been doing lots of security UI work.
Yes, what's there right now does look sort of silly.  :)

My reactions are that yeah, eTLD+1 makes sense here, and should be easier now that nsIEffectiveTLDService has been improved.

I understand what Florian's saying in comment 9 about the risks of sending, e.g., confidential information over http, but I think whether or not we choose to talk about the security of the channel, we can do so better than just preserving the scheme, which users don't find all that intuitive anyhow.

I don't think a lock overlay is a terrible idea here - despite being a conflated signal, the lock here would be a bit more accurate - closer to "this channel is secure" than "you can trust this site" because at this point, they've already made the trust decision about the site/app operator.  As Shawn points out though, this assumes they are using a scheme like https, and I also think we could fix the rest of this bug without worrying about signaling channel encryption, and handle that discussion separately.
(In reply to comment #10)
> Well, we could display a lock icon (probably a bad idea there) for secure ones,
> but will everything be over http?  I mean, (dmose correct me if I'm wrong since
> you are more familiar with the spec), couldn't it dispatch to another protocol
> if it wanted to?

Yes.  All the lock could really tell you is whether the handler entry page is over https.

(In reply to comment #11)
> According to the comment here we can't:
> http://mxr.mozilla.org/seamonkey/source/browser/components/preferences/applications.js#1657

I suspect that whether that comment is accurate may depend on how the particular web application is structured.  It seems like in at least some (most? all?) cases, we should be able to ask the favicon service for an entry corresponding to uri.prePath.  I suspect myk knows more, as it was his comment; I've added him to the CC for advice...

(In reply to comment #14)
> My reactions are that yeah, eTLD+1 makes sense here, and should be easier now
> that nsIEffectiveTLDService has been improved.

Yeah, this sounds right to me too.

As far as the lock goes, I kinda like it, but I see that it has some issues too.  I guess I'd vote for including it unless our ui-reviewer of record objects.  beltzner?

Whiteboard: [proto] → [proto][needs ui-r, new patch, r]
Do remember that the icon is site-controlled, and I'd hesitate to allow a site to display whatever icon it liked here if it was interested in spoofing and redirecting to the actual site.
That's why if we display the icon we need the hostname too.
Whiteboard: [proto][needs ui-r, new patch, r] → [proto][needs ui-r beltzner, new patch, r]
(In reply to comment #15)
> (In reply to comment #14)
> > My reactions are that yeah, eTLD+1 makes sense here, and should be easier now
> > that nsIEffectiveTLDService has been improved.
> 
> Yeah, this sounds right to me too.

Agreed.

> As far as the lock goes, I kinda like it, but I see that it has some issues
> too.  I guess I'd vote for including it unless our ui-reviewer of record
> objects.  beltzner?

We're deprecating the lock metaphor in Fx3, so I'd say "no".
Comment on attachment 288790 [details] [diff] [review]
patch v1

Let's get this in for now, we can discuss the protocol issue later. My feeling is that we should only show the protocol if it's https:// as a way of satisfying people who say "I don't want to use it if it isn't https!" more than anything else.

As I stated above, I don't think the lock icon is a good idea - it's a nice shorthand, yes, but a little too overloaded in meaning.
Attachment #288790 - Flags: ui-review?(beltzner) → ui-review+
Attached patch patch v1.1 (same unbitrotted) (obsolete) — Splinter Review
As per comment 19, let's get this reviewed and move the discussion about eTLD+1/hostname/protocol/lock icon/etc... to a followup bug.
Attachment #288790 - Attachment is obsolete: true
Attachment #296421 - Flags: review?(mano)
Whiteboard: [proto][needs ui-r beltzner, new patch, r] → [proto][needs review mano]
Looking at it again, i disagree with the comment in _getIconURLForWebApp (applications.js), we should still try to use the favicon service for uri.*prePath* and only then default to favicon.icon.
Attached patch patch v2 (obsolete) — Splinter Review
Ok, using the favicon service and falling back to favicon.ico if it fails.
Attachment #296421 - Attachment is obsolete: true
Attachment #298406 - Flags: review?(mano)
Attachment #296421 - Flags: review?(mano)
Comment on attachment 298406 [details] [diff] [review]
patch v2

s/padding-left/-moz-padding-start/

r=mano otherwise.
Attachment #298406 - Flags: review?(mano) → review+
Whiteboard: [proto][needs review mano] → [proto][needs updated patch, checkin]
(In reply to comment #23)
> (From update of attachment 298406 [details] [diff] [review])
> s/padding-left/-moz-padding-start/
>

I should make a sticky note of this.  It's at least the third time you notice it in my code!
Attachment #298406 - Attachment is obsolete: true
Checking in toolkit/mozapps/handling/content/handler.xml;
/cvsroot/mozilla/toolkit/mozapps/handling/content/handler.xml,v  <--  handler.xml
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/mozapps/handling/content/dialog.js;
/cvsroot/mozilla/toolkit/mozapps/handling/content/dialog.js,v  <--  dialog.js
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/themes/winstripe/mozapps/handling/handling.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/handling/handling.css,v  <--  handling.css
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/themes/pinstripe/mozapps/handling/handling.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/handling/handling.css,v  <--  handling.css
new revision: 1.3; previous revision: 1.2
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [proto][needs updated patch, checkin] → [proto]
Depends on: 413567
(In reply to comment #20)

> As per comment 19, let's get this reviewed and move the discussion about
> eTLD+1/hostname/protocol/lock icon/etc... to a followup bug.
> 

Filed bug 413567 to track this issue.
You need to log in before you can comment on or make changes to this bug.