1511834 - Site Information header in reader mode looks ugly

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P3)

Description

[(no longer active)]

Attachment: Screenshot

See the screenshot, it shows the about:reader?url=foo URL...

Comment 1

[Manish [:manishkk]]

these lines are code related to this bug?
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-siteIdentity.js#763,779

Comment 2

[Tim Nguyen :ntim]

(In reply to Manish [:manishkk] from comment #1)
> these lines are code related to this bug?
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> siteIdentity.js#763,779

Line 763 doesn't seem related, as about:reader does not have a EV certificate (hence it does not match the this._isEV check).

Comment 3

[(no longer active)]

What Tim said!

Comment 4

[Manish [:manishkk]]

Attachment: no ugly header.png

:Ehsan

but same URL working fine for me.

Comment 5

[Tim Nguyen :ntim]

(In reply to Manish [:manishkk] from comment #4)

Created attachment 9035803 [details]
no ugly header.png

:Ehsan

but same URL working fine for me.

You need to click the reader icon in the URL bar (the one that looks like a page).

Comment 6

[Johann Hofmann [:johannh]]

Some more info on how to solve this bug:

This is how the URL bar figures out the right url to display from an about:reader url: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/base/content/urlbarBindings.xml#1065

We should do the same thing in refreshIdentityPopup, e.g. here: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/base/content/browser-siteIdentity.js#745

It would also be great to have a test for this.

Comment 7

[Neha]

I would like to work on this.
Could this bug be assigned to me?
Thanks!

Comment 8

[Johann Hofmann [:johannh]]

Yup, thank you!

Comment 9

[Neha]

Attachment: Bug 1511834 - Improving site Information header in reader mode r=johannh

Comment 10

[Neha]

Hi, Johann,
I have made a patch regarding the site information header issue.
I also saw the files browser_mixed_content_cert_override.js and browser_check_identity_state.js tests where refreshIdentityPopup() is used.
Could you guide me as to where I should write a test for this.
Thanks!

Comment 11

[Johann Hofmann [:johannh]]

browser_check_identity_state.js is pretty close to what you want to do, it loads different sites like example.com and checks their security indicators.

You might want to do the same for some URL like about:reader?url=http://example.com, but then we also would like to verify that identity-popup-mainView-panel-header-span contains the right URL (and doesn't contain about:reader).

Does that make sense?

Thanks!

Comment 12

[Neha]

Hello Johann,
I have made the requested changes.
Please review.

Comment 13

[Johann Hofmann [:johannh]]

Hi Neha, thanks for the great contribution.

Before landing this, it would be great if you could do a Try Push. Try is basically Continuous Integration for Firefox, making sure that tests are passing for your patch.

Getting access is outlined here: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

You will need to file a bug and need someone to vouch for you. I'd be happy to do that if you CC me on that bug.

Afterwards, make sure you perform the steps outlined in https://wiki.mozilla.org/ReleaseEngineering/TryServer#Configuration and (most importantly) in https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/auth.html in order to get access on your local command line.

When you have access, to schedule a try run, you can use the handy ./mach try command. A possible syntax for this bug could be:

./mach try -b do -p win64,linux64,macosx64 -u mochitests -t none

(runs all mochitests on Windows, OSX and Linux)

Let me know if you need any help with that!

Comment 14

[Neha]

Hi Johann,
I got the access and scheduled a try run.
It has completed.
Please review.
Thanks!

Comment 15

[Johann Hofmann [:johannh]]

Great, thanks, but I would need the link to the try run to review it :)

Based on searching treeherder for your email address I think it's this one, right? https://treeherder.mozilla.org/#/jobs?repo=try&revision=214a0e0c93b3c65397e121876edad1376af997a3

That looks great to me, feel free to set checkin-needed now!

Comment 16

[Neha]

Hello Johann,
yes, this is the one I submitted!

Could you guide me as to how to set the checkin-needed option for this?
I went and searched the treeherder(https://treeherder.mozilla.org/#/jobs?repo=try&revision=214a0e0c93b3c65397e121876edad1376af997a3) page, but couldn't figure out how to set it.

Thanks!

Comment 17

[Johann Hofmann [:johannh]]

You can do it in this bug, just select "Edit Bug" and modify the "keywords" field to include "checkin-needed" (it should autocomplete it for you).

Comment 18

[Neha]

Thanks!
I have made the required changes.
Please review.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a14eec92d6e
Improving site Information header in reader mode r=johannh

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5a14eec92d6e