Closed
Bug 795968
Opened 11 years ago
Closed 9 years ago
Create the about:reader page
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 37
People
(Reporter: jaws, Assigned: Margaret)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 8 obsolete files)
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.
Reporter | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•11 years ago
|
||
Actually, chrome://browser/content/aboutReader.xhtml should be sufficient.
Reporter | ||
Comment 2•11 years ago
|
||
Bah, I don't think comment #1 was clear. Use chrome://browser/content/aboutReader.xhtml instead of chrome://browser/content/reader/reader.xhtml.
Comment 3•11 years ago
|
||
Attachment #667288 -
Flags: review?(lucasr.at.mozilla)
Attachment #667288 -
Flags: review?(jaws)
Reporter | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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+
Comment 6•11 years ago
|
||
I updated the html to reflect the previous comments
Attachment #667732 -
Flags: review?(jaws)
Reporter | ||
Comment 7•11 years ago
|
||
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-
Comment 8•11 years ago
|
||
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)
Reporter | ||
Comment 9•11 years ago
|
||
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).
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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.
Comment 12•11 years ago
|
||
I changed the aboutReader.xhtml to aboutReader.html
Attachment #667995 -
Flags: review?(lucasr.at.mozilla)
Attachment #667995 -
Flags: review?(jaws)
Reporter | ||
Updated•11 years ago
|
Attachment #667757 -
Attachment is obsolete: true
Attachment #667757 -
Flags: review?(jaws)
Reporter | ||
Updated•11 years ago
|
Attachment #667995 -
Attachment is patch: true
Reporter | ||
Comment 13•11 years ago
|
||
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+
Comment 14•11 years ago
|
||
Fixed the trailing whitespace and missing comma
Attachment #667995 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
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)
Comment 15•11 years ago
|
||
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
Assignee | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8537523 -
Flags: review?(jaws)
Assignee | ||
Comment 18•9 years ago
|
||
/r/1527 - Bug 795968 - Create about:reader page Pull down this commit: hg pull review -r 5c4ec69301d1b0fa78ab118398afc03f80b7095e
Assignee | ||
Updated•9 years ago
|
Assignee: vorce67 → margaret.leibovic
Flags: needinfo?(vorce67)
Assignee | ||
Updated•9 years ago
|
Attachment #670429 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
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.
Reporter | ||
Updated•9 years ago
|
Attachment #8537523 -
Flags: review?(jaws) → review+
Reporter | ||
Comment 20•9 years ago
|
||
https://reviewboard.mozilla.org/r/1525/#review983 Ship It!
Assignee | ||
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/86b156ead17e
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/86b156ead17e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment 23•9 years ago
|
||
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 ?
Assignee | ||
Comment 24•9 years ago
|
||
(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.
Comment 25•8 years ago
|
||
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
status-firefox37:
--- → verified
status-firefox38:
--- → verified
Flags: qe-verify+
Assignee | ||
Comment 26•8 years ago
|
||
Attachment #8537523 -
Attachment is obsolete: true
Attachment #8617999 -
Flags: review+
Assignee | ||
Comment 27•8 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•