Hello Yuchen, On 24-02-04 01:50, Yuchen Pei wrote:
Hi Mónica, On Tue 2024-01-30 15:04:22 -0600, Mónica Gómez wrote:Signed-off-by: Mónica Gómez <[email protected]> --- NEWS | 4 + html/preferences_panel/pref.js | 85 ++++++++++++++++++- html/preferences_panel/preferences_panel.html | 10 ++- html/preferences_panel/prefs.css | 16 ++++ manifest.json | 2 +- test/spec/LibreJSSpec.js | 43 ++++++++++ 6 files changed, 154 insertions(+), 6 deletions(-)Thanks a lot for the updated patch. The next two weeks I have less time to sit down at my computer for some uninterrupted time outside of work, so please bear with me with the actual review. Meanwhile I took a look at the patch and a few questions come to my mind that I would like to figure out (but if anyone has answers feel free to reply):
Thanks for your response.
This could definitely be adapted to read the config file on startup. It could be implemented as a separate function that reads the config file at the specified path or, to avoid repeating code, the `restoreSettings` function could read an argument with the route of the config file, and if such argument is `undefined` or `null` then it could show the open dialog. That way, we could use the same function to read the conf file on startup, and at the same the user could choose their own config file in case they want to change it, that then can be copied to the default LibreJS settings folder.- how will this feature work with or be developed into incorprating the other idea of having librejs read the config file for black and white lists on startup?
I wasn't sure about using an existing library because I was afraid of involuntarily making LibreJS less lightweight and/or less maintainable. For that I would appreciate if anyone could give their opinion about which is the best option.- in your patch it reads and writes a conf file by manually parsing - could it be better or necessary to use an existing library that handles such files, or would it be easy to do the manual parsing now and switch to a library when needed?
From the manual tests I made using IceCat and Firefox it seems to handle such inline scripts correctly. I suspect you mean that it might give some problems since inline scripts include characters like `#` and `//` that could be mistaken for comments. I was aware of that, so I implemented it in a way that only the lines that begin with `#` are considered as such, and if a given line includes `#` and `//` characters but they're not at the beginning then it is read as a normal URL. As I have never really worked with conf files in a development environment before, I'm not sure if what I did would be considered as a hack, or if it was the right way to implement it.- external scripts are ok in the conf file as they are shown as "normal" http(s) urls with some globbing patterns, like https://forum.members.fsf.org/* but inline scripts are represented in a funny way, like so --8<---------------cut here---------------start------------->8---inline://www.fsf.org#(<SCRIPT>) // @license mag…8855c4df2063479cf018bb81d75c6434c99b104fa0fcc40a45920792711650ec--8<---------------cut here---------------end--------------->8--- The above is what represents the inline scripts on the fsf homepage: --8<---------------cut here---------------start------------->8---// @license magnet:?xt=urn:btih:1f739d935676111cfff4b4693e3816e664797050&dn=gpl-3.0.txt GPL-3.0-or-latervar _paq = window._paq = window._paq || [];/* tracker methods like "setCustomDimension" should be called before "trackPageView" */_paq.push(['trackPageView']); _paq.push(['enableLinkTracking']); (function() { var u="https://piwik.fsf.org/";; _paq.push(['setTrackerUrl', u+'matomo.php']); _paq.push(['setSiteId', '5']);var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0]; g.type='text/javascript'; g.async=true; g.src=u+'matomo.js'; s.parentNode.insertBefore(g,s);})(); // @license-end --8<---------------cut here---------------end--------------->8--- will such lossy encoding reliably enough?
As for now they just cover the act of creating a config file and then trying to read it. I struggled quite a bit in figuring out how to implement the tests, so I would appreciate a lot if anyone could give some feedback before adding the inline url cases.- what do the tests cover? do they cover for example inline url cases?
-- -------------------------------------- Best regards, Mónica Gómez <[email protected]> PGP Key: C6E176B5ED0712210C205917DE3142A1D89C649B https://www.autumn64.xyz
OpenPGP_0xDE3142A1D89C649B.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
