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)
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)
58 bytes,
text/x-review-board-request
|
Margaret
:
review+
|
Details |
202.74 KB,
image/jpeg
|
Details | |
3.13 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•8 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 1•8 years ago
|
||
This could be caused by me and bug 1269832.
Assignee | ||
Comment 2•8 years ago
|
||
Yep, this should be 'strippedURL': https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#294
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
(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.
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4d16f6389b493807621e5359c4a07b909f2c749f Bug 1272340 - ToolbarDisplayLayout: Handle about:reader URLs. r=margaret
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4d16f6389b49
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 8•8 years ago
|
||
Want to request uplift to aurora 48 since this is marked as a regression from 48?
Flags: needinfo?(s.kaspari)
Reporter | ||
Comment 9•8 years ago
|
||
Verified as fixed using: Device: ONE A2001 (Android 5.1.1) Build: Firefox for Android 49.0a1(2016-05-24)
Reporter | ||
Updated•8 years ago
|
Assignee | ||
Comment 10•8 years ago
|
||
(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 11•8 years ago
|
||
Comment on attachment 8757914 [details] [diff] [review] 1272340_urlbar_AURORA.patch Regression, taking it
Attachment #8757914 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 12•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/be0703a058c6
Reporter | ||
Comment 13•8 years ago
|
||
Verified as fixed using: Device: Nexus 7 (Android 5.1.1) Build: Firefox for Android 48.0a2(2016-05-31)
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Version: unspecified → 48 Branch
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•