A few updates to popup.js

[skylize]

The chages aren't as massive as the diff makes them look, mostly just moved things around. All changes were only to popup.js, except adding a global I missed to the header comment on background.js.

The globals comment is mainly for JsHint. It also adds clarity since you can see what's not local to the file, like import statements in other languages.

1. Put the updates to slider inputs into loops. Found one loop was used several times, so I put it in a function.

2. Removed the try/catch blocks. I analyzed them hard, and determined they were noise in the code with little-to-no real value.

   As we discussed, we're already in a sandbox. All you were doing with any potential error is a console.log on the background page which isn't worth it. If something's misbehaving, just right click the popup and click Inspect Popup. The only risk is the extension doesn't work, which will probably happen anyways if there's an error.

   Another major point to keep in mind is try/catch is completely synchronous. If you do something inside the block that has a callback, errors in the callback will not be caught. Same goes for event listeners. So it's probably not offering you the safety you think it is.

   I did save the message passing code as a function at the bottom. If later one of us wants to put any try/catch blocks back in, we can just do try {...} catch(e) {forwardErrToBackground(e);} to get that functionality back.

3. Removed several if (chrome...) functionality tests. They aren't needed because we're inside an extension that only runs in chrome.
4. Renamed a few things and moved some things around for clarity. Abstracted a few bits of repeated code. Tried to pull a value off the current event target, allowing deletion of a function that loops over DOM elements (but that introduced a bug and I ultimately had to return the old code, we'll solve it later if doesn't get wiped away in the refactoring)
5. Did some more abstracting of duplicate code. Fixed the bugs I caused.

Things are pretty tightly coupled with presets.js in a way that's less than ideal. Presets unintuitively handles a lot more than than the presets. I'm going to dig into that next. I think I'll need to break things apart in a more drastic way.

So far I'm just packing together similar actions and trimming fat so it's easier to follow what's going on. I'm finding there is state kept in too many different places, too many magical functions that "change something somewhere" and manage to lead to the right result.

What I'm basically going to shoot for is 3 clear layers. One layer will respond to events with simple instructions ("this is what I want to do"), another layer will be functions to handle the logic of those instructions ("this is how I'm gonna do it"). Finally a layer at the bottom has the sole job of storing/retrieving state ("this is what I have done").

I want to do all this through only small commits that don't break anything. You might see some pull requests that look kind of ugly as things get reorganized.

[ejci]

Thanks for the awesome work and for your time. I will test it and merge it. Im adding one piece of functionality today and will publish new version soon.

As we discussed, we're already in a sandbox. All you were doing with any potential error is a console.log I was only concerned about page script. I looked at the docs and also page context scripts are running in separate context. For some reason (my bad) I was thinking the injected script shares the same context as others scripts on page. The try/catch blocks were there mostly to remove chrome.* errors when developing (see my comment bellow)

Removed several if (chrome...) functionality tests. They aren't needed because we're inside an extension that only runs in chrome. Yes its useless ... but I was using it to debug the popup page as a "regular" page (served with simple http server). In that case some of chrome.* stuff was throwing errors as it was undefined (as its not running in extension context).

too many magical functions that "change something somewhere" and manage to lead to the right result Yep :) Thats why I pointed you to that file and not to page.js as there was lot of "magic". I had a plan to completely rewrite it with Angular as I wanted to use Material design (https://material.angularjs.org). The presets.js file was a start to decouple at least parts of logic but I ended with more chaos :D because I didnt have time to do it properly.

Thanks again. Pls update also readme.md and add your self as contributor. I will also then mention you in chrome webstore.

[skylize]

Some random thought as I'm looking through code.

• You have a tendency to init variables as false. It would help me understand your intentions better if you reserve false for variables actually intended to be booleans. Instead, just declare it. It gets a default value undefined. That gives the same result when you say if (myvar) doSomething(); Same for return false at the end of a function that would never return true. I can understand better what's happening if the non-values somewhat reflect the possible future values. Some other options include null for intended object, '' for intended string, 0 for intended number.
• Looking at this crossorigin hack in page.js. Sometimes the orgin will be https, which means anonymous credentials won't work. I would guess in that scenario a session has already been created with the needed cookies. If my thought is right, we could do a simple test on the src attribute. If src is 'https...' instead of 'http...', then set crossorigin to use-credentials instead of anonymous. Does that sound like it would work?

[ejci]

• You have a tendency to init variables as false. It would help...

Yes I agree its a good practice. No need to explain why. Let say I have previous experience with JS development ;) but Im "lazy" to follow good practices when Im prototyping.

• Looking at this crossorigin hack in page.js...

My understanding is that use-credentials has nothing to do with https. Its about exchanging the user credentials and not about the protocol (http://www.w3.org/TR/cors/#user-credentials). Im still testing daily on various pages to make sure that the "hack" is working. Im adding also switch on/off per domain (context menu option) as not all pages are streaming media with proper headers.

[skylize]

Yes I agree its a good practice. No need to explain why.

Cool. There's no way for me to know whether something is a bad habit, or a concept you haven't thought about, or maybe even intentional because you just like it that way. So instead of just asking you to do something different, I wanted to share how it could help if you made a small adjustment in the future. Since you're clearly knowledgeable on the subject, I'll try to avoid long winded explanations of commonly known best practices. I welcome any criticism of my code or practices that will help blend with your own style. I'm already making the adjustment of using semicolons, which in my own code, I only use at the beginning of lines that start with (.

My understanding is that use-credentials has nothing to do with https.

Here's the first sentence of the link you gave me.

The term user credentials for the purposes of this specification means cookies, HTTP authentication, and client-side SSL certificates that would be sent based on the user agent's previous interactions with the origin.

So it clearly has something to do with HTTP. I very briefly scanned some of the other info on the page. My best interpretation so far is that a server could theoretically be expecting cookie authentication, in which case you would want to provide the credentials. However the type of security model that would require that is strongly discouraged. As such, I think it's safe to ignore it for now, but keep it in mind as a possible solution if we're getting bug reports for authenticated websites.