Closed Bug 695204 Opened 8 years ago Closed 8 years ago

Display site security (larry)

Categories

(Firefox for Android :: General, defect, P2)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 15
Tracking Status
fennec 12+ ---

People

(Reporter: elan, Assigned: Margaret)

References

(Depends on 1 open bug)

Details

(Whiteboard: [QA+])

Attachments

(6 files, 4 obsolete files)

* steal what we use to do with
* UI Needed
* Use native popup window (see twitter app)
Priority: P1 → P2
Whiteboard: [QA+]
not needed?
I think it's a good P2
Assignee: nobody → margaret.leibovic
What should this look like? I'm going to start by trying to imitate what we have in XUL Fennec right now, but this is an opportunity to change that if there's something better we can do.
Keywords: uiwanted
I had to switch my attention back to desktop Firefox, so I'm going to unassign myself in case someone else can get to this before me.
Assignee: margaret.leibovic → nobody
Assignee: nobody → ju.vermet
Julien, a good first step would be to style the background of the favicon button (ibarlow is working on a nice design).

The security state is notified in the onSecurityChange() method of the web progress listener athttp://mxr.mozilla.org/projects-central/source/birch/mobile/chrome/content/browser.js#624

Here's firefox desktop implementation:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4551
Note: Styling the background of the favicon is bug 695477, which is a prerequisite for this bug. Check with Lucas and take bug 695477 if you want.
I'm waiting for Lucas answer before submitting the patch. I asked him if it's possible to take bug 695477.
Depends on: 695477
bug 695477 has been assigned to me but as the design will change the summary has changed and I'm now waiting for sriram to fix it before posting my patch for this bug.
On the IRC, we had a talk about where we need to find the lock. We need a lock in the tab bar, this is sure. What do you think about a lock in the tabs menu http://www.flickr.com/photos/61892693@N03/6302522277/in/photostream/ ?

In my opinion this is useless but sriram seemed interested by this feature if I well understood.
Attached patch Patch v1 (obsolete) — Splinter Review
There was an error in the orgininal onSecurityChange function. There were 4 arguments instead of 3. As there is no "lock" ready in the BrowserToolbar, I just change the background color of the favicon.

I hope it's what you were looking for :)
Attachment #571717 - Flags: review?(mark.finkle)
Comment on attachment 571717 [details] [diff] [review]
Patch v1

The patch looks good, but we need to track the security state in the Tab, so when we change tabs we set the favicon color.

I'll post an update with that change
Attachment #571717 - Flags: review?(mark.finkle) → review-
Hmm, I'll post this on bug 695477, since this is color/indicator only
Attachment #571717 - Attachment is obsolete: true
Depends on: 703115
Duplicate of this bug: 703637
Summary: Site Security → Display site security (larry)
Duplicate of this bug: 705337
Are we doing a doorhanger off of the lock for this? And changing the color (or background tile) for the lock for ssl vs. EV?
A doorhanger-like popupwindow would be ok for implementation. Color of lock could change too. Throw up some specs.
Specs and assets to follow soon
Attached image Design specs
Assets to follow shortly
From Firefox 3 we stopped using padlock in the location bar:
http://www.dria.org/wordpress/archives/2008/05/06/635/

And removed it from the status bar too (bug 558551)
> During the site identity work of Firefox 3, we decided to move away from using
> the lock icon as a symbol for SSL because users were interpreting it as meaning
> "this site is run by good people" as opposed to its actual meaning.

Our Firefox's site identity (and security) concept is "users need to check site icon only" from Firefox 3 isn't it?

Why you'll re-introduce obsolete lock icon again, only for mobile?

We should keep the same concept of site identity both with desktop Firefox and mobile Firefox. Site icon and around it (background color) should be used to show identity and security of the site.
tracking-fennec: --- → 11+
Is this still 11+ ? It'd most likely come with string changes at this point.
(In reply to Axel Hecht [:Pike] from comment #21)
> Is this still 11+ ? It'd most likely come with string changes at this point.

Agreed. This is not on track for Fx11. Moving to Fx12.
tracking-fennec: 11+ → 12+
Removing uiwanted since Ian posted mockups.  Feel free add back if/when more ui input is needed!
Group: mozilla-confidential
Keywords: uiwanted
Group: mozilla-confidential
Julien, have you been working on this? I'd like to take this bug if you're too busy to work on it.
Yes Margaret, I'm very busy right now, feel free to take it.
Assignee: ju.vermet → margaret.leibovic
Attached patch WIP (browser.js bits) (obsolete) — Splinter Review
This puts the gecko pieces in place to get data for the UI. This is mostly a trimmed down port of XUL fennec's IdentityHandler, although I took stuff from desktop's gIdentityHandler that never made its way to mobile (and I had to revert some of the changes made for the custom browser.xul binding).
(In reply to Axel Hecht [:Pike] from comment #21)
> Is this still 11+ ? It'd most likely come with string changes at this point.

Also, I should note, strings for this already exist, so we wouldn't need to make any string changes. (Some should probably be moved to Java eventually, but we wouldn't need to do that to make the UI work.)
Attached patch WIP (mostly done) (obsolete) — Splinter Review
Issues left to sort out:
-Arrow points to the wrong place on non-ICS devices (and in landscape mode) because its position is hard-coded (I'll probably have to make multiple layout files)
-Popup doesn't get dismissed when you switch away from the app
-Tapping on the lock icon while the popup is open won't dismiss the popup, it will make it re-show
Attachment #600186 - Attachment is obsolete: true
Attachment #619088 - Flags: feedback?(mark.finkle)
Comment on attachment 619088 [details] [diff] [review]
WIP (mostly done)


>diff --git a/mobile/android/base/SiteIdentityPopup.java b/mobile/android/base/SiteIdentityPopup.java
>+    public static final String VERIFIED_COLOR = "#77BAFF";
>+    public static final String IDENTIFIED_COLOR = "#B7D46A";

We might be able to put these in colors.xml

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
>+<!-- Localization note (iidentity_run_by) : This string appears between a

identity_run_by
>diff --git a/mobile/android/base/resources/drawable-hdpi/larry_blue.png b/mobile/android/base/resources/drawable-hdpi/larry_blue.png


These files seem kinda large, just from what I see in base64. We might want to crunch them, or I could be off base and the size is not too bad.

>diff --git a/mobile/android/base/resources/layout/site_identity_popup.xml b/mobile/android/base/resources/layout/site_identity_popup.xml

Not blocking you to land, but I wonder if we could reuse the doorhanger "outer" XML, or share it between larry and doorhangers.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>   onSecurityChange: function(aWebProgress, aRequest, aState) {
>+    // Don't need to do anything if the data we use to update the UI hasn't
>+    // changed
>+    if (this._state == aState &&
>+        !this._hostChanged) {

One line please

>+  getIdentityData : function() {
>+    var result = {};
>+    var status = this._lastStatus.QueryInterface(Components.interfaces.nsISSLStatus);
>+    var cert = status.serverCert;

Could be 'let'

>+  }
>+};
> function OverscrollController(aTab) {

Add some blank lines


Let's get some UX feedback and land this!
Attachment #619088 - Flags: feedback?(mark.finkle) → feedback+
(In reply to Mark Finkle (:mfinkle) from comment #31)

> >diff --git a/mobile/android/base/resources/drawable-hdpi/larry_blue.png b/mobile/android/base/resources/drawable-hdpi/larry_blue.png
> 
> 
> These files seem kinda large, just from what I see in base64. We might want
> to crunch them, or I could be off base and the size is not too bad.

Ian, can you help me with this?

> >diff --git a/mobile/android/base/resources/layout/site_identity_popup.xml b/mobile/android/base/resources/layout/site_identity_popup.xml
> 
> Not blocking you to land, but I wonder if we could reuse the doorhanger
> "outer" XML, or share it between larry and doorhangers.

Yeah, that sounds like a good follow-up.
Attached patch patch (obsolete) — Splinter Review
I added code to hide the popup in onResume, and I fixed the arrow positioning issues. It points to the correct place in portrait/landscape on my Neuxs S, and it points to the correct place in portrait on my Galaxy Nexus, but it's still a tiny bit off in landscape mode on the Galaxy Nexus, so I need to look into that more. I decided to just hide it on orientation change, so that we don't need to deal with re-adjusting on the fly.
Attachment #619088 - Attachment is obsolete: true
Attachment #620017 - Flags: review?(mark.finkle)
Updated with new pngs that Ian gave me.
Attachment #620017 - Attachment is obsolete: true
Attachment #620017 - Flags: review?(mark.finkle)
Attachment #620030 - Flags: review?(mark.finkle)
Attachment #620030 - Flags: review?(mark.finkle) → review+
Flags: in-litmus?(fennec)
Depends on: 751205
Depends on: 751206
https://hg.mozilla.org/mozilla-central/rev/5430670457ac

Also filed some follow-ups for the small issues with this patch.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Depends on: 751446
Depends on: 752172
Behavior covered in test case:

https://moztrap.mozilla.org/manage/cases/_detail/6298/

The test case was added in the BFTs run in the Security feature suite
Flags: in-litmus?(fennec) → in-moztrap+
Security larry is accessible on Nightly 18.0a1 2012-09-02, Aurora 17.0a2 2012-09-02 using the Samsung Galaxy Tab 2 7.0 (Android 4.0.4) and Samsung Galaxy R (Android 2.3.4).
Status: RESOLVED → VERIFIED
Depends on: 785156
You need to log in before you can comment on or make changes to this bug.