Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

On another answer another answer by @rolfl @rolfl, he suggests to wrap the comments in a /** @preserve */ block, like this:

  • Please, think about performance!!! Please!!!

    Not everybody has an Intel Xeon 14-core 2.6GHz CPU! Sometimes you have a little phone with a single core. You have to love performance! Marry performance!

    You have this:

     $('.message').each(function() { //loop through EXISTING messages to find oneboxable messages
    

    On my Memer project Memer project (at the time it was just a chat "translator") I do this:

     $('#chat .message:not([data-checked="1"])').foreach( [...] );
    

    Now you say: "But you said not to use pseudo-selectors!". To which I say: Correct, but this one will still be optimized by the browser because it runs on document.querySelectorAll. At least in the browsers we care about, like Chrome and Firefox. You can remove the #chat bit there if your tests show increase in speed.

    This avoids you processing the same messages over and over again.

    In fact, what I have on memer is this:

     $('#chat .message:not([data-checked="1"])')
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Which goes even further to check if it isn't a oneboxed thing already. Yes, it is slower than your code, but in this case we will cheat on performance in exchange of correctness and readability. Slow and steady on this one!

  • Don't mix APIs!

    A few lines below, almost at the end, you have this:

     observer.observe($('#chat')[0], { //observe with the mutation observer for NEW messages
     childList: true,
     subtree: true
     });
    

    Sure, jQuery will optimize $('#chat') into document.getElementById('chat') but it still has quite an high cost. This is pure laziness. Just write this:

     //observe with the mutation observer for NEW messages
     observer.observe(document.getElementById('chat'), {
     childList: true,
     subtree: true
     });
    

    In fact, I would do even better:

     var chatElem = document.getElementById('chat');
     setTimeout(function() {
     [...]
     //observe with the mutation observer for NEW messages
     observer.observe(chatElem, {
     childList: true,
     subtree: true
     });
     });
    

    The #chat won't go away! You can store it there and you are done! In fact, you can ever speed a bit our last point:

     $('.message:not([data-checked="1"])', chatElem)
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Oh yeah! We are back on performance!

  • I REPEAT: YOU HAVE TOO MUCH JQUERY!!!

    I will not re-iterate over this if. I will just de-jQuerylize(?) that whole .each():

On another answer by @rolfl, he suggests to wrap the comments in a /** @preserve */ block, like this:

  • Please, think about performance!!! Please!!!

    Not everybody has an Intel Xeon 14-core 2.6GHz CPU! Sometimes you have a little phone with a single core. You have to love performance! Marry performance!

    You have this:

     $('.message').each(function() { //loop through EXISTING messages to find oneboxable messages
    

    On my Memer project (at the time it was just a chat "translator") I do this:

     $('#chat .message:not([data-checked="1"])').foreach( [...] );
    

    Now you say: "But you said not to use pseudo-selectors!". To which I say: Correct, but this one will still be optimized by the browser because it runs on document.querySelectorAll. At least in the browsers we care about, like Chrome and Firefox. You can remove the #chat bit there if your tests show increase in speed.

    This avoids you processing the same messages over and over again.

    In fact, what I have on memer is this:

     $('#chat .message:not([data-checked="1"])')
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Which goes even further to check if it isn't a oneboxed thing already. Yes, it is slower than your code, but in this case we will cheat on performance in exchange of correctness and readability. Slow and steady on this one!

  • Don't mix APIs!

    A few lines below, almost at the end, you have this:

     observer.observe($('#chat')[0], { //observe with the mutation observer for NEW messages
     childList: true,
     subtree: true
     });
    

    Sure, jQuery will optimize $('#chat') into document.getElementById('chat') but it still has quite an high cost. This is pure laziness. Just write this:

     //observe with the mutation observer for NEW messages
     observer.observe(document.getElementById('chat'), {
     childList: true,
     subtree: true
     });
    

    In fact, I would do even better:

     var chatElem = document.getElementById('chat');
     setTimeout(function() {
     [...]
     //observe with the mutation observer for NEW messages
     observer.observe(chatElem, {
     childList: true,
     subtree: true
     });
     });
    

    The #chat won't go away! You can store it there and you are done! In fact, you can ever speed a bit our last point:

     $('.message:not([data-checked="1"])', chatElem)
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Oh yeah! We are back on performance!

  • I REPEAT: YOU HAVE TOO MUCH JQUERY!!!

    I will not re-iterate over this if. I will just de-jQuerylize(?) that whole .each():

On another answer by @rolfl, he suggests to wrap the comments in a /** @preserve */ block, like this:

  • Please, think about performance!!! Please!!!

    Not everybody has an Intel Xeon 14-core 2.6GHz CPU! Sometimes you have a little phone with a single core. You have to love performance! Marry performance!

    You have this:

     $('.message').each(function() { //loop through EXISTING messages to find oneboxable messages
    

    On my Memer project (at the time it was just a chat "translator") I do this:

     $('#chat .message:not([data-checked="1"])').foreach( [...] );
    

    Now you say: "But you said not to use pseudo-selectors!". To which I say: Correct, but this one will still be optimized by the browser because it runs on document.querySelectorAll. At least in the browsers we care about, like Chrome and Firefox. You can remove the #chat bit there if your tests show increase in speed.

    This avoids you processing the same messages over and over again.

    In fact, what I have on memer is this:

     $('#chat .message:not([data-checked="1"])')
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Which goes even further to check if it isn't a oneboxed thing already. Yes, it is slower than your code, but in this case we will cheat on performance in exchange of correctness and readability. Slow and steady on this one!

  • Don't mix APIs!

    A few lines below, almost at the end, you have this:

     observer.observe($('#chat')[0], { //observe with the mutation observer for NEW messages
     childList: true,
     subtree: true
     });
    

    Sure, jQuery will optimize $('#chat') into document.getElementById('chat') but it still has quite an high cost. This is pure laziness. Just write this:

     //observe with the mutation observer for NEW messages
     observer.observe(document.getElementById('chat'), {
     childList: true,
     subtree: true
     });
    

    In fact, I would do even better:

     var chatElem = document.getElementById('chat');
     setTimeout(function() {
     [...]
     //observe with the mutation observer for NEW messages
     observer.observe(chatElem, {
     childList: true,
     subtree: true
     });
     });
    

    The #chat won't go away! You can store it there and you are done! In fact, you can ever speed a bit our last point:

     $('.message:not([data-checked="1"])', chatElem)
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Oh yeah! We are back on performance!

  • I REPEAT: YOU HAVE TOO MUCH JQUERY!!!

    I will not re-iterate over this if. I will just de-jQuerylize(?) that whole .each():

Added a review about another piece of code
Source Link
Ismael Miguel
  • 6.1k
  • 2
  • 24
  • 62

setTimeout: (at the very end of your code)

I must say that the stuff here is a lot more readable! Thank you!

But there's still some things:

  • Please, think about performance!!! Please!!!

    Not everybody has an Intel Xeon 14-core 2.6GHz CPU! Sometimes you have a little phone with a single core. You have to love performance! Marry performance!

    You have this:

     $('.message').each(function() { //loop through EXISTING messages to find oneboxable messages
    

    On my Memer project (at the time it was just a chat "translator") I do this:

     $('#chat .message:not([data-checked="1"])').foreach( [...] );
    

    Now you say: "But you said not to use pseudo-selectors!". To which I say: Correct, but this one will still be optimized by the browser because it runs on document.querySelectorAll. At least in the browsers we care about, like Chrome and Firefox. You can remove the #chat bit there if your tests show increase in speed.

    This avoids you processing the same messages over and over again.

    In fact, what I have on memer is this:

     $('#chat .message:not([data-checked="1"])')
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Which goes even further to check if it isn't a oneboxed thing already. Yes, it is slower than your code, but in this case we will cheat on performance in exchange of correctness and readability. Slow and steady on this one!

  • Don't mix APIs!

    A few lines below, almost at the end, you have this:

     observer.observe($('#chat')[0], { //observe with the mutation observer for NEW messages
     childList: true,
     subtree: true
     });
    

    Sure, jQuery will optimize $('#chat') into document.getElementById('chat') but it still has quite an high cost. This is pure laziness. Just write this:

     //observe with the mutation observer for NEW messages
     observer.observe(document.getElementById('chat'), {
     childList: true,
     subtree: true
     });
    

    In fact, I would do even better:

     var chatElem = document.getElementById('chat');
     setTimeout(function() {
     [...]
     //observe with the mutation observer for NEW messages
     observer.observe(chatElem, {
     childList: true,
     subtree: true
     });
     });
    

    The #chat won't go away! You can store it there and you are done! In fact, you can ever speed a bit our last point:

     $('.message:not([data-checked="1"])', chatElem)
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Oh yeah! We are back on performance!

  • I REPEAT: YOU HAVE TOO MUCH JQUERY!!!

    I will not re-iterate over this if. I will just de-jQuerylize(?) that whole .each():

 [...].each(function() {
 var links = this.querySelectorAll('a[href*="github.com"]');
 
 //Bail out if there are no links, could replace `> -1` with `~`
 if (!links.length || this[textProp].trim().indexOf(' ') > -1) {
 return;
 }
 
 //pass URL and message to the function which will go on to call useApi and add the onebox
 extractFromUrlAndGetInfo(links[links.length - 1].href, $(this));
 });

setTimeout: (at the very end of your code)

I must say that the stuff here is a lot more readable! Thank you!

But there's still some things:

  • Please, think about performance!!! Please!!!

    Not everybody has an Intel Xeon 14-core 2.6GHz CPU! Sometimes you have a little phone with a single core. You have to love performance! Marry performance!

    You have this:

     $('.message').each(function() { //loop through EXISTING messages to find oneboxable messages
    

    On my Memer project (at the time it was just a chat "translator") I do this:

     $('#chat .message:not([data-checked="1"])').foreach( [...] );
    

    Now you say: "But you said not to use pseudo-selectors!". To which I say: Correct, but this one will still be optimized by the browser because it runs on document.querySelectorAll. At least in the browsers we care about, like Chrome and Firefox. You can remove the #chat bit there if your tests show increase in speed.

    This avoids you processing the same messages over and over again.

    In fact, what I have on memer is this:

     $('#chat .message:not([data-checked="1"])')
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Which goes even further to check if it isn't a oneboxed thing already. Yes, it is slower than your code, but in this case we will cheat on performance in exchange of correctness and readability. Slow and steady on this one!

  • Don't mix APIs!

    A few lines below, almost at the end, you have this:

     observer.observe($('#chat')[0], { //observe with the mutation observer for NEW messages
     childList: true,
     subtree: true
     });
    

    Sure, jQuery will optimize $('#chat') into document.getElementById('chat') but it still has quite an high cost. This is pure laziness. Just write this:

     //observe with the mutation observer for NEW messages
     observer.observe(document.getElementById('chat'), {
     childList: true,
     subtree: true
     });
    

    In fact, I would do even better:

     var chatElem = document.getElementById('chat');
     setTimeout(function() {
     [...]
     //observe with the mutation observer for NEW messages
     observer.observe(chatElem, {
     childList: true,
     subtree: true
     });
     });
    

    The #chat won't go away! You can store it there and you are done! In fact, you can ever speed a bit our last point:

     $('.message:not([data-checked="1"])', chatElem)
     .attr('data-checked', 1)
     .find('>.content')
     .filter(':not(:has(.onebox))')
    

    Oh yeah! We are back on performance!

  • I REPEAT: YOU HAVE TOO MUCH JQUERY!!!

    I will not re-iterate over this if. I will just de-jQuerylize(?) that whole .each():

 [...].each(function() {
 var links = this.querySelectorAll('a[href*="github.com"]');
 
 //Bail out if there are no links, could replace `> -1` with `~`
 if (!links.length || this[textProp].trim().indexOf(' ') > -1) {
 return;
 }
 
 //pass URL and message to the function which will go on to call useApi and add the onebox
 extractFromUrlAndGetInfo(links[links.length - 1].href, $(this));
 });
Added info about `@preserve`
Source Link
Ismael Miguel
  • 6.1k
  • 2
  • 24
  • 62

As an alternative to that check for the hostname, you can use last.hostname.match(/^(?:www\.)github\.com$/i) instead.


On another answer by @rolfl , he suggests to wrap the comments in a /** @preserve */ block, like this:

/** @preserve
// ==UserScript==
.....
// ==/UserScript==
*/

This will prevent Firefox from deleting your comments, and still allows to parse them as part of the userscript.

As an alternative to that check for the hostname, you can use last.hostname.match(/^(?:www\.)github\.com$/i) instead.


On another answer by @rolfl , he suggests to wrap the comments in a /** @preserve */ block, like this:

/** @preserve
// ==UserScript==
.....
// ==/UserScript==
*/

This will prevent Firefox from deleting your comments, and still allows to parse them as part of the userscript.

Small edit, forgot `www.gitbub.com`
Source Link
Ismael Miguel
  • 6.1k
  • 2
  • 24
  • 62
Loading
Source Link
Ismael Miguel
  • 6.1k
  • 2
  • 24
  • 62
Loading
default

AltStyle によって変換されたページ (->オリジナル) /