Disable widevine prompt in Tor windows and force HTTPS - #39423
Disable widevine prompt in Tor windows and force HTTPS#39423diracdeltas wants to merge 4 commits into
Conversation
📋 Code Owners Summary5 file(s) changed, 1 with assigned owners 1 team(s) affected: Owners and Their Files
|
There was a problem hiding this comment.
Pull request overview
Disables Widevine permission prompts in Tor windows and requires encrypted Widevine component downloads.
Changes:
- Suppresses Widevine install/restart prompts for Tor profiles.
- Replaces the Chromium patch with a Brave installer policy requiring HTTPS.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
browser/brave_drm_tab_helper.cc |
Excludes Tor profiles from Widevine prompts. |
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc |
Adds the HTTPS-enforcing installer wrapper. |
patches/chrome-browser-component_updater-widevine_cdm_component_installer.cc.patch |
Removes the superseded implementation patch. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| #include "brave/browser/widevine/widevine_utils.h" | ||
|
|
||
| #define RegisterWidevineCdmComponent RegisterWidevineCdmComponent_ChromiumImpl |
There was a problem hiding this comment.
PLSTR-003 bans new uses of a macro to edit upstream code, and specifically says to "never rename an upstream function to _ChromiumImpl just to wrap it" — the #undef immediately after the include is the documented symptom. This replaces a working plaster (rewrite/chrome/browser/component_updater/widevine_cdm_component_installer.cc.yaml, deleted here) with that pattern, and the resulting RegisterWidevineCdmComponent_ChromiumImpl is never called by anything. The sibling widevine_cdm_component_installer.h.yaml rewrite is kept, so the plaster route is already in place for this file pair. (❌ Never Use a Macro to Edit Upstream Code in New Code — Use a Plaster)
|
|
||
| } // namespace | ||
|
|
||
| void RegisterWidevineCdmComponent(ComponentUpdateService* cus, |
There was a problem hiding this comment.
nit: this override has no explanation comment. The rewrite yaml it replaces spelled out the reasons ("Skip registering Widevine when the user has it disabled", "Forward the completion callback instead of dropping it") -- worth carrying that into a comment here, like the one you added on the policy subclass. (Add Explanation Comments in chromium_src Override Files)
|
|
||
| #include "chrome/browser/component_updater/widevine_cdm_component_installer.h" | ||
|
|
||
| #include <memory> |
There was a problem hiding this comment.
nit: since you added and , worth adding base/memory/scoped_refptr.h (base::MakeRefCounted), components/component_updater/component_installer.h (ComponentInstaller) and content/public/browser/browser_thread.h (DCHECK_CURRENTLY_ON) too -- right now they only come transitively from the included upstream .cc. chromium_src/components/update_client/update_client.cc does this. (Always Include What You Use (IWYU))
| Profile* profile = | ||
| Profile::FromBrowserContext(web_contents()->GetBrowserContext()); | ||
|
|
||
| if (profile->IsTor()) { |
There was a problem hiding this comment.
nit: the branch right below has "// If the user already opted in, don't offer it." -- worth a matching one-liner here on why Tor windows are never offered Widevine, like the comment you added in OnEvent(). (Document Non-Obvious Failure Branches)
fix brave/brave-browser#58453
see issue for test plan