Closed Bug 948896 Opened 6 years ago Closed 6 years ago

Move inline scripts and styles into separate file for mobile/android/chrome/content/aboutDownloads.xhtml (about:downloads)

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: freddyb, Assigned: j.l.madushan)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 3 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.
I like to work on this bug, But I can't understand clearly what I have to do. It seems all the javascript codes are already in a file called aboutDownloads.js.
Flags: needinfo?(fbraun)
Thank you for taking a look. 
The term "inline scripts" also covers event handlers. In this case, only this one instance of "onload" has to be placed in the external script file.
Flags: needinfo?(fbraun)
Can someone help me to move foreword? I removed 'onload' and 'onunload' handlers from this line

<body dir="&locale.dir;" onload="Downloads.init();" onunload="Downloads.uninit();"> 

and added this two lines to js file. But when I go to downloads the download list is still hidden. What am I doing wrong? 

document.addEventListener("load", Downloads.init, true);
window.addEventListener("unload", Downloads.uninit, false);
(In reply to Madushan Nishantha from comment #3)
> document.addEventListener("load", Downloads.init, true);
> window.addEventListener("unload", Downloads.uninit, false);

Do you see any errors in logcat (run 'adb logcat | grep Gecko' on your desktop with your phone attached)? The above code will cause the function Downloads.init to be run on load, but it will be run in the global context. Instead you could try something like:

> window.addEventListener("DOMContentLoaded", Downloads.init.bind(Downloads), true);
> window.addEventListener("unload", Downloads.uninit.bind(Downloads), false);
There are lots of errors, But I think the one related to this is this,

E/GeckoConsole( 7094): [JavaScript Error: "TypeError: Downloads is undefined" {file: "chrome://browser/content/aboutDownloads.js" line: 41}]

But it's defined in the file as

let Downloads = {
  init: function dl_init() {.......
Can you upload a WIP (work in progress) patch for us to look at? It sounds like you have a JS syntax error that's causing the file to not be loaded properly.
Attached patch bug948896-wip.patch (obsolete) — Splinter Review
Things started working after making changes :wesj suggested. Can someone take a look at it and tell if any canges needed to be made?
Comment on attachment 8364911 [details] [diff] [review]
bug948896-wip.patch

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

::: mobile/android/chrome/content/aboutDownloads.js
@@ +599,5 @@
>      return this;
>    }
>  }
> +
> +window.addEventListener("DOMContentLoaded", Downloads.init.bind(Downloads), true);

The patch looks good but it omits moving the onclick event handlers.
Can you put the DOMContentLoaded event handler in a function expression that also adds click handlers to the respective elements?
Attachment #8364911 - Flags: feedback+
The original inline handler doesn't have an onclick event handler. I can't understand what exactly I have to do. Can you explain it a little more?
And can someone assign me to this bug? :)
Assignee: nobody → j.l.madushan
According to http://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutDownloads.xhtml#29, there are quite a few onclick attributes that need to be moved in the JS file.
I tried to move them to a function like this. But it's not working. What's wrong?

window.addEventListener("DOMContentLoaded", handle_events, true);
window.addEventListener("unload", Downloads.uninit.bind(Downloads), false);

function handle_events() {
	window.addEventListener("DOMContentLoaded", Downloads.init.bind(Downloads), true);
	document.getElementById("contextmenu-open").addEventListener("click", ContextMenus.open, false);
	document.getElementById("contextmenu-retry").addEventListener("click", ContextMenus.retry, false);
	document.getElementById("contextmenu-remove").addEventListener("click", ContextMenus.remove, false);
	document.getElementById("contextmenu-pause").addEventListener("click", ContextMenus.pause, false);
	document.getElementById("contextmenu-resume").addEventListener("click", ContextMenus.resume, false);
	document.getElementById("contextmenu-cancel").addEventListener("click", ContextMenus.cancel, false);
	document.getElementById("contextmenu-removeall").addEventListener("click", ContextMenus.removeAll, false);
	}
Attached patch bug948896.patch (obsolete) — Splinter Review
This is the full patch for this bug. Built and tested. Can someone take a look at it and submit to the source?
Attachment #8364911 - Attachment is obsolete: true
Attachment #8373153 - Flags: review?(fbraun)
Comment on attachment 8373153 [details] [diff] [review]
bug948896.patch

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

This looks good.
Let's get this another review from Margaret.
Attachment #8373153 - Flags: review?(margaret.leibovic)
Attachment #8373153 - Flags: review?(fbraun)
Attachment #8373153 - Flags: feedback+
Comment on attachment 8373153 [details] [diff] [review]
bug948896.patch

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

This is on the right track, but I have some suggestions to make this simpler.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +599,5 @@
>      return this;
>    }
>  }
> +
> +window.addEventListener("DOMContentLoaded", function() {handle_events();}, true);

Add the DOMContentLoaded listener on the document, not the window. Also, instead of creating this handle_events function, you should just specify Downloads.init as the callback here.

@@ +600,5 @@
>    }
>  }
> +
> +window.addEventListener("DOMContentLoaded", function() {handle_events();}, true);
> +window.addEventListener("unload", function() {Downloads.uninit();}, false);

Instead of creating this anonymous function, you should be able to just specify Downloads.uninit.bind(Downloads) as the callback function.

@@ +603,5 @@
> +window.addEventListener("DOMContentLoaded", function() {handle_events();}, true);
> +window.addEventListener("unload", function() {Downloads.uninit();}, false);
> +
> +function handle_events(){
> +	Downloads.init();

2-space indentation, please.

@@ +610,5 @@
> +	document.getElementById("contextmenu-remove").addEventListener("click", function() {ContextMenus.remove();}, false);
> +	document.getElementById("contextmenu-pause").addEventListener("click", function() {ContextMenus.pause();}, false);
> +	document.getElementById("contextmenu-resume").addEventListener("click", function() {ContextMenus.resume();}, false);
> +	document.getElementById("contextmenu-cancel").addEventListener("click", function() {ContextMenus.cancel();}, false);
> +	document.getElementById("contextmenu-removeall").addEventListener("click", function() {ContextMenus.removeAll();}, false);

You could just put these addEventListener calls inside ContextMenus.init, and then you can directly reference the callback functions without the anonymous function wrappers, like this:

document.getElementById("contextmenu-open").addEventListener("click", this.open.bind(this), false);

To avoid all these listeners, I feel like a better solution would be to just add one click listener to the "downloadmenu" element, then have one click handler that chooses the correct action based on the target. However, about:downloads isn't used very frequently, so it's probably not worth optimizing.
Attachment #8373153 - Flags: review?(margaret.leibovic) → review-
Attached patch bug948896.patch (obsolete) — Splinter Review
I've done the changes margaret.leibovic suggested. But I can't understand how to do the last one. The optimizing thing. 
And what does actually "Downloads.init.bind(Downloads)" statement do?
Attachment #8373153 - Attachment is obsolete: true
Attachment #8374616 - Flags: review?(margaret.leibovic)
(In reply to Madushan Nishantha from comment #16)

Thanks for updating the patch!

> Created attachment 8374616 [details] [diff] [review]
> bug948896.patch
> 
> I've done the changes margaret.leibovic suggested. But I can't understand
> how to do the last one. The optimizing thing. 

Basically, I'm suggesting that we only have one click listener for the parent element, as opposed to many for all the children. But it's okay, we don't need to worry about actually implementing that :)

> And what does actually "Downloads.init.bind(Downloads)" statement do?

The bind makes sure the `this` keyword refers to the Downloads object when Downloads.init is called here. This is important because the body of Downloads.init expects this to be the case. For more info on .bind() you can read docs here:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind
(In reply to :Margaret Leibovic from comment #17)
Thanks for the explanation :) Can you take a look at the patch and see if anything is wrong?..
Comment on attachment 8374616 [details] [diff] [review]
bug948896.patch

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

This looks great, thanks! Once you post a new patch that addresses my one comment, you can set the "checkin-needed" keyword on this bug.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +44,5 @@
>  
>    init: function() {
>      document.addEventListener("contextmenu", this, false);
> +    document.getElementById("contextmenu-open").addEventListener("click", this.open.bind(this), false);
> +	document.getElementById("contextmenu-retry").addEventListener("click", this.retry.bind(this), false);

Oops, looks like some tab characters ended up in here. Please post a new patch that replaces these with spaces (it's good to take a look at your text editor settings to make sure it's set to use spaces, not tabs).
Attachment #8374616 - Flags: review?(margaret.leibovic) → review+
Status: NEW → ASSIGNED
Attached patch bug948896.patchSplinter Review
That's strange. It looks good in the source file. Something must've happen when the diff genarated. Here is a new one. :)
Attachment #8374616 - Attachment is obsolete: true
Attachment #8376968 - Flags: review?(margaret.leibovic)
Attachment #8376968 - Flags: feedback?
Attachment #8376968 - Flags: checkin?
Comment on attachment 8376968 [details] [diff] [review]
bug948896.patch

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

That looks great, thanks.

In the future, you only need to flip the review? flag. The feedback? flag is a similar flag you can use to just get feedback on a patch earlier in the process of writing it, when it's not quite ready for review yet. And you don't need to use the checkin? flag, we usually just set that on bugs with lots of patches, where some have been checked in and some haven't.
Attachment #8376968 - Flags: review?(margaret.leibovic)
Attachment #8376968 - Flags: review+
Attachment #8376968 - Flags: feedback?
Attachment #8376968 - Flags: checkin?
The checkin-needed keyword on the bug will make sure someone checks this patch in for you. Thanks for your help!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2cc946bb55c4

Please make sure that future patches have proper commit information per the link below. Makes life much easier for those landing on your behalf :)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
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]
https://hg.mozilla.org/mozilla-central/rev/2cc946bb55c4
Status: ASSIGNED → RESOLVED
Closed: 6 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 30
(In reply to :Margaret Leibovic from comment #21)
So what do I need to do when I fixed the bug to get it added to the Mozilla central? Thanks for all your help so far :)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #23)
I'm actually using mozilla github repo, not the HG one. Can you point me in a detection how can I add those comments on a git diff converted to hg patch using these (https://github.com/mozilla/moz-git-tools)
(In reply to Madushan Nishantha from comment #25)
> (In reply to :Margaret Leibovic from comment #21)
> So what do I need to do when I fixed the bug to get it added to the Mozilla
> central? Thanks for all your help so far :)

It's already taken care of! I used the "checkin-needed" keyword, and RyanVM kindly landed your patch for you on mozilla-central.
You need to log in before you can comment on or make changes to this bug.