Video Consumption Module Functionality #58
Conversation
…Haven't tested it integrated with the new version of web-science yet
|
I know this module still needs work, but if I could get an initial review to tell me what I should be looking to work on in terms of style/convention that would be really helpful. |
| * message sending Promise remains open. This error does not affect functionality, | ||
| * because we do not depend on resolving the Promise (i.e., a response to the | ||
| * page visit stop message). | ||
| * @module WebScience.Measurements.content-scripts.videoConsumption |
There was a problem hiding this comment.
Update to match the current @module conventions.
| * | ||
| * # Note | ||
| * * The console.log represent the proper functionality (if an expected console.log does not appear, there is a bug). | ||
| * * After the testing infrastructure is added, all the console.log statements can be removed. |
There was a problem hiding this comment.
Would get rid of this and the various console.log debugging output statements, and instead use the built-in Firefox debugger at about:debugging. It's exceptionally helpful.
| console.log("videoConsumption content script running"); | ||
|
|
||
| /** | ||
| * The ID for a timer to periodically check for new videos on the page. |
There was a problem hiding this comment.
Here and throughout, video elements specifically.
| */ | ||
|
|
||
|
|
||
| /* Outer function encapsulation to maintain unique variable scope for each content script */ |
There was a problem hiding this comment.
You can remove this IIFE wrapper when you switch to Rollup bundling. Our Rollup configuration for WebScience automatically outputs IIFE content scripts.
| /** | ||
| * @typedef VideoConsumption | ||
| * @type {object} | ||
| * @property {object} video - the video element from the DOM. | ||
| * @property {number} playbackStart - the timestamp when the playback started, or -1 if the playback hasn't started yet. | ||
| * @property {number} playbackDuration - the total amount of time this video has played on this page. | ||
| */ | ||
|
|
||
| /** | ||
| * Array of objects that contain details about the consumption of a particular video. | ||
| * @type {VideoConsumption[]} | ||
| */ | ||
| let videoConsumptions = []; | ||
|
|
||
| /** | ||
| * Set of video DOM elements seen. If a video DOM element has been seen, then a listener has already been attached to it. | ||
| * @type{Set<video>} | ||
| */ | ||
| let videosSeen = new Set(); |
There was a problem hiding this comment.
I'd suggest a different approach to the data structures:
- A
WeakMapthat maps video elements to unique video element IDs. When you have a DOM node in a collection you should almost always reference the node weakly, since it could be removed from the DOM. - A
Mapthat maps unique video element IDs to playback data that you'll eventually aggregate and post to the background script.
| /** | ||
| * A RegExp for the page match patterns. | ||
| * @type {RegExp|null} | ||
| */ | ||
| let matchPatternsRegExp = null; | ||
|
|
||
| /** | ||
| * The registered page navigation content script. | ||
| * @type {RegisteredContentScript|null} | ||
| */ | ||
| let registeredContentScript = null; | ||
|
|
||
| /** | ||
| * Whether to notify the page data listener about private windows. | ||
| */ | ||
| let notifyAboutPrivateWindows = false; |
There was a problem hiding this comment.
These values should be per-listener. See the pageNavigation and linkExposure PRs for examples.
| // for object inheritance | ||
| delete videoData.type; | ||
|
|
||
| onVideoData.notifyListeners([ videoData ]); |
There was a problem hiding this comment.
See the pageNavigation and linkExposure PRs for examples of how to make this notification listener-specific.
| export async function startMeasurement({ | ||
| matchPatterns // causes error when empty, so we need to change default value | ||
| }) { |
There was a problem hiding this comment.
Here and throughout, wouldn't use the term "measurement" for what the module is doing. We've moved away from that model, and are instead following WebExtensions event conventions.
| pageVisitStartTime: "number", | ||
| pageVisitStopTime: "number", | ||
| playbackDuration: "number", | ||
| ytChannel: "string", // TODO: what to put here if this might be empty? |
There was a problem hiding this comment.
Would just send an empty string if there is no channel.
| function stopMeasurement() { | ||
| messaging.unregisterListener("WebScience.Measurements.VideoConsumption.VideoData", videoDataListener) | ||
| registeredContentScript.unregister(); | ||
| registeredContentScript = null; | ||
| notifyAboutPrivateWindows = false; | ||
| matchPatternsRegExp = null; | ||
| } |
There was a problem hiding this comment.
See the pageNavigation and linkExposure PRs for examples of how to do per-listener teardown.
Fixes #26.
Haven't tested it integrated with the new version of web-science yet, but if this code can get upstreamed to web-science, the radicalization study should be easier to build.