Add embedded player context menus - #462
Conversation
d13e159 to
4c501d4
Compare
4c501d4 to
f645829
Compare
| export const GetYouTubeURLFromMenuInfo = ( | ||
| info: ContextMenuClickData, | ||
| ): string | undefined => { | ||
| const candidates = [info.frameUrl, info.linkUrl, info.srcUrl, info.pageUrl]; |
There was a problem hiding this comment.
High / wrong target: Chrome supplies both frameUrl (the containing frame) and linkUrl (the clicked href) for a link inside an iframe. Because frameUrl is always first, a link or playlist action inside an embedded player opens the current frame URL instead of the link the user selected. The test that prefers the frame URL currently codifies this mismatch. URL selection needs to follow the menu action/target type rather than one global priority order.
| { | ||
| frameId, | ||
| }, | ||
| )) as VideoPlayerInfo | undefined; |
There was a problem hiding this comment.
High / wrong playing video: the frame response carries time and dimensions but not the current video identity. A /embed/videoseries?list=... document has no video ID, and a playlist frame can advance while its document URL still identifies the original/static entry. OpenPopoutBackgroundHelper then derives the ID only from that frame URL, so the captured timestamp can be applied to the first or wrong video instead of the one being played when the menu was opened.
| rotation: number = 0, | ||
| ): Promise<boolean> => { | ||
| return OpenMenuPopout(info, tab, { | ||
| includeList: true, |
There was a problem hiding this comment.
High / video-vs-playlist behavior: every frame action, including the item labeled Open Embedded Video, hard-codes includeList: true. For /embed/VIDEO_ID?list=..., choosing that video action therefore opens the playlist, and there is no distinct embedded-playlist action. The tests named embedded video/playlist use the link menu IDs, so they never exercise OpenEmbeddedVideo and miss this branch.
| } | ||
|
|
||
| if (success !== true) { | ||
| NotifyMenuActionFailed(); |
There was a problem hiding this comment.
Cross-PR integration risk with #471: this layer maps every non-true result to the generic failed-to-determine-video/playlist notification. #471 deliberately reports open/context-preservation failures with more specific notifications. If the branches are combined unchanged, a handled failure can produce a second, misleading generic notification. The popout.ts conflict and notification ownership need to be reconciled together.
|
|
||
| const menus = await GetMenus(); | ||
| menus.forEach((menu) => browser.contextMenus.create(menu)); | ||
| for (const menu of menus) { |
There was a problem hiding this comment.
High / MV3 lifecycle: the click listener above is registered only after removeAll has been awaited. Chrome requires extension event listeners to be registered synchronously during initial service-worker script execution; otherwise an event that wakes a dormant worker can be dispatched before the listener exists and be missed. That means the persisted menu can be clicked after worker termination, InitMenus starts, and the click does nothing. Register onClicked synchronously at initialization and keep asynchronous menu rebuilding separate.
rthaut
left a comment
rthaut
left a comment
There was a problem hiding this comment.
Critical review summary
I found four high-severity correctness/lifecycle issues that should be treated as must-fix before merge. I left inline comments on each concrete path.
Correctness and MV3
- Dormant MV3 workers can miss menu clicks. CreateMenus awaits contextMenus.removeAll before registering onClicked, but Chrome requires extension service-worker listeners to be registered synchronously during initial script execution: https://developer.chrome.com/docs/extensions/develop/concepts/service-workers/events#declare-events. A click that wakes the worker can arrive before the listener exists.
- Link actions inside embeds resolve the wrong target. OnClickData can contain both frameUrl and linkUrl, but the global candidate order always chooses frameUrl. The link/playlist action therefore opens the containing player rather than the clicked link.
- Playing playlist frames lose the current video identity. The frame response includes time/dimensions but no video ID/URL. videoseries embeds have no ID in the document URL, and advancing playlists can keep a static frame URL, so the captured timestamp can be applied to the first or wrong video.
- The embedded action labeled as a video action always preserves playlist context. OpenEmbeddedVideo hard-codes includeList: true, while no separate embedded-playlist action exists. This contradicts the video-only versus playlist distinction described in the PR.
There is no restart-sensitive mutable module state beyond constants; the lifecycle problem is listener timing rather than lost state.
Cross-PR integration
The success fallback in OnMenuClicked will emit the generic failed-to-determine notification for any non-true result. When #471 is combined, that can duplicate or overwrite the meaning of its specific open/context-preservation notifications. The popout.ts conflict and notification ownership need deliberate reconciliation, as #471 already notes.
Tests and types
The new tests cover basic URL selection, list inclusion for link menu IDs, failure notification, and IsPopoutPlayerURL parsing. They do not cover service-worker restart dispatch, the actual OpenEmbeddedVideo IDs, current-video identity in videoseries/advanced playlists, current-popout rotation dispatch, or the documentUrlPatterns globs. Two tests named as embedded video/playlist cases use link menu IDs, which is why the hard-coded embedded includeList behavior passes unnoticed.
VideoPlayerInfo is structurally used consistently for time/size, but it is not sufficient for the risky playlist path because it omits the current video identity and is accepted through an unchecked sendMessage response cast.
i18n and maintainability
All new English message keys are referenced; I found no orphaned new key or missing new submenu label. I also found no hard documented-standard violation. Non-blocking maintainability concerns are the duplicated action matrix between menu creation and click dispatch, identical delegate wrappers for embedded-video and playlist actions, and the repeated time/width/height data clump across content and background layers.
A lower-risk parsing edge remains in YOUTUBE_POPOUT_PLAYER_URL_PATTERNS: ?__ytpp= and &__ytpp= are substring globs, so a parameter such as not__ytpp=1 can expose rotation menus even though IsPopoutPlayerURL correctly rejects it.
Validation
Pinned head: 7983ddb against merge-base 5e23ba3, using Node 24.18.0.
- npm ci: passed (audit reported 34 existing dependency vulnerabilities: 2 low, 6 moderate, 22 high, 4 critical)
- npm run compile: passed
- npm run lint: passed
- npm run test:unit: passed (59/59)
- npm run build:chrome: passed
- npm run build:firefox: passed
- git diff --check: passed
Both builds emitted the existing greater-than-500-kB chunk warning. I did not implement fixes or make a formal approval/request-changes decision.
|
Review complete on PR head 7983ddb (comment-only; no approval/request-changes verdict). Result: four high-severity findings were posted inline:
I also flagged the #471 duplicate/misleading-notification merge hazard, the substring-glob edge in popout URL menu patterns, and meaningful test gaps. New i18n keys are all referenced, and I found no restart-sensitive mutable state or hard documented-style violation. Local validation passed: npm ci, compile, lint, 59/59 unit tests, Chrome MV3 build, Firefox MV3 build, and git diff --check. npm audit reported 34 dependency vulnerabilities, and both builds retained the existing large-chunk warning. |
1 participant
Summary
__ytppURL parameter.Closes #456.
Validation
npm run test:unitnpm run compilenpm run lintnpm run build:chromenpm run build:firefox