2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Hello, thanks for answering! I'm very green on bash so thanks you for the insights. I wanted the script not to remove mergedList if it was already present. So in fact i'm comparing names, strings. Should i use single o r double quotes here too? \$\endgroup\$ – WhiteEyeTree Oct 28 '15 at 8:03
  • \$\begingroup\$ Unlike C, either single or double quotes can surround a string (and there's no such thing as a "char"; it's just a short string.) The difference is in how special characters are handled. Single quotes do nothing to any special characters; the string contents is exactly what you see inside the single quotes. (Of course there is no way at all to include a single quote in a single quoted string, but there are ways around that if it's what you need.) Double quotes are more involved; see here for a good starting point: unix.stackexchange.com/a/68748/135943 \$\endgroup\$ – Wildcard Oct 28 '15 at 8:15
  • \$\begingroup\$ But take another look at your script; the way filename is set (by a loop over the file glob expansion of *.txt) there is no way that the variable $filename could ever NOT end in .txt. So it will never contain the string mergedList. \$\endgroup\$ – Wildcard Oct 28 '15 at 8:18
  • \$\begingroup\$ Will do, thank you again! You're right, mergedList will never have the .txt format, i should have compared basenames but it would be pointless since that check is true all the times. Must have slipped that out! Cheers \$\endgroup\$ – WhiteEyeTree Oct 28 '15 at 9:57
  • \$\begingroup\$ Sure! :) If my answer was helpful, the way we say thanks on stackexchange is with an upvote, and/or clicking the checkmark to "accept" the answer. codereview.stackexchange.com/help/someone-answers \$\endgroup\$ – Wildcard Oct 28 '15 at 10:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.