Preview CL for adding an Infobar facility to Google Chrome Frame....

chrome_frame/infobars/internal/displaced_window.cc

Index: chrome_frame/infobars/internal/displaced_window.cc
===================================================================
--- chrome_frame/infobars/internal/displaced_window.cc	(revision 0)
+++ chrome_frame/infobars/internal/displaced_window.cc	(revision 0)
@@ -0,0 +1,85 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome_frame/infobars/internal/displaced_window.h"
+
+#include "base/logging.h"
+#include "chrome_frame/utils.h"
+
+DISABLE_RUNNABLE_METHOD_REFCOUNT(DisplacedWindowManager);
+
+DisplacedWindowManager::DisplacedWindowManager() {
+}
+
+bool DisplacedWindowManager::Initialize(HWND displaced_hwnd,
+                                        Delegate* delegate) {
+  DCHECK(delegate != NULL);

[tommi (sloooow) - chröme, 2010/11/24 16:08:24]

fyi - when you run gcl lint on this change list, you'll probably get
recommendations to use DCHECK_NE and DCHECK_EQ for these.

[erikwright (departed), 2010/12/01 20:05:52]

I did run lint. It didn't report this. Furthermore, using DCHECK_NE(delegate,
NULL) reports the following:

s:\src\chromium\src\base/logging.h(495) : error C2446: '!=' : no conversion from
'const int' to 'DisplacedWindowManager::Delegate *const '

The only option would be to write DCHECK_NULL and DCHECK_NOT_NULL... maybe a
good idea for a later date?

+  DCHECK(delegate_ == NULL);
+  delegate_.reset(delegate);
+
+  PinModule();

[tommi (sloooow) - chröme, 2010/11/24 16:08:24]

add a comment for why this class requires pinning

[erikwright (departed), 2010/12/01 20:05:52]

(NB this code moved to subclassing_window.h - I added a comment there).

I inherited this from the CEEE implementation. I think it might be required,
depending on how IE unloads BHO modules, but I don't know.

I think BHOs are more or less guaranteed to stay loaded until near the end of
the browser process' life, and I think that the tab window probably lives that
long as well. In that case, it can't hurt to pin, and depending on the order of
tab window destruction vs. BHO unloading, might be essential.

On the other hand, if:
1) a tab window could be destroyed in the middle of the process life
2) it is guaranteed to be destroyed before BHOs are unloaded
3) there are typical use cases where a BHO might be unloaded mid-process-life
Then it might be a mistake to pin the module.

Ultimately, I think it's safest to pin. Let me know what you think?

+  if (!SubclassWindow(displaced_hwnd)) {
+    LOG(DFATAL) << "Failed to subclass IE Renderer HWND for infobar "
+                << "installation.";
+    return false;
+  }
+
+  return true;
+}
+
+void DisplacedWindowManager::UpdateLayout() {
+  PushTask(
+      NewRunnableMethod(this, &DisplacedWindowManager::DoUpdateLayout));
+}
+
+void DisplacedWindowManager::PushTask(Task* task) {
+  task_queue_.Push(task);

[tommi (sloooow) - chröme, 2010/11/24 16:08:24]

since there's no locking, can we DCHECK that this call is being made on the
correct thread?
E.g. you can add a utility method that exists only when DCHECK does that would
look something like this:

bool CurrentThreadIsWindowThread(HWND hwnd) {
  return ::GetCurrentThreadId() == GetWindowThreadProcessId(hwnd, NULL);
}

and then use it in methods like these:

DCHECK(CurrentThreadIsWindowThread(m_hWnd));

[erikwright (departed), 2010/12/01 20:05:52]

This code no longer exists. But the comment is relevant:

Nothing in the infobars implementation is threadsafe, so these checks would have
to go everywhere. If you think it's important, I'd suggest putting the checks at
the external entry points, which are:

InfobarManagerImpl (Show, Hide, HideAll)
InfobarWindow::FrameImpl (GetFrameWindow, CloseInfobar)

Everything else is internal only (and clearly indicated as such by directory
structure) and has no threading, so I think that thread-safety checks within the
module would be unnecessary.

What do you think?

+  if (IsWindow())

[tommi (sloooow) - chröme, 2010/11/24 16:08:24]

Is there a chance that the window doesn't exist at this point?
if not, maybe you could add

else
  NOTREACHED();

[erikwright (departed), 2010/12/01 20:05:52]

I'm uncertain about what happens when a window is destroyed. Is WM_DESTROY
guaranteed to be the last message sent to the window? I assume that IsWindow is
true during processing of WM_DESTROY and false afterwards?

If my assumptions are correct, then this check is not required because the
client of this class is notified when it is destroyed and should no longer
invoke on it.

I've removed any IsWindow checks that I thought were superfluous, and the basic
suite of testcases has not yielded any problems.

+    PostMessage(TM_RUN_TASK_QUEUE, 0, 0);
+}
+
+LRESULT DisplacedWindowManager::OnRunTaskQueue(UINT message,
+                       WPARAM wparam,

[tommi (sloooow) - chröme, 2010/11/24 16:08:24]

indent either 4 spaces or to the same indent as the first parameter

[grt (UTC plus 2), 2010/11/25 18:00:13]

indentation

[erikwright (departed), 2010/12/01 20:05:52]

Done.

[erikwright (departed), 2010/12/01 20:05:52]

Done.

+                       LPARAM lparam,
+                       BOOL& handled) {
+  task_queue_.Run();
+  handled = TRUE;
+  return 0;
+}
+
+void DisplacedWindowManager::DoUpdateLayout() {
+  // Call SetWindowPos with SWP_FRAMECHANGED for IE window, then IE
+  // window would receive WM_NCCALCSIZE to recalculate its client size.
+  if (IsWindow())

[tommi (sloooow) - chröme, 2010/11/24 16:08:24]

I think the coding guidelines call for {} when the body of a condition spans
multiple lines (even if there's only one semicolon).  maybe I'm wrong

[erikwright (departed), 2010/12/01 20:05:52]

Done.

+    ::SetWindowPos(m_hWnd,
+                   NULL, 0, 0, 0, 0,
+                   SWP_NOACTIVATE | SWP_NOMOVE | SWP_NOSIZE | SWP_NOZORDER |
+                   SWP_FRAMECHANGED);
+}
+
+LRESULT DisplacedWindowManager::OnNcCalcSize(BOOL calc_valid_rects,
+                                             LPARAM lparam) {
+  // Ask the original window proc to calculate the 'natural' size of the window.
+  LRESULT ret = DefWindowProc(WM_NCCALCSIZE,
+                              static_cast<WPARAM>(calc_valid_rects), lparam);
+  // Whether calc_valid_rects is true or false, we could treat beginning of

[amit, 2010/11/24 14:21:35]

This is somewhat unlikely but if calc_valid_rects is TRUE and the original
window proc returns valid rectangles overlapping with infobar it could lead to
painting issues during sizing. Please double check the sizing, moving etc. with
info bar displayed. Alternatively, you can assert that IntersectRect of infobar
rect and the other two rects should be empty.

[erikwright (departed), 2010/12/01 20:05:52]

Great point. To be honest I didn't fully understand this event before reading
you comment. I looked into it further, and here's what I think:

1) If the DefWindowProc returns anything other than WVR_VALIDRECTS, I can
essentially do as I do now, which is to say ignore the unused 2nd and 3rd rects
and leave the window's align/redraw request intact.
2) In the case of WVR_VALIDRECTS, if we end up adjusting the rect, there is no
surefire way to "do the right thing" to respect the original window's
intentions. The only solution is to return WVR_REDRAW.

Does that seem right to you?

+  // lparam as a RECT object.
+  RECT* rect = reinterpret_cast<RECT*>(lparam);
+  if (delegate_ != NULL)
+    delegate_->AdjustDisplacedWindowDimensions(rect);
+
+  return ret;
+}
+
+// The displaced window has been destroyed. Inform the InfobarManagerImpl, who
+// will orphan this instance. We will delete ourselves in OnFinalMessage.
+void DisplacedWindowManager::OnDestroy() {
+  if (delegate_ != NULL)
+    delegate_->OnDisplacedWindowDestroyed();
+  delegate_.reset();
+}
+
+void DisplacedWindowManager::OnFinalMessage(HWND) {
+  delete this;
+}
Property changes on: chrome_frame\infobars\internal\displaced_window.cc
___________________________________________________________________
Added: svn:eol-style
   + LF