Nested try/catch blocks in master

[skylize]

I want to help refactor your code so it's easier to optimize and update. Some of the scripts have big giant try blocks with smaller try blocks nested inside of them. It would be quite helpful if you could go through and put a comment //DANGEROUS after any function calls that you already know have risk of failure.

[ejci]

Feel free to refactor. Can you tell me which files contain nested try catch?

[skylize]

page.js

I thought there more ,but it's just that one. If I understand the code correctly, the outer try block can really be some kind of if/then condition. Try/catch can be limited to observer = new MutationObserver(handleMutation); and I guess for some reason eq.attach?

As far as MutationObserver, that is depecrated and known for its awful optimization. It would be better to listen for events. Events bubble up, so we can listen on the body like this:

document.body.attachEventListener('someevent', function(evt){
  if ( !(evt.target in arrayOfKnownAudioElements)) {
     arrayOfKnownAudioElements.push(evt.target)
    // do whatever to init a new context
  }
}

I'm just starting to look at the html5 media spec, so I don't know which event to use here, but anything that always happens at the beginning, but not too often later. Even a play event might do the trick.

[ejci]

The main try catch block is there to prevent error bubbling to console. So if there is a error during initialization it will not "spoil" the page context but it will "eat" the error and send a message to background script that is not running in page context.

I need to use MutationObserver as I need to know of there is a DOM change on a page and if that change is adding/removing any relevant (video/audio) elements. Also it will fire in case there is attribute change (for example audio/video src attribute change).

[skylize]

Sorry this is all just talk right now, I haven't had another chance to dig into the code.

This plugin runs on every single page a user opens. You don't need to worry about microptimzing, but it's important to be thoughtful about big wastes in memory. The javascript compiler typically does more optimization for you than you could ever do yourself. So the most important thing you can do is write code in way that lets the compiler do it's job.

Try/catch prevents compiler optimization of the code inside the block. If you put one function call inside the block it makes almost no difference. But if you do a bunch of stuff in the block, all that stuff that could be optimized isn't. So yeah, try/catch around MutationObserver, but not all the logic of the program. Use if/else instead for the heavy lifting. I'll try to mostly take care of that for you, which is why I was hoping for some input on what you know to be risky function calls.

MutationObserver is super tempting, because it simplifies things a lot. But it was never implemented well for performance and all the browsers have abandoned it (I think because they can't get it to perform). It's still there and works slowly for now, but it's getting almost no attention in source updates and will eventually disappear completely. In the meantime, using it is a bad idea if you can avoid it.

I can offer more specific suggestions as I start refactoring. But the thing to consider going forward is that you can probably gather up all the info you need with some thoughtful event handling. For instance, listen for all xhr requests, and recheck all your elements if src changed. Browser events, simple loops, and reading attributes are all well optimized.

[ejci]

I appreciate your effort. And Im really glad that you are looking on the code and for your feedback to the code. However I don't agree about the try/catch thing and also about MutationObserver.

try/catch performance Yes, in general you are kind of right (depends on JS engine though). But it doesn't make much sense to micro-optimize. Its much more idiot proof (kind of more readable) and its not causing any significant performance hit as its executed only once (on page load for each tab). I agree that the code should be written much nicer (lot of stuff can be refactored). Its written poorly because its the way how Im used to work (create proof of concept, test if people like it, add functionality step by step, test, add more functionality,...). The other thing is that this script is running in page context and as each page can have some weird stuff (overridden globals, weird libraries included,...) it can behave not like you would expect. So try/catch is safe and it will prevent any extension errors going to console.

MutationObserver Its the only reliable option to use in this case (imho). Creating some kind of "smart" event listeners will complicate lot of things and you will miss lots of cases (content created dynamically on runtime, content coming from ajax request,...). Im not aware (but maybe Im wrong) of any depreciation / abandonment of MutationObserver by Chrome. Do you have any source for that? Listening to XHR requests and rechecking attributes of all elements will cause much more performance issues then using MutationObserver (no jsperf proof for that though...).

Im not trying to say Im against refactoring. This code really needs it. But I prefer to refactor things that are actually bad. For example popup.js is a mess. Its a spaghetti code that really needs a refactoring. Let me know what you think.

[skylize]

Happy to start with the popup.

My mistake on MutationObserver, I mixed it up with DOM Mutation Events which was deprecated before a replacement existed. I've been out of the loop a few years.

Don't need to worry about clobbered globals. The extension runs in a sandbox. You can only access your own code and the DOM (I actually started to work on namespacing CONST until I remembered that it didn't matter.) Also we have the benefit of only supporting Chrome, so fewer unknowns.

I agree, idiot proof is good for prototyping. You did the prototyping already, and an excellent job of it. I'm discussing refactoring, which is a different goal. I hope I can be helpful with that. I'll get back to you when I have some real code to offer.

[ejci]

Don't need to worry about clobbered globals. The extension runs in a sandbox Good point. You are right about that.

Anyway, thanks for your time and help with cleaning the code. I really appreciate it.