Create the about:reader page

VERIFIED FIXED in Firefox 37

Status

()

defect
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: jaws, Assigned: Margaret)

Tracking

(Blocks 1 bug)

Trunk
Firefox 37
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox37 verified, firefox38 verified)

Details

Attachments

(2 attachments, 8 obsolete attachments)

This bug is to set up the about handler for about:reader. The page should be a blank HTML page for now and should not have chrome privileges.

To do this, you'll need to add to /browser/components/about/AboutRedirector.cpp and /browser/components/build/nsModule.cpp, as well as add a new (for now) blank HTML page that will be loaded from chrome://browser/content/reader/reader.xhtml.

You will need to set URI_SAFE_FOR_UNTRUSTED_CONTENT and ALLOW_SCRIPT for the page. See https://mxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#26 and https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIAboutModule for documentation on these two flags.
Status: NEW → ASSIGNED
Actually, chrome://browser/content/aboutReader.xhtml should be sufficient.
Bah, I don't think comment #1 was clear.

Use chrome://browser/content/aboutReader.xhtml instead of chrome://browser/content/reader/reader.xhtml.
Attachment #667288 - Flags: review?(lucasr.at.mozilla)
Attachment #667288 - Flags: review?(jaws)
Comment on attachment 667288 [details] [diff] [review]
Added blank html page for about:reader page

Review of attachment 667288 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I'm forwarding the review request to Felipe just to get another set of eyes on this change.

::: browser/base/content/aboutReader.xhtml
@@ +12,5 @@
> +  <p>Coming Soon!</p>
> +  </body>
> +
> +
> +</html>
\ No newline at end of file

The html here should just be,
<html xmlns="http://www.w3.org/1999/xhtml">
  <head>
    <title/>
  </head>
  <body/>
</html>
based on http://mathiasbynens.be/notes/minimal-html and also because any human-readable text should be in a localizable file. We'll get to that later, but for now we shouldn't have any user-facing text hardcoded in one language.
Attachment #667288 - Flags: review?(lucasr.at.mozilla)
Attachment #667288 - Flags: review?(jaws)
Attachment #667288 - Flags: review?(felipc)
Attachment #667288 - Flags: feedback+
Comment on attachment 667288 [details] [diff] [review]
Added blank html page for about:reader page

Review of attachment 667288 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, only needs to address Jared's feedback.
Attachment #667288 - Flags: review?(felipc) → feedback+
I updated the html to reflect the previous comments
Attachment #667732 - Flags: review?(jaws)
Comment on attachment 667732 [details] [diff] [review]
Added blank html page for about:reader page

Review of attachment 667732 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/aboutReader.xhtml
@@ +1,1 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">

We need the license header and XML version/encoding added back, just like how http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutRobots.xhtml#1 has it.

@@ +2,5 @@
> +  <head>
> +    <title/>
> +  </head>
> +  <body/>
> +</html>
\ No newline at end of file

There should be a newline at the end of the file too.
Attachment #667732 - Flags: review?(jaws) → review-
Added back the license information and added a newline at the end
Attachment #667288 - Attachment is obsolete: true
Attachment #667732 - Attachment is obsolete: true
Attachment #667747 - Flags: review?(jaws)
Matt, please go through these files (specifically aboutReader.xhtml) and remove any carriage returns that were added. These files should have Linux line endings (LF only).
I believe I got rid of all the carriage returns. Let me know if I missed anything else.
Attachment #667747 - Attachment is obsolete: true
Attachment #667747 - Flags: review?(jaws)
Attachment #667757 - Flags: review?(jaws)
Comment on attachment 667757 [details] [diff] [review]
Added blank html page for about:reader page

Review of attachment 667757 [details] [diff] [review]:
-----------------------------------------------------------------

The aboutReader.xhtml should actually be an "html" (instead of xhtml) file instead as it will contain arbitrary HTML code parsed from pages.
I changed the aboutReader.xhtml to aboutReader.html
Attachment #667995 - Flags: review?(lucasr.at.mozilla)
Attachment #667995 - Flags: review?(jaws)
Attachment #667757 - Attachment is obsolete: true
Attachment #667757 - Flags: review?(jaws)
Attachment #667995 - Attachment is patch: true
Comment on attachment 667995 [details] [diff] [review]
Added blank html page for about:reader page

Review of attachment 667995 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but let's hold off on landing it until Monday/Tuesday of next week since it won't have any benefit for Firefox 18. See https://wiki.mozilla.org/RapidRelease/Calendar for the calendar of the Firefox merges for more information on this.

::: browser/base/content/aboutReader.html
@@ +2,5 @@
> +
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +    - License, v. 2.0. If a copy of the MPL was not distributed with this
> +    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +    

nit: Remove these whitespace characters on this line.

::: browser/components/about/AboutRedirector.cpp
@@ +78,5 @@
>    { "preferences", "chrome://browser/content/preferences/in-content/preferences.xul",
>      nsIAboutModule::ALLOW_SCRIPT },
> +  { "reader", "chrome://browser/content/aboutReader.xhtml",
> +    nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
> +    nsIAboutModule::ALLOW_SCRIPT }

nit: Include a comma after the closing curly bracket here, like the one above.
Attachment #667995 - Flags: review?(lucasr.at.mozilla)
Attachment #667995 - Flags: review?(jaws)
Attachment #667995 - Flags: review+
Fixed the trailing whitespace and missing comma
Attachment #667995 - Attachment is obsolete: true
Attachment #668306 - Attachment description: Added blank html page for about:reader page → Added blank html page for about:reader page (ready for checkin on Fx19)
I realized that I was still referencing the aboutReader page as a .xhtml in the jar.mn and AboutRedirector.cpp
Attachment #668306 - Attachment is obsolete: true
Matt, would you be interested in updating your patch here to get it landed? Once I finish bug 793920, we'll just need a simple patch that adds the AboutRedirector.cpp and nsModule.cpp bits of your patch.
Flags: needinfo?(vorce67)
Attachment #8537523 - Flags: review?(jaws)
/r/1527 - Bug 795968 - Create about:reader page

Pull down this commit:

hg pull review -r 5c4ec69301d1b0fa78ab118398afc03f80b7095e
Assignee: vorce67 → margaret.leibovic
Flags: needinfo?(vorce67)
Attachment #670429 - Attachment is obsolete: true
FYI, I just modeled this after the same parameters we use for Fennec:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js?force=1#62

This will work now that I landed bug 793920.
Attachment #8537523 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/86b156ead17e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Posted image reader.jpg
Using an hourly build with this patch, when I enter:  about:reader I get the attachment image.  What is that supposed to be, or tell me ?
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #23)
> Created attachment 8539206 [details]
> reader.jpg
> 
> Using an hourly build with this patch, when I enter:  about:reader I get the
> attachment image.  What is that supposed to be, or tell me ?

That is expected. I haven't hooked up the plumbing yet to inject JS into this page, which I'll do in bug 1111142.

However, just loading "about:reader" will never do anything useful, since it looks for a URL parameter to parse (this is the case with the implementation that's been shipping on Firefox for Android). The expected use case is that we'll open this page programmatically with the correct parameter when a user selects something in the UI (such as a "reader mode" button).

However, this report makes me think we should file a follow-up bug figure out something useful to show if a user just types "about:reader", since that may happen more often on desktop, where it's easier to type.
Blocks: 1113795
Verified fixed on Nightly 38.0a1 (2015-02-18) and Aurora 37.0a2 (2015-02-18), using Ubuntu 14.04 (x64), Windows 8.1 (x86) and Mac OS X 10.9.5. A blank page is displayed on m-c, along with the controls sidebar and the "Loading..." message, while on m-a the interface is as described in Comment 23 & Comment 24.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Attachment #8537523 - Attachment is obsolete: true
Attachment #8617999 - Flags: review+
You need to log in before you can comment on or make changes to this bug.