Closed Bug 948899 Opened 7 years ago Closed 7 years ago

Move inline scripts and styles into separate file for mobile/android/chrome/content/aboutAddons.xhtml (URL=about:addons)

Categories

(Firefox for Android :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: freddy, Assigned: errietta)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 6 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.
Hi can I take this please?
Sure!
Assignee: nobody → errietta
Attached patch Work so far (obsolete) — Splinter Review
Hi,
As seen in the attachment I added event listeners in aboutAddons.js and removed them from the XML file, however some functions don't work anymore.
For example, clicking on the disable button gives me E/GeckoConsole(19707): [JavaScript Error: "TypeError: this.setEnabled is not a function" {file: "chrome://browser/content/aboutAddons.js" line: 440}]. Any pointers?
Attachment #8349392 - Flags: feedback?(fbraun)
Flags: needinfo?(fbraun)
Comment on attachment 8349392 [details] [diff] [review]
Work so far

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

I must admit that I don't really understand the underlying problem yet. I will try to find some time to build and test this tomorrow.

Thanks for this first patch, the general approach looks really good!
In the meantime, you could address the inline CSS.
Do you intend to address the inline styles in a separate patch?
Attachment #8349392 - Flags: feedback?(fbraun) → feedback+
Hi, I found the problem with this I think. I'm working on the CSS now and then I'll provide a patch with both :)
Attached patch Proposed patch (obsolete) — Splinter Review
How's this?
Attachment #8349392 - Attachment is obsolete: true
Attachment #8349442 - Flags: review?(fbraun)
Comment on attachment 8349442 [details] [diff] [review]
Proposed patch

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

::: mobile/android/chrome/content/aboutAddons.js
@@ +85,5 @@
>    Addons.getAddons();
>    showList();
>    ContextMenus.init();
> +
> +  document.getElementById('uninstall-btn').addEventListener('click', function() { Addons.uninstall(); }, false);

Drive-by comment: It looks like your previous problem was caused by the callback function getting called with the wrong `this` context, so that's why wrapping the callback in a function like this fixed the issue.

However, I think a better solution would be to do Addons.uninstall.bind(this);

Also, in general we use double quote ("") instead of single quotes ('') to wrap strings, so we should do that in here to be consistent.

@@ +98,5 @@
> +  var header_button = document.getElementById('header-button');
> +  header_button.addEventListener(
> +    'click',
> +    function() {
> +      openLink (header_button);

freddyb ran into a similar issue in bug 948894. Instead of needing to wrap the openLink call in an anonymous function, we should try updating the openLink function to take an event instead of an element.
(In reply to :Margaret Leibovic from comment #7)
> @@ +98,5 @@
> > +  var header_button = document.getElementById('header-button');
> > +  header_button.addEventListener(
> > +    'click',
> > +    function() {
> > +      openLink (header_button);
> 
> freddyb ran into a similar issue in bug 948894. Instead of needing to wrap
> the openLink call in an anonymous function, we should try updating the
> openLink function to take an event instead of an element.

AFAIU there are other calls of openLink which are not bound to an Event Listener, so you have to make sure that they are accommodated as well.
Flags: needinfo?(fbraun)
Comment on attachment 8349442 [details] [diff] [review]
Proposed patch

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

Good progress!

::: mobile/android/themes/core/aboutAddons.css
@@ +269,5 @@
> +}
> +
> +#addons-list, #addons-details {
> +  display: none;
> +}

You select a lot of tags to be either 'display: none;' or 'display: -moz-box;'. I suggest you create another class for those two cases and add it to all the affected tags isntead. What do you think?
Attachment #8349442 - Flags: review?(fbraun)
(In reply to :Margaret Leibovic from comment #7)
> @@ +98,5 @@
> > +  var header_button = document.getElementById('header-button');
> > +  header_button.addEventListener(
> > +    'click',
> > +    function() {
> > +      openLink (header_button);
> 
> freddyb ran into a similar issue in bug 948894. Instead of needing to wrap
> the openLink call in an anonymous function, we should try updating the
> openLink function to take an event instead of an element.

There's

>   outer.addEventListener("click", function() {
>      openLink(document.getElementById("header-button"));
>   }.bind(this), true);

which calls openLink on another element than the event target, so then how would we accommodate for that?


(In reply to Frederik Braun [:freddyb] from comment #9)
> Comment on attachment 8349442 [details] [diff] [review]
> Proposed patch
> 
> Review of attachment 8349442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good progress!
> 
> ::: mobile/android/themes/core/aboutAddons.css
> @@ +269,5 @@
> > +}
> > +
> > +#addons-list, #addons-details {
> > +  display: none;
> > +}
> 
> You select a lot of tags to be either 'display: none;' or 'display:
> -moz-box;'. I suggest you create another class for those two cases and add
> it to all the affected tags isntead. What do you think?

I copied a lot of that from the original XML. I assume that show-on* and hide-on* classes are applied on what the user does so getting rid of that might break something?
Flags: needinfo?(fbraun)
(In reply to Errietta Kostala from comment #10)
> There's
> 
> >   outer.addEventListener("click", function() {
> >      openLink(document.getElementById("header-button"));
> >   }.bind(this), true);
> 
> which calls openLink on another element than the event target, so then how
> would we accommodate for that?
> 
So, we either keep the function-wrapped instance you have already added or you check in openLink if it has gotten an Element or an Event. You could find out if it's an Element with |if varName instnaceof Node|, you should rename the paremeter to aEventOrElement or so... which looks a bit dirty to me. Maybe we stick with the wrapped function, but frankly I don't know what's preferred in our codebase. 

Margaret, what do you think?


> > You select a lot of tags to be either 'display: none;' or 'display:
> > -moz-box;'. I suggest you create another class for those two cases and add
> > it to all the affected tags isntead. What do you think?
> 
> I copied a lot of that from the original XML. I assume that show-on* and
> hide-on* classes are applied on what the user does so getting rid of that
> might break something?


Yes, don't remove any of the existing classes. You can have more than one class per Element, i.e. |<span class="foo bar">|
Flags: needinfo?(fbraun)
(In reply to Errietta Kostala from comment #10)

> > freddyb ran into a similar issue in bug 948894. Instead of needing to wrap
> > the openLink call in an anonymous function, we should try updating the
> > openLink function to take an event instead of an element.
> 
> There's
> 
> >   outer.addEventListener("click", function() {
> >      openLink(document.getElementById("header-button"));
> >   }.bind(this), true);
> 
> which calls openLink on another element than the event target, so then how
> would we accommodate for that?

All methods that are used as event handlers (call in response to an event) are passed an event object, whether you ask for it or not. So this could work:

   outer.addEventListener("click", function(aEvent) {
      openLink(aEvent);
   }.bind(this), true);

or even this:

   outer.addEventListener("click", openLink, true);

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

> > You select a lot of tags to be either 'display: none;' or 'display:
> > -moz-box;'. I suggest you create another class for those two cases and add
> > it to all the affected tags isntead. What do you think?
> 
> I copied a lot of that from the original XML. I assume that show-on* and
> hide-on* classes are applied on what the user does so getting rid of that
> might break something?

I agree with you. Making less changes is better. We typically file new bugs if we find some code that could be refactored, but is out of scope in the current bug.
Attached patch Work so far (obsolete) — Splinter Review
Hi,

Here's what I have so far. However, doing document.getElementById("uninstall-btn").addEventListener("click", Addons.uninstall.bind(Addons), false); breaks it with the following error: E/GeckoConsole( 4994): [JavaScript Error: "TypeError: aKey is undefined" {file: "chrome://browser/content/aboutAddons.js" line: 252}]

I think the problem is that   uninstall: function uninstall(aAddon) {
is getting the click event instead of a valid addon object so addon.id isn't defined
Attachment #8350985 - Flags: feedback?(mark.finkle)
Flags: needinfo?(mark.finkle)
Attached patch proposed patch (obsolete) — Splinter Review
Hi,

I was told here and on IRC to do   document.getElementById("uninstall-btn").addEventListener("click", Addons.uninstall.bind(this), false); or   document.getElementById("uninstall-btn").addEventListener("click", Addons.uninstall.bind(Addons), false);

but neither passes the right sort of thing onto Addons.uninstall and friends. I'm doing   document.getElementById("uninstall-btn").addEventListener("click", function() { Addons.uninstall() }, false); for now instead, since that works fine. Is that ok or how do we figure out how to make .bind work?
Attachment #8349442 - Attachment is obsolete: true
Attachment #8350985 - Attachment is obsolete: true
Attachment #8350985 - Flags: feedback?(mark.finkle)
Attachment #8356569 - Flags: review?(fbraun)
Comment on attachment 8356569 [details] [diff] [review]
proposed patch

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

Mh, this is confusing to me too. It seems this should be bound to the element, you're attaching the eventlistener to?
Attachment #8356569 - Flags: review?(fbraun) → review+
When we do Addons.uninstall.bind(Addons), it passes the event object as an argument. .uninstall() expects either nothing, or an addon as an argument, so that's not going to work. The code I replaced called it with no argument at all.
function() { Addons.uninstall() }

...is equivalent to...

Addons.uninstall.bind(this);

So, if you want to use bind, you should be binding the current context, not the Addons context.

Also, please make sure to flag me or another mobile peer for review when you have a patch you'd like to land.
Attached patch proposed patch (obsolete) — Splinter Review
Attachment #8356569 - Attachment is obsolete: true
Attachment #8357999 - Flags: review?(margaret.leibovic)
Comment on attachment 8357999 [details] [diff] [review]
proposed patch

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

::: mobile/android/chrome/content/aboutAddons.js
@@ +92,5 @@
> +
> +  document.getElementById("uninstall-btn").addEventListener("click", function() { Addons.uninstall() }, false);
> +  document.getElementById("cancel-btn").addEventListener("click", function() { Addons.cancelUninstall() }, false);
> +  document.getElementById("disable-btn").addEventListener("click", function() { Addons.disable() }, false);
> +  document.getElementById("enable-btn").addEventListener("click", function() { Addons.enable() }, false);

Thinking about this more, instead of adding these event listeners here, we could create a Addons.init method similar to ContextMenus.init, then doing Addons.enable.bind(this) from within Addons.init should end up binding the correct context.

In fact, you could just rename Addons.getAddons() to Addons.init(), and add these event listeners in there.

I don't have a strong preference whether we try that or just land this code as-is, so it's up to you if you want to try this and upload a new patch.

@@ +531,5 @@
>    }
>  }
> +
> +window.addEventListener('load', init, false);
> +window.addEventListener('unload', uninit, false);

Nit: Use double quotes instead of single quotes here.
Attachment #8357999 - Flags: review?(margaret.leibovic) → review+
Attached patch Proposed patch (obsolete) — Splinter Review
Hi,

I think this is better now - I broke uninstall in 2 functions, 1 for the current addon and one for passing an addon as a param and it seems to work fine.
Attachment #8357999 - Attachment is obsolete: true
Attachment #8358345 - Flags: review?(margaret.leibovic)
Comment on attachment 8358345 [details] [diff] [review]
Proposed patch

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

Nice improvement! Thanks for putting the extra work into this :)

::: mobile/android/chrome/content/aboutAddons.js
@@ +440,4 @@
>      if (!addon)
>        return;
>  
> +    this.uninstall (addon);

Nit: Remove the space before the (.
Attachment #8358345 - Flags: review?(margaret.leibovic) → review+
Attached patch patchSplinter Review
I removed that now =]
Attachment #8358345 - Attachment is obsolete: true
Attachment #8359301 - Flags: review?(margaret.leibovic)
Comment on attachment 8359301 [details] [diff] [review]
patch

Thanks!
Attachment #8359301 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
Flags: needinfo?(mark.finkle)
https://hg.mozilla.org/integration/fx-team/rev/86d408022528
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/86d408022528
Status: NEW → RESOLVED
Closed: 7 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 29
You need to log in before you can comment on or make changes to this bug.