Closed Bug 1078508 Opened 5 years ago Closed 5 years ago

Design and implement doorhanger for unidentified security state (new tablet)

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 9 obsolete files)

375.08 KB, image/png
Details
1.78 MB, image/png
antlam
: feedback+
Details
16.95 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Similar to desktop so when the not-identified globe is clicked (bug 1071267), we display a "This website does not supply identity information." doorhanger, or similar.

Any thoughts on design and strings, Anthony?
Hmmm. I'll NI me to think about this more but for the time being, parity with Desktop does seem like a good place to start (in terms of string). 

I can't seem to recall what that looks like on Mobile ATM, have you got a screen shot handy perhaps?
Flags: needinfo?(alam)
Attached image Security doorhanger
Note that this image has some WIP patches and should be slightly different than the current build.
I'd like to give the top and bottom some more padding to free it up a bit (double what we have currently). But, for V1, let's aim for parity first and polish doorhangers as a part of a larger effort so we can include mobile devices too.

How does that sound?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Sounds good to me.

Just to be clear: I'm taking no action on V1 here.
Blocks: new-tablet-v2
No longer blocks: new-tablet-v1
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #4)
> Sounds good to me.
> 
> Just to be clear: I'm taking no action on V1 here.

Awesome. FYI https://bugzilla.mozilla.org/show_bug.cgi?id=1058818 is where the meta bug is filed to clean up doorhangers. :) I've also engaged in conversations with Desktop side so we can keep some consistency there as well (they're looking to possibly improve theirs).
Summary: Design and implement doorhanger for unidentified security state → Design and implement doorhanger for unidentified security state (new tablet)
Spoke IRL: antlam okayed us to implement something similar to desktop.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Small cleanup: Params are easier to test and more predictable than using member variables.
Attachment #8521400 - Flags: review?(bnicholson)
Changed something I didn't mean to.
Attachment #8521403 - Flags: review?(bnicholson)
Attachment #8521400 - Attachment is obsolete: true
Attachment #8521400 - Flags: review?(bnicholson)
Not strictly necessary, but seemed a little more readable and safe (i.e. IDE's missing case warnings).
Attachment #8521406 - Flags: review?(bnicholson)
Attachment #8521403 - Attachment is obsolete: true
Attachment #8521403 - Flags: review?(bnicholson)
Comment on attachment 8521406 [details] [diff] [review]
Part 2: Change SiteIdentityPopup.updateUi to use a switch statement

No longer relevant because rebase.
Attachment #8521406 - Attachment is obsolete: true
Attachment #8521406 - Flags: review?(bnicholson)
Attachment #8521420 - Attachment is obsolete: true
Attachment #8521420 - Flags: review?(bnicholson)
Attachment #8522923 - Flags: review?(lucasr.at.mozilla)
Attached image Unknown site identity
Anthony, what do you think?

...vroom, vroom.
Attachment #8522924 - Flags: feedback?(alam)
On comment 12: due to a bad hg qref error, I merged p1 w/ p2 - oops! Both are in the patch in comment 12 and still pretty readable. See comment 7 for motivation on taking the SiteIdentity parameter in updateIdentity.
Really, really bad with the qref these days.
Attachment #8522931 - Flags: review?(lucasr.at.mozilla)
Attachment #8522923 - Attachment is obsolete: true
Attachment #8522923 - Flags: review?(lucasr.at.mozilla)
On about: pages, desktop shows "This is a secure Firefox site" or similar - we
were showing "This site has an unindentified identity." or similar, which is
misleading and bad. Part 2 fixes this by not showing a doorhanger.

Really, really, really bad at qref, so this is included here.

Added a followup for implementing the "This is a secure Firefox site", as per
desktop. See bug 1099088.
Attachment #8522944 - Flags: review?(lucasr.at.mozilla)
Attachment #8522931 - Attachment is obsolete: true
Attachment #8522931 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8522944 [details] [diff] [review]
Add "unknown site identity" popup

Review of attachment 8522944 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just need the missing layout file to give r+.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +474,5 @@
>  with that structure, consider a translation which ignores the preceding domain and
>  just addresses the organization to follow, e.g. "This site is run by " -->
>  <!ENTITY identity_run_by "which is run by">
> +<!ENTITY identity_no_info "This website does not supply identity information.">
> +<!ENTITY identity_not_encrypted "Your connection to this website is not encrypted.">

I assume we're just using the same strings than desktop?

::: mobile/android/base/resources/layout/site_identity.xml
@@ +21,2 @@
>  
> +        <include layout="@layout/site_identity_unknown" />

Where's this layout? Forgot to include in the patch? I was going to suggest stubbing this bug it's probably not worth it.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ +101,5 @@
> +
> +        mIdentity.setVisibility(View.VISIBLE);
> +        mContent.setPadding(0, 0, 0, 0);
> +
> +        final boolean isIdentityKnown = siteIdentity.getSecurityMode() != SecurityMode.UNKNOWN;

nit: enclose expression with parens.
Attachment #8522944 - Flags: review?(lucasr.at.mozilla) → feedback+
-_-
Attachment #8522958 - Flags: review?(lucasr.at.mozilla)
Attachment #8522944 - Attachment is obsolete: true
Comment on attachment 8522958 [details] [diff] [review]
Add "unknown site identity" popup

Review of attachment 8522958 [details] [diff] [review]:
-----------------------------------------------------------------

Yep.
Attachment #8522958 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8522924 [details]
Unknown site identity

Looks likes it's a good place to start! :) I'm happy with this for V1.
Attachment #8522924 - Flags: feedback?(alam) → feedback+
Priority: -- → P1
Attachment #8522958 - Attachment is obsolete: true
(In reply to Lucas Rocha (:lucasr) from comment #17)
> I assume we're just using the same strings than desktop?

Yep.

> Where's this layout? Forgot to include in the patch?

Added.

> nit: enclose expression with parens.

Done.
Apparently forgot some imports.
Attachment #8524852 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3ddd0d4aa067
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.