Skip to content

Commit 1460dee

Browse files
amaitlandamaitland
authored
Core - ClientAdapter improve HasParent checks (#4443)
* Core - Improve ClientAdapter::GetBrowserWrapper logic Simplify the logic - For non popup return immediately (most common scenario) - Check for IBrowser instance for matching popup - Check for HasParent and return the browser Instance - Return nullptr if no matching IBrowser wrapper found * Core - ClientAdapter::OnFrameCreated called before OnAfterCreated - The very first call to ClientAdapter::OnFrameCreated happens before OnAfterCreated leaving us with a IBrowser instance that's null Create an IBrowser instance that's scoped to the method call * Core - ClientAdapter improve HasParent checks - HasParent check has now been replaced with IsHostedBrowser which has additional check to make sure the internal IWebBrowser.Set* methods are only called for a IBrowser instance that's directly associated with a ChromiumWebBrowser instance This resolves issues with DevTools being opened for a popup hosted in a ChromiumWebBrowser instance. Related discussion #4438
1 parent 6e11059 commit 1460dee

File tree

3 files changed

+260
-38
lines changed

3 files changed

+260
-38
lines changed

‎CefSharp.Core.Runtime/Internals/ClientAdapter.cpp‎

Lines changed: 83 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -75,23 +75,62 @@ namespace CefSharp
7575

7676
IBrowser^ ClientAdapter::GetBrowserWrapper(int browserId, bool isPopup)
7777
{
78+
if (!isPopup)
79+
{
80+
return _browser;
81+
}
82+
83+
IBrowser^ popupBrowser;
84+
if (_popupBrowsers->TryGetValue(browserId, popupBrowser))
85+
{
86+
return popupBrowser;
87+
}
88+
89+
// For popups that were hosted using a ChromiumWebBrowser instance
7890
if (_browserControl->HasParent)
7991
{
8092
return _browser;
8193
}
8294

83-
if (isPopup)
95+
return nullptr;
96+
}
97+
98+
// Is a main browser if isPopuo == false or the IBrowser instance is directly associated
99+
// with the ChromiumWebBrowser instance. Should be true in cases
100+
// where ChromiumWebBrowser is instanciated directly or
101+
// when a popup is hosted in a ChromiumWebBrowser instance
102+
// For popups hosted in ChromiumWebBrowser instances it's important
103+
// that DevTools popups return false;
104+
bool ClientAdapter::IsMainBrowser(bool isPopup, int browserId)
105+
{
106+
// Main browser is always true
107+
if (!isPopup)
84108
{
85-
IBrowser^ popupBrowser;
86-
if (_popupBrowsers->TryGetValue(browserId, popupBrowser))
87-
{
88-
return popupBrowser;
89-
}
109+
return true;
110+
}
90111

91-
return nullptr;
112+
// If popup and HasParent == false then always false
113+
if (!_browserControl->HasParent)
114+
{
115+
return false;
92116
}
93117

94-
return _browser;
118+
// This method is called from OnAfterCreated before _cefBrowser is set
119+
// If the _cefBrowser reference is null then this should be a ChromiumWebBrowser
120+
// hosted as a popup
121+
if (!_cefBrowser.get())
122+
{
123+
return true;
124+
}
125+
126+
// For popups hosted in ChromiumWebBrowser instance directly (non DevTools popup)
127+
// then return true;
128+
if (_cefBrowser->GetIdentifier() == browserId)
129+
{
130+
return true;
131+
}
132+
133+
return false;
95134
}
96135

97136
void ClientAdapter::CloseAllPopups(bool forceClose)
@@ -180,14 +219,7 @@ namespace CefSharp
180219

181220
auto browserWrapper = gcnew CefBrowserWrapper(browser);
182221

183-
auto isPopup = browser->IsPopup() && !_browserControl->HasParent;
184-
185-
if (isPopup)
186-
{
187-
// Add to the list of popup browsers.
188-
_popupBrowsers->Add(browser->GetIdentifier(), browserWrapper);
189-
}
190-
else
222+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
191223
{
192224
_browserHwnd = browser->GetHost()->GetWindowHandle();
193225
_cefBrowser = browser;
@@ -199,6 +231,11 @@ namespace CefSharp
199231
_browserAdapter->OnAfterBrowserCreated(browserWrapper);
200232
}
201233
}
234+
else
235+
{
236+
// Add to the list of popup browsers.
237+
_popupBrowsers->Add(browser->GetIdentifier(), browserWrapper);
238+
}
202239

203240
auto handler = _browserControl->LifeSpanHandler;
204241

@@ -227,7 +264,6 @@ namespace CefSharp
227264

228265
void ClientAdapter::OnBeforeClose(CefRefPtr<CefBrowser> browser)
229266
{
230-
auto isPopup = browser->IsPopup() && !_browserControl->HasParent;
231267
auto handler = _browserControl->LifeSpanHandler;
232268

233269
if (handler != nullptr)
@@ -240,20 +276,18 @@ namespace CefSharp
240276
handler->OnBeforeClose(_browserControl, %browserWrapper);
241277
}
242278

243-
if (isPopup)
279+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
280+
{
281+
_cefBrowser = nullptr;
282+
}
283+
else
244284
{
245285
// Remove from the browser popup list.
246286
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), true);
247287
_popupBrowsers->Remove(browser->GetIdentifier());
248288
// Dispose the CefBrowserWrapper
249289
delete browserWrapper;
250290
}
251-
//TODO: When creating a new ChromiumWebBrowser and passing in a newBrowser to OnBeforePopup
252-
//the handles don't match up (at least in WPF), need to investigate further.
253-
else if (_browserHwnd == browser->GetHost()->GetWindowHandle() || _browserControl->HasParent)
254-
{
255-
_cefBrowser = nullptr;
256-
}
257291

258292
BrowserRefCounter::Instance->Decrement(_browserControl->GetType());
259293
}
@@ -263,7 +297,7 @@ namespace CefSharp
263297
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
264298
auto args = gcnew LoadingStateChangedEventArgs(browserWrapper, canGoBack, canGoForward, isLoading);
265299

266-
if (!browser->IsPopup() || _browserControl->HasParent)
300+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
267301
{
268302
_browserControl->SetLoadingStateChange(args);
269303
}
@@ -281,7 +315,7 @@ namespace CefSharp
281315
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
282316
auto args = gcnew AddressChangedEventArgs(browserWrapper, StringUtils::ToClr(address));
283317

284-
if (!browser->IsPopup() || _browserControl->HasParent)
318+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
285319
{
286320
_browserControl->SetAddress(args);
287321
}
@@ -354,15 +388,15 @@ namespace CefSharp
354388
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
355389
auto args = gcnew TitleChangedEventArgs(browserWrapper, StringUtils::ToClr(title));
356390

357-
if (browser->IsPopup() && !_browserControl->HasParent)
391+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
358392
{
359-
// Set the popup window title
360-
auto hwnd = browser->GetHost()->GetWindowHandle();
361-
SetWindowText(hwnd, std::wstring(title).c_str());
393+
_browserControl->SetTitle(args);
362394
}
363395
else
364396
{
365-
_browserControl->SetTitle(args);
397+
// Set the popup window title
398+
auto hwnd = browser->GetHost()->GetWindowHandle();
399+
SetWindowText(hwnd, std::wstring(title).c_str());
366400
}
367401

368402
auto handler = _browserControl->DisplayHandler;
@@ -426,7 +460,7 @@ namespace CefSharp
426460
returnFlag = handler->OnTooltipChanged(_browserControl, tooltip);
427461
}
428462

429-
if (!browser->IsPopup() || _browserControl->HasParent)
463+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
430464
{
431465
_tooltip = tooltip;
432466
_browserControl->SetTooltipText(_tooltip);
@@ -442,7 +476,7 @@ namespace CefSharp
442476

443477
auto args = gcnew ConsoleMessageEventArgs(browserWrapper, (LogSeverity)level, StringUtils::ToClr(message), StringUtils::ToClr(source), line);
444478

445-
if (!browser->IsPopup() || _browserControl->HasParent)
479+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
446480
{
447481
_browserControl->OnConsoleMessage(args);
448482
}
@@ -461,7 +495,7 @@ namespace CefSharp
461495
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
462496
auto args = gcnew StatusMessageEventArgs(browserWrapper, StringUtils::ToClr(value));
463497

464-
if (!browser->IsPopup() || _browserControl->HasParent)
498+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
465499
{
466500
_browserControl->OnStatusMessage(args);
467501
}
@@ -512,7 +546,7 @@ namespace CefSharp
512546
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
513547
CefFrameWrapper frameWrapper(frame);
514548

515-
if (!browser->IsPopup() || _browserControl->HasParent)
549+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
516550
{
517551
_browserControl->OnFrameLoadStart(gcnew FrameLoadStartEventArgs(browserWrapper, %frameWrapper, (CefSharp::TransitionType)transitionType));
518552
}
@@ -529,7 +563,7 @@ namespace CefSharp
529563
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
530564
CefFrameWrapper frameWrapper(frame);
531565

532-
if (!browser->IsPopup() || _browserControl->HasParent)
566+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
533567
{
534568
_browserControl->OnFrameLoadEnd(gcnew FrameLoadEndEventArgs(browserWrapper, %frameWrapper, httpStatusCode));
535569
}
@@ -547,7 +581,7 @@ namespace CefSharp
547581
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
548582
CefFrameWrapper frameWrapper(frame);
549583

550-
if (!browser->IsPopup() || _browserControl->HasParent)
584+
if (IsMainBrowser(browser->IsPopup(), browser->GetIdentifier()))
551585
{
552586
_browserControl->OnLoadError(gcnew LoadErrorEventArgs(browserWrapper, %frameWrapper,
553587
(CefErrorCode)errorCode, StringUtils::ToClr(errorText), StringUtils::ToClr(failedUrl)));
@@ -1117,7 +1151,18 @@ namespace CefSharp
11171151
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
11181152
auto frameWrapper = gcnew CefFrameWrapper(frame);
11191153

1120-
handler->OnFrameCreated(_browserControl, browserWrapper, frameWrapper);
1154+
if (browserWrapper == nullptr)
1155+
{
1156+
// Very first OnFrameCreated called may happen before OnAfterCreated
1157+
// so we have to create a new wrapper that's lifespan is scoped to this single call.
1158+
CefBrowserWrapper browserWrapper(browser);
1159+
1160+
handler->OnFrameCreated(_browserControl, %browserWrapper, frameWrapper);
1161+
}
1162+
else
1163+
{
1164+
handler->OnFrameCreated(_browserControl, browserWrapper, frameWrapper);
1165+
}
11211166
}
11221167
}
11231168

‎CefSharp.Core.Runtime/Internals/ClientAdapter.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ namespace CefSharp
8787
void CloseAllPopups(bool forceClose);
8888
void MethodInvocationComplete(MethodInvocationResult^ result);
8989
IBrowser^ GetBrowserWrapper(int browserId);
90+
bool IsMainBrowser(bool isPopup, int browserId);
9091

9192
// CefClient
9293
virtual DECL CefRefPtr<CefLifeSpanHandler> GetLifeSpanHandler() override { return this; }

0 commit comments

Comments
 (0)