Clean up the usage/meaning of UIT_GetMainWndHandle()

[GoogleCodeExporter]

Attached is a patch that cleans up the use of this method to return the "main", 
as in "frame", window (IOW, the GA_ROOT window on Windows) rather than the 
contained browser window which can be directly obtained via the 
GetWindowHandle() method.

The patch changes the UIT_GetMainWndHandle() method to return the m_hWndParent 
window whenever there is an off-screen rendering, and now also when 
m_hWndParent is not null. Otherwise, it returns the m_hWnd handle as before. 
m_hWndParent is set by clients when they want to be the parent of the child 
browser window.

In my review of the code, most places that used UIT_GetMainWindowHandle() 
really wanted the frame window and not the container window of the browser.

Summary of the patch...

Removes the GetAncestor(*) calls in a few places since the resulting window 
handle is now already the root.

Since mouse x,y of the context menu is in the frame's web view window 
coordinates; I switched it to use that window's handle when converting to 
screen coordinates.

Lastly, the WM_SIZE handler in the browser container should just use the handle 
for itself to get the client rect that needs to be passed on to web view window.

Original issue reported on code.google.com by gusver...@gmail.com on 7 Jul 2011 at 9:41

Attachments:

• cleanup_UIT_GetMainWndHandle.patch

[GoogleCodeExporter]

A few problems with your patch:

1. UIT_CloseView(UIT_GetMainWndHandle()) in 
CefBrowserImpl::UIT_DestroyBrowser() will close the parent window instead of 
the browser window.

2. ShowWindow(root, SW_SHOWNORMAL) in BrowserWebViewDelegate::show() won't 
behave as expected if m_hWndParent isn't a root window.

Original comment by magreenb...@gmail.com on 11 Jul 2011 at 4:47

[GoogleCodeExporter]

Correct, my solution is not ideal. Yeah, so this limits the functionality to 
just one browser per parent window where the parent must be the root rather 
than leaving open the possibility of having multiple browsers, possibly in a 
container window,
somewhere in a root window.

Let me try again...

I am trying to solve the "window.close()" problem for any contained browser 
window that is a child of another window. This is not working right in the most 
recent couple of builds. (modal dialog support changed the miss-behavior a bit).

How about something like this:

* Add a bool CefLifeSpanHandler::QueryCanClose() in the browser's WM_CLOSE call 
so that clients can override which window gets closed by denying the close and 
handling it from a higher level, allowing it to proceed as is, or denying it 
for some other reason. Return true to allow it to close, false otherwise.

* Move the re-enabling of the opener window for modal dialogs to a method that 
clients can call (rather than forwarding the WM_CLOSE call from the parent 
window to the browser.) 

// This must be called if the browser is modal and the modal window is 
// handled by CEF's modal loop.
void CefBrowser::PrepareToCloseModalWindow()
{
  // We must re-enable the opener (owner of the modal window)
  // before we close the popup to avoid focus/activation/z-order issues.
  if (browser->opener_ && browser->opener_was_disabled_by_modal_loop_) {
      HWND owner = ::GetAncestor(browser->opener_, GA_ROOT);
      ::EnableWindow(owner, TRUE);
  }
}

* CefBrowser::PrepareToCloseModalWindow() and 
CefLifeSpanHandler::OnBeforeClose() 
  would get called if QueryCanClose() returned true.

* If the contained browser window is allowed to close, it will be removed 
  from its parent window. Clients can use the OnBeforeClose() callback to 
  cleanup the parent window's layout.

Thoughts?

I have a test case for window.close() at 
http://www.gusverdun.com/cef/modal/close.html

Original comment by gusver...@gmail.com on 12 Jul 2011 at 9:51

[GoogleCodeExporter]

Attached is a patch that reworks the solution to handle the window.close() case 
when the browser is contained while allowing for multiple contained browser 
windows.

I added the proposed bool CefLifeSpanHandler::QueryCanClose() and moved the  
CefLifeSpanHandler::OnBeforeClose() call back to the UIT_DestroyBrowser() where 
it was before the modal window support changed it.

I changed the name of the proposed PrepareToCloseModalWindow() to just 
PrepareToCloseBrowser() which only needs to be called when closing the parent 
window of a contained browser window.

Thanks for taking a look at this patch.

Original comment by gusver...@gmail.com on 14 Jul 2011 at 7:17

Attachments:

• cef_fix_window_close_for_contained_browsers_r267.patch

[GoogleCodeExporter]

Perhaps we're make this too complicated. These are the close steps as I 
understand them for modal windows:

1. Re-enable the parent window in WM_CLOSE. WM_CLOSE messages are currently 
sent directly by the client or indirectly by calling CefBrowser::CloseBrowser().

2. Quit the modal loop in WM_DESTROY. WM_DESTROY messages are sent when a 
parent window is destroyed or when the WM_CLOSE message is allowed to 
propagate. WM_DESTROY calls UIT_DestroyBrowser().

We need to give the client an opportunity to handle both of these events. If 
the client doesn't handle them then we need to do so internally.

This is what I think should happen:

1. Add a new DoClose() callback that will be executed first thing when a 
WM_CLOSE message is received. If the client returns false from DoClose() then 
the WM_CLOSE message will be allowed to propagate resulting in a WM_DESTROY 
message. If the client returns true from DoClose() then CEF will return 
immediately without propagating the WM_CLOSE message and the client can either 
cancel the close (by doing nothing) or perform some other action (like 
destroying a parent window) that eventually results in the browser window 
receiving a WM_DESTROY message. Eliminate the current OnBeforeClose() callback 
in WM_CLOSE. If the user provided their own modal loop implementation they 
would re-enable the parent window in DoClose().

2. Always call OnBeforeClose() in UIT_DestroyBrowser(). Eliminate the current 
QuitModal() callback in UIT_DestroyBrowser(). If the user provided their own 
modal loop implementation they would exit that loop in OnBeforeClose().

What do you think?

Original comment by magreenb...@gmail.com on 14 Jul 2011 at 8:47

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Sorry for the delay in my response... I was on a much needed vacation. :)

Attached is an updated patch with your suggestions. I still need a way execute 
the cleanup code that re-enables the opener window when I close the parent 
window of a browser window. I am calling this method 
CefBrowser::ParentWindowWillClose() rather than PrepareToCloseBrowser() to make 
it clear that this only matters to clients when the browser window is contained 
in some other window they own.

Thanks for your suggestions.

Original comment by gusver...@gmail.com on 26 Jul 2011 at 4:42

Attachments:

• cef_fix_window_close_for_contained_browsers_r268.patch

[GoogleCodeExporter]

Looks great, thank you. Committed with minor documentation and style changes as 
revision 271.

Original comment by magreenb...@gmail.com on 3 Aug 2011 at 3:36

• Changed state: Fixed