Concerned about having add-on disabled with the next update

Add-ons addons.mozilla.org
#21

It is not exactly clean code you get when writing a user script. I would prefer to generate the user script from a common codebase that can also be used with the add-on version. I have three different versions of my add-on that use shared code and browser-specific code.

#22

Addon was passed therefore points listed were not cause for rejection at the time. They are suggestions for improvements.

innerHTML (or similar method): that means a method of converting strings into HTML
That is what DOMParser.parseString does.

innerHTML converts & inserts strings to DOM (a 1 step process)

Your addon converts strings to DOM and then appends the created DOM (a 2 step process)

There are security and performance concerns with such methods. You can search the net for further information on the subject.

Suggestions for efficient methods of inserting elements were given with the review.

Addon developers are free to chose the method that they prefer and as long as it does not have security/privacy/performance impact on the browser or user, there should not be a problem.

Please note that userscript & addon are not exactly the same thing. If the addon behaves like a userscript, then it can be a userscript and it wont require a review.
Addon can insert CSS from a separate file easily.
Similarly, addon can include & insert images.

All of the above can easily be accomplished.

Finally, I would like to thank MartijnJ & noitidart who covered everything that needed to be covered. :thumbsup:

#23

Thank you for your attention, erosman.

Your addon converts strings to DOM and then appends the created DOM (a 2 step process)

There are security and performance concerns with such methods. You can search the net for further information on the subject.

I believe that they are not warranted in this case, especially since

a) none of the element code being used is remote, all code used is local and generated from the script, it doesn’t use anything from the page at all either
b) the difference between creating multiple elements with full attributes already included with one line of code or creating all of them via createElement + setAttribute is monumental, even if performance is bigger it will be negligible and the overhead is far greater to justify the possible performance gain, which is not yet known wether it is faster or slower (I have searched and the few results are too old to be reliable)
c) the same security concerns (dynamic values in element creation) surrounding the method I am using are also very well present in the setAttribute method required for adding attributes after using createElement, which is not justifiable since it would render any method of adding attributes to any elements completely useless.

Suggestions for efficient methods of inserting elements were given with the review.

The review clearly refferred to using a different method for inserting elements other than innerHTML, which is never used once anywhere in the code, thus the confusion.

Please note that userscript & addon are not exactly the same thing. If the addon behaves like a userscript, then it can be a userscript and it wont require a review.

This is why I stated it in the post, I also thought the same. The only use it makes from the Firefox environment is simple storage for saving and loading preferences, no other internal code is being used which can be hijacked. From now on should I add your information to the reviewer notes?

EDIT: Or do you mean that I should just publish it as a userscript? The reason why I also port it to an add-on is for the users that don’t want the overhead of a userscript manager just for one userscript.

Addon can insert CSS from a separate file easily.
Similarly, addon can include & insert images.

I am aware of that, but it would force me to make a unique userscript version just for the Firefox add-on, I am not interested in doing that.

Addon was passed therefore points listed were not cause for rejection at the time. They are suggestions for improvements.

They felt like ultimatums, thus my worry:

“we can’t allow the inclusion of resources as data: URIs in static JavaScript strings”
“innerHTML (…) should not be set using dynamic values.”

The only point I might follow is the one regarding data URIs, I will try to use a linked sprite instead in the following updates.

#24

Thanks man keep up the great work! I didn’t realize it passed so this is really a non-issue haha.

#25

I am beginning to think that it is absolutely pointless to inform the reviewers why the suggestions made in their notes will not be followed, I’ve even added it to the notes for reviewers but it made absolutely no difference. They keep adding them and recently two more notes have been added which do not relate in any way to any new content that was added to the add-on, one of which isn’t even clear about what it refers to.

I understand and welcome improvement suggestions, but constantly repeating them even after they have been discussed and rejected is becoming a bit of an harassing stance. I am starting to feel as if my add-on is not welcomed to the amo store if I don’t comply with the increasing list of what seems to be personal standards.

If my add-on was causing browser lag, crashes or posing security risks I would understand, but it is not and the notes of “improvements” will adversely impact my ability to maintain the same source code foundation across multiple environments.

Perhaps the best option is to follow the reviewers’ example and ignore these notes as well, all other options seem to not have any effect. I just hope that this will not result in having my add-on disabled.

1 Like
#26

If you ignore the person who reviews the addon to ensure it follows the standards set by Mozilla, it’ll likely get disabled. Instead of getting upset and complaining, just do what they ask.

#27

The standards wil force me to make a unique version of my software just for the AMO store, this is something I am not willing to do as I explained before.

One of the standards does not offer any alternative to creating multiple elements with multiple attributes from a string to append to the DOM (noted innerHTML yet is not used even once), the other new standard isn’t even comprehensible:

  1. Please store your objects/patterns in non-executable JSON files, and load and
    parse via XHR.
    Look at the example on the following page (especially
    ’response.json’)
    https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/request

This kind of hostility is making me realize that perhaps contributing to the AMO store is a mistake, I’d never expect to experience anything like this.

With an increasing number of developers giving up on developing add-ons for Firefox and with the upcoming add-on policy changes that will certainly drive away more developers, I was expecting to see a more welcoming environment focused on inspiring more developers, not the opposite.

I am sorry to see that this is how things are and apologies if I bothered anyone.

#28

“Hostility” is surprising. They are only trying to help you enhance your code. They approved it. It’s like going to school and your teacher says “I have a suggestion for you do it like this”. Is that hostile?

If it is or ins’t would you go to the media and say “my teacher gave me a suggestion, they gave me an A (equiavlanet of AMO approve), but they gave me a suggestion how dare they???”. So the teacher gave you an A grade and you’re taking suggestions as hostility? :stuck_out_tongue:

This is wrong. All the recommendations can be applied to all other browsers, enhancing it everywhere.

Don’t take this post as being hostile too, I’m just finding things a little funny so pointing it out. :stuck_out_tongue:

#29

“Hostility” is surprising. They are only trying to help you enhance your code.

It was mostly related to the previous user reply and the note regarding the use of data URIs.

So the teacher gave you an A grade and you’re taking suggestions as hostility?

The A grade is pointless if the teacher is going to flunk you at the end of the year because you didn’t follow the suggestions the teacher gave.

This is wrong. All the recommendations can be applied to all other browsers, enhancing it everywhere.

The software is also being published as a userscript, the recommendations cannot be applied because there is no way of applying them in a userscript, as I clearly said before. Userscripts have no ability to load local CSS files and no localizations.

1 Like
#30

AMO is definitely not flunking you in anyway haha so bringing that teacher example to real life, that teacher definitely is not flunking you at the end of the year. :stuck_out_tongue:

That’s fair, and you expressed that to the teacher who gave you an A and is not going to be flunking you. I really don’t see why this is an issue. You expressed yourself, and AMO reviewers accepted your thoughts even before you expressed them by approving your work. Are you upset they are giving you suggestions? I’m sure if you ask people not to comment on your work they’ll oblige.

#31

AMO is definitely not flunking you in anyway haha so bringing that teacher example to real life, that teacher definitely is not flunking you at the end of the year.

Letting me know that if I don’t use a different method other than innerHTML will result in a smaller chance of being approved in the future completely disproves your assertive analogy:

“Removing the innerHTML will improve add-on’s prospect of approval.”

Which, again, I already mentioned before.

Are you upset they are giving you suggestions?

Not upset, worried that the growing list of “suggestions” will might eventually lead to a rejection.

1 Like
#32

@jorgev I’d be very interested in getting an official response on this. Can not fixing comments like this be a reason for not passing the AMO review?

It would be very helpful if the reviewers clearly separated suggestions for further improvements and issues that may prevent an add-on from passing the review, especially now that passing a review is a prerequisite for getting “signed”.

1 Like
#33

Interesting that the addon passed review. Back in June I had one fail on inner html. It was not an advisory. That also was a mature userscript - I’d converted it into a private addon because Fennec could’t run userscripts at the time - it can now.

I subsequently hit the same problem with converting another userscript. The validator gave a warning so I assumed it would fail and changed it.

But the point I really want to make is that the ‘Appendix E - Dom building…’ page I was referred to is a bit daunting and could do with a few more examples relevant to what I was doing - add a one-line stylesheet. For a very amateur addon builder like me it was hard to understand and apply. All that code just to do this? In fact I never got it working - I did my simple style changes another way. My fault, I know, but just a suggestion.

#34

Yep its quite easy, here is how to do it with that:

var myStylesheet = ['html:style', {src: 'blah'}];
document.documentElement.appendChild(jsonToDOM(myStylesheet, document, {}));
#35

Re: DaveRo

Depending on which scope it is, there are many ways to do it. For example, in addition to what was pointed out by noitidart

Using link tag and pointing to the file

<link rel="stylesheet" type="text/css" media="screen" href="screen.css" />

Creating style tag

var css = document.createElement('style');
vat txt = '......................'; // for a short CSS
// For a long CSS, read from a separate file and then use
css.textContent = txt; 
// alternative (slightly faster) to above line
css.appendChild(document.createTextNode(txt)); 
document.head.appendChild(css);

Insert rules into the stylesheet

stylesheet.insertRule(rule, index)

https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule

#36

I still don’t know which method of appending elements to the DOM is “acceptable”. One of the notes makes reference to innerHTML (and similar methods), yet all the explanations provided in this thread (including the ones by the reviwer himself) are related to element creation, not one is related to element insertion which is what has been clearly noted in his list, except the example provided in jsonToDOM which also makes use of the same method as I use, appendChild.

I only use appendChild and insertBefore (insertBefore only when it is needed), I don’t use any library whatsoever (no jQuery either), and that is it, I don’t use any other method of inserting elements to the DOM, yet I keep getting notified to use different methods. Which ones? Why is the discussion constantly focused on element creation from string when none of the notes mentioned anything about it?

The reviewer claims that innerHTML means converting strings into HTML, yet the note clearly makes reference to the method of inserting elements to DOM, including all methods similar to innerHTML. Then he noted that “Suggestions for efficient methods of inserting elements were given with the review.” confirming again that his note is related to element DOM insertion, not element creation, yet none of the mentioned suggestions given provide any alternative method of doing such, they continue using appendChild exactly like what I was already using. I don’t see how much I can emphasize this dilemma, I am already using the supposedly acceptable method of inserting elements to the DOM, yet I keep getting warned to use a different method which is the one I already use.

It is confusing and problematic to provide this kind of inadequate information that might lead to add-on rejection. I am not well informed by the provided notes and when I tried to procure more explanations (this thread) I still got none related to the notes that have been listed, and received no replies to the emails I sent, which was TheOne’s suggestion.

It is proving to be a fruitless exercise to continue trying to find coherent explanations and alternatives to the issues I listed in this thread, and I don’t mean element creation.

#37

notidart, erosman: Thanks to you both. I’ll persevere!

I don’t know how common it is for people to convert userscripts to addons, but nearly all mine started that way and I converted them into content scripts to use them on Fennec. All but one is private but all obviously all have to be signed. So I understand the OP’s irritation.

#38

DaveRo:
I have 100s of userscript and I have converted some of them to Addon. While they use similar methods, addon is a lot more powerful. In fact, IMHO, if a userscript just wants to behave like a userscript, it should then stay a usersript. Userscripts are easy to edit, easy to update and dont require review by AMO.
Addon are more complicated and have a review process. If the developer doesnt need or want to use the power of the addon, it is easier to keep the script as Userscript :slight_smile:
Addon will only need to be signed if you want to publish them. If it is private, they function without being sigend.
In fact all my own addon that are installed on my computer are not signed (since I edit them and they are dev) and they work fine (but have a warning on them). The same addon are singed on AMO if I install them from AMO.

Particle:
Just some examples of performance:
Note: Everything bellow works in any browser

YouTube Plus starts with 1070 lines of CSS. The css is made into an array with1000+ text items.
At the end the array is combined used a function join()

CSS
1- Instead of an array, it can remain a string ie

Instead of:

styleSheet = [
  // start| Playlist spacer
  "@media screen and (min-width: 640px){\n",
  "    .part_playlist_spacer #watch-appbar-playlist{\n",
  "        margin-left: 0 !important;\n",
  "    }\n",
  "}\n",
// ... etc
].join('');

Following performs much better

styleSheet = "@media screen and (min-width: 640px){\n" +
  "    .part_playlist_spacer #watch-appbar-playlist{\n" +
  ....

If you compare the two, you will find out the second method performs many times faster than the first because the entire time it remains a string while in the first method, an array has to created, items added, then with the function overhead to join and thus convert the array into a string.

2- Moving the css to an external file (wouldn’t work for a userscript) but for an addon it improves the memory and performance of the addon since the JavaScript compiler doesn’t need to read through and parse then above 1000+ lines as JavScript.

defSets
Moving defSets to a separate defSets.json (wouldn’t work for a userscript) and reading it via XHR (XMLHttpRequest) again has the performance benefit of JavaScript compiler not having to parse it.
The Localization section of above object in an addon should be moved to a proper localization method and not as part of of a JS (wouldn’t work for a userscript)

innerHTM (or similar method)
Once again, strings are created as arrays and then joined which is less efficient as keeping it as a string.

The proper way for example in the following case:

controls = [
  "<div id='player-console'>\n",
  "    <div id='autoplay-button' class='yt-uix-tooltip" + ((parSets.VID_PLR_ATPL) ? " active" : "") + "' data-tooltip-text='" + userLang("CNSL_AP") + "''></div>\n",
  "    <div id='loop-button' class='yt-uix-tooltip' data-tooltip-text='" + userLang("CNSL_RPT") + "'></div>\n",
  "    <div id='seek-map' class='yt-uix-tooltip' data-tooltip-text='" + (storyBoard ? userLang("CNSL_SKMP") : userLang("CNSL_SKMP_OFF")) + "'" + ((!storyBoard) ? "style='opacity:0.2;'" : "") + "></div>\n",
  "    <div id='save-thumbnail-button' class='yt-uix-tooltip' data-tooltip-text='" + userLang("CNSL_SVTH") + "'></div>\n",
  "    <div id='screenshot-button' class='yt-uix-tooltip' data-tooltip-text='" + userLang("CNSL_SS") + "''></div>\n",
  "    <div id='sidebar-button' class='yt-uix-tooltip' data-tooltip-text='" + userLang("CNSL_SDBR") + "'" + ((window.opener) ? " style='display:none'" : "") + "></div>\n",
  "    <div id='fullbrowser-button' class='yt-uix-tooltip' data-tooltip-text='" + userLang("CNSL_FLBR") + "'></div>\n",
  "    <div id='cinemamode-button' class='yt-uix-tooltip' data-tooltip-text='" + userLang("CNSL_CINM_MD") + "'></div>\n",
  "    <div id='framestep-button' class='yt-uix-tooltip' data-tooltip-text='" + userLang("CNSL_FRME") + "'></div>\n",
  "</div>\n"
  ].join("");

The proper DOM creation method is: (it may take longer to write but it performs many many times better) (works in addons & scripts)

var div1 = document.createElement('div');
div1.id = 'player-console';
var div2 = document.createElement('div');
div2.id = 'autoplay-button';
div2.className = 'yt-uix-tooltip';
 // ..... the rest

div1.appendChild(div2);
// etc
#39

Thank you erosman for finally addressing the issues with so much clarity.

Regarding userscript ports, people like me port them because users often prefer not to install an entire add-on just for one single userscript and instead of being forced with its overhead they have the choice of a lighter alternative as an add-on port. There’s also the benefits of publishing on the AMO store, especially for reaching users that have no clue about what userscripts are or how to install/use them.

Regarding the array vs string, I’ve already converted all string arrays into “+” strings (before?) yesterday (jsPerf is finally working again so I was able to profile the difference between array join and string +, the difference was significative).

Regarding the methods that cannot be replicated in the standalone userscript, it is as I already mentioned before, I will not use them for the reasons I previously mentioned.

Regarding the DOM creation method, the method you suggested not only is vastly more verbose as it also makes the code orders of magnitude harder to read and maintain, nonetheless I have replaced the DOM parser method with createContextualFragment complemented with script/rogue code sanitization (removes any executable code that might be injected by any means possible, even if all the strings used are local, don’t use remote data and are non-executable). Hopefully this will be a more acceptable method since it has better performance and is safe, would you agree?

#40

If the strings converted to DOM were not local. addon would probably have failed the review.

I understand that creating the DOM nodes is cumbersome … but as an example on performance:
This is just a simple text (and not many elements)

elem.appendChild(document.createTextNode('test')); //x6 faster than innerHTML
elem.textContent = 'test';  //x5 faster than innerHTML
elem.innerHTML = 'test';

Also read my comment regarding ‘userscript vs addon’ (they are not the same thing)

Good luck