Closed Bug 1272340 Opened 8 years ago Closed 8 years ago

about:reader URL is displayed in Reader View

Categories

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

48 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox47 unaffected, firefox48 verified, firefox49 verified, fennec48+)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox47 --- unaffected
firefox48 --- verified
firefox49 --- verified
fennec 48+ ---

People

(Reporter: TeoVermesan, Assigned: sebastian)

Details

(Keywords: regression)

Attachments

(3 files)

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
tracking-fennec: --- → ?
This could be caused by me and bug 1269832.
(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.
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/
Attachment #8752134 - Flags: review?(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.
Attachment #8752134 - Flags: review?(margaret.leibovic) → review+
tracking-fennec: ? → 48+
https://hg.mozilla.org/mozilla-central/rev/4d16f6389b49
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Want to request uplift to aurora 48 since this is marked as a regression from 48?
Flags: needinfo?(s.kaspari)
Attached image Pp2Tds-w.jpg
Verified as fixed using:
Device: ONE A2001 (Android 5.1.1)
Build: Firefox for Android 49.0a1(2016-05-24)
(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]: -
Flags: needinfo?(s.kaspari)
Attachment #8757914 - Flags: approval-mozilla-aurora?
Comment on attachment 8757914 [details] [diff] [review]
1272340_urlbar_AURORA.patch

Regression, taking it
Attachment #8757914 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using:
Device: Nexus 7 (Android 5.1.1)
Build: Firefox for Android 48.0a2(2016-05-31)
Status: RESOLVED → VERIFIED
Version: unspecified → 48 Branch
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.