Various fixes

[lbmaian]

Commits:

1. Watchmarker balks on the recommended videos in the YT watch page. refresh now handles such video recommendations
2. Fix minor switch-like if/else chain issue.
3. chrome.runtime.onMessage listeners are now explicitly synchronous. This prevents "The message port closed before a response was received." in chrome.runtime.lastError that causes funcSendmessage to effectively always retry 100 times.
4. Improve regex usage, removing unnecessary groups
5. Replace unreliable thumbnail request-based lookup with explicit lookups from content script via new youtubeLookup message. It was unreliable due to image caching sometimes preventing thumbnail requests. This mostly applies to shorts, since watched non-shorts should be handled by youtubeEnsure.
6. Revision of commit 5: Make youtubeLookup reply via callback rather than sending youtubeMark. Also now makes the background chrome.runtime.onMessage listener asynchonous while still avoiding the error that was fixed in commit 3.

You seem to be using some sort of transpiling framework so this may not be directly merge-able, but it shouldn't be difficult to replicate these changes in your pre-transpiled source.

[sniklaus]

Lots of good stuff in here. I'll merge and then make a commit to clean up some stuff. Thank you!

[sniklaus]

Quick question, as shown below your modifications no longer mark videos as watched if we can't figure out their title. To me, it seems better to mark a video as watch that we know has been watched even if we don't know the title. Or am I missing something?

            var objTitle = document.querySelector('a[title][href^="/watch?v=' + strIdent + '"], a[title][href^="/shorts/' + strIdent + '"], a[href^="/watch?v=' + strIdent + '"] #video-title[title]');
            if (objTitle === null) {
                console.error('could not find title for video', strIdent)
            } else {
                chrome.runtime.sendMessage({
                    'strMessage': 'youtubeEnsure',
                    'strIdent': strIdent,
                    'strTitle': objTitle.title
                });
            }

[sniklaus]

It would also be great to hear why if (objData.tabId < 0) { changed to if (objTab.id < 0) {. I am sure you have a reason for it, I just want to learn.

[lbmaian]

Thanks for merging!

Quick question, as shown below your modifications no longer mark videos as watched if we can't figure out their title. To me, it seems better to mark a video as watch that we know has been watched even if we don't know the title. Or am I missing something?

I wasn't sure if you actually wanted them to be ensured watched, because before this change, it simply errored and so wouldn't mark it (and subsequent progress videos) as watched. It probably does make sense to ensure they are watched.

It would also be great to hear why if (objData.tabId < 0) { changed to if (objTab.id < 0) {. I am sure you have a reason for it, I just want to learn.

objData in chrome.tabs.onUpdated listener is actually changeInfo and doesn't contain a tabId. See its schema here: https://developer.chrome.com/docs/extensions/reference/tabs/#event-onUpdated

[lbmaian]

Although now that I look at it, I think you could just use the first argument (intTab) rather than objTab.id in the chrome.tabs.onUpdated listener.

[sniklaus]

I wasn't sure if you actually wanted them to be ensured watched, because before this change, it simply errored and so wouldn't mark it (and subsequent progress videos) as watched.

I just checked again and the background script ignores videos with unknown title in the ensure mechanism as shown below. I think I will still stick with the original code, it will cause some unnecessary message but it is less code. And less code is good code. :slightly_smiling_face:

https://github.com/sniklaus/youtube-watchmarker/blob/50c90d591662372cf4e7df973d8be2b2f93ae0d0/background.js#L967

objData in chrome.tabs.onUpdated listener is actually changeInfo and doesn't contain a tabId. See its schema here: https://developer.chrome.com/docs/extensions/reference/tabs/#event-onUpdated

Thanks for clarifying, guess the tabId used to be supplied but no longer is. Good catch!

Although now that I look at it, I think you could just use the first argument (intTab) rather than objTab.id in the chrome.tabs.onUpdated listener.

Yeah, I would think so too. But why didn't I use the first argument in the first place then when I wrote the code? There must be more to the story. :thinking:

Last question, again just asking to learn. You are using a if (objSender.tab && objSender.tab.id >= 0) { before calling the Youtube.lookup function, should we have this check before Youtube.ensure as well?

[lbmaian]

I just checked again and the background script ignores videos with unknown title in the ensure mechanism as shown below. I think I will still stick with the original code, it will cause some unnecessary message but it is less code. And less code is good code. 🙂

It originally errored due to the querySelector returning null, and thus querySelector(...).title throwing an error. Even if you did pass a null/undefined title to the background, it would've just errored there in strTitle.trim().

edit: Actually, that's still technically possible if objTitle.title is somehow still null/undefined, but as long as objTitle is found, I didn't see objTitle.title ever null/undefined in practice. edit2: Nevermind, strTitle is coerced to empty string if it's null/undefined in Youtube.ensure objGet

The main change was the addition of this to the querySelector call: a[href^="/watch?v=' + strIdent + '"] #video-title[title]'

That's still necessary to find titles for video recommendations. The check/error-log was just added for safety.

[lbmaian]

Last question, again just asking to learn. You are using a if (objSender.tab && objSender.tab.id >= 0) { before calling the Youtube.lookup function, should we have this check before Youtube.ensure as well?

I'm not sure the tab id check is actually necessary. I only added it for youtubeLookup because the callback is actually used by the tab's content page, while youtubeEnsure is fire-and-forget, and I saw tab id checks elsewhere in the code.

[lbmaian]

BTW, before this PR, video recommendations did often show up as marked but that was due to the thumbnail-request-based lookup, i.e. if the database already had it as watched. Though as stated in the PR description, the thumbnail-request-based approach was unreliable due to image caching (and was thus replaced).

The first commit of this PR ensures that those video recommendations are registered in the database as watched via youtubeEnsure and marked as watched client-side.

[sniklaus]

The main change was the addition of this to the querySelector call: a[href^="/watch?v=' + strIdent + '"] #video-title[title]'

Ahh, I see that now, thanks for explaining!

I'm not sure the tab id check is actually necessary. I only added it for youtubeLookup because the callback is actually used by the tab's content page, while youtubeEnsure is fire-and-forget, and I saw tab id checks elsewhere in the code.

I think I will try without the check and see what happens. :slightly_smiling_face:

[sniklaus]

I pushed 0cb11b1f30c17243505710aa14741a3ac9c5c2b8 to make some style changes and packaged it up to try, links for Chrome and for Firefox (rename the Firefox one to "manifest.xpi"). I will try it for a week and then release it if I don't find any issues. Let me know if you get a chance to try it to see whether it incorporates everything you found, no worries though in case you are busy.

[lbmaian]

It still looks correct to me, though I did leave some comments on that commit.

[sniklaus]

That is very kind of you, thank you!

[sniklaus]

I just published the new version, thank you for this awesome contribution!

[lbmaian]

You're welcome!