Re: [PATCH] Add the capability of creating and restoring backups of the user's whitelisted and blacklisted websites

[Yuchen Pei]

Hi Autumn64 (Mónica?),

Thanks a lot for the patch.

On Tue 2024-01-23 11:33:11 -0600, Autumn64 wrote:

> ---
>  NEWS                                          |  4 ++
>  html/preferences_panel/pref.js                | 51 ++++++++++++++++++-
>  html/preferences_panel/preferences_panel.html |  8 +++
>  html/preferences_panel/prefs.css              | 12 +++++
>  manifest.json                                 |  2 +-
>  package.json                                  |  7 +++
>  6 files changed, 82 insertions(+), 2 deletions(-)
>  create mode 100644 package.json

Can you please add some tests? Some cases in test/spec/LibreJSSpec.js
that exercise the functionality, as well as some edge cases should be
sufficient.

> diff --git a/NEWS b/NEWS
> index d4b730c..27b130f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,3 +1,7 @@
> +New in 7.21.2
> +     * Add the capability of creating and restoring backups of the user's
> +     whitelisted and blacklisted websites
> +
>  New in 7.21.1

>    * Fix eval loophole with Function constructor
> diff --git a/html/preferences_panel/pref.js b/html/preferences_panel/pref.js
> index d90e72e..5c52447 100644
> --- a/html/preferences_panel/pref.js
> +++ b/html/preferences_panel/pref.js
> @@ -215,6 +215,53 @@
>          options.appendChild(option);
>        }
>        widget.appendChild(options);
> +    },
> +
> +    createBackup(){
> +      this.syncAll();
> +      browser.storage.local.get(["pref_whitelist", 
> "pref_blacklist"]).then((result) =>{
> +        const whitelisted = result.pref_whitelist || "";
> +        const blacklisted = result.pref_blacklist || "";
> +
> +        const backup_data = JSON.stringify({
> +          pref_whitelist: whitelisted,
> +          pref_blacklist: blacklisted
> +        });
> +        const blob = new Blob([backup_data], {type: "application/json"});

This causes each of the two lists to be represented as one single string
of comma separated patterns, which is not as user-friendly than a list
of strings, if the user wants to edit the json by hand. Can we make it a
list of strings?

JSON is plaintext and better than the opaque format in the firefox
sqlite database[1]. But it does not allow comments. One could say add
"comments" fields in the json file but that is a hack and pretty
inconvenient way to leave comments. It would be nice to adopt a
plaintext format that allows user comments so that users can document
the file inline. How about say the Unix style conf format, used by
dunst, xdg, mpv, etc.?

> +        const link = document.createElement("a");
> +        link.href = URL.createObjectURL(blob);
> +        link.download = "librejs_backup.json";
> +        document.body.appendChild(link);
> +        link.click();
> +        document.body.removeChild(link);

Looks hacky. How about change the create/restore buttons to "a"
elements, and add a mousedown event listener to the create link, which
creates the json file content and updates directly the attributes of the
"a"? Like how Redirector does it (see js/importexport.js):

https://github.com/einaregilsson/Redirector

> +      });
> +    },
> +
> +    restoreBackup(){
> +      const input = document.getElementById("fileInput");
> +      input.click();
> +
> +      input.addEventListener("change", (event) =>{
> +        const file = event.target.files[0];
> +
> +        if (!file) return;
> +

Please remove the trailing space.

> +        const reader = new FileReader();
> +        reader.onload = (e) =>{
> +          const content = e.target.result;
> +          const data = JSON.parse(content);
> +          if (data.pref_whitelist === undefined || data.pref_blacklist === 
> undefined) return;
> +
> +          const realData = {
> +            pref_whitelist: data.pref_whitelist,
> +            pref_blacklist: data.pref_blacklist
> +          }
> +          browser.storage.local.set(realData);
> +          this.syncAll();
> +        };
> +
> +        reader.readAsText(file);
> +      });
>      }
>    };

> @@ -257,10 +304,12 @@
>      click(e) {
>        const { target } = e;
> -      const match = /^cmd-(white|black|delete)(list-site)?/.exec(target.id);
> +      const match = /^cmd-(white|black|delete)(list-site)?/.exec(target.id) 
> || /^cmd-(create|restore)(backup)?/.exec(target.id);
>        if (!match) return;
>        e.preventDefault();
>        const cmd = match[1];
> +      if (cmd === "create") Controller.createBackup();
> +      if (cmd === "restore") Controller.restoreBackup();
>        if (cmd === "delete") {
>          Controller.deleteSelection();
>          return;
> diff --git a/html/preferences_panel/preferences_panel.html 
> b/html/preferences_panel/preferences_panel.html
> index 081ae07..98458ed 100644
> --- a/html/preferences_panel/preferences_panel.html
> +++ b/html/preferences_panel/preferences_panel.html
> @@ -76,6 +76,14 @@
>          <label for="pref_body">Body</label>
>          <textarea id="pref_body" rows="5"></textarea>
>        </fieldset>
> +

Please remove the trailing space.

>  [... 47 lines elided]

> diff --git a/package.json b/package.json

The package.json file is unnecessary, please remove it from the patch.

> [... 13 lines elided]

Best,
Yuchen

--
Dr Yuchen Pei | https://ypei.org | Timezone: UTC+11
PGP Key: 47F9 D050 1E11 8879 9040  4941 2126 7E93 EF86 DFD0
https://ypei.org/assets/ypei-pubkey.txt