Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: prevent loading spinner when launching utility process on Windows#43657

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@nikwen
Copy link
Member

Description of Change

Closes#43341.

When a process is started on Windows, the mouse cursor turns into a loading spinner. This makes sense for UI processes (e.g. new windows) but not for background utility processes.

Especially if an app launches many utility processes, the end user might see a large number of confusing loading spinners where the end user doesn’t know where they are coming from.

Try this Fiddle gist for a demonstration of how awful that behavior can be:https://gist.github.com/nikwen/a705df773120f46805d521c6485a9f2e

This pull request disables the loading spinner.

Before the PR
Before.mp4
After the PR
After.mp4

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
    • Three tests were flaky on my Windows computer. They sometimes timed out. However, I don't think they have anything to do with this PR.
      • BrowserWindow module "transparent" option should not make background transparent if falsy
      • BrowserWindow module "titleBarOverlay" option sets Window Control Overlay with title bar height of 40
      • app module app.requestSingleInstanceLock prevents the second launch of app
  • tests arechanged or added
    • I’m not sure how to add a test for this. If you think it’s important, I’d appreciate a hint on how I could test the behavior.
  • relevant documentation, tutorials, templates and examples are changed or added
  • PR release notes describe the change in a way relevant to app developers, and arecapitalized, punctuated, and past tense.

Release Notes

Notes: Fixed mouse cursor turning into loading spinner when starting utility process on Windows.

@nikwennikwen requested a review froma team as acode ownerSeptember 9, 2024 23:59
@electron-cationelectron-cationbot added new-pr 🌱PR opened recently labelsSep 9, 2024
@nikwen
Copy link
MemberAuthor

@deepak1556 You are the utility process expert. If you have time to review this, I’d appreciate it a lot. Thank you so much in advance!

@deepak1556deepak1556 added semver/patchbackwards-compatible bug fixes target/31-x-yPR should also be added to the "31-x-y" branch. target/32-x-yPR should also be added to the "32-x-y" branch. target/33-x-yPR should also be added to the "33-x-y" branch. labelsSep 10, 2024
Copy link
Member

@deepak1556deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This makes sense, Thanks for working on this!

I would treat this as a bug fix rather than a feature change, since the spinner was never intended.

Copy link
Member

@codebyterecodebytere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

i'm 👍🏻 on the goal here, but if we're going to add it in as a configurable option despite Electron wanting it off in all cases we should try to upstream it to Chromium and see what their feedback is.

If they indicate they're not open to it, we should make this patch surface smaller by just disabling the cursor in all cases since that's what we're doing in practice.

@deepak1556
Copy link
Member

The patch only controls the utility process code path, we don't adjust the process launch choices for renderer. I don't think we want to change the feedback in all cases.

Ideally, the way to reduce this patch surface would be rewrite the utility process api directly on top of thebase::Launch. Most of the patch including the other options like env, std handles is rerouting the options from theUtilityProcessHost all the way down tobase::Launch through the different embedding layers.

@deepak1556
Copy link
Member

Given this issue is only visible when launching a significant number of utility process in a short time which would not be the case in upstream, not sure if it would be of value in upstream to have an option in theUtilityProcessHost. But I am 👍 to give this a shot in upstreaming.

@nikwen
Copy link
MemberAuthor

Thanks for the reviews and comments,@deepak1556 and@codebytere!

As@deepak1556 said, the "feedback cursor off" option is passed down multiple layers (like all other Electron utility process options) so that it doesn't affect other process types.

Re upstreaming:

  • Which part of the patch would you like to upstream and how would it provide value to Chromium?
  • Would anyone be willing to assist with that? I'm not sure I could look into upstreaming anytime soon due to time constraints.

@nikwen
Copy link
MemberAuthor

ThemacOS build failure was due to an auth problem. I'll sign the commit soon.

codebytere reacted with thumbs up emoji

@codebytere
Copy link
Member

codebytere commentedSep 10, 2024•
edited
Loading

@nikwen we'd want to upstream the option to configure in such a way that we don't need to have this patch, essentially. How we do that i don't feel as strongly about - my goal is just to get rid of the patch.

To answer the rest of your first question - it's less specifically about whether it would provide value to Chromium and more about how this patch complies with ourpatch policy - I'd prefer we not float something like this indefinitely if we can help it!

To your second question - i can take a shot at it :)

@deepak1556
Copy link
Member

deepak1556 commentedSep 10, 2024•
edited
Loading

@codebytere if the goal is to remove the patch in its entirety then we should evaluate usingbase::Launch api directly. The only reason I chose to wrap on top ofUtilityProcessHost is to reuse the service configurations and lifetime managements that have been setup for existing utility process in Chromium but I don't think there is a technical blocker to writing our own process host and configuring the services on top of process launch Apis.

Edit: We also have patches to customize the current process host to emit exit codes, and hacks to preserve shutdown ordering like

// Shutdown utility process created with Electron API before
// stopping Node.js so that exit events can be emitted. We don't let
// content layer perform this action since it destroys
// child process only after this step (PostMainMessageLoopRun) via
// BrowserProcessIOThread::ProcessHostCleanUp() which is too late for our
// use case.
// https://source.chromium.org/chromium/chromium/src/+/main:content/browser/browser_main_loop.cc;l=1086-1108
//
// The following logic is based on
// https://source.chromium.org/chromium/chromium/src/+/main:content/browser/browser_process_io_thread.cc;l=127-159
//
// Although content::BrowserChildProcessHostIterator is only to be called from
// IO thread, it is safe to call from PostMainMessageLoopRun because thread
// restrictions have been lifted.
// https://source.chromium.org/chromium/chromium/src/+/main:content/browser/browser_main_loop.cc;l=1062-1078
for (content::BrowserChildProcessHostIteratorit(
content::PROCESS_TYPE_UTILITY);
!it.Done(); ++it) {
if (it.GetDelegate()->GetServiceName() == node::mojom::NodeService::Name_) {
auto& process = it.GetData().GetProcess();
if (!process.IsValid())
continue;
auto utility_process_wrapper =
api::UtilityProcessWrapper::FromProcessId(process.Pid());
if (utility_process_wrapper)
utility_process_wrapper->Shutdown(0/* exit_code*/);
}
}
. This would be a chance to remove that as well.

nikwen and codebytere reacted with thumbs up emoji

@nikwen
Copy link
MemberAuthor

Thank you,@codebytere! I appreciate that. :)

Your explanation makes sense. To give the Chromium team a reason to incorporate the change, it might be useful to find a process in Chrome that would benefit from having this flag set.

As a side note: When making changes to the patch file, I made sure to keep the maintenance burden as low as possible by only changing the code in places that were already being modified.

@nikwen
Copy link
MemberAuthor

@deepak1556 I like that idea!

@nikwennikwenforce-pushed theutility-process-loading-spinner branch from2659632 to53d2b61CompareSeptember 10, 2024 20:45
@nikwen
Copy link
MemberAuthor

Commit is signed now. (No code changes in the force push.)

codebytere reacted with thumbs up emoji

@electron-cationelectron-cationbot removed the new-pr 🌱PR opened recently labelSep 11, 2024
Copy link
Member

@codebyterecodebytere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Given migrating tobase::Launch will be a heftier task and this is a fairly trivial addition to existing maintenance burden I'd be ok with merging this as-is and working on the migration as a follow up after some thought.

nikwen reacted with heart emoji
@nikwen
Copy link
MemberAuthor

Is anything else required from my side to get this merged?

@jkleinscjkleinsc merged commitf84ffc8 intoelectron:mainSep 16, 2024
@release-clerk
Copy link

Release Notes Persisted

Fixed mouse cursor turning into loading spinner when starting utility process on Windows.

@trop
Copy link
Contributor

tropbot commentedSep 16, 2024

I was unable to backport this PR to "32-x-y" cleanly;
you will need to perform thisbackport manually.

@trop
Copy link
Contributor

tropbot commentedSep 16, 2024

I was unable to backport this PR to "31-x-y" cleanly;
you will need to perform thisbackport manually.

@troptropbot added needs-manual-bp/32-x-y and removed target/32-x-yPR should also be added to the "32-x-y" branch. target/31-x-yPR should also be added to the "31-x-y" branch. labelsSep 16, 2024
@trop
Copy link
Contributor

tropbot commentedSep 16, 2024

I have automatically backported this PR to "33-x-y", please check out#43731

@troptropbot added in-flight/33-x-y and removed target/33-x-yPR should also be added to the "33-x-y" branch. labelsSep 16, 2024
@nikwen
Copy link
MemberAuthor

Thanks for your help,@deepak1556,@codebytere and@jkleinsc! 🙌

I'll work on the manual backports this week.

@troptropbot added merged/33-x-yPR was merged to the "33-x-y" branch. and removed in-flight/33-x-y labelsSep 17, 2024
yangannyx pushed a commit to yangannyx/electron that referenced this pull requestOct 21, 2024
electron#43657)fix: prevent spinning cursor when launching utility process on Windows
yangannyx pushed a commit to yangannyx/electron that referenced this pull requestOct 21, 2024
electron#43657)fix: prevent spinning cursor when launching utility process on Windows
yangannyx pushed a commit to yangannyx/electron that referenced this pull requestOct 21, 2024
electron#43657)fix: prevent spinning cursor when launching utility process on Windows
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jkleinscjkleinscjkleinsc approved these changes

@deepak1556deepak1556deepak1556 approved these changes

@codebyterecodebyterecodebytere approved these changes

@ckerrckerrAwaiting requested review from ckerr

Assignees

No one assigned

Labels

merged/33-x-yPR was merged to the "33-x-y" branch.needs-manual-bp/31-x-yneeds-manual-bp/32-x-ysemver/patchbackwards-compatible bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature Request]: Launch utility process without loading spinner on Windows

4 participants

@nikwen@deepak1556@codebytere@jkleinsc
[8]ページ先頭
©2009-2025 Movatter.jp