[CustomTab] 3 different types of displaying site info in URL bar should be designed in custom tab while there is only one in chrome

VERIFIED FIXED in Firefox 55

Status

()

Firefox for Android
General
VERIFIED FIXED
9 months ago
7 months ago

People

(Reporter: ryang, Assigned: walkingice)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(12 attachments)

635.48 KB, image/png
Details
3.23 MB, image/png
Details
2.22 MB, image/png
Details
1.64 MB, image/png
Details
130.03 KB, image/png
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details | Review
59 bytes, text/x-review-board-request
sebastian
: review+
Details | Review
59 bytes, text/x-review-board-request
sebastian
: review+
Details | Review
59 bytes, text/x-review-board-request
sebastian
: review+
Details | Review
59 bytes, text/x-review-board-request
sebastian
: review+
Details | Review
59 bytes, text/x-review-board-request
sebastian
: review+
Details | Review
619.22 KB, image/png
Details
[CustomTab]  3 different types of displaying site info  in URL bar should be designed in custom tab while there is only one type in chrome.

In firefox browser we, 3 different types of displaying site info in URL Bar is possible.
refer to the attachment .
Created attachment 8828634 [details]
chrome has only one tye of site info

chrome has only one type of site info
Created attachment 8828635 [details]
Firefox has 3  types of site info in URL Bar- Type1

Firefox has 3 types of site info in URL Bar- Type1
Created attachment 8828636 [details]
Firefox has 3  types of site info in URL Bar- Type2

Firefox has 3  types of site info in URL Bar- Type2
Created attachment 8828638 [details]
Firefox has 3  types of site info in URL Bar- Type3

Firefox has 3  types of site info in URL Bar- Type3
Hi Jack, 

would you please kindly provide the UX spec for the reference of engineering efforts ?

Thank you !!
Flags: needinfo?(jalin)

Comment 6

9 months ago
Created attachment 8830278 [details]
Screen Shot 2017-01-25 at 9.08.38 pm.png

Please refer to the attachment. The attached is mockup and our visual designer is working on redesign them.

Thank you
Jack
Flags: needinfo?(jalin)
Hi Julian, 

is the UX spec in comment6 enough for engineering team to work on this ? 
Thank you  very much !
Flags: needinfo?(walkingice0204)
(Assignee)

Comment 8

9 months ago
The draft is enough to me to survey how to implement it.
Assignee: nobody → walkingice0204
Flags: needinfo?(walkingice0204)
Blocks: 1337236
No longer blocks: 1337236
Blocks: 1337236
(Assignee)

Updated

7 months ago
See Also: → bug 1347037
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I don't really know this code, so I think :sebastian should review these patches.
Flags: needinfo?(walkingice0204)
(Assignee)

Updated

7 months ago
Attachment #8847001 - Flags: review?(nchen) → review?(s.kaspari)
Attachment #8847002 - Flags: review?(nchen) → review?(s.kaspari)
Attachment #8847003 - Flags: review?(nchen) → review?(s.kaspari)
Attachment #8847004 - Flags: review?(nchen) → review?(s.kaspari)
Attachment #8847005 - Flags: review?(nchen) → review?(s.kaspari)
Attachment #8847006 - Flags: review?(nchen) → review?(s.kaspari)
(Assignee)

Comment 16

7 months ago
Jim: Understood, I changed the reviewer to Sebastian, thanks!
Flags: needinfo?(walkingice0204)

Comment 17

7 months ago
mozreview-review
Comment on attachment 8847001 [details]
Bug 1332546 - Refactory ActionBar by extracting methods to another class

https://reviewboard.mozilla.org/r/120038/#review122486

Awesome. That's pretty nice!

::: commit-message-6d38a:1
(Diff revision 1)
> +Bug 1332546 - Refactory ActionBar

nit: The title of the commit could be a bit more descriptive :)
Attachment #8847001 - Flags: review?(s.kaspari) → review+

Comment 18

7 months ago
mozreview-review
Comment on attachment 8847002 [details]
Bug 1332546 - ensure ActionBar color is not translucent

https://reviewboard.mozilla.org/r/120040/#review122488

Are there some apps that set a translucent color? What does Chrome do?
Attachment #8847002 - Flags: review?(s.kaspari) → review+

Comment 19

7 months ago
mozreview-review
Comment on attachment 8847003 [details]
Bug 1332546 - Apply default actionbar color

https://reviewboard.mozilla.org/r/120042/#review122490
Attachment #8847003 - Flags: review?(s.kaspari) → review+

Comment 20

7 months ago
mozreview-review
Comment on attachment 8847004 [details]
Bug 1332546 - Add CustomView to ActionBar of CustomTabsActivity

https://reviewboard.mozilla.org/r/120044/#review122492

::: mobile/android/base/java/org/mozilla/gecko/toolbar/SecurityModeUtil.java:22
(Diff revision 1)
> +
> +/**
> + * Util class which encapsulate logic of how CustomTabsActivity treats SiteIdentity.
> + * TODO: Bug 1347037 - This class should be reusable for other components
> + */
> +public class SecurityModeUtil {

This looks like a great class for some unit tests.

Also: Let's file a follow-up bug to use this in BrowserApp too. Fragmenting the logic will bite us as soon as we make changes.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/SecurityModeUtil.java:75
(Diff revision 1)
> +                ? securityModeMap.get(securityMode)
> +                : Mode.UNKNOWN;
> +    }
> +
> +    // Security mode constants, which map to the icons / levels defined in:
> +    // http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/resources/drawable/customtabs_site_security_level.xml

This URL returns a 404 error. On the right side of MXR you can find a permalink that will always link to the version you are looking at and not change when the repository changes.

::: mobile/android/base/resources/drawable/customtabs_site_security_icon.xml:6
(Diff revision 1)
> +<level-list xmlns:android="http://schemas.android.com/apk/res/android">
> +
> +    <item
> +        android:drawable="@drawable/site_security_unknown"
> +        android:maxLevel="0"/>

Isn't this list the same as site_security_level.xml?
Attachment #8847004 - Flags: review?(s.kaspari) → review+

Comment 21

7 months ago
mozreview-review
Comment on attachment 8847005 [details]
Bug 1332546 - add initialization checking to SiteIdentityPopup

https://reviewboard.mozilla.org/r/120046/#review122530

::: commit-message-39ec9:1
(Diff revision 1)
> +Bug 1332546 - add initialization checking to SiteIdentityPopop

Popop :)
Attachment #8847005 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 22

7 months ago
mozreview-review-reply
Comment on attachment 8847004 [details]
Bug 1332546 - Add CustomView to ActionBar of CustomTabsActivity

https://reviewboard.mozilla.org/r/120044/#review122492

> This looks like a great class for some unit tests.
> 
> Also: Let's file a follow-up bug to use this in BrowserApp too. Fragmenting the logic will bite us as soon as we make changes.

Agree :D

I would like to write a test for this class, however so far I don't have enough clue(spec) to do this. I created bug #1347037 as the follow-up bug. (Maybe title of the bug is not proper one?)

> This URL returns a 404 error. On the right side of MXR you can find a permalink that will always link to the version you are looking at and not change when the repository changes.

404 due to this patch haven't not landed. I think once #1347037 complete, this (for custom tabs) drawble should be removed, so I just use this link for now.

> Isn't this list the same as site_security_level.xml?

Similiar but a bit different. For example, in site_security_level.xml, maxLevel 1 and 2 refer to same drawable. And this file should be removed due to bug #1347037
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

7 months ago
mozreview-review
Comment on attachment 8847006 [details]
Bug 1332546 - Add SiteIdentityPopup to CustomTabsActivity

https://reviewboard.mozilla.org/r/120048/#review123418
Attachment #8847006 - Flags: review?(s.kaspari) → review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 30

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b65a8df84b11
Refactory ActionBar by extracting methods to another class r=sebastian
https://hg.mozilla.org/integration/autoland/rev/b296196d1859
ensure ActionBar color is not translucent r=sebastian
https://hg.mozilla.org/integration/autoland/rev/4796c44dda66
Apply default actionbar color r=sebastian
https://hg.mozilla.org/integration/autoland/rev/540020bee3a6
Add CustomView to ActionBar of CustomTabsActivity r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c9df402008ca
add initialization checking to SiteIdentityPopup r=sebastian
https://hg.mozilla.org/integration/autoland/rev/1edd52437ecb
Add SiteIdentityPopup to CustomTabsActivity r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b65a8df84b11
https://hg.mozilla.org/mozilla-central/rev/b296196d1859
https://hg.mozilla.org/mozilla-central/rev/4796c44dda66
https://hg.mozilla.org/mozilla-central/rev/540020bee3a6
https://hg.mozilla.org/mozilla-central/rev/c9df402008ca
https://hg.mozilla.org/mozilla-central/rev/1edd52437ecb
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Created attachment 8853923 [details]
2017_04_03_14_17_29_.png

Verified as fixed in build (2017-04-02);
Device: Huawei Honor (Android 5.1.1).
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.