search

mozilla / lightbeam-we

Web Extension version of the Firefox Lightbeam add-on
https://addons.mozilla.org/en-GB/firefox/addon/lightbeam/
Mozilla Public License 2.0
179 stars 61 forks source link

Refactor store.setThirdParty #178

Closed biancadanforth closed 7 years ago

biancadanforth commented 7 years ago

Removed the nested if statements for clarity. This may help in troubleshooting #110.

Also added an eslint rule to ensure there's a space on either side of operators so we don't get things like:

if (someVar ===4) {
  // do stuff
}
jonathanKingston commented 7 years ago

This has introduced some linting issues in viz.js which I was going to raise at the time :).

I also need to double check the logic changes as it's not just a refactor, this is also part of the most complex code in Lightbeam so I'm nervous to merge right now.

jonathanKingston commented 7 years ago

It looks like isVisible = false is never set, which from my initial checking looks like it would never be treated as having an allowList at all.

biancadanforth commented 7 years ago

This was my husband's attempt to prove that the logic could be greatly simplified (he's been saying this for weeks now, and took a stab at it last night). The way I checked it was to compare the visualization on this branch to master for a few sites, which is not a complete check I acknowledge.

Let's leave the logic as is for now; I'll revisit this time permitting, otherwise you can close it. :)

jonathanKingston commented 7 years ago

I didn't actually load it, I just dunno how it would ever be a not visible node from the code I looked at. I can leave it open however I worry about changing this.

biancadanforth commented 7 years ago

Going to close this PR for now, as it's not critical. We can always revisit later.