Closed Bug 795968 Opened 12 years ago Closed 10 years ago

Create the about:reader page

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 37
Tracking Flags:
Tracking Status
firefox37 --- verified
firefox38 --- verified

People

(Reporter: jaws, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

reader.jpg
33.90 KB, image/jpeg
Details
MozReview Request: Bug 795968 - Create about:reader page
39 bytes, text/x-review-board-request
jaws
: review+
Details 
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
Blocks: 795981
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37

    
    

    
  

      
      
      
      

        Attached 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

          status-firefox37:
          --- → verified

          status-firefox38:
          --- → 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: