refactor: eliminate DecrementCapturerCount patch

[codebytere]

Description of Change

Refactors webContents.{increment|decrement}CapturerCount to remove usage of private WebContents::DecrementCapturerCount exposed via patch. We now store the handle received from calls to IncrementCapturerCount and api::WebContents::DecrementCaptureCount now calls RunAndReset on the stored capture handle.

Functionality previously exposed through webContents.{increment|decrement}CapturerCount has now been folded into webContents.capturePage.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: none.

[deepak1556]

What is the necessity to allow for multiple increment capture count calls during the lifetime of a capture session ? When the API was exposed #21679 it didn't showcase an example that required multiple calls to the increment API. Also chromium's contract for the API https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents.h;l=627-661; states

1. Increment capture count is needed to disable the renderer optimizations when backgrounded or occluded
2. Each increment capture count call should have a corresponding decrement capture count call.

With that being the case, wouldn't the following suffice

• Before starting webContents.capturePage() allow users to call webContents.IncrementCapturerCount() with hidden/awake state and store the generated capture handle.

capture_handle_ = web_contents()->IncrementCapturerCount(size, stay_hidden, stay_awake);

• If there is already a capture session in progress which can be verified with content::WebContents::IsBeingCaptured(), the call to webContents.IncrementCapturerCount() can throw an error.

• Reset the capture handle if set, when api::webContents::OnCapturePageDone() is called

My current concern is that callers of the Electron API are responsible to balance out the increment/decrement calls and if they don't, webContents will end up with a weird visibility state. Instead of relying on users to understand the inner details of the chromium API, we could perform the balancing automatically with only a single increment capturer count for a capture session. This would allow us to remove both the api::webContents::DecrementCapturerCount API and its corresponding patch. Also, I don't see any usages of multiple increment capture counts within Chromium, so if there is a need for it in Electron would be good to have a test or example in the docs. I think that when the API was created in #21679, DecrementCapturerCount was still a public API and it was exposed alongside others as default.

[codebytere]

@deepak1556 you're right imo - i was mostly keying off comments in the previous PR to restore this behavior. I'd also personally prefer to only maintain one at any given time and store it as such. I'll update the PR.

[ckerr]

My current concern is that callers of the Electron API are responsible to balance out the increment/decrement calls and if they don't, webContents will end up with a weird visibility state. Instead of relying on users to understand the inner details of the chromium API, we could perform the balancing automatically with only a single increment capturer count for a capture session.

I like the idea that increment + decrement is done internally when the promise is created + settled. It feels like having this in public API is a footgun with no real advantage?

What would you all think if we:

1. handle increment / decrement internally
2. make the public increment / decrement API no-ops so that it doesn't interfere with the internal work in previous step
3. deprecate the public increment / decrement API for future removal

[codebytere]

@ckerr i'm good with that! i agree it opens up more avenues for potential issues than anything else. However, i do think users would potentially want to control values of stayAwake & stayHidden 🤔 so i'd want to think about what workflows/needs we might be preventing w/ this change. So i'd be at minimum in favor of removing decrementCapturerCount, but i think incrementCapturerCount still serves a potential need.

[ckerr]

However, i do think users would potentially want to control values of stayAwake & stayHidden

Just brainstorming here so feel free to push back, but maybe stayAwake and stayHidden could be optional args to capturePage(), defaulting to false when not provided?

Is there any use case for incrementCapturerCount() other than as a precursor for capturePage()?

[codebytere]

@ckerr ooh good idea!! I pushed the decrement removal/deprecation - @deepak1556 thoughts on removing incrementCapturerCount as well and proceeding with @ckerr's idea above?

[deepak1556]

I like the idea to unify incrementCaptureCount with capturePage. I also don't see any usages outside of the capture context.

[nornagon]

nice, very glad to have these methods go away.

[jkleinsc]

API LGTM

[nornagon]

can we have a test for stayhidden/stayawake?

[codebytere]

@nornagon i can try but stayAwake isn't really testable here and nearly all our documentVisibility tests are disabled/flaky.

[nornagon]

API LGTM

[codebytere]

Alright well for some reason cation is struggling here but merging given two API LGTMs and all concerns addressed.

[release-clerk]

No Release Notes