Closed
Bug 948899
Opened 12 years ago
Closed 11 years ago
Move inline scripts and styles into separate file for mobile/android/chrome/content/aboutAddons.xhtml (URL=about:addons)
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
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)
12.93 KB,
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•12 years ago
|
||
Hi can I take this please?
Assignee | ||
Comment 3•12 years ago
|
||
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)
Reporter | ||
Comment 4•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
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 :)
Assignee | ||
Comment 6•12 years ago
|
||
How's this?
Attachment #8349392 -
Attachment is obsolete: true
Attachment #8349442 -
Flags: review?(fbraun)
Comment 7•12 years ago
|
||
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.
Reporter | ||
Comment 8•12 years ago
|
||
(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)
Reporter | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Comment 10•12 years ago
|
||
(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)
Reporter | ||
Comment 11•12 years ago
|
||
(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)
Comment 12•12 years ago
|
||
(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.
Comment 13•12 years ago
|
||
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
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)
Reporter | ||
Comment 16•11 years ago
|
||
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+
Assignee | ||
Comment 17•11 years ago
|
||
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.
Comment 18•11 years ago
|
||
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.
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #8356569 -
Attachment is obsolete: true
Attachment #8357999 -
Flags: review?(margaret.leibovic)
Comment 20•11 years ago
|
||
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+
Assignee | ||
Comment 21•11 years ago
|
||
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 22•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
I removed that now =]
Attachment #8358345 -
Attachment is obsolete: true
Attachment #8359301 -
Flags: review?(margaret.leibovic)
Comment 24•11 years ago
|
||
Comment on attachment 8359301 [details] [diff] [review]
patch
Thanks!
Attachment #8359301 -
Flags: review?(margaret.leibovic) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Flags: needinfo?(mark.finkle)
Comment 25•11 years ago
|
||
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]
Status: NEW → 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 29
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
•