Closed Bug 948883 Opened 11 years ago Closed 10 years ago

Move inline scripts and styles into separate file for toolkit/content/aboutSupport.xhtml (URL=about:support)

Tracking

()

Status:

RESOLVED FIXED

Milestone:

Firefox 31

People

(Reporter: freddy, Assigned: greenm9)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 5 obsolete files)

hg version of proposed git patch 10 years ago Michael Green 4.71 KB, patch		Details \| Diff \| Splinter Review
Proposed 948883.patch 10 years ago Michael Green 7.04 KB, patch	freddy : feedback+	Details \| Diff \| Splinter Review
patch-948883.patch 10 years ago Michael Green 7.03 KB, patch	Unfocused : review- greenm9 : feedback+	Details \| Diff \| Splinter Review
patch-948883.patch 10 years ago Michael Green 3.87 KB, patch	Unfocused : feedback-	Details \| Diff \| Splinter Review
patch-948883.patch 10 years ago Michael Green 7.28 KB, patch	Unfocused : review+	Details \| Diff \| Splinter Review
patch-948883.patch 10 years ago Michael Green 7.05 KB, patch	greenm9 : review+	Details \| Diff \| Splinter Review

Frederik Braun [:freddy]

Reporter

Description

•

11 years ago

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.

Georgiana [:gia]

Comment 1

•

11 years ago

I think the inline scripts for toolkit/content/aboutSupport.xhtml had already moved into 
toolkit/content/aboutSupport.js .

Georgiana [:gia]

Updated

•

10 years ago

Keywords: checkin-needed

Ryan VanderMeulen [:RyanVM]

Comment 2

•

10 years ago

checkin-needed means that there's a patch that needs landing.

Keywords: checkin-needed

Tim Taubert [:ttaubert] (inactive)

Comment 3

•

10 years ago

(In reply to Georgiana [:gia] from comment #1)
> I think the inline scripts for toolkit/content/aboutSupport.xhtml had
> already moved into 
> toolkit/content/aboutSupport.js .

Yes, it seems that we don't need to fix anything about this file. Thank you for checking, Georgiana!

Status: NEW → RESOLVED

Closed: 10 years ago

Resolution: --- → INVALID

Frederik Braun [:freddy]

Reporter

Comment 4

•

10 years ago

I think we're overlooking something here ;)
I see a lot of onclick and style attributes that should be merged into aboutSupport.js and aboutSupport.css respectively.

Status: RESOLVED → REOPENED

Resolution: INVALID → ---

Tim Taubert [:ttaubert] (inactive)

Comment 5

•

10 years ago

Oh, yes. Sorry I forgot about those.

Anup Kumar [:reznord]

Comment 6

•

10 years ago

Hi, I am interested in working on this bug. So can you please assign this to me?

Thanks in advance,

Ekanan Ketunuti

Updated

•

10 years ago

Assignee: nobody → allamsetty.anup

Anup Kumar [:reznord]

Updated

•

10 years ago

Assignee: allamsetty.anup → nobody

Michael Green

Assignee

Comment 7

•

10 years ago

Hi, I am interested in working on this bug as part of a university project. If possible can it be assigned to me? Thankyou

Georgiana [:gia]

Updated

•

10 years ago

Assignee: nobody → greenm9

Michael Green

Assignee

Comment 8

•

10 years ago

How would I go about debugging the changes I have made?

Status: REOPENED → ASSIGNED

Flags: needinfo?

Michael Green

Assignee

Comment 9

•

10 years ago

Wondering if I can have any feedback on my progress so far, not sure on how to test? https://github.com/mozilla/gecko-dev/pull/6

Flags: needinfo?(fbraun)

Michael Green

Assignee

Comment 10

•

10 years ago

Attached patch hg version of proposed git patch (obsolete) — Details — Splinter Review

Attachment #8372196 - Flags: review+

Frederik Braun [:freddy]

Reporter

Comment 11

•

10 years ago

Comment on attachment 8372196 [details] [diff] [review]
hg version of proposed git patch

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

Thanks for this patch, it looks like a good first try!
Please address these comments and flag me with "feedback?" again.

::: toolkit/content/aboutSupport.js
@@ +605,5 @@
> +var ShowUpdateHistory-button = document.getElementById("ShowUpdateHistory-button");
> +ShowUpdateHistory-button.addEventListener("click",showUpdateHistory());
> +
> +var ProfileDir-button = document.getElementById("ProfileDir-button");
> +ProfileDir-button.addEventListener("click",openProfileDirectory());
\ No newline at end of file

Most of your introduced variable names are invalid to JavaScript. Please look through our Coding Guidelines: https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style

::: toolkit/themes/windows/global/aboutSupport.css
@@ +118,5 @@
> +}
> +
> +#prefs-user-js-section{
> +  display:none;
> +}

Your CSS patch should work, but you could consider creating two classes (one with display:block and one with display:none). And assign those to the elements instead of unique IDs.

Attachment #8372196 - Flags: review+

Frederik Braun [:freddy]

Reporter

Updated

•

10 years ago

Flags: needinfo?(fbraun)

Flags: needinfo?

Florian Bender

Comment 12

•

10 years ago

(In reply to Michael Green from comment #9)
> Wondering if I can have any feedback on my progress so far, not sure on how
> to test? https://github.com/mozilla/gecko-dev/pull/6

That repository is read-only, any pull request will be ignored.

Michael Green

Assignee

Comment 13

•

10 years ago

Attached patch Proposed 948883.patch (obsolete) — Details — Splinter Review

Attachment #8372196 - Attachment is obsolete: true

Attachment #8384641 - Flags: feedback?(fbraun)

Frederik Braun [:freddy]

Reporter

Comment 14

•

10 years ago

Comment on attachment 8384641 [details] [diff] [review]
Proposed 948883.patch

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

Looks OK. I'll give it a test later on.

::: toolkit/content/aboutSupport.xhtml
@@ +28,5 @@
>    </head>
>  
>    <body dir="&locale.dir;">
>  
> +    <div id="reset-box">

You have introduced a CSS class "hidden", I think you want to use it here.

Attachment #8384641 - Flags: feedback?(fbraun) → feedback+

Frederik Braun [:freddy]

Reporter

Comment 15

•

10 years ago

Comment on attachment 8384641 [details] [diff] [review]
Proposed 948883.patch

Looks good from here. Let's get a Toolkit module peer to review!

Attachment #8384641 - Flags: review?(bmcbride)

Michael Green

Assignee

Comment 16

•

10 years ago

I have added the CSS class I missed, should I add another attachment?

Frederik Braun [:freddy]

Reporter

Comment 17

•

10 years ago

Ah, right I forgot that part. Please obsolete the previous patch, carry over my "feedback+" rating and ask Blair for a review on the new patch.
Thanks! :)

Michael Green

Assignee

Comment 18

•

10 years ago

Attached patch patch-948883.patch (obsolete) — Details — Splinter Review

Attachment #8384641 - Attachment is obsolete: true

Attachment #8384641 - Flags: review?(bmcbride)

Attachment #8386776 - Flags: review?(bmcbride)

Attachment #8386776 - Flags: feedback+

Blair McBride [:Unfocused] (UNAVAILABLE)

Comment 19

•

10 years ago

Comment on attachment 8386776 [details] [diff] [review]
patch-948883.patch

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

Almost there

::: toolkit/content/aboutSupport.js
@@ +589,5 @@
> +/**
> + * Set up event listeners for buttons.
> + */
> +function setupEventListeners(){
> +  document.getElementById("show-update-history-button").onclick = function(event) {

Always use addEventListener() to handle events.

Also, you can shorten these lines a bit. This file defines a $ function, which is just document.getElementById(). So you can do:
  $("show-update-history-button")

@@ +591,5 @@
> + */
> +function setupEventListeners(){
> +  document.getElementById("show-update-history-button").onclick = function(event) {
> +	  var prompter = Cc["@mozilla.org/updates/update-prompt;1"].createInstance(Ci.nsIUpdatePrompt);
> +	  prompter.showUpdateHistory(window);

Mozilla code always uses 2 spaces for indentation - you're used a tab characters here.

@@ +600,5 @@
> +  document.getElementById("copy-raw-data-to-clipboard").onclick = function(event){
> +	copyRawDataToClipboard(this);
> +  }
> +  document.getElementById("copy-to-clipboard").onclick = function(event){
> +	copyContentsToClipboard();

For cases like this, you can simplify by just registering this function as the event listener:

  document.getElementById("copy-to-clipboard").addEventListener("click", copyContentsToClipboard);

::: toolkit/content/aboutSupport.xhtml
@@ +26,5 @@
>      <script type="application/javascript;version=1.7"
>              src="chrome://global/content/resetProfile.js"/>
>    </head>
>    <body dir="&locale.dir;">
> +    <div id="reset-box class="hide">

Typo here - missing the end quote at the end of the id attribute value. 

But also, this shouldn't have the "hide" class. The original inline style never had display:none, so adding it here is changing the behaviour.

::: toolkit/themes/windows/global/aboutSupport.css
@@ +102,5 @@
>    margin: auto;
>  }
> +
> +.block {
> +display:block;

Indent the contents of rules by 2 spaces.

@@ +105,5 @@
> +.block {
> +display:block;
> +}
> +
> +.hide {

Convention is to name this class "hidden".

Attachment #8386776 - Flags: review?(bmcbride) → review-

Michael Green

Assignee

Comment 20

•

10 years ago

Attached patch patch-948883.patch (obsolete) — Details — Splinter Review

I have made the changes that were mentioned by Blair.

Michael

Attachment #8386776 - Attachment is obsolete: true

Attachment #8390055 - Flags: feedback?(fbraun)

Attachment #8390055 - Flags: feedback?(bmcbride)

Blair McBride [:Unfocused] (UNAVAILABLE)

Comment 21

•

10 years ago

Comment on attachment 8390055 [details] [diff] [review]
patch-948883.patch

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

Can you post the full diff? This is changes made on top of your other changes - what we ideally want is changes to a clean tree.

The reason being is that it's hard to review just a diff of a diff - generally reviewers want each revision to be a complete standalone diff, to see the whole picture. We have tools to get an interdiff (diff between two revisions of a patch) when it's useful.

Attachment #8390055 - Flags: feedback?(fbraun)

Attachment #8390055 - Flags: feedback?(bmcbride)

Attachment #8390055 - Flags: feedback-

Michael Green

Assignee

Comment 22

•

10 years ago

Attached patch patch-948883.patch (obsolete) — Details — Splinter Review

Changes against the clean tree,

Michael

Attachment #8390055 - Attachment is obsolete: true

Attachment #8390468 - Flags: feedback?(bmcbride)

Blair McBride [:Unfocused] (UNAVAILABLE)

Comment 23

•

10 years ago

Comment on attachment 8390468 [details] [diff] [review]
patch-948883.patch

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

Awesome :) Good to go!

Attachment #8390468 - Flags: feedback?(bmcbride) → review+

Ekanan Ketunuti

Updated

•

10 years ago

Keywords: checkin-needed

Ryan VanderMeulen [:RyanVM]

Comment 24

•

10 years ago

Doesn't apply to fx-team.

Keywords: checkin-needed

Michael Green

Assignee

Comment 25

•

10 years ago

Hi, just wondering what the above comments means? Are any changes needed? 
Thanks! :)

Ekanan Ketunuti

Comment 26

•

10 years ago

(In reply to Michael Green from comment #25)
> Hi, just wondering what the above comments means? Are any changes needed? 
> Thanks! :)

This patch doesn't apply cleanly against current fx-team tip (https://hg.mozilla.org/integration/fx-team/) 
i guess because this patch has no newline at end of file, but current fx-team has newline at eof.

-------
$ hg qpush patch-948883.patch

applying patch-948883.patch
patching file toolkit/content/aboutSupport.js
Hunk #2 FAILED at 574
1 out of 2 hunks FAILED -- saving rejects to file toolkit/content/aboutSupport.js.rej
patching file toolkit/content/aboutSupport.xhtml
Hunk #6 FAILED at 314
1 out of 6 hunks FAILED -- saving rejects to file toolkit/content/aboutSupport.xhtml.rej
patching file toolkit/themes/windows/global/aboutSupport.css
Hunk #1 FAILED at 83
1 out of 1 hunks FAILED -- saving rejects to file toolkit/themes/windows/global/aboutSupport.css.rej
patch failed to apply
toolkit/content/aboutSupport.js
toolkit/content/aboutSupport.xhtml
patch failed, rejects left in working dir
errors during apply, please fix and refresh patch-948883.patch
-------

Please post a patch based on current fx-team tree and re-request checkin.

Michael Green

Assignee

Comment 27

•

10 years ago

Attached patch patch-948883.patch — Details — Splinter Review

Attachment #8390468 - Attachment is obsolete: true

Attachment #8395321 - Flags: review+

Attachment #8395321 - Flags: checkin?(ananuti)

Ekanan Ketunuti

Comment 28

•

10 years ago

Thank you, Michael. Tree sheriff will get to this soon.
Try run for reference: https://tbpl.mozilla.org/?tree=Try&rev=c7af18348ce3

Keywords: checkin-needed

Ryan VanderMeulen [:RyanVM]

Updated

•

10 years ago

Attachment #8395321 - Flags: checkin?(ananuti)

Ryan VanderMeulen [:RyanVM]

Comment 29

•

10 years ago

https://hg.mozilla.org/integration/fx-team/rev/12b62c93d740

Keywords: checkin-needed

Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][fixed-in-fx-team]

Ryan VanderMeulen [:RyanVM]

Comment 30

•

10 years ago

https://hg.mozilla.org/mozilla-central/rev/12b62c93d740

Status: ASSIGNED → RESOLVED

Closed: 10 years ago → 10 years ago

Resolution: --- → FIXED

Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][fixed-in-fx-team] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js]

Target Milestone: --- → Firefox 31

You need to log in before you can comment on or make changes to this bug.