Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state.

chrome/browser/ui/views/chrome_views_delegate.cc

 // Copyright (c) 2012 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/browser/ui/views/chrome_views_delegate.h"
 
 #include <memory>
 
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 81 matching lines @@
 PrefService* GetPrefsForWindow(const views::Widget* window) {
   Profile* profile = GetProfileForWindow(window);
   if (!profile) {
     // Use local state for windows that have no explicit profile.
     return g_browser_process->local_state();
   }
   return profile->GetPrefs();
 }
 
 #if defined(OS_WIN)
-bool MonitorHasTopmostAutohideTaskbarForEdge(UINT edge, HMONITOR monitor) {
+bool MonitorHasAutohideTaskbarForEdge(UINT edge, HMONITOR monitor) {
   APPBARDATA taskbar_data = { sizeof(APPBARDATA), NULL, 0, edge };
   taskbar_data.hWnd = ::GetForegroundWindow();
 
   // TODO(robliao): Remove ScopedTracker below once crbug.com/462368 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "462368 MonitorHasTopmostAutohideTaskbarForEdge"));
+          "462368 MonitorHasAutohideTaskbarForEdge"));
 
   // MSDN documents an ABM_GETAUTOHIDEBAREX, which supposedly takes a monitor
   // rect and returns autohide bars on that monitor.  This sounds like a good
   // idea for multi-monitor systems.  Unfortunately, it appears to not work at
   // least some of the time (erroneously returning NULL) and there's almost no
   // online documentation or other sample code using it that suggests ways to
   // address this problem. We do the following:-
   // 1. Use the ABM_GETAUTOHIDEBAR message. If it works, i.e. returns a valid
   //    window we are done.
   // 2. If the ABM_GETAUTOHIDEBAR message does not work we query the auto hide
@@ skipping 12 matching lines @@
 
     taskbar_data.hWnd = ::FindWindow(L"Shell_TrayWnd", NULL);
     if (!::IsWindow(taskbar_data.hWnd))
       return false;
 
     SHAppBarMessage(ABM_GETTASKBARPOS, &taskbar_data);
     if (taskbar_data.uEdge == edge)
       taskbar = taskbar_data.hWnd;
   }
 
-  if (::IsWindow(taskbar) &&
-      (GetWindowLong(taskbar, GWL_EXSTYLE) & WS_EX_TOPMOST)) {
+  if (::IsWindow(taskbar)) {
     if (MonitorFromWindow(taskbar, MONITOR_DEFAULTTONEAREST) == monitor)
       return true;
     // In some cases like when the autohide taskbar is on the left of the
     // secondary monitor, the MonitorFromWindow call above fails to return the
     // correct monitor the taskbar is on. We fallback to MonitorFromPoint for
     // the cursor position in that case, which seems to work well.
     POINT cursor_pos = {0};
     GetCursorPos(&cursor_pos);
     if (MonitorFromPoint(cursor_pos, MONITOR_DEFAULTTONEAREST) == monitor)
       return true;
   }
   return false;
 }
 
 int GetAppbarAutohideEdgesOnWorkerThread(HMONITOR monitor) {
   DCHECK(monitor);
 
   int edges = 0;
-  if (MonitorHasTopmostAutohideTaskbarForEdge(ABE_LEFT, monitor))
+  if (MonitorHasAutohideTaskbarForEdge(ABE_LEFT, monitor))
     edges |= views::ViewsDelegate::EDGE_LEFT;
-  if (MonitorHasTopmostAutohideTaskbarForEdge(ABE_TOP, monitor))
+  if (MonitorHasAutohideTaskbarForEdge(ABE_TOP, monitor))
     edges |= views::ViewsDelegate::EDGE_TOP;
-  if (MonitorHasTopmostAutohideTaskbarForEdge(ABE_RIGHT, monitor))
+  if (MonitorHasAutohideTaskbarForEdge(ABE_RIGHT, monitor))
     edges |= views::ViewsDelegate::EDGE_RIGHT;
-  if (MonitorHasTopmostAutohideTaskbarForEdge(ABE_BOTTOM, monitor))
+  if (MonitorHasAutohideTaskbarForEdge(ABE_BOTTOM, monitor))
     edges |= views::ViewsDelegate::EDGE_BOTTOM;
   return edges;
 }
 #endif
 
 #if defined(USE_ASH)
 void ProcessAcceleratorNow(const ui::Accelerator& accelerator) {
   // TODO(afakhry): See if we need here to send the accelerator to the
   // FocusManager of the active window in a follow-up CL.
   ash::WmShell::Get()->accelerator_controller()->Process(accelerator);
@@ skipping 281 matching lines @@
 
 ui::ContextFactory* ChromeViewsDelegate::GetContextFactory() {
   return content::GetContextFactory();
 }
 
 std::string ChromeViewsDelegate::GetApplicationName() {
   return version_info::GetProductName();
 }
 
 #if defined(OS_WIN)
+// We use the SHAppBarMessage API to get the taskbar autohide state. This API

[Peter Kasting, 2016/12/02 01:56:41]

Perhaps this comment should be moved up to where we had the TOPMOST check, and
start with something like "On Windows versions earlier than Windows 7, taskbars
could easily be made always-on-top or not, in which case we'd only want to
return a taskbar with WS_EX_TOPMOST set.  However, this causes problems."  Then
you could say most of the rest of the comment, but modify point 4 and the
closing paragraph a bit.

[ananta, 2016/12/02 06:09:14]

Done.

+// spins a modal loop which could cause callers to be reentered. This may not
+// be desirable and to avoid that we retrieve the taskbar state in a worker
+// thread. This fixes the reentrancy problems but introduces subtle timing
+// issues like the one below:
+// 1. A maximized chrome window is fullscreened.
+// 2. It is switched back to maximized.
+// 3. In the process the window gets a WM_NCCACLSIZE message which calls us to
+//    get the autohide state.
+// 4. The worker thread is invoked and the main thread is suspended. The worker

[Peter Kasting, 2016/12/02 01:56:41]

Is the main thread always suspended, or is it just that the worrisome case is
the one when the main thread is for some reason suspended?

[ananta, 2016/12/02 06:09:14]

The latter.

+//    thread calls the API to get the autohide state. The code also checked
+//    whether the taskbar was a topmost window. That fails in this case because
+//    the main window on the main thread is still in the process of switching
+//    away from fullscreen.
+// 5. The main thread resumes and does not leave space for the taskbar and
+//    hence it does not popup up when hovered on.
+//
+// To fix point 4 above we removed the check for the WS_EX_TOPMOST style as
+// that is the default on Windows 7+. Additionally we also want to ensure
+// that we query the taskbar state at the point when a window switches fully
+// away from fullscreen. The best time appears to be when we receive the
+// ABN_FULLSCREENAPP notification from the shell. Please refer to the
+// HWNDMessageHandler::OnAppBarMessage() for the code.
 int ChromeViewsDelegate::GetAppbarAutohideEdges(HMONITOR monitor,
                                                 const base::Closure& callback) {
   // Initialize the map with EDGE_BOTTOM. This is important, as if we return an
   // initial value of 0 (no auto-hide edges) then we'll go fullscreen and
   // windows will automatically remove WS_EX_TOPMOST from the appbar resulting
   // in us thinking there is no auto-hide edges. By returning at least one edge
   // we don't initially go fullscreen until we figure out the real auto-hide
   // edges.
   if (!appbar_autohide_edge_map_.count(monitor))
     appbar_autohide_edge_map_[monitor] = EDGE_BOTTOM;
@@ skipping 43 matching lines @@
   return ViewsDelegate::GetDialogRelatedButtonHorizontalSpacing();
 }
 
 #if !defined(USE_ASH)
 views::Widget::InitParams::WindowOpacity
 ChromeViewsDelegate::GetOpacityForInitParams(
     const views::Widget::InitParams& params) {
   return views::Widget::InitParams::OPAQUE_WINDOW;
 }
 #endif