1272340 - about:reader URL is displayed in Reader View

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce:
1. Go to news.google.com
2. Choose an article and enter reader view

Expected results:
- The main domain (origin) for long URLs should be displayed in URL Bar

Actual results:
- "about:reader?url=https%.." is displayed

Note;
regression:
10-05 not affected
11-05 affected

pushlog: 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=043082cb7bd8490c60815f67fbd1f33323ad7663&tochange=674a552743785c28c75866969aad513bd8eaf6ae

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

This could be caused by me and bug 1269832.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Yep, this should be 'strippedURL':
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#294

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> Yep, this should be 'strippedURL':
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/toolbar/ToolbarDisplayLayout.java#294

Actually there's no reason for the last else block - introduced in bug 1269832: updateAndColorTitleFromFullURL() handles cases with/without base domain and different protocols just well.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1272340 - ToolbarDisplayLayout: Handle about:reader URLs. r=margaret

This patch changes two things:
* Check if the URL is http/https after stripping the about:reader URL.
* Always call updateAndColorTitleFromFullURL() as fallback for URL formatting (like in previous versions)

Review commit: https://reviewboard.mozilla.org/r/52431/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52431/

Comment 5

[:Margaret Leibovic]

Comment on attachment 8752134 [details]
MozReview Request: Bug 1272340 - ToolbarDisplayLayout: Handle about:reader URLs. r=margaret

https://reviewboard.mozilla.org/r/52431/#review49886

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:272
(Diff revision 1)
> -        final boolean isHttpOrHttps = StringUtils.isHttpOrHttps(url);
>          final String baseDomain = tab.getBaseDomain();
>  
>          String strippedURL = stripAboutReaderURL(url);
>  
> +        final boolean isHttpOrHttps = StringUtils.isHttpOrHttps(strippedURL);

This is kinda weird because technically the connection is always secure on a reader view page because the content is hosted in the browser. But I guess this does indicate whether or not the original content was downloaded over http vs. https, so it makes sense to display it this way.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/4d16f6389b493807621e5359c4a07b909f2c749f
Bug 1272340 - ToolbarDisplayLayout: Handle about:reader URLs. r=margaret

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4d16f6389b49

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to request uplift to aurora 48 since this is marked as a regression from 48?

Comment 9

[Teodora Vermesan (:TeoVermesan)]

Attachment: Pp2Tds-w.jpg

Verified as fixed using:
Device: ONE A2001 (Android 5.1.1)
Build: Firefox for Android 49.0a1(2016-05-24)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1272340_urlbar_AURORA.patch

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #8)
> Want to request uplift to aurora 48 since this is marked as a regression
> from 48?

Yeah, this is the updated patch for Aurora.


Approval Request Comment
[Feature/regressing bug #]: Regression introduced in bug 1269832.
[User impact if declined]: about:reader is shown in URL bar.
[Describe test coverage new/current, TreeHerder]: Manual testing + Patch is in Nightly for a week now.
[Risks and why]: Low - patch is tested in Nightly and it's using the code that was in use on tablets already.
[String/UUID change made/needed]: -

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8757914 [details] [diff] [review]
1272340_urlbar_AURORA.patch

Regression, taking it

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/be0703a058c6

Comment 13

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using:
Device: Nexus 7 (Android 5.1.1)
Build: Firefox for Android 48.0a2(2016-05-31)