Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1952473002:
Fix for 504133 - wandering identity switcher button (Closed)
DescriptionFix for wandering identity switcher button when magnified
BUG=504133
Committed: https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7
Cr-Commit-Position: refs/heads/master@{#391595}
Committed: https://crrev.com/13715b01cf03787843ff7f78bc002c604942ea36
Cr-Commit-Position: refs/heads/master@{#393977}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Cleaup. #
Total comments: 12
Patch Set 3 : Update based on feedback. #
Total comments: 6
Patch Set 4 : Results of git cl format #Patch Set 5 : This patch should fix the regressions and ensure the Identity switcher lands in the correct location #Patch Set 6 : Removed unnecessary 'using' statement. #
Total comments: 7
Patch Set 7 : Changes based on review feedback. #
Total comments: 4
Patch Set 8 : Code style fixes #Patch Set 9 : Must have relied on an include chain which changed for gfx::Point. Now explicitly included. #
Messages
Total messages: 41 (15 generated)
Description was changed from ========== Include order changes Include order change, re-wraped comments Removed some trailing whitespace Fix for wandering identity switcher button when magnified BUG= ========== to ========== Fix for wandering identity switcher button when magnified BUG=504133 ==========
kylixrd@chromium.org changed reviewers: + sky@chromium.org
This change should take into account the per-monitor DPI settings as handled by display::win::ScreenWin. https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:11: #include "ui/display/win/screen_win.h" Pre-submit checks complained about the include order... it seems that the order was intentional since the first include was this file's own include. https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:17: int GetMinimizeButtonOffsetForWindow(HWND hwnd, bool was_activated = true) { TODO: remove default parameter. https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:23: if (button_bounds.left != button_bounds.right) { If these values are the same (typically 0), then DWM composition is disabled for this window or is globally disabled. https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:36: if (!dwm_button_pos) { If for whatever reason, any of the above fails, fall back to the old message technique. https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:62: return dip_point.x(); From inspecting the code in display::win::ScreenWin, ClientToDIPPoint seems to convert the pixel-based point to DIP based points using the scaling factor of the screen on which the given HWND lives. If that's not the case, then please advise. https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:94: // until we get the activate. TODO: update comment https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:95: if (/*was_activated_ || */!ui::win::IsAeroGlassEnabled() || TODO: cleanup Always attempt to get the button offset since DwmGetWindowAttribute doesn't suffer from the strange issue highlighted in this comment. https://codereview.chromium.org/1952473002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:118: GetMinimizeButtonOffsetForWindow(hwnd_, was_activated_); TODO: cleaner way to handle this?
https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:19: POINT minimize_button_corner = { 0 }; POINT->gfx::Point https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:28: if (GetWindowRect(hwnd, reinterpret_cast<LPRECT>(&window_bounds))) Why do you need a reinterpret_cast here? https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:29: { nit: { on previous line
kulshin@chromium.org changed reviewers: + kulshin@chromium.org
https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:7: #include "chrome/browser/ui/views/frame/minimize_button_metrics_win.h" Keep this as the first include. In general, Chromium follows the Google C++ style guide for include order. https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:28: if (GetWindowRect(hwnd, reinterpret_cast<LPRECT>(&window_bounds))) Do you actually need the reinterpret_cast here? Also opening brace should be on same line. https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:39: // similarly the state never seems to change (titlebar_info.rgstate). Indent comment to be aligned with the code, or move outside the if block. Hint: "git cl format" is your friend (most of the time). https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:42: SendMessage(hwnd, WM_GETTITLEBARINFOEX, 0, Do we still need this fallback? In what cases does this give us a useful unswer that DwmGetWindowAttribute does not?
https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:7: #include "chrome/browser/ui/views/frame/minimize_button_metrics_win.h" On 2016/05/03 23:40:52, Ilya Kulshin wrote: > Keep this as the first include. In general, Chromium follows the Google C++ > style guide for include order. Ok, thanks. https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:28: if (GetWindowRect(hwnd, reinterpret_cast<LPRECT>(&window_bounds))) On 2016/05/03 23:40:52, Ilya Kulshin wrote: > Do you actually need the reinterpret_cast here? My C++ skills are clearly lacking here... You're probably correct. >Also opening brace should be on > same line. Of course. https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:42: SendMessage(hwnd, WM_GETTITLEBARINFOEX, 0, On 2016/05/03 23:40:52, Ilya Kulshin wrote: > Do we still need this fallback? In what cases does this give us a useful unswer > that DwmGetWindowAttribute does not? When DWM composing is disable globally or even on a per-window basis, DwmGetWindowAttribute returns an empty rect (all 0s)
https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:19: POINT minimize_button_corner = { 0 }; On 2016/05/03 23:40:47, sky wrote: > POINT->gfx::Point This is a POINT because it is used down in MapWindowPoints.
lgtm
Run git cl format and LGTM https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:19: POINT minimize_button_corner = { 0 }; On 2016/05/04 14:25:10, kylix_rd wrote: > On 2016/05/03 23:40:47, sky wrote: > > POINT->gfx::Point > > This is a POINT because it is used down in MapWindowPoints. Got it. https://codereview.chromium.org/1952473002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:31: { button_bounds.left + window_bounds.left, 0 }; nit: run git cl format (this should at least be 4 space indented). https://codereview.chromium.org/1952473002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:53: titlebar_info.rgrect[2].left == titlebar_info.rgrect[2].right || formatting here is equally wrong (again, git cl format will fix it). https://codereview.chromium.org/1952473002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:55: STATE_SYSTEM_OFFSCREEN | here too.
https://codereview.chromium.org/1952473002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:31: { button_bounds.left + window_bounds.left, 0 }; On 2016/05/04 17:42:03, sky wrote: > nit: run git cl format (this should at least be 4 space indented). Done. https://codereview.chromium.org/1952473002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:53: titlebar_info.rgrect[2].left == titlebar_info.rgrect[2].right || On 2016/05/04 17:42:03, sky wrote: > formatting here is equally wrong (again, git cl format will fix it). Done. https://codereview.chromium.org/1952473002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:55: STATE_SYSTEM_OFFSCREEN | On 2016/05/04 17:42:03, sky wrote: > here too. Done.
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kulshin@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1952473002/#ps60001 (title: "Results of git cl format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952473002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952473002/60001
Message was sent while issue was closed.
Description was changed from ========== Fix for wandering identity switcher button when magnified BUG=504133 ========== to ========== Fix for wandering identity switcher button when magnified BUG=504133 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix for wandering identity switcher button when magnified BUG=504133 ========== to ========== Fix for wandering identity switcher button when magnified BUG=504133 Committed: https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7 Cr-Commit-Position: refs/heads/master@{#391595} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7 Cr-Commit-Position: refs/heads/master@{#391595}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1953903002/ by kylixrd@chromium.org. The reason for reverting is: Causes a regression on systems which are using a scaling factor of >100%. This includes Windows 7,8.x & 10. Will need to figure out why and how to fix a problem with the device scale factor calculation using display::win::ScreenWin.ClientToDIPPoint();.
Message was sent while issue was closed.
Might it be related to us flooring to 100% in some cases? On Thu, May 5, 2016 at 2:05 PM, <kylixrd@chromium.org> wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/1953903002/ by kylixrd@chromium.org. > > The reason for reverting is: Causes a regression on systems which are using > a > scaling factor of >100%. This includes Windows 7,8.x & 10. > > Will need to figure out why and how to fix a problem with the device scale > factor calculation using display::win::ScreenWin.ClientToDIPPoint();. > > https://codereview.chromium.org/1952473002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/05/05 21:21:14, sky wrote: > Might it be related to us flooring to 100% in some cases? > > On Thu, May 5, 2016 at 2:05 PM, <mailto:kylixrd@chromium.org> wrote: > > A revert of this CL (patchset #4 id:60001) has been created in > > https://codereview.chromium.org/1953903002/ by mailto:kylixrd@chromium.org. > > > > The reason for reverting is: Causes a regression on systems which are using > > a > > scaling factor of >100%. This includes Windows 7,8.x & 10. > > > > Will need to figure out why and how to fix a problem with the device scale > > factor calculation using display::win::ScreenWin.ClientToDIPPoint();. > > > > https://codereview.chromium.org/1952473002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > It varies based on the width of the window, so I'd guess it's a dip<->pixel confusion.
Message was sent while issue was closed.
Description was changed from ========== Fix for wandering identity switcher button when magnified BUG=504133 Committed: https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7 Cr-Commit-Position: refs/heads/master@{#391595} ========== to ========== Fix for wandering identity switcher button when magnified BUG=504133 Committed: https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7 Cr-Commit-Position: refs/heads/master@{#391595} ==========
Hopefully this will fix the regression *and* ensure the identity switcher button is placed in the exact same pixel location. There are comments in the source explaining the need for some of the additional code in order to ensure pixel-perfect placement. https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:145: if (was_activated_ || !ui::win::IsAeroGlassEnabled() || This was the cause of the regression. The code was over-zealously caching the button offset. Once the window has been activated once, it should continue to calculate the actual location of the Identity switcher button and not return the cached value.
https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:27: int GetDefaultButtonBoundsOffset() { Add a comment describing how to determine the offset if a new OS version comes out. It would also be good to have something to remind us to update this code when we get a new version. Maybe a DCHECK? Or is that too aggressive? https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:28: return (GetVersion() >= base::win::VERSION_WIN10) I think this code would be more readable as a series of if-else statements. https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:44: bool was_activated) { Consider making these into member functions so you don'thave to pass around was_activated and hwnd
https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:27: int GetDefaultButtonBoundsOffset() { On 2016/05/09 19:53:23, Ilya Kulshin wrote: > Add a comment describing how to determine the offset if a new OS version comes > out. It would also be good to have something to remind us to update this code > when we get a new version. Maybe a DCHECK? Or is that too aggressive? I think that adding a DCHECK may be too aggressive. All things being equal, these defaults should never actually be used since the true offset is determined programatically. These values are only used in the short time prior to the window receiving WM_ACTIVATE and WM_GETTITLEBARINFOEX can be used. https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:28: return (GetVersion() >= base::win::VERSION_WIN10) On 2016/05/09 19:53:23, Ilya Kulshin wrote: > I think this code would be more readable as a series of if-else statements. Done. https://codereview.chromium.org/1952473002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:44: bool was_activated) { On 2016/05/09 19:53:23, Ilya Kulshin wrote: > Consider making these into member functions so you don'thave to pass around > was_activated and hwnd Done.
lgtm, although I'm not a committer, so you should probably also get an lgtm from a committer.
Bueller...Bueller? :)
LGTM https://codereview.chromium.org/1952473002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:24: const int kInvalidOffset = (int)0x80000000; static_cast<int> https://codereview.chromium.org/1952473002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:32: else if (GetVersion() >= base::win::VERSION_WIN8) nit: no else after a return (see style guide).
Code style fixes. https://codereview.chromium.org/1952473002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/1952473002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:24: const int kInvalidOffset = (int)0x80000000; On 2016/05/12 16:04:10, sky wrote: > static_cast<int> Done. https://codereview.chromium.org/1952473002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:32: else if (GetVersion() >= base::win::VERSION_WIN8) On 2016/05/12 16:04:10, sky wrote: > nit: no else after a return (see style guide). Done.
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kulshin@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1952473002/#ps140001 (title: "Code style fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952473002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952473002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kulshin@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1952473002/#ps160001 (title: "Must have relied on an include chain which changed for gfx::Point. Now explicitly included.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952473002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952473002/160001
Message was sent while issue was closed.
Description was changed from ========== Fix for wandering identity switcher button when magnified BUG=504133 Committed: https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7 Cr-Commit-Position: refs/heads/master@{#391595} ========== to ========== Fix for wandering identity switcher button when magnified BUG=504133 Committed: https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7 Cr-Commit-Position: refs/heads/master@{#391595} ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Fix for wandering identity switcher button when magnified BUG=504133 Committed: https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7 Cr-Commit-Position: refs/heads/master@{#391595} ========== to ========== Fix for wandering identity switcher button when magnified BUG=504133 Committed: https://crrev.com/88d78eca5976e5c58f036aa0b87257653af6c1b7 Cr-Commit-Position: refs/heads/master@{#391595} Committed: https://crrev.com/13715b01cf03787843ff7f78bc002c604942ea36 Cr-Commit-Position: refs/heads/master@{#393977} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/13715b01cf03787843ff7f78bc002c604942ea36 Cr-Commit-Position: refs/heads/master@{#393977} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
