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

[CEF125+] Fix DevTools to popup #221

Merged
merged 2 commits into from
Jul 10, 2024
Merged

[CEF125+] Fix DevTools to popup #221

merged 2 commits into from
Jul 10, 2024

Conversation

HashidaTKS
Copy link
Contributor

@HashidaTKS HashidaTKS commented Jul 9, 2024

Which issue(s) this PR fixes:

#203

What this PR does / why we need it:

DevTools doesn't popup and is fixed on a current tab window when we use Chrome bootstrap and Alloy runtime style.

FYI:
chromiumembedded/cef#3685
https://cef-builds.spotifycdn.com/docs/120.1/classCefLifeSpanHandler.html#a42d853d18238d79d2b17d04277bd636a
https://magpcss.org/ceforum/viewtopic.php?f=6&t=19792&p=55334&hilit=Devtools+SetAsPopup#p55334

Known Issues

These are known issues.
They will be fixed after this PR.

How to verify the fixed issue:

  • Open DevTools by Help->About->Show Developer Tools.
    • Confirm that DevTools popups.

image

DevTools doesn't popup and is fixed on a current tab window
when we use Chrome bootstrap and Alloy runtime style.
@HashidaTKS
Copy link
Contributor Author

image

image

現状の実装でも動きはするのだが、上記のようにChromiumアイコンのまま起動してしまう。

@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Jul 9, 2024

@ashie
@kenhys
@yashirot

実装は無視していただいて、 #221 (comment) のようにDevToolsがChromeのアイコンで表示されるようになることについて、これで問題ないと思うか、問題ありと思うか意見をいただけませんか?
私はChronosのアイコンが使われるようにした方が良いと思っています。
(ただ、その一方、CEF125+でこのDevToolsのアイコンを変更する方法が発見できていません。)

@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Jul 9, 2024

Cefのサンプルコードや、 https://magpcss.org/ceforum/viewtopic.php?f=6&t=19792&p=55334 の内容を元に色々試しているが、上手くいっていない。

chromiumembedded/cef#3685 にはChrome bootstrapとAlloy runtime styleについて、

DevTools popups are Chrome style only (cannot be windowless).

とあるので、従来のようなウィンドウレスでChronosのウィンドウに結合するような使い方はできなさそう。
そのため、Chrome styleのDevToolsウィンドウを起動し、その上でアイコンを変更するような対処が必要ではないかと考えている。

@kenhys
Copy link
Contributor

kenhys commented Jul 9, 2024

私はChronosのアイコンが使われるようにした方が良いと思っています。

Chronosのアイコンが使われるようにするのに賛成です。

@kenhys
Copy link
Contributor

kenhys commented Jul 9, 2024

ためしに.dllのリソースを直接ツールで置き換えるとかできないものだろうか。

@ashie
Copy link
Member

ashie commented Jul 9, 2024

私はChronosのアイコンが使われるようにした方が良いと思っています。 (ただ、その一方、CEF125+でこのDevToolsのアイコンを変更する方法が発見できていません。)

簡単にできるならもちろんその方が良いですが、一方でやり方が分からないならそんなに労力をかけるようなところでもないかなと思います。
製品コンセプト的には一般ユーザーが使うものではなくてトラブルシューティング時に外形的な情報だけでは判断がつかない時にだけしょうがなく使うものなので、多少見栄えが悪くても問題ないと思います。

@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Jul 9, 2024

ためしに.dllのリソースを直接ツールで置き換えるとかできないものだろうか。

コメントありがとうございます。
Visual Studioでdllに含まれているイメージの変更ができるので試したのですが、8bitのイメージリソースの変更はサポートされていたのですが、32bitのリソースの変更がサポートされておらず、変更しての確認ができませんでした。。。
実際にこの方針で修正する場合は、CEFをセルフビルドする際にChronosのアイコンに差し替えてビルドするのが良いのではないかと思っています。

@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Jul 9, 2024

私はChronosのアイコンが使われるようにした方が良いと思っています。 (ただ、その一方、CEF125+でこのDevToolsのアイコンを変更する方法が発見できていません。)

簡単にできるならもちろんその方が良いですが、一方でやり方が分からないならそんなに労力をかけるようなところでもないかなと思います。 製品コンセプト的には一般ユーザーが使うものではなくてトラブルシューティング時に外形的な情報だけでは判断がつかない時にだけしょうがなく使うものなので、多少見栄えが悪くても問題ないと思います。

ありがとうございます。
そうですね、DevToolsなのでそこまで重要度は高くないということで、後2-3時間ほど見て解決方法がわからなければ、一旦この内容で(実装はもう少し調整して)レビュー依頼を出そうと考えています。

@kenhys
Copy link
Contributor

kenhys commented Jul 9, 2024

実際にこの方針で修正する場合は、CEFをセルフビルドする際にChronosのアイコンに差し替えてビルドするのが良いのではないかと思っています。

そうですね、そちらのほうがよさそうです。

libcef.dllのアイコンを置き換えるだけならResourceHackerでもできた。
image

@HashidaTKS
Copy link
Contributor Author

libcef.dllのアイコンを置き換えるだけならResourceHackerでもできた。

ありがとうございます。
CEFのリソースの方を変更できれば修正可能ということがわかり、良かったです。

@HashidaTKS
Copy link
Contributor Author

判明している問題: NetworkやPerformanceのダウンロードをしようとするとクラッシュする。

@HashidaTKS
Copy link
Contributor Author

判明している問題: NetworkやPerformanceのダウンロードをしようとするとクラッシュする。

検証時CEF126を使っていたが、beta版のCEF127で試したら直っていた。

@HashidaTKS
Copy link
Contributor Author

chromiumembedded/cef#3724

これと同件と思われる。

@yashirot
Copy link
Contributor

簡単にできるならもちろんその方が良いですが、一方でやり方が分からないならそんなに労力をかけるようなところでもないかなと思います。

同意見でした。

CEFのリソースの方を変更できれば修正可能ということがわかり、良かったです。

Chronosのアイコンでいけるみたいですね。異論ありません。

@kenhys
Copy link
Contributor

kenhys commented Jul 10, 2024

https://github.com/chromium/chromium/tree/main/chrome/app/theme/chromium
たぶんこのあたりの.icoを置き換えたらよいのではないだろうか。(未検証)

@HashidaTKS
Copy link
Contributor Author

そうですね、そのあたりの.icoを置き換えたらいけそうな気がします。

@HashidaTKS
Copy link
Contributor Author

DevToolsからNetworkを表示して、左下の履歴から適当なURLを開こうとすると、Chronosで開くと同時にChromiumでも開いてしまう。

@HashidaTKS
Copy link
Contributor Author

v14.1.119.1のDevToolsでは何も開かない。
"Open in new tab"を押しても何も開かないので、それはそれでどうなのか。

@HashidaTKS
Copy link
Contributor Author

まあ、一旦は制限事項でで良いか。

@HashidaTKS
Copy link
Contributor Author

DevToolsからNetworkを表示して、左下の履歴から適当なURLを開こうとすると、Chronosで開くと同時にChromiumでも開いてしまう。

これは #223 に起票した。

CefBrowserSettings& settings,
CefRefPtr<CefDictionaryValue>& extra_info,
bool* use_default_window)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

現状は実装が空なのだが、 https://github.com/ThinBridge/Chronos/pull/221/files#diff-62b66b5116e3e70c2a991899db0ba77b0bee41543e09a6aaf9b6e1f014f206f9R1564 で言及していることもあり、削除せずに空実装のまま残している。

@HashidaTKS
Copy link
Contributor Author

https://github.com/chromium/chromium/tree/main/chrome/app/theme/chromium たぶんこのあたりの.icoを置き換えたらよいのではないだろうか。(未検証)

こちらは別のリポジトリでの対応が必要で、本PRでは対応できないため、次回使用するCEFのバージョンが決まった際に、チャレンジするタスクとして #224 を起票しました。

@HashidaTKS
Copy link
Contributor Author

@kenhys @ashie

現状でDevToolsをポップアップとして起動することはできているので、レビューをお願いできないでしょうか?

Descriptionの方にも記載していますが

  • アイコンの変更は後のタスクとして [CEF125+]Fix DevTools icon #224 に起票しています。
  • DevToolsのNetworkタブで通信したファイルを新しいタブで開こうとすると、ChronosのタブとChromiumのウィンドウの二つが開く問題がありますが、これはCEFの問題と思われるので、CEFの方に報告しています。
  • DevToolsのNetworkタブでHARファイルをダウンロードしようとするとクラッシュ問題がありますが、これはCEF126の問題で、CEF127以降を使えば問題ありません

@HashidaTKS HashidaTKS marked this pull request as ready for review July 10, 2024 08:19
@HashidaTKS HashidaTKS merged commit 245e840 into master Jul 10, 2024
3 checks passed
@HashidaTKS HashidaTKS deleted the fix-devtools branch July 10, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants