Closed
Bug 948896
Opened 11 years ago
Closed 11 years ago
Move inline scripts and styles into separate file for mobile/android/chrome/content/aboutDownloads.xhtml (about:downloads)
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: freddy, 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)
|
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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.
|
Assignee | |
Comment 1•11 years ago
|
||
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)
|
Reporter | |
Comment 2•11 years ago
|
||
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)
|
|
Assignee | |
Comment 3•11 years ago
|
||
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);
|
||
Comment 4•11 years ago
|
||
(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);
|
|
Assignee | |
Comment 5•11 years ago
|
||
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() {.......
|
||
Comment 6•11 years ago
|
||
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.
|
|
Assignee | |
Comment 7•11 years ago
|
||
Things started working after making changes :wesj suggested. Can someone take a look at it and tell if any canges needed to be made?
|
|
Reporter | |
Comment 8•11 years ago
|
||
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+
|
|
Assignee | |
Comment 9•11 years ago
|
||
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?
|
|
Assignee | |
Comment 10•11 years ago
|
||
And can someone assign me to this bug? :)
|
|
Reporter | |
Updated•11 years ago
|
Assignee: nobody → j.l.madushan
|
|
Reporter | |
Comment 11•11 years ago
|
||
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.
|
|
Assignee | |
Comment 12•11 years ago
|
||
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);
}
|
|
Assignee | |
Comment 13•11 years ago
|
||
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)
|
|
Reporter | |
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
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-
|
|
Assignee | |
Comment 16•11 years ago
|
||
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)
|
|
||
Comment 17•11 years ago
|
||
(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
|
|
Assignee | |
Comment 18•11 years ago
|
||
(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 19•11 years ago
|
||
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+
|
|
Reporter | |
Updated•11 years ago
|
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 20•11 years ago
|
||
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 21•11 years ago
|
||
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?
|
|
||
Comment 22•11 years ago
|
||
The checkin-needed keyword on the bug will make sure someone checks this patch in for you. Thanks for your help!
Keywords: checkin-needed
|
||
Comment 23•11 years ago
|
||
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]
|
|
||
Comment 24•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 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
|
|
Assignee | |
Comment 25•11 years ago
|
||
(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 :)
|
|
Assignee | |
Comment 26•11 years ago
|
||
(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)
|
|
||
Comment 27•11 years ago
|
||
(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.
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•