Closed Bug 948889 Opened 9 years ago Closed 9 years ago

Move inline scripts and styles into separate file for browser/base/content/aboutTabCrashed.xhtml (URL:about:tabcrashed(?))

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: freddy, Assigned: gia)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][qa-])

Attachments

(1 file, 2 obsolete files)

With the current plan to harden the security of Firefox, we want to disallow internal privileged pages to use inline JavaScript. Since their amount is fairly limited, we start this by rewriting them bit by bit.

(This file is not used yet)
Attached patch move_inline_js (obsolete) — Splinter Review
Attachment #8350930 - Flags: review?(ttaubert)
Attachment #8350930 - Flags: review?(gavin.sharp)
Comment on attachment 8350930 [details] [diff] [review]
move_inline_js

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

::: browser/base/content/aboutTabCrashed.js
@@ +1,4 @@
> +/**
> +* 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/. 

Tiny nit: can you please remove the trailing space here?

@@ +5,5 @@
> +*/
> +
> +function parseQueryString() {
> +      let url = document.documentURI;
> +      let queryString = url.replace(/^about:tabcrashed?e=tabcrashed/, "");

Please use two spaces for indentation and fix indentation for the whole file.

::: browser/base/content/aboutTabCrashed.xhtml
@@ +41,5 @@
>        <button id="tryAgain">&tabCrashed.tryAgain;</button>
>      </div>
>    </body>
>  
> +  <script type="text/javascript;version=1.8" src="chrome://browser/base/content/aboutTabCrashed.js"></script>

Must be: chrome://browser/content/aboutTabCrashed.js
Attachment #8350930 - Flags: review?(ttaubert)
Attachment #8350930 - Flags: review?(gavin.sharp)
Attachment #8350930 - Flags: feedback+
Attached patch move_inlinejs_v2 (obsolete) — Splinter Review
Thank you for you review ! Here is the second version. I hope it's ok.
Attachment #8351272 - Flags: review?(ttaubert)
Attachment #8351272 - Attachment is patch: true
Attachment #8350930 - Attachment is obsolete: true
Comment on attachment 8351272 [details] [diff] [review]
move_inlinejs_v2

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

This looks great! r=me with the tiny nit below fixed. You don't need to re-requested review, you can just post a new patch with the fix and mark that as checkin-needed. Thanks!

::: browser/base/content/aboutTabCrashed.xhtml
@@ +41,5 @@
>        <button id="tryAgain">&tabCrashed.tryAgain;</button>
>      </div>
>    </body>
>  
> +  <script type="text/javascript;version=1.8" src="chrome://browser/content/aboutTabCrashed.js"></script>

Nit:

<script type="text/javascript;version=1.8" src="chrome://browser/content/aboutTabCrashed.js"/>
Attachment #8351272 - Flags: review?(ttaubert) → review+
Attached patch move_inlinejs_v3Splinter Review
Attachment #8351272 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8351347 [details] [diff] [review]
move_inlinejs_v3

>Move inline scripts and styles into separate file

For future reference, the commit message should include the bug number and the reviewer as well as the description. I've used:

> Bug 948889 - Move the inline script in aboutTabCrashed.xhtml into a separate file; r=ttaubert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b09a4078912
Assignee: nobody → georgiana.chelu93
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b09a4078912
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.