948894 - Move inline scripts and styles into separate file for mobile/android/chrome/content/aboutApps.xhtml (about:apps)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Frederik Braun [:freddy] (OOO until August 18th)]

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.

Comment 1

[Frederik Braun [:freddy] (OOO until August 18th)]

Attachment: putting inline JS into the existing aboutApps.js file

Trying to solve this one on my own, so future volunteers have a sample to look at. This patch should explain how to move things around.
Lucas: Bugzilla suggested you review this, is this OK?

(Haven't built or pushed to try yet)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8346630 [details] [diff] [review]
putting inline JS into the existing aboutApps.js file

>diff --git a/mobile/android/chrome/content/aboutApps.js b/mobile/android/chrome/content/aboutApps.js

>+window.onload = function onWindowLoad() {
>+  var body = document.querySelector("body")
>+  body.onload = onLoad;
>+
>+  var addToHomescreenLabel = document.querySelector("#addToHomescreenLabel")
>+  addToHomescreenLabel.onclick = function() { ContextMenus.addToHomescreen(); }
>+
>+  var uninstallLabel = document.querySelector("#uninstallLabel")
>+  uninstallLabel.onclick = function() { ContextMenus.uninstall(); }
>+
>+  var headerButton = document.querySelector("#header-button")
>+  headerButton.onclick = function()  { openLink(headerButton); }
>+
>+  var listItem = document.querySelector("div.list-item");
>+  listItem.onclick = function() {  openLink(listItem); }
>+
>+}
>\ No newline at end of file


Some notes:
* Don't use querySelector for elements with IDs
* Use let instead of var

* We prefer to use window.addEventListener("load", onLoad);
  You could add it after line 25 as a global

* Same is true for the other events
  * The ContextMenus handlers should be added in ContextMenus.init() like this one:
    http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutApps.js#44

   Something like: document.getElementById("addToHomescreenLabel").addEventListener("click", ContextMenus.addToHomescreen);

* Add a new line to the end of the file

Lucas is on vacation, reassigning to Margaret

Comment 3

[Frederik Braun [:freddy] (OOO until August 18th)]

Attachment: putting inline JS into the existing aboutApps.js file (rev. 2)

Thanks for the feedback, Mark.
I've addressed your comments in this follow-up.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8347094 [details] [diff] [review]
putting inline JS into the existing aboutApps.js file (rev. 2)

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

Thanks for addressing Mark's comments! I have some more suggestions I'd like to see get addressed before we land this.

::: mobile/android/chrome/content/aboutApps.js
@@ +22,5 @@
>      .QueryInterface(Ci.nsIInterfaceRequestor)
>      .getInterface(Ci.nsIDOMWindow)
>      .QueryInterface(Ci.nsIDOMChromeWindow));
>  
> +window.addEventListener("load", onWindowLoad);

Instead of listening for "load" on the window, we can just listen for "DOMContentLoaded" on the document (this will happen earlier). And as I mention down below, we can just call the existing onLoad function in here.

@@ +44,5 @@
>  
>    init: function() {
>      document.addEventListener("contextmenu", ContextMenus, false);
> +    document.getElementById("addToHomescreenLabel").addEventListener("click", ContextMenus.addToHomescreen);
> +    document.getElementById("uninstallLabel").addEventListener("click", ContextMenus.uninstall);

Nit: Explicitly specify the third parameter as false in these addEventListener calls (same comment applies everywhere). You can also s/ContextMenus/this (and you can update the listener above to do the same thing).

@@ +159,5 @@
>        document.getElementById("main-container").classList.add("hidden");
>    }
>  }
> +
> +function onWindowLoad() {

Instead of creating this new onWindowLoad function, let's just use the current onLoad function, and we can add the new event listeners in there.

@@ +164,5 @@
> +  let body = document.querySelector("body")
> +  body.addEventListener("load", onLoad);
> +  let headerButton = document.getElementById("header-button")
> +  headerButton.addEventListener("click", function()  { openLink(headerButton); });
> +  let listItem = document.querySelector("div.list-item");

You should give this element an id, since I see there's only one element in the HTML that matches this.

In fact, instead of doing these two different calls, we could use querySelectorAll to find all the elements with a "pref" attribute, then add listeners to them (since that's what we're really doing here). What about something like this?

Array.prototype.forEach.call(document.querySelectorAll("[pref]"), elt => elt.addEventListener("click", openLink, false));

@@ +165,5 @@
> +  body.addEventListener("load", onLoad);
> +  let headerButton = document.getElementById("header-button")
> +  headerButton.addEventListener("click", function()  { openLink(headerButton); });
> +  let listItem = document.querySelector("div.list-item");
> +  listItem.addEventListener("click", function() {  openLink(listItem); });

Instead of wrapping a function around openLink like this, let's just update openLink to take an event as a parameter instead of an element, and then we can just get at that element with event.target. With that change you can just pass in openLink directly as the handler.

Comment 5

[Frederik Braun [:freddy] (OOO until August 18th)]

Attachment: 8347094: putting inline JS into the existing aboutApps.js file (rev. 3)

Thanks for the review!
The Array.prototype.forEach-thing is a terrible read, but I have taken your suggestions in this third revision. Can you take another look?

Comment 6

[:Margaret Leibovic]

(In reply to Frederik Braun [:freddyb] from comment #5)

> The Array.prototype.forEach-thing is a terrible read

You can blame NodeLists for not being real arrays! :)

I was having fun using some advanced JS features, but for a less tricky-looking implementation, you can also just use a regular for loop if you prefer that.

let elmts = document.querySelectorAll("[prefs]");
for (let i = 0; i < elmts.length; i++) {
  elmts[i].addEventListener(...);
}

Comment 7

[:Margaret Leibovic]

Comment on attachment 8348589 [details] [diff] [review]
8347094: putting inline JS into the existing aboutApps.js file (rev. 3)

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

Looks good to me, thanks! Feel free to update the forEach bit before landing if you prefer the other approach I suggested.

::: mobile/android/chrome/content/aboutApps.js
@@ +22,5 @@
>      .QueryInterface(Ci.nsIInterfaceRequestor)
>      .getInterface(Ci.nsIDOMWindow)
>      .QueryInterface(Ci.nsIDOMChromeWindow));
>  
> +document.addEventListener("DOMContentLoaded", onLoad);

Nit: Add false as the third parameter here.

@@ +88,5 @@
>  
> +  Array.prototype.forEach.call(document.querySelectorAll("[pref]"),
> +                               elt => elt.addEventListener("click",
> +                                                           openLink,
> +                                                           false));

Nit: You could put these three parameters all on the same line, we usually favor long lines over extra lines with little content.

Comment 8

[Frederik Braun [:freddy] (OOO until August 18th)]

Attachment: putting inline JS into the existing aboutApps.js file (final)

Thanks for the review, carrying it over for this follow-up that addresses your nits.
Somebody should really spec Nodelists to be proper Arrays.. :)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/337c0ae0b790

Comment 10

[:Margaret Leibovic]

Looks like there were some typos in the patch that landed. I pushed a fix:
https://hg.mozilla.org/integration/fx-team/rev/ebfa7ef811a6

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/337c0ae0b790
https://hg.mozilla.org/mozilla-central/rev/ebfa7ef811a6

Comment 12

[Frederik Braun [:freddy] (OOO until August 18th)]

(In reply to :Margaret Leibovic from comment #10)
> Looks like there were some typos in the patch that landed. I pushed a fix:

Ooops, thank you.