Bug 14272

This is the Windows version of this bug. The Mac version is bug 14282.

<rdar://problem/5712795>

The Mac and cross platform code landed in r35719.

Created attachment 32606 [details] Height Patch

Comment on attachment 32606 [details] Height Patch It's really great to see this getting fixed! I have a few comments about the general approach to this fix: I think it's not good to be duplicating more code from WebKit/mac in WebKit/win. A much better way to fix this would be to move most of the WebKit/mac code down into WebCore, where it can be shared by WebKit/win. The idea would be to have all the logic down in WebCore, and only call up to WebKit when absolutely necessary. A rough division of responsibility would be: WebCore: * Calculates new window sizes * Saves/restores window size preference WebKit: * Actually resizes the windows to the dimensions specified by WebCore So I think you should redo this in at least two parts: 1) Move most WebKit/mac logic related to the window size down to WebCore 2) Add in the WebKit/win bits Please feel free to ask more questions here in the bug!

Created attachment 32688 [details] Cross Platform Fix + Refactoring Updated patch taking Adam's feedback into account, moving more logic into InspectorController and away from the clients.

Comment on attachment 32688 [details] Cross Platform Fix + Refactoring > + * inspector/InspectorController.cpp: Moved resizing logic from InspectorClient to here That should be "WebInspectorClient.mm", not "InspectorClient". > + virtual long getInspectedWebViewHeight() = 0; We normally only prefix function names with "get" if they return their result through an out-parameter. Since that is not the case here, I'd just call this "inspectedWebViewHeight()". Since WebCore doesn't really know what a "WebView" is, I'd also rename it to "inspectedPageHeight()". But this function doesn't return the height of the inspected page. It returns the height the page would be if the Inspector weren't docked. We should either rename the function to indicate that, or make it truly return the inspected page's height and make InspectorController responsible for adding in the docked Inspector's height. The latter option seems conceptually simpler to me. > @@ -188,6 +191,12 @@ InspectorController::InspectorController > ASSERT_ARG(page, page); > ASSERT_ARG(client, client); > ++s_inspectorControllerCount; > + > + // Set the initial attached height to the users preferred attached height > + // It might not start out attached, but this variable needs to be populated before > + // it could be attached Typo: "users" should be "user's". You should also end your sentences with periods. > + long attachedHeight = round(max(static_cast<float>(250.0), > + static_cast<float>(min(static_cast<float>(height), > + static_cast<float>(m_client->getInspectedWebViewHeight() * .75))))); You should put the constants into named variables. I think that would reduce the need for casting, too: static const minimumAttachedHeight = 250; static const maximumAttachedHeightRatio = 0.75; setAttachedWindowHeight(height) { long attachedHeight = round(max(minimumAttachedHeight, min(static_cast<float>(height), maximumAttachedHeightRatio * inspectedPageHeight()))); } Why is defaultAttachedHeight unsigned when we're using long elsewhere? > + // Save a preference for the users preferred attached height > + Setting heightSetting; > + heightSetting.set(attachedHeight); > + setSetting(inspectorAttachedHeightName, heightSetting); It would probably be better to add an explicit Setting constructor that takes a long. > +++ WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp (working copy) > @@ -112,6 +112,17 @@ String InspectorClient::hiddenPanels() > notImplemented(); > return String(); > } > + > +long getInspectedWebViewHeight() > +{ > + notImplemented(); > + return 0; > +} > + > +void setInitialAttachedHeight(long height) > +{ > + notImplemented(); > +} These function names need to be prefixed with "InspectorClient::". (Ditto for the other platforms, though the prefix will be different for each.) > @@ -69,6 +72,7 @@ public: > private: > void updateWindowTitle() const; > > + long m_initialAttachedHeight; It looks like this member doesn't get initialized by the constructor. > +- (long)getInspectedWebViewHeight > +{ > + ASSERT(_attachedToInspectedWebView); > + > + // For calculations, we want the height of the frame view and the height of the attached inspector > + // to calculate the correct bounds This comment doesn't really make sense to me as written. But if we either rename the function or change its behavior as described above, you probably won't need a comment at all. > @@ -332,7 +363,7 @@ void WebInspectorClient::updateWindowTit > > _attachedToInspectedWebView = YES; > > - [self setAttachedWindowHeight:[[NSUserDefaults standardUserDefaults] integerForKey:WebKitInspectorAttachedViewHeightKey]]; > + [self setAttachedWindowHeight:_attachedHeight]; I think you can get rid of the definition of WebKitInspectorAttachedViewHeightKey now. Should we migrate the old NSUserDefaults-style preference to the new InspectorController::Setting-style preference? > @@ -381,23 +412,21 @@ void WebInspectorClient::updateWindowTit > > - (void)setAttachedWindowHeight:(unsigned)height > { > - [[NSUserDefaults standardUserDefaults] setInteger:height forKey:WebKitInspectorAttachedViewHeightKey]; > - > if (!_attachedToInspectedWebView) > return; > - > - WebFrameView *frameView = [[_inspectedWebView mainFrame] frameView]; > - NSRect frameViewRect = [frameView frame]; > - > - CGFloat attachedHeight = round(MAX(250.0, MIN(height, (NSHeight([_inspectedWebView frame]) * 0.75)))); > - > + > + _attachedHeight = height; Why do we need to store this value? Can't we just calculate it by querying the bounds of the Inspector window? > @@ -76,6 +74,7 @@ WebInspectorClient::WebInspectorClient(W > , m_webViewHwnd(0) > , m_shouldAttachWhenShown(false) > , m_attached(false) > + , m_attachedHeight(0) You're leaving m_attachedParentHeight uninitialized here. > void WebInspectorClient::setAttachedWindowHeight(unsigned height) > { > - // FIXME: implement this. > + RECT inspectedRect; > + GetClientRect(m_inspectedWebViewHwnd, &inspectedRect); > + > + int webViewX = inspectedRect.left; > + int webViewY = m_attachedParentHeight - height; > + int webViewWidth = inspectedRect.right - inspectedRect.left; > + int webViewHeight = height; > + > + m_attachedHeight = height; Why do we need to store this value? Can't we just calculate it by querying the client rect of the Inspector window? > + SendMessage(m_inspectedWebViewHwnd, WM_SETREDRAW, FALSE, NULL); > + SendMessage(m_webViewHwnd, WM_SETREDRAW, FALSE, NULL); We use 0 instead of NULL in C++ files. (This comment applies elsewhere in this patch.) Should we be doing this WM_SETREDRAW stuff in onWebViewWindowPosChanging, too? > + ::SetWindowPos(m_inspectedWebViewHwnd, 0, webViewX, inspectedRect.top, webViewWidth, m_attachedParentHeight, SWP_NOZORDER); > + ::SetWindowPos(m_webViewHwnd, 0, webViewX, webViewY, webViewWidth, webViewHeight, SWP_NOZORDER); You should remove the "::" prefixes here, or add them to the other Win32 API calls in this function (whatever is consistent with the rest of the file). In general, in new code you should leave them off. > @@ -410,12 +440,16 @@ void WebInspectorClient::onWebViewWindow > WINDOWPOS* windowPos = reinterpret_cast<WINDOWPOS*>(lParam); > ASSERT_ARG(lParam, windowPos); > > + // Cache this value because GetClientRect doesn't give the correct parent height > + // And whenever the height updates and an inspector is attached, this function will be called first. > + m_attachedParentHeight = windowPos->cy; Why do we need to store this value? Can't we calculate it by getting the client rects of the inspected WebView and the Inspector, and adding their heights?

Created attachment 32761 [details] Take 3 Fixes comments made by aroben about previous patch.

Comment on attachment 32761 [details] Take 3 The improvements here look really good. > + virtual long inspectorPageHeight() = 0; Do we really need the client to give us this information? I think m_page->mainFrame()->view()->visibleHeight() would give us the same information. > + virtual long inspectedPageHeight() = 0; For the same reason, it seems like we don't need inspectedPageHeight(), either. We can do the same thing as above, but use m_inspectedPage instead of m_page. > + static const float minimumAttachedHeight = 250.0; > + static const float maximumAttachedHeightRatio = 0.75; I think you should move these up next to defaultAttachedHeight. > + long attachedHeight = round(max(minimumAttachedHeight, > + min(static_cast<float>(height), (m_client->inspectedPageHeight() + m_client->inspectorPageHeight()) * maximumAttachedHeightRatio))); > + > + // Save a preference for the user's preferred attached height and pass it to the client > + setSetting(inspectorAttachedHeightName, Setting(attachedHeight)); > + m_client->setInitialAttachedHeight(attachedHeight); > + > + m_client->setAttachedWindowHeight(attachedHeight); It seems bad that we have to call both of these functions one after the other. I think that we can get rid of setInitialAttachedHeight completely. I think the way this should work is: 1) Whenever the Inspector becomes attached (either because it is being shown for the first time and is starting out attached, or because the user clicked the button to attach it), the client should set the height of the Inspector to the value stored in the settings (so the client would ask the InspectorController for that height value) 2) Whenever the height of the attached Inspector changes, the InspectorController saves the new height to settings and calls InspectorClient::setAttachedWindowHeight, and the client obliges. So there's no need for the whole "initial attached height" concept here. That will let the clients get rid of their m_initialAttachedHeight members. > private: > void updateWindowTitle() const; > > + long m_initialAttachedHeight; You still aren't initializing this in the constructor (in WebInspectorClient.mm). But as I explained above, I think we can get rid of this entirely. > void WebInspectorClient::setAttachedWindowHeight(unsigned height) > { > - // FIXME: implement this. > + if (!m_attached) > + return; > + > + RECT inspectedRect; > + GetClientRect(m_inspectedWebViewHwnd, &inspectedRect); > + > + int webViewWidth = inspectedRect.right - inspectedRect.left; > + int totalHeight = inspectedPageHeight() + inspectorPageHeight(); > + > + SetWindowPos(m_webViewHwnd, 0, 0, totalHeight - height, webViewWidth, height, SWP_NOZORDER); > + SetWindowPos(m_inspectedWebViewHwnd, 0, 0, 0, webViewWidth, totalHeight, SWP_NOZORDER); > + > + RedrawWindow(m_webViewHwnd, 0, 0, RDW_INVALIDATE | RDW_ALLCHILDREN | RDW_UPDATENOW); > + RedrawWindow(m_inspectedWebViewHwnd, 0, 0, RDW_INVALIDATE | RDW_ALLCHILDREN | RDW_UPDATENOW); > } I guess you decided we don't need the WM_SETREDRAW stuff after all? I think after making the changes I described above, we won't need any new InspectorClient functions at all! That should make the patch a lot smaller and simpler. Sorry my thinking on this wasn't clearer before.

Created attachment 32812 [details] Attach Bug To get bug: 1) Open web inspector, and get it in attached mode. 2) Detach it, and resize the window vertically when it is in detached mode. 3) Re-attach it. You will see it down too far on the screen, the whole inspector won't be showing.

Created attachment 32813 [details] Attach Bug To reproduce bug: 1) Open inspector, and attach it. 2) Detach it, and resize the detached inspector window. 3) Re-attach it, it will be too far down on the screen, the whole inspector won't be visible until you resize the whole window (the inspected web view + inspector).

Created attachment 32883 [details] Re-attaching Bug

Created attachment 32889 [details] Fixed Reattach Bug - No variable Caching

Comment on attachment 32889 [details] Fixed Reattach Bug - No variable Caching > +long InspectorController::constrainedAttachedWindowHeight(long preferredHeight, long totalWindowHeight) > +{ > + if (preferredHeight == 0) { We normally write this as if (!preferredHeight). > @@ -323,7 +320,8 @@ void WebInspectorClient::updateWindowTit > > if (_shouldAttach) { > WebFrameView *frameView = [[_inspectedWebView mainFrame] frameView]; > - > + NSRect frameViewRect = [frameView frame]; I don't think this local variable is really useful. > @@ -331,8 +329,8 @@ void WebInspectorClient::updateWindowTit > [frameView setAutoresizingMask:(NSViewWidthSizable | NSViewHeightSizable | NSViewMinYMargin)]; > > _attachedToInspectedWebView = YES; > - > - [self setAttachedWindowHeight:[[NSUserDefaults standardUserDefaults] integerForKey:WebKitInspectorAttachedViewHeightKey]]; > + > + [self setAttachedWindowHeight:[_inspectedWebView page]->inspectorController()->constrainedAttachedWindowHeight(0, NSHeight(frameViewRect))]; Why do both InspectorClient implementations have to call setAttachedWindowHeight themselves? Can't the InspectorController make this call for us (passing in the height of the inspected page from before InspectorClient::attachWindow was called)? If InspectorController did it for us, we could get rid of the constrainedAttachedWindowHeight function. > @@ -240,6 +238,16 @@ void WebInspectorClient::attachWindow() > > closeWindowWithoutNotifications(); > showWindowWithoutNotifications(); > + > + HWND hostWindow; > + if (SUCCEEDED(m_inspectedWebView->hostWindow((OLE_HANDLE*)&hostWindow))) Please use reinterpret_cast instead of a C-style cast. > + SendMessage(hostWindow, WM_SIZE, 0, 0); // Make sure we have the correct size What happens if you omit this SendMessage call? > + RECT hostWindowRect; > + GetClientRect(hostWindow, &hostWindowRect); > + > + // This re-constrains our attached height, in case while the inspector was detached, they resized the inspected window, > + // and drawing it at the size it used to be could lead to the inspector taking up the whole window or more Please put a period at the end of this comment. > void WebInspectorClient::setAttachedWindowHeight(unsigned height) > { > - // FIXME: implement this. > + if (!m_attached) > + return; > + > + HWND hostWindow; > + if (!SUCCEEDED(m_inspectedWebView->hostWindow((OLE_HANDLE*)&hostWindow))) You should use reinterpret_cast here, and FAILED() instead of !SUCCEEDED(). > + return; > + > + RECT hostWindowRect; > + GetClientRect(hostWindow, &hostWindowRect); > + > + RECT inspectedRect; > + GetClientRect(m_inspectedWebViewHwnd, &inspectedRect); > + > + int totalHeight = hostWindowRect.bottom - hostWindowRect.top; > + int webViewWidth = inspectedRect.right - inspectedRect.left; > + > + SetWindowPos(m_webViewHwnd, 0, 0, totalHeight - height, webViewWidth, height, SWP_NOZORDER); > + SetWindowPos(m_inspectedWebViewHwnd, 0, 0, 0, webViewWidth, totalHeight, SWP_NOZORDER); This doesn't seem right. Shouldn't the height of m_inspectedWebViewHwnd be set to totalHeight - height?

Created attachment 32903 [details] Resize Attached Inspector

Comment on attachment 32903 [details] Resize Attached Inspector > + int inspectedHeight = m_inspectedPage->mainFrame()->view()->visibleHeight(); There seems to be a mix of "long", "int", and "unsigned" when talking about attached window heights. It would be good to use a single type as much as possible. "inspectedHeight" is a confusing name. We aren't inspecting the height, we're inspecting the page. "inspectedPageHeight" would be better. > m_client->attachWindow(); > + > + Setting attachedHeight = setting(inspectorAttachedHeightName); > + int preferredHeight = attachedHeight.type() == Setting::IntegerType ? attachedHeight.integerValue() : defaultAttachedHeight; > + m_client->setAttachedWindowHeight(constrainedAttachedWindowHeight(preferredHeight, inspectedHeight)); It would be good to add a comment here about how we have to reconstrain the height based on the current height of the inspected page. > +long InspectorController::constrainedAttachedWindowHeight(long preferredHeight, long totalWindowHeight) This can just be a static helper function. It doesn't need to be a member of the InspectorController class. > + // Save a preference for the user's preferred attached height and pass it to the client > + setSetting(inspectorAttachedHeightName, Setting(attachedHeight)); We aren't saving the preferred height, we're saving the constrained height. I don't think this comment is needed at all, though. > void InspectorController::showWindow() > { > ASSERT(enabled()); > + > + int inspectedHeight = m_inspectedPage->mainFrame()->view()->visibleHeight(); > + > m_client->showWindow(); > + > + Setting attachedHeight = setting(inspectorAttachedHeightName); > + int preferredHeight = attachedHeight.type() == Setting::IntegerType ? attachedHeight.integerValue() : defaultAttachedHeight; > + m_client->setAttachedWindowHeight(constrainedAttachedWindowHeight(preferredHeight, inspectedHeight)); > } We only need to do this if we're going to start out attached, right? Again, I think a comment would be useful. It might also be worth adding a FIXME about how it's strange that we have to do this both here and in attachWindow; we should only have to do it once. It might be that we can fix this eventually by moving more code down into WebCore, but we don't have to do that now. > @@ -323,7 +320,7 @@ void WebInspectorClient::updateWindowTit > > if (_shouldAttach) { > WebFrameView *frameView = [[_inspectedWebView mainFrame] frameView]; > - > + Please undo this whitespace change. > - (void)setAttachedWindowHeight:(unsigned)height > { > - [[NSUserDefaults standardUserDefaults] setInteger:height forKey:WebKitInspectorAttachedViewHeightKey]; > - > if (!_attachedToInspectedWebView) > return; > - > - WebFrameView *frameView = [[_inspectedWebView mainFrame] frameView]; > - NSRect frameViewRect = [frameView frame]; > - > - CGFloat attachedHeight = round(MAX(250.0, MIN(height, (NSHeight([_inspectedWebView frame]) * 0.75)))); > - > + Please remove the whitespace on this line. > void WebInspectorClient::setAttachedWindowHeight(unsigned height) > { > - // FIXME: implement this. > + if (!m_attached) > + return; > + > + HWND hostWindow; > + if (!SUCCEEDED(m_inspectedWebView->hostWindow((OLE_HANDLE*)&hostWindow))) > + return; > + > + RECT hostWindowRect; > + GetClientRect(hostWindow, &hostWindowRect); > + > + RECT inspectedRect; > + GetClientRect(m_inspectedWebViewHwnd, &inspectedRect); > + > + int totalHeight = hostWindowRect.bottom - hostWindowRect.top; > + int webViewWidth = inspectedRect.right - inspectedRect.left; > + > + SetWindowPos(m_webViewHwnd, 0, 0, totalHeight - height, webViewWidth, height, SWP_NOZORDER); > + SetWindowPos(m_inspectedWebViewHwnd, 0, 0, 0, webViewWidth, totalHeight, SWP_NOZORDER); Please add a comment here explaining why totalHeight is the right height to set m_inspectedWebViewHwnd to. So close! I'm really happy with how this is shaping up.

Created attachment 32968 [details] Resize Attach Height

Comment on attachment 32968 [details] Resize Attach Height > void InspectorController::showWindow() > { > ASSERT(enabled()); > + > + unsigned inspectedPageHeight = m_inspectedPage->mainFrame()->view()->visibleHeight(); > + > m_client->showWindow(); > + > + Setting attachedHeight = setting(inspectorAttachedHeightName); > + unsigned preferredHeight = attachedHeight.type() == Setting::IntegerType ? attachedHeight.integerValue() : defaultAttachedHeight; > + > + // This call might not go through (if the window starts out detached), but if the window is initially created attached, > + // InspectorController::attachWindow is never called, so we need to make sure to set the attachedWindowHeight. > + m_client->setAttachedWindowHeight(constrainedAttachedWindowHeight(preferredHeight, inspectedPageHeight)); I think it's worth adding a FIXME here about cleaning this up so we can share this logic with attachWindow. This turned out really nicely. Thanks for doing all those revisions and working out the bugs! r=me

Committed in revision 46071.