Chrome extension to get youtube video's URL

Question

I needed something that would get the URL of a chosen video from YouTube and store it in a file.

Since I didn't find a way to write/edit an existing file with the URL alone, I had to use a patchwork of JavaScript and bash to stitch together the complete URL.

I'm asking if there were things I could have done better or with a different approach.

Here's the code of the extension:

manifest.json:

{   
    "manifest_version": 2   ,

    "name": "Adder",
    "description": "Add video's URL to file for future download",
    "version": "1.3.1",
    "icons": { "128": "icons/square_youtube.png"},
    "background": { 
        "scripts": ["background.js"],
        "persistent": false
     },

     "permissions": [
        "tabs",
        "downloads",
        "declarativeContent"
    ],

    "page_action": { 
        "default_name": "Adder", 
        "default_icon": "icons/square_youtube.png", 
        "default_popup": "popup.html", 
        "default_title": "Add it!"  
    }
}

background.js:

// Update the declarative rules on install or upgrade.

chrome.runtime.onInstalled.addListener(function() {
    chrome.declarativeContent.onPageChanged.removeRules(undefined, function() {
            chrome.declarativeContent.onPageChanged.addRules([{
                conditions: [
                    // When a page contains a <video> tag...
                    new chrome.declarativeContent.PageStateMatcher({
                        css: ["video"]
                    })
                ],
                // ... show the page action.
                actions: [new chrome.declarativeContent.ShowPageAction() ]
            }]);
    });
});

popup.js (popup.html just call this script):

var stripThis = "https://www.youtube.com/watch?v=";

chrome.tabs.query({active: true, lastFocusedWindow: true}, function(tabs) {
    var currentUrl = tabs[0].url;
    var smallUrl = currentUrl.substring(stripThis.length, currentUrl.length);
    var pathDL = "URLs/"+smallUrl+".txt";

    chrome.downloads.download({url: currentUrl, filename:pathDL}, function(downloadId) {});
});

Let me explain what this does in particular:

I built a panel_action extension that only shows if there is a video in the page. Then if I click on the icon in the omnibar, the extension download the WHOLE html page in a .txt file named after the videoID.

Then using a bash script I take all the names of the files and merge them in a single list.

Here's the bash script:

#!/bin/bash
DownloadAndConvert() {
    echo Avvio...
    echo Importo URLs da files...
    echo creo lista unificata...
    cd /home/faceless/Dropbox/URLs/
    count=`ls -1 *.txt 2>/dev/null | wc -l`
    YTBSTR="https://www.youtube.com/watch?v="
    if [ $count != 0 ]; then
        touch mergedList
        for filename in *.txt; do
            echo $YTBSTR$(basename "$filename" .txt) >> mergedList
            if [ $filename != mergedList ]; then
                rm $filename
            fi
        done
        download
        cleanup
    elif [ -e mergedList ]; then
        echo La lista è già presente! - Procedo...
        download
        cleanup
    else
        echo Non ci sono video! - Esco
        exit
    fi
}
download() {
    echo Inizio Scaricamento...
    youtube-dl -x -a "mergedList" --no-playlist 
}

cleanup() {
    echo Pulisco lista...
    rm mergedList
    echo FINE
}

trap exit SIGINT
DownloadAndConvert

I then use another program (built exactly for this task) to download and convert as desired all the video in that list.

This system feel a lot clumsy and it is mostly a patchwork of snippets of code took here and there (especially the JavaScript ones). That's why I'm asking for insights for improvements!

In the bash script, I could most likely just use the videoID instead of the whole URL. I still have to check youtube-dl documentation, but it doesn't change much.

I chose the download location (with some ln hacks) in such a way that the videoID.txt would end up in the same shared folder in dropbox.

Answer 1

I don't know Javascript, but I will review the bash part.

First, replace if [ $count != 0 ]; then with if [ "$count" -ne 0 ]; then. The double quotes are a best practice though not crucial in this case (more on that later). The -ne is for arithmetic comparisons; != is for string comparisons.

The next bit which jumped out at me was:

if [ $filename != mergedList ]; then
    rm $filename
fi

Two things here: One, unless you have a very specific reason not to, you should always put $variablenames in double quotes. Otherwise special characters can break your script. (You did the same thing earlier in the script with YTBSTR and count, but those don't really matter because due to the way you assigned their value, you can determine that it is safe. Still, it wouldn't hurt to double quote them.) In this context the lack of double quotes is not safe.

(Actually I read somewhere that putting the static value first is safer, i.e. [ mergedlist != "$filename" ], but I don't know enough to be sure if that is true or to explain why.)

Two: This line compares a string $filename which was gotten from a file glob for *.txt, with a static string mergedList. The two will never, ever match. $filename will always end in .txt because of the way it is defined. So you may as well remove that check entirely!

Next, you should single quote the output of echo. In this case your echo commands will work, but for instance if you remove the space after the ! you will get an error. Single quoting disables all special character interpretation.

There are definitely some other things here to fix—for instance there seem to be some assumptions made about the file mergedList that could cause weird errors or even security holes—but the above is a start.