Skip to content

fix: ensure chrome colors are initialized - #35034

Merged
codebytere merged 4 commits into
mainfrom
fix-pip-colors
Aug 22, 2022
Merged

fix: ensure chrome colors are initialized#35034
codebytere merged 4 commits into
mainfrom
fix-pip-colors

Conversation

@MarshallOfSound

@MarshallOfSound MarshallOfSound commented Jul 23, 2022

Copy link
Copy Markdown
Member

Fixes #34406

After https://chromium-review.googlesource.com/c/chromium/src/+/3526815 we need to initialize chromes colors if we want to support pip.

Notes: Picture-In-Picture mode no longer becomes a red rectangle on hover

@MarshallOfSound MarshallOfSound added semver/patch backwards-compatible bug fixes target/19-x-y labels Jul 23, 2022
@electron-cation electron-cation Bot added new-pr 🌱 PR opened recently and removed new-pr 🌱 PR opened recently labels Jul 23, 2022
zcbenz
zcbenz reviewed Jul 25, 2022
View reviewed changes

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is linking error on Windows

..\..\third_party\llvm-build\Release+Asserts\bin\lld-link.exe /OUT:./electron.exe /nologo -libpath:..\..\third_party\llvm-build\Release+Asserts\lib\clang\15.0.0\lib\windows "-libpath:../../../../Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30133/ATLMFC/lib/x64" "-libpath:../../../../Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30133/lib/x64" "-libpath:../../../../Program Files (x86)/Windows Kits/NETFXSDK/4.8/lib/um/x64" "-libpath:../../../../Program Files (x86)/Windows Kits/10/lib/10.0.20348.0/ucrt/x64" "-libpath:../../../../Program Files (x86)/Windows Kits/10/lib/10.0.20348.0/um/x64" /MACHINE:X64  /PDB:./electron.exe.pdb @./electron.exe.rsp
lld-link: error: undefined symbol: bool __cdecl ShouldCustomDrawSystemTitlebar(void)

@leonardxfce-dialpad

leonardxfce-dialpad commented Jul 25, 2022

Copy link
Copy Markdown

The missing definition looks being defined here:
https://github.com/electron/electron/blob/main/shell/browser/ui/views/win_frame_view.cc
https://github.com/electron/electron/blob/main/shell/browser/ui/views/win_frame_view.h

@ShukantPal

ShukantPal commented Jul 30, 2022

Copy link
Copy Markdown

Thanks @MarshallOfSound!

@h615861768

h615861768 commented Aug 1, 2022

Copy link
Copy Markdown

Thank you @MarshallOfSound!

@codebytere

codebytere commented Aug 1, 2022

Copy link
Copy Markdown
Member

I think we need to add chrome/browser/ui/color/win/native_chrome_color_mixer_win.cc to chromium_src/BUILD.gn

@ghost ghost mentioned this pull request Aug 5, 2022
Merged
1 task
@leonardxfce-dialpad

leonardxfce-dialpad commented Aug 10, 2022

Copy link
Copy Markdown

@MarshallOfSound, sorry for the ping, could you apply the suggestions we wrote, if you have your hands full of work I can try to do a similar PR trying to address this problem (I know nothing C++ code but I can follow your code and use the suggestions here) just let me know.

@MarshallOfSound MarshallOfSound added the target/21-x-y PR should also be added to the "21-x-y" branch. label Aug 15, 2022
@codebytere
codebytere merged commit 9b2b199 into main Aug 22, 2022
@codebytere
codebytere deleted the fix-pip-colors branch August 22, 2022 14:38
@release-clerk

release-clerk Bot commented Aug 22, 2022

Copy link
Copy Markdown

Release Notes Persisted

Picture-In-Picture mode no longer becomes a red rectangle on hover

@trop trop Bot mentioned this pull request Aug 22, 2022
Merged
@trop

trop Bot commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

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

@trop trop Bot mentioned this pull request Aug 22, 2022
Merged
@trop trop Bot mentioned this pull request Aug 22, 2022
Merged
@trop

trop Bot commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

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

@trop

trop Bot commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

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

@trop trop Bot added in-flight/21-x-y and removed target/21-x-y PR should also be added to the "21-x-y" branch. target/20-x-y labels Aug 22, 2022
@arnaudbud

arnaudbud commented Aug 22, 2022

Copy link
Copy Markdown

/trop run backport-to 18-x-y

@trop

trop Bot commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

@arnaudbud is not authorized to run PR backports.

@PikachuEXE

PikachuEXE commented Aug 23, 2022

Copy link
Copy Markdown

@arnaudbud I have tested 18.x it there is no such issue

@trop trop Bot added merged/20-x-y merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/20-x-y labels Aug 23, 2022
@ghost ghost mentioned this pull request Nov 9, 2022
Open
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@MarshallOfSound @VerteDinde @khalwa
fix: ensure chrome colors are initialized (electron#35034)
81a896b 
* fix: ensure chrome colors are initialized

* build: fix linking on windows

* build: fix linking on windows

* build: add needed files to chromium_src/BUILD

Co-authored-by: VerteDinde <keeleymhammond@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codebytere codebytere codebytere approved these changes
@VerteDinde VerteDinde VerteDinde approved these changes
+1 more reviewer
@zcbenz zcbenz zcbenz left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

merged/21-x-y PR was merged to the "21-x-y" branch. semver/patch backwards-compatible bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: red background on hover to PiP

9 participants

@MarshallOfSound @leonardxfce-dialpad @ShukantPal @h615861768 @codebytere @arnaudbud @PikachuEXE @zcbenz @VerteDinde