-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Video Consumption Module Functionality #58
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest a different approach to the data structures:
- A
WeakMap
that 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
Map
that 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.