Skip to content
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

fix: wide string concatenation #40909

Merged

Conversation

trop[bot]
Copy link
Contributor

@trop trop bot commented Jan 8, 2024

Backport of #40892

See that PR for details.

Notes: Fixed default protocol handler behavior on Windows.

trop bot and others added 3 commits January 8, 2024 06:02
Co-authored-by: clavin <clavin@electronjs.org>
Co-authored-by: clavin <clavin@electronjs.org>
Co-authored-by: clavin <clavin@electronjs.org>
@trop trop bot requested a review from clavin January 8, 2024 06:03
@trop trop bot mentioned this pull request Jan 8, 2024
@trop trop bot added 28-x-y backport This is a backport PR semver/patch backwards-compatible bug fixes labels Jan 8, 2024
@trop trop bot requested a review from a team as a code owner January 8, 2024 06:13
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

FTBTS:

../../electron/shell/browser/browser_win.cc(145,44): error: no member named 'AsWStringView' in namespace 'base'; did you mean 'AsWStringPiece'?
  145 |     *exe = base::StrCat({*exe, L" ", base::AsWStringView(joined_launch_args)});
      |                                      ~~~~~~^~~~~~~~~~~~~
      |                                            AsWStringPiece
../..\base/strings/string_util_win.h(94,21): note: 'AsWStringPiece' declared here
   94 | inline WStringPiece AsWStringPiece(StringPiece16 str) {
      |                     ^
1 error generated.
ninja: build stopped: subcommand failed.
gn desc out/Default v8:run_mksnapshot_default args > out/Default/default_mksnapshot_args

@ckerr
Copy link
Member

ckerr commented Jan 8, 2024

FTBTS:

Probably needs to be base::AsWStringPiece in the older branches due to https://chromium-review.googlesource.com/c/chromium/src/+/5010979

This code is being merged to a branch before the change from crrev.com/c/5010979
@clavin
Copy link
Member

clavin commented Jan 8, 2024

2 test failures on the WoA build:

not ok 651 contentTracing stopRecording does not crash on empty string
  Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-content-tracing-spec.ts)
  Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-content-tracing-spec.ts)
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)
not ok 652 contentTracing stopRecording calls its callback with a result file path
  Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-content-tracing-spec.ts)
  Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-content-tracing-spec.ts)
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)

These seem unrelated and likely flakes. Happy to rerun, but the important tests for this change did pass so I'm leaning towards dismissing the two failures as flakes.

@VerteDinde VerteDinde merged commit 4120b8f into 28-x-y Jan 8, 2024
13 checks passed
@VerteDinde VerteDinde deleted the trop/28-x-y-bp-fix-wide-string-concatenation-1704693757902 branch January 8, 2024 22:34
Copy link

release-clerk bot commented Jan 8, 2024

Release Notes Persisted

Fixed default protocol handler behavior on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
28-x-y backport This is a backport PR semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants