Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed enhancements #28

Open
Procyon-b opened this issue Sep 27, 2019 · 3 comments
Open

Proposed enhancements #28

Procyon-b opened this issue Sep 27, 2019 · 3 comments

Comments

@Procyon-b
Copy link
Contributor

Procyon-b commented Sep 27, 2019

@gregsadetsky @cxw42

I have created 2 more branches in my fork.

The first one, dyn-final dyn-sites, implements the handling of "dynamic sites" with a mutation observer. I call dynamic sites, sites that create their form elements dynamically. These forms can't be fixed by the userscript if it doesn't watch for modifications of the DOM tree.
Since this is hostname specific, and user defined, a background page and options page have been added

A second branch, dyn-whitelist, adds support for whitelisting a domain (only for a duration of 90 seconds). Apart from an new popup.html page, the background and userscript are only very lightly modified to handle this.

Examples of dynamic site:

  • www.nytimes.com - chrome creates a CSE only when the page displayed is the root index page. And you have to wait for the page to be completely loaded (our autoDetect(true,'Load') would have been fired).
  • www.brico.be
@cxw42
Copy link
Collaborator

cxw42 commented Nov 24, 2019

@Procyon-b As always, thanks very much, and my apologies for the delay in reviewing! I added a PR (#31) for dyn-sites and am trying it out. I would like to look at that first, then move on to dyn-whitelist.

I have an update and two comments:

  • The code is large enough now that I added a code formatter for consistent style. The format is the defaults from beautifier.io. My PR has those applied.
  • I'm having a hard time following some of the short variable names. For example, in background.js:toAr(), it's not immediately obvious to me what RE and RE2 are doing. For the sake of maintainability, I would like clearer names before merging.
  • Much of the new code seems to relate to parsing the options description. Could we use an NPM module instead? For example, a quick search turned up https://www.papaparse.com/ , which converts CSV <-> JSON. Could we take CSV (or another format) as input from the user, and use a module to do the parsing? Hand-rolled parsers + user-provided data is a recipe for security holes, so I would strongly prefer to use a parser that has more people using it and finding bugs.

Please let me know your thoughts, either here or on the PR. Thanks!

@Procyon-b
Copy link
Contributor Author

Procyon-b commented Nov 24, 2019

@cxw42 No need to apologize. ;)

  • dyn-whitelist is only a small modification of dyn-sites (+25 lines in background, mostly to clear the whitelist after 90s) diff here
  • I'll check the variables name. The 2 branches are mainly a proof of concept. They need a re-write.
  • Regarding the use of a module/library, I wasn't tempted to add too much code to a small extension.
    I hate using libraries of hundreds/thousands of line just for a couple of capabilities. ;)
    Most of the time it slows things down: I wrote a patch for an extension that uses jquery. A 1-2 line(s) of jquery command takes >50 times longer than my 1 line basic js (that still fit perfectly in a jquery function expansion).
  • The only problem with user-data could be in querySelectorAll(), in the userscript. But it should bail quickly when fed with invalid data.
  • The reason I keep the data in an unparsed form is to keep the comments.

Let me explain why I ended with a messy code for handling the dynamic site list.

When I first tried the logic of MutationObserver I had the list hardcoded in the background page. And the idea was to keep it that way for the end user.
Then the reasoning came that an outdated filter could potentially overload the pages of a website: if the observer attach itself to the root (or close to) of the document, the function handling the mutation will get called for all modifications of the page. Even if this function has a small footprint, who knows what can go wrong? And the extension would stay that way until the next update.
So I made a really simple option page that could handle syntax parsing. Somehow I had to show the user what the parsed result looks like: each line is parsed and the original text is rewritten from the parsed result.
Then it got messy when I added comments, and duplicate hosts. Quickly coded but messy result. ;)
I will scrap that, and rewrite it in a more simple way.

All this is also the reason why the userscript is the file where my modifications are cleaner and more straightforward (I think).

@Procyon-b
Copy link
Contributor Author

The revised code is now available.
I have created a new branch dyn-final to replace the previous "dyn-sites". And dyn-whitelist has been recreated with the new version.

The source code is now smaller and cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants