Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2542533002:
Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. (Closed)
DescriptionDon't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state.
We query the autohide taskbar state when a window is maximized to ensure
that we leave just enough space at the edge so that the taskbar shows up
when the mouse is moved there.
Once we find the taskbar via the SHAppBarMessage API we also check if the
taskbar is a topmost window. While this works in most cases, I found cases
where in the taskbar loses the topmost style before the window is resized
away from fullscreen. This can presumably happen if there are other
activation changes going on at the same time, or if the machine is slow.
I think it is safe to remove the topmost style check for the taskbar. In
any case we also check the monitor where the taskbar is showing up to
ensure that they are the same. Also this code only executes when a window
is maximized. From my testing this works correctly in all cases.
Added code to register for shell fullscreen notifications via SHAppBarMessage in hwnd_message_handler.cc.
We call SendFrameChanged() if we are in maximized window and a window switched away from fullscreen
BUG=152241
Committed: https://crrev.com/abe5bdca3d7d17b9f139a4ce9e856f446d33d24a
Cr-Commit-Position: refs/heads/master@{#436157}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Register for fullscreen notifications from the shell and send a frame changed if we are maximized #Patch Set 3 : Fix compile failures #Patch Set 4 : Added comments #
Total comments: 6
Patch Set 5 : Revert changes to hwnd_message_handler.cc and add comments #
Total comments: 14
Patch Set 6 : Revert changes to hwnd_message_handler.cc and add comments #Patch Set 7 : Address review comments #Messages
Total messages: 57 (33 generated)
ananta@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2542533002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_views_delegate.cc (left): https://codereview.chromium.org/2542533002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:143: (GetWindowLong(taskbar, GWL_EXSTYLE) & WS_EX_TOPMOST)) { Found this patch from 2+ years ago which had this code. Could not find log beyond that.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ananta@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2542533002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_views_delegate.cc (left): https://codereview.chromium.org/2542533002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:143: (GetWindowLong(taskbar, GWL_EXSTYLE) & WS_EX_TOPMOST)) { On 2016/11/30 02:16:18, ananta wrote: > Found this patch from 2+ years ago which had this code. Could not find log > beyond that. You didn't link to the patch, but I tracked this down further. (Just gotta be persistent about tracing through git...) It was added intentionally here: https://codereview.chromium.org/66055 See also https://bugs.chromium.org/p/chromium/issues/detail?id=20#c135 , and maybe https://bugs.chromium.org/p/chromium/issues/detail?id=20#c121 . I'd like to understand this stuff more closely before OKing removing this. It seems like we don't want non-topmost autohide bars to reserve any space, and I'm worried this would regress that.
https://codereview.chromium.org/2542533002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_views_delegate.cc (left): https://codereview.chromium.org/2542533002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:143: (GetWindowLong(taskbar, GWL_EXSTYLE) & WS_EX_TOPMOST)) { On 2016/11/30 07:30:30, Peter Kasting wrote: > On 2016/11/30 02:16:18, ananta wrote: > > Found this patch from 2+ years ago which had this code. Could not find log > > beyond that. > > You didn't link to the patch, but I tracked this down further. (Just gotta be > persistent about tracing through git...) > > It was added intentionally here: > > https://codereview.chromium.org/66055 > > See also https://bugs.chromium.org/p/chromium/issues/detail?id=20#c135 , and > maybe https://bugs.chromium.org/p/chromium/issues/detail?id=20#c121 . > > I'd like to understand this stuff more closely before OKing removing this. It > seems like we don't want non-topmost autohide bars to reserve any space, and I'm > worried this would regress that. I found this link on msdn https://msdn.microsoft.com/en-us/library/windows/desktop/bb787947(v=vs.85).aspx which states that the option to have a not on top taskbar does not exist anymore. (From Win 7+) that is. We don't care about vista or XP anymore. With this it should be safe to remove the topmost check?
On 2016/11/30 20:37:47, ananta wrote: > https://codereview.chromium.org/2542533002/diff/1/chrome/browser/ui/views/chr... > File chrome/browser/ui/views/chrome_views_delegate.cc (left): > > https://codereview.chromium.org/2542533002/diff/1/chrome/browser/ui/views/chr... > chrome/browser/ui/views/chrome_views_delegate.cc:143: (GetWindowLong(taskbar, > GWL_EXSTYLE) & WS_EX_TOPMOST)) { > On 2016/11/30 07:30:30, Peter Kasting wrote: > > On 2016/11/30 02:16:18, ananta wrote: > > > Found this patch from 2+ years ago which had this code. Could not find log > > > beyond that. > > > > You didn't link to the patch, but I tracked this down further. (Just gotta be > > persistent about tracing through git...) > > > > It was added intentionally here: > > > > https://codereview.chromium.org/66055 > > > > See also https://bugs.chromium.org/p/chromium/issues/detail?id=20#c135 , and > > maybe https://bugs.chromium.org/p/chromium/issues/detail?id=20#c121 . > > > > I'd like to understand this stuff more closely before OKing removing this. It > > seems like we don't want non-topmost autohide bars to reserve any space, and > I'm > > worried this would regress that. > > I found this link on msdn > https://msdn.microsoft.com/en-us/library/windows/desktop/bb787947(v=vs.85).aspx > which states that the option to have a not on top taskbar does not exist > anymore. (From Win 7+) that is. We don't care about vista or XP anymore. With > this it should be safe to remove the topmost check? It's true that they removed the option. I did see a variety of posts online suggesting hacks to get it back. It would be nice to support those hacks. However, if this bit is really untrustworthy, and it's causing the linked bug, then removing this seems better than keeping it. LGTM
Description was changed from ========== Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. We query the autohide taskbar state when a window is maximized to ensure that we leave just enough space at the edge so that the taskbar shows up when the mouse is moved there. Once we find the taskbar via the SHAppBarMessage API we also check if the taskbar is a topmost window. While this works in most cases, I found cases where in the taskbar loses the topmost style before the window is resized away from fullscreen. This can presumably happen if there are other activation changes going on at the same time, or if the machine is slow. I think it is safe to remove the topmost style check for the taskbar. In any case we also check the monitor where the taskbar is showing up to ensure that they are the same. Also this code only executes when a window is maximized. From my testing this works correctly in all cases. BUG=152241 ========== to ========== Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. We query the autohide taskbar state when a window is maximized to ensure that we leave just enough space at the edge so that the taskbar shows up when the mouse is moved there. Once we find the taskbar via the SHAppBarMessage API we also check if the taskbar is a topmost window. While this works in most cases, I found cases where in the taskbar loses the topmost style before the window is resized away from fullscreen. This can presumably happen if there are other activation changes going on at the same time, or if the machine is slow. I think it is safe to remove the topmost style check for the taskbar. In any case we also check the monitor where the taskbar is showing up to ensure that they are the same. Also this code only executes when a window is maximized. From my testing this works correctly in all cases. Added code to register for shell fullscreen notifications via SHAppBarMessage in hwnd_message_handler.cc. We call SendFrameChanged() if we are in maximized window and a window switched away from fullscreen BUG=152241 ==========
Description was changed from ========== Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. We query the autohide taskbar state when a window is maximized to ensure that we leave just enough space at the edge so that the taskbar shows up when the mouse is moved there. Once we find the taskbar via the SHAppBarMessage API we also check if the taskbar is a topmost window. While this works in most cases, I found cases where in the taskbar loses the topmost style before the window is resized away from fullscreen. This can presumably happen if there are other activation changes going on at the same time, or if the machine is slow. I think it is safe to remove the topmost style check for the taskbar. In any case we also check the monitor where the taskbar is showing up to ensure that they are the same. Also this code only executes when a window is maximized. From my testing this works correctly in all cases. Added code to register for shell fullscreen notifications via SHAppBarMessage in hwnd_message_handler.cc. We call SendFrameChanged() if we are in maximized window and a window switched away from fullscreen BUG=152241 ========== to ========== Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. We query the autohide taskbar state when a window is maximized to ensure that we leave just enough space at the edge so that the taskbar shows up when the mouse is moved there. Once we find the taskbar via the SHAppBarMessage API we also check if the taskbar is a topmost window. While this works in most cases, I found cases where in the taskbar loses the topmost style before the window is resized away from fullscreen. This can presumably happen if there are other activation changes going on at the same time, or if the machine is slow. I think it is safe to remove the topmost style check for the taskbar. In any case we also check the monitor where the taskbar is showing up to ensure that they are the same. Also this code only executes when a window is maximized. From my testing this works correctly in all cases. Added code to register for shell fullscreen notifications via SHAppBarMessage in hwnd_message_handler.cc. We call SendFrameChanged() if we are in maximized window and a window switched away from fullscreen BUG=152241 ==========
I updated the patchset to register for fullscreen notifications from the shell and send a frame changed if we receive a notification that a window switched from fullscreen.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/01 00:43:19, ananta wrote: > I updated the patchset to register for fullscreen notifications from the shell > and send a frame changed if we receive a notification that a window switched > from fullscreen. Are these changes complementary to each other? Is one insufficient without the other?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2016/12/01 00:53:11, Peter Kasting wrote: > On 2016/12/01 00:43:19, ananta wrote: > > I updated the patchset to register for fullscreen notifications from the shell > > and send a frame changed if we receive a notification that a window switched > > from fullscreen. > > Are these changes complementary to each other? Is one insufficient without the > other? They are unrelated to each other. I added the fullscreen notification handlers to try and ensure that we attempt an nccalcsize just after the fullscreen window has gone.
On 2016/12/01 04:29:27, ananta wrote: > On 2016/12/01 00:53:11, Peter Kasting wrote: > > On 2016/12/01 00:43:19, ananta wrote: > > > I updated the patchset to register for fullscreen notifications from the > shell > > > and send a frame changed if we receive a notification that a window switched > > > from fullscreen. > > > > Are these changes complementary to each other? Is one insufficient without > the > > other? > > They are unrelated to each other. I added the fullscreen notification handlers > to try and > ensure that we attempt an nccalcsize just after the fullscreen window has gone. What's the user-visible effect of doing/not doing that? Are these both covered by the same bug?
On 2016/12/01 04:41:13, Peter Kasting wrote: > On 2016/12/01 04:29:27, ananta wrote: > > On 2016/12/01 00:53:11, Peter Kasting wrote: > > > On 2016/12/01 00:43:19, ananta wrote: > > > > I updated the patchset to register for fullscreen notifications from the > > shell > > > > and send a frame changed if we receive a notification that a window > switched > > > > from fullscreen. > > > > > > Are these changes complementary to each other? Is one insufficient without > > the > > > other? > > > > They are unrelated to each other. I added the fullscreen notification handlers > > to try and > > ensure that we attempt an nccalcsize just after the fullscreen window has > gone. > > What's the user-visible effect of doing/not doing that? Are these both covered > by the same bug? Yes. Hopefully with these fixes the complaints with maximized chrome windows covering the autohide taskbar will come down.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/01 05:17:05, ananta wrote: > On 2016/12/01 04:41:13, Peter Kasting wrote: > > On 2016/12/01 04:29:27, ananta wrote: > > > On 2016/12/01 00:53:11, Peter Kasting wrote: > > > > On 2016/12/01 00:43:19, ananta wrote: > > > > > I updated the patchset to register for fullscreen notifications from the > > > shell > > > > > and send a frame changed if we receive a notification that a window > > switched > > > > > from fullscreen. > > > > > > > > Are these changes complementary to each other? Is one insufficient > without > > > the > > > > other? > > > > > > They are unrelated to each other. I added the fullscreen notification > handlers > > > to try and > > > ensure that we attempt an nccalcsize just after the fullscreen window has > > gone. > > > > What's the user-visible effect of doing/not doing that? Are these both > covered > > by the same bug? > > Yes. Hopefully with these fixes the complaints with maximized chrome windows > covering the autohide > taskbar will come down. You answered my second question but not my first. I'm sorry to be so pedantic here, but this stuff is _extremely_ subtle and very confusing to track down later. Knowing the exact effect of every tiny change here and how to test it will come in handy seven years on when trying to figure out whether it's safe to tweak or remove these sorts of small changes. So what I'm hoping for is detailed, precise steps on exactly what effects one should expect to see with/without each change and how to test them, expressed as comments in the code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/01 06:21:13, Peter Kasting wrote: > On 2016/12/01 05:17:05, ananta wrote: > > On 2016/12/01 04:41:13, Peter Kasting wrote: > > > On 2016/12/01 04:29:27, ananta wrote: > > > > On 2016/12/01 00:53:11, Peter Kasting wrote: > > > > > On 2016/12/01 00:43:19, ananta wrote: > > > > > > I updated the patchset to register for fullscreen notifications from > the > > > > shell > > > > > > and send a frame changed if we receive a notification that a window > > > switched > > > > > > from fullscreen. > > > > > > > > > > Are these changes complementary to each other? Is one insufficient > > without > > > > the > > > > > other? > > > > > > > > They are unrelated to each other. I added the fullscreen notification > > handlers > > > > to try and > > > > ensure that we attempt an nccalcsize just after the fullscreen window has > > > gone. > > > > > > What's the user-visible effect of doing/not doing that? Are these both > > covered > > > by the same bug? > > > > Yes. Hopefully with these fixes the complaints with maximized chrome windows > > covering the autohide > > taskbar will come down. > > You answered my second question but not my first. > > I'm sorry to be so pedantic here, but this stuff is _extremely_ subtle and very > confusing to track down later. Knowing the exact effect of every tiny change > here and how to test it will come in handy seven years on when trying to figure > out whether it's safe to tweak or remove these sorts of small changes. > > So what I'm hoping for is detailed, precise steps on exactly what effects one > should expect to see with/without each change and how to test them, expressed as > comments in the code. The only way I was able to reproduce the taskbar issues was via breakpoints in the code and running the autohide detection code in the debugger. Based on what I saw in the debugging session, the hypotheses appeared to be if you introduce just enough delay in the main thread, the worker thread which detect autohide state may not see the state of the taskbar correctly. This could possibly explain the issues which at least some folks are seeing in the field. The topmost check removal in the autohide check code could potentially alleviate part of the problems I allured to above where the taskbar is still not topmost as the fullscreen window hasn't changed state yet. The taskbar notification messages again are intended to run an nccalcsize just after the taskbar knows that a full screen window has changed state. I understand that this is all very iffy. But sadly that is how these bugs are, which explains why we are seeing these issues after 7 years since the first set of fixes went in :(
On 2016/12/01 06:46:20, ananta wrote: > I understand that this is all very iffy. But sadly that is how these bugs are, > which explains why we are seeing these > issues after 7 years since the first set of fixes went in :( It'd be OK with me for us to merely have hard-to-test hypotheses about how these will affect things. I'm just looking for the comments in the code to be exact and detailed about why, if the hypotheses are true, we need these fixes. For example, in your reply above, if "the taskbar is still not topmost as the fullscreen window hasn't changed state yet", when do we think the fullscreen window _will_ change state? Why would moving this check to happen at (whatever point that is) not be an option? For the NCCALCSIZE, why does running that help us? That sort of thing. Enough so that someone with no context of the event and message flow -- which describes me right now -- can understand not only why this might work, but why no other type of fix would make sense.
On 2016/12/01 07:13:43, Peter Kasting wrote: > On 2016/12/01 06:46:20, ananta wrote: > > I understand that this is all very iffy. But sadly that is how these bugs are, > > which explains why we are seeing these > > issues after 7 years since the first set of fixes went in :( > > It'd be OK with me for us to merely have hard-to-test hypotheses about how these > will affect things. I'm just looking for the comments in the code to be exact > and detailed about why, if the hypotheses are true, we need these fixes. For > example, in your reply above, if "the taskbar is still not topmost as the > fullscreen window hasn't changed state yet", when do we think the fullscreen > window _will_ change state? Why would moving this check to happen at (whatever > point that is) not be an option? For the NCCALCSIZE, why does running that help > us? That sort of thing. Enough so that someone with no context of the event > and message flow -- which describes me right now -- can understand not only why > this might work, but why no other type of fix would make sense. Added comments in the ChromeViewsDelegate::GetAppbarAutohideEdges() function which attempt to explain this. PTAL
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm too tired to think really thoroughly about this tonight (will look tomorrow), but at a glance, this definitely helps a ton. Thanks!! The problem space is so much more clear now. I will probably have some suggestions on things which were unclear to me, likely related to the hwnd message handler side of things.
https://codereview.chromium.org/2542533002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2542533002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:469: // We use the SHAppBarMessage API to get the taskbar autohide state. This API 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. https://codereview.chromium.org/2542533002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:478: // 4. The worker thread is invoked and the main thread is suspended. The worker 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? https://codereview.chromium.org/2542533002/diff/60001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2542533002/diff/60001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2417: SendFrameChanged(); Now that we removed the TOPMOST check, does this still have any effect? It seems like it would have an effect if, while Chrome was fullscreened, an app bar would be missing entirely, or in a different position, than when not fullscreened, from the perspective of ABM_GETAUTOHIDEBAR/ABM_GETTASKBARPOS (i.e. the checking code in MonitorHasAutohideTaskbarForEdge()). In that case, we'd want to make sure we check the autohide state after we come out of fullscreen mode. But is that the case with the TOPMOST removal? It seems like fullscreening should just change appbar z-order, not affect the results of those two functions, which means this shouldn't actually matter. (If it did matter, it seems like a better fix than asking the shell for notifications here would be modifying views/aura itself to do this notification once we're sufficiently out of fullscreen mode.)
https://codereview.chromium.org/2542533002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2542533002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:469: // We use the SHAppBarMessage API to get the taskbar autohide state. This API On 2016/12/02 01:56:41, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/2542533002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:478: // 4. The worker thread is invoked and the main thread is suspended. The worker On 2016/12/02 01:56:41, Peter Kasting wrote: > 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? The latter. https://codereview.chromium.org/2542533002/diff/60001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2542533002/diff/60001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2417: SendFrameChanged(); On 2016/12/02 01:56:41, Peter Kasting wrote: > Now that we removed the TOPMOST check, does this still have any effect? > > It seems like it would have an effect if, while Chrome was fullscreened, an app > bar would be missing entirely, or in a different position, than when not > fullscreened, from the perspective of ABM_GETAUTOHIDEBAR/ABM_GETTASKBARPOS (i.e. > the checking code in MonitorHasAutohideTaskbarForEdge()). In that case, we'd > want to make sure we check the autohide state after we come out of fullscreen > mode. But is that the case with the TOPMOST removal? It seems like > fullscreening should just change appbar z-order, not affect the results of those > two functions, which means this shouldn't actually matter. > > (If it did matter, it seems like a better fix than asking the shell for > notifications here would be modifying views/aura itself to do this notification > once we're sufficiently out of fullscreen mode.) I added this mostly for correctness sake, i.e querying the autohide state when the taskbar says that a fullscreen window has gone away which is the case we generally struggle with. However you are right that with the TOPMOST check removal this is just superficial. I will revert this part of the change.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hwnd_message_handler.cc have a bunch of changes in them now that I'm not sure are related. Did you mean to make these? LGTM on the chrome_views_delegate.cc changes. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:144: // subtle timing bugs like the one below: Nit: Since this partly duplicates your later comment, maybe just say "There is a potential race condition here:" in place of this paragraph. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:150: // worker Nit: Rewrap I might remove "and the main thread is suspended" from here, as it implies the thread is always suspended, and since that isn't what you mean, the text below seems clear enough. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:156: // In this case the taskbar would not have the topmost bit set. Nit: Maybe "would not" -> "might not yet"? https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:158: // hence it does not popup up when hovered on. Nit: popup -> pop; remove "on"? https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:492: // We use the SHAppBarMessage API to get the taskbar autohide state. This API Nit: This whole comment probably belongs directly above the PostTaskAndReplyWithResult() call. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:494: // be desirable and to avoid that we retrieve the taskbar state in a worker Nit: I'd remove "This may not be desirable and", which is implied (and it's definitely undesirable, not just maybe) https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:497: // ChromeViewsDelegate::OnGotAppbarAutohideEdges() function. Nit: I'd omit this last sentence, as that's really a concern in that function itself, not to readers here.
The hwnd_message_handler changes have gone now. It was fixed with sync to tip https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:144: // subtle timing bugs like the one below: On 2016/12/02 21:11:30, Peter Kasting wrote: > Nit: Since this partly duplicates your later comment, maybe just say "There is a > potential race condition here:" in place of this paragraph. Done. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:150: // worker On 2016/12/02 21:11:30, Peter Kasting wrote: > Nit: Rewrap > > I might remove "and the main thread is suspended" from here, as it implies the > thread is always suspended, and since that isn't what you mean, the text below > seems clear enough. Done. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:156: // In this case the taskbar would not have the topmost bit set. On 2016/12/02 21:11:30, Peter Kasting wrote: > Nit: Maybe "would not" -> "might not yet"? Done. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:158: // hence it does not popup up when hovered on. On 2016/12/02 21:11:30, Peter Kasting wrote: > Nit: popup -> pop; remove "on"? Done. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:492: // We use the SHAppBarMessage API to get the taskbar autohide state. This API On 2016/12/02 21:11:30, Peter Kasting wrote: > Nit: This whole comment probably belongs directly above the > PostTaskAndReplyWithResult() call. Done. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:494: // be desirable and to avoid that we retrieve the taskbar state in a worker On 2016/12/02 21:11:30, Peter Kasting wrote: > Nit: I'd remove "This may not be desirable and", which is implied (and it's > definitely undesirable, not just maybe) Done. https://codereview.chromium.org/2542533002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:497: // ChromeViewsDelegate::OnGotAppbarAutohideEdges() function. On 2016/12/02 21:11:30, Peter Kasting wrote: > Nit: I'd omit this last sentence, as that's really a concern in that function > itself, not to readers here. Done.
LGTM
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2542533002/#ps120001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480729638410300,
"parent_rev": "efca2869707ac86543f83070d6459db2fe947dba", "commit_rev":
"00da2a9239a38c74b6556438bd14dab4d4226965"}
Message was sent while issue was closed.
Description was changed from ========== Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. We query the autohide taskbar state when a window is maximized to ensure that we leave just enough space at the edge so that the taskbar shows up when the mouse is moved there. Once we find the taskbar via the SHAppBarMessage API we also check if the taskbar is a topmost window. While this works in most cases, I found cases where in the taskbar loses the topmost style before the window is resized away from fullscreen. This can presumably happen if there are other activation changes going on at the same time, or if the machine is slow. I think it is safe to remove the topmost style check for the taskbar. In any case we also check the monitor where the taskbar is showing up to ensure that they are the same. Also this code only executes when a window is maximized. From my testing this works correctly in all cases. Added code to register for shell fullscreen notifications via SHAppBarMessage in hwnd_message_handler.cc. We call SendFrameChanged() if we are in maximized window and a window switched away from fullscreen BUG=152241 ========== to ========== Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. We query the autohide taskbar state when a window is maximized to ensure that we leave just enough space at the edge so that the taskbar shows up when the mouse is moved there. Once we find the taskbar via the SHAppBarMessage API we also check if the taskbar is a topmost window. While this works in most cases, I found cases where in the taskbar loses the topmost style before the window is resized away from fullscreen. This can presumably happen if there are other activation changes going on at the same time, or if the machine is slow. I think it is safe to remove the topmost style check for the taskbar. In any case we also check the monitor where the taskbar is showing up to ensure that they are the same. Also this code only executes when a window is maximized. From my testing this works correctly in all cases. Added code to register for shell fullscreen notifications via SHAppBarMessage in hwnd_message_handler.cc. We call SendFrameChanged() if we are in maximized window and a window switched away from fullscreen BUG=152241 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. We query the autohide taskbar state when a window is maximized to ensure that we leave just enough space at the edge so that the taskbar shows up when the mouse is moved there. Once we find the taskbar via the SHAppBarMessage API we also check if the taskbar is a topmost window. While this works in most cases, I found cases where in the taskbar loses the topmost style before the window is resized away from fullscreen. This can presumably happen if there are other activation changes going on at the same time, or if the machine is slow. I think it is safe to remove the topmost style check for the taskbar. In any case we also check the monitor where the taskbar is showing up to ensure that they are the same. Also this code only executes when a window is maximized. From my testing this works correctly in all cases. Added code to register for shell fullscreen notifications via SHAppBarMessage in hwnd_message_handler.cc. We call SendFrameChanged() if we are in maximized window and a window switched away from fullscreen BUG=152241 ========== to ========== Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state. We query the autohide taskbar state when a window is maximized to ensure that we leave just enough space at the edge so that the taskbar shows up when the mouse is moved there. Once we find the taskbar via the SHAppBarMessage API we also check if the taskbar is a topmost window. While this works in most cases, I found cases where in the taskbar loses the topmost style before the window is resized away from fullscreen. This can presumably happen if there are other activation changes going on at the same time, or if the machine is slow. I think it is safe to remove the topmost style check for the taskbar. In any case we also check the monitor where the taskbar is showing up to ensure that they are the same. Also this code only executes when a window is maximized. From my testing this works correctly in all cases. Added code to register for shell fullscreen notifications via SHAppBarMessage in hwnd_message_handler.cc. We call SendFrameChanged() if we are in maximized window and a window switched away from fullscreen BUG=152241 Committed: https://crrev.com/abe5bdca3d7d17b9f139a4ce9e856f446d33d24a Cr-Commit-Position: refs/heads/master@{#436157} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/abe5bdca3d7d17b9f139a4ce9e856f446d33d24a Cr-Commit-Position: refs/heads/master@{#436157} |
