Closed
Bug 402620
Opened 17 years ago
Closed 17 years ago
protocol handling dialog should display icons and hostnames for web applications
Categories
(Firefox :: File Handling, defect, P2)
Firefox
File Handling
Tracking
()
RESOLVED
FIXED
Firefox 3 beta2
People
(Reporter: dmosedale, Assigned: florian)
References
(Depends on 1 open bug)
Details
(Whiteboard: [proto])
Attachments
(2 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Both web apps and local handler apps.
Flags: blocking-firefox3?
Reporter | ||
Comment 1•17 years ago
|
||
I suspect this wants to block Firefox3, as it looks somewhat goofy now.
Whiteboard: [proto]
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M10
Reporter | ||
Updated•17 years ago
|
Priority: -- → P3
Reporter | ||
Updated•17 years ago
|
Assignee: nobody → f.qu
Reporter | ||
Comment 2•17 years ago
|
||
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).
Assignee | ||
Comment 3•17 years ago
|
||
(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?
Updated•17 years ago
|
Priority: P3 → P2
Reporter | ||
Comment 4•17 years ago
|
||
Yeah, I just added a comment to bug 391576 about coming up with an icon for that case.
Assignee | ||
Comment 5•17 years ago
|
||
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
Assignee | ||
Comment 6•17 years ago
|
||
Assignee | ||
Comment 7•17 years ago
|
||
Assignee | ||
Updated•17 years ago
|
Attachment #288790 -
Flags: ui-review?(beltzner)
Comment 8•17 years ago
|
||
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).
Assignee | ||
Comment 9•17 years ago
|
||
(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".
Comment 10•17 years ago
|
||
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.
Assignee | ||
Comment 11•17 years ago
|
||
(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.
Comment 12•17 years ago
|
||
I would ask some places devs about that to be sure (maybe that should change?).
Comment 13•17 years ago
|
||
cc'ing Johnathan since he's been doing lots of security UI work.
Comment 14•17 years ago
|
||
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.
Reporter | ||
Comment 15•17 years ago
|
||
(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?
Reporter | ||
Updated•17 years ago
|
Whiteboard: [proto] → [proto][needs ui-r, new patch, r]
Comment 16•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
That's why if we display the icon we need the hostname too.
Reporter | ||
Updated•17 years ago
|
Whiteboard: [proto][needs ui-r, new patch, r] → [proto][needs ui-r beltzner, new patch, r]
Comment 18•17 years ago
|
||
(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 19•17 years ago
|
||
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+
Assignee | ||
Comment 20•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [proto][needs ui-r beltzner, new patch, r] → [proto][needs review mano]
Comment 21•17 years ago
|
||
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.
Assignee | ||
Comment 22•17 years ago
|
||
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 23•17 years ago
|
||
Comment on attachment 298406 [details] [diff] [review]
patch v2
s/padding-left/-moz-padding-start/
r=mano otherwise.
Attachment #298406 -
Flags: review?(mano) → review+
Reporter | ||
Updated•17 years ago
|
Whiteboard: [proto][needs review mano] → [proto][needs updated patch, checkin]
Assignee | ||
Comment 24•17 years ago
|
||
(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
Assignee | ||
Comment 25•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Whiteboard: [proto][needs updated patch, checkin] → [proto]
Assignee | ||
Comment 26•17 years ago
|
||
(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.
Description
•