Should this change be scoped only to macOS or is it fine to show webcontents before the window for all platforms ?

Should this change be scoped only to macOS or is it fine to show webcontents before the window for all platforms ?

I think that - if there is not apparent reason for it - it's better to have the same code on both platforms. It does not cause any regressions on Windows according to spec tests.

There are following failures on macOS bot:

not ok 1523 WebContentsView setBorderRadius capture should render with cutout corners
  Expected color at (10, 10) to match '#0000ff', but got '#dadada'. See the artifact 'color-mismatch-9530684af33638ec6cd0ca7b.png' for more information.
  AssertionError: Expected color at (10, 10) to match '#0000ff', but got '#dadada'. See the artifact 'color-mismatch-9530684af33638ec6cd0ca7b.png' for more information.
      at ScreenCapture._expectImpl (electron/spec/lib/screen-helpers.ts:192:13)
      at Context.<anonymous> (electron/spec/api-web-contents-view-spec.ts:325:11)
not ok 1524 WebContentsView setBorderRadius capture should allow resetting corners
  Expected color at (10, 10) to match '#00b140', but got '#dadada'. See the artifact 'color-mismatch-36ed8e96c9df4c97bf337d5f.png' for more information.
  AssertionError: Expected color at (10, 10) to match '#00b140', but got '#dadada'. See the artifact 'color-mismatch-36ed8e96c9df4c97bf337d5f.png' for more information.
      at ScreenCapture._expectImpl (electron/spec/lib/screen-helpers.ts:192:13)
      at Context.<anonymous> (electron/spec/api-web-contents-view-spec.ts:338:9)
not ok 1525 WebContentsView setBorderRadius capture should render when set before attached
  Expected color at (10, 10) to match '#0000ff', but got '#dadada'. See the artifact 'color-mismatch-f172f29a661d387d09758905.png' for more information.
  AssertionError: Expected color at (10, 10) to match '#0000ff', but got '#dadada'. See the artifact 'color-mismatch-f172f29a661d387d09758905.png' for more information.
      at ScreenCapture._expectImpl (electron/spec/lib/screen-helpers.ts:192:13)
      at Context.<anonymous> (electron/spec/api-web-contents-view-spec.ts:354:9)

I can't repro it locally. All of them pass on my macOS with and without the fix. Are these tests flaky?

Don't know much about the tests.

Just a note that macOS tests run with reduced motion and reduced transparency:

defaults write com.apple.universalaccess reduceMotion -bool true
defaults write com.apple.universalaccess reduceTransparency -bool true

Might be the reason or might not be. Worth checking though.

(You can also set these settings in the macOS System Settings app).

I believe all tests passed on the retest as I see all green now.

I've pushed a fixup restoring check if window is not modal in BaseWindow. It's necessary as it's possible to create instance of BaseWindow which is not BrowserWindow.

In the pipeline for a fixup I pushed earlier today following test failed on macOS:

not ok 832 webContents module printToPDF() with custom page sizes

This test doesn't even display any BrowserWindow so it's highly unlikely that my changes caused the problem. I am also not able to reproduce the problem on my mac. I can also see that there was a crash during testrun on Windows machine during one of the tests for ipc.

1. Is it expected that spec tests fail like this?
2. Will failed pipelines be retested automatically or some action from my side is required?

Looks like a flake. It happens sometimes (although we try to minimize it, of course).

Will failed pipelines be retested automatically or some action from my side is required?

Just comment and a maintainer will rerun. I just started a new test run.

It failed again. This time following CHECK failed:

ok 928 chrome extensions chrome.webRequest does not take precedence over Electron webRequest - WebSocket
[2688:0521/165253.321:FATAL:content\browser\btm\btm_database.cc:150] sql::Database is not opened.

Is there any additional work required from me to ge a review?

I'm not familiar enough with the code to review. (Someone else will get to it at some point.)

I think it would be nice to have a test though. That way, we can prevent it from regressing again in the future.

I think it would be nice to have a test though. That way, we can prevent it from regressing again in the future.

I don't know if it's possible to write spec test which wouldn't be flaky as this is a very brief white flash.

@CezaryKulakowski can you sign your commits?

Commit signing

This repo enforces commit signatures for all incoming PRs.
To sign your commits, see GitHub's documentation on Telling Git about your signing key.

@CezaryKulakowski please rebase this!

@CezaryKulakowski please rebase this!

I've just pushed a fix rebased to the newest master.

@CezaryKulakowski one last rebase and I'll get this merged. Sorry for the delay.

@CezaryKulakowski one last rebase and I'll get this merged. Sorry for the delay.

done

Closing the fix as the bug has been closed as "closed as not planned". I'm going to keep the fix in our repo as a patch to Electron.

Is there anything preventing us from merging it?

Release Notes Persisted

fixed white flash on call to BrowserWindow.show

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

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

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