Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1869163003:
Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. (Closed)
DescriptionRefactored GlassBrowserFrameView and BrowserDesktopTreeHostWin.
Renamed many methods and constants. Updated their associated comments
to be consistent with how they're actually used and to hopefully be less
confusing to future readers. Also did some minor code cleanup.
No visible differences are intended as a result of this change.
Committed: https://crrev.com/05e4802e0ac10a178c9718ad21091d6e8a6c4a74
Cr-Commit-Position: refs/heads/master@{#388542}
Patch Set 1 #
Total comments: 17
Patch Set 2 : addressed ananta's comments #
Total comments: 54
Patch Set 3 : addressed peter's comments #Patch Set 4 : edited a comment #
Total comments: 8
Patch Set 5 : minor edits #Patch Set 6 : opaque edits #
Total comments: 10
Patch Set 7 : revision #
Total comments: 4
Patch Set 8 : final edits #Patch Set 9 : fix merge error #Messages
Total messages: 27 (10 generated)
Description was changed from ========== Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. Renamed many methods and constants and updated their associated comments to be consistent with how they're actually used and to hopefully be less confusing to future readers. Also did some minor code cleanup. No visible differences are intended as a result of this change. ========== to ========== Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. Renamed many methods and constants and updated their associated comments to be consistent with how they're actually used and to hopefully be less confusing to future readers. Also did some minor code cleanup. No visible differences are intended as a result of this change. ==========
bsep@chromium.org changed reviewers: + ananta@chromium.org, kylixrd@chromium.org, pkasting@chromium.org
Description was changed from ========== Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. Renamed many methods and constants and updated their associated comments to be consistent with how they're actually used and to hopefully be less confusing to future readers. Also did some minor code cleanup. No visible differences are intended as a result of this change. ========== to ========== Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. Renamed many methods and constants. Updated their associated comments to be consistent with how they're actually used and to hopefully be less confusing to future readers. Also did some minor code cleanup. No visible differences are intended as a result of this change. ==========
Looks good overall https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:30: const int kDWMFrameBorderExtension = 3; Please add the DIPs suffix to the constant. It would make it easier for folks who don't read comments :) https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:281: gfx::Point pixel_margin = gfx::win::DIPToScreenPoint(dip_margin); Please make sure that this works as expected for fractional scales like 1.25, 1.5 etc. The original code was just returning kClientEdgeThickness + 1 all the time which looks broken. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:312: int GlassBrowserFrameView::NonClientTopBorderThickness(bool restored) const { Can we replace this boolean flag with an enum?. Would help in the readability of the code using this function. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:320: int GlassBrowserFrameView::ClientBorderThickness(bool restored) const { Ditto for bool restored. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:358: // extended the frame ::NonClientTopBorderThickness() beyond the visible area. Remove the :: https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:534: const bool md = ui::MaterialDesignController::IsModeMaterial(); Better name than md please. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.h:61: // which is sometimes different than ::NonClientBorderThickness. Does not Please change to NonClientBorderThickness() https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: bool RTLSpecialLayout() const; Do we need this function?. It just calls into the base helper
https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:534: const bool md = ui::MaterialDesignController::IsModeMaterial(); On 2016/04/11 21:12:11, ananta wrote: > Better name than md please. FWIW we use this name pretty consistently across a lot of the views UI code, and I think it's reasonably clear. I'd like to avoid changing this.
https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:30: const int kDWMFrameBorderExtension = 3; On 2016/04/11 21:12:11, ananta wrote: > Please add the DIPs suffix to the constant. It would make it easier for folks > who don't read comments :) Done. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:281: gfx::Point pixel_margin = gfx::win::DIPToScreenPoint(dip_margin); On 2016/04/11 21:12:11, ananta wrote: > Please make sure that this works as expected for fractional scales like > 1.25, 1.5 etc. The original code was just returning kClientEdgeThickness + 1 all > the time which looks broken. I tested it and it looks fine at 1.25 and 1.5. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:312: int GlassBrowserFrameView::NonClientTopBorderThickness(bool restored) const { On 2016/04/11 21:12:11, ananta wrote: > Can we replace this boolean flag with an enum?. Would help in the readability of > the code using this function. I agree that this is hard to read but I don't think an enum would make it better, since the value is an override of the expected behavior. Plus this is a pattern used in a bunch of places so I think fixing this would have to be a separate patch anyway. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:320: int GlassBrowserFrameView::ClientBorderThickness(bool restored) const { On 2016/04/11 21:12:11, ananta wrote: > Ditto for bool restored. See above. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:358: // extended the frame ::NonClientTopBorderThickness() beyond the visible area. On 2016/04/11 21:12:11, ananta wrote: > Remove the :: Done everywhere. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:534: const bool md = ui::MaterialDesignController::IsModeMaterial(); On 2016/04/11 21:22:11, Peter Kasting wrote: > On 2016/04/11 21:12:11, ananta wrote: > > Better name than md please. > > FWIW we use this name pretty consistently across a lot of the views UI code, and > I think it's reasonably clear. I'd like to avoid changing this. I agree with Peter. It's a local bool so I think it's fine. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.h:61: // which is sometimes different than ::NonClientBorderThickness. Does not On 2016/04/11 21:12:11, ananta wrote: > Please change to NonClientBorderThickness() Done. https://codereview.chromium.org/1869163003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: bool RTLSpecialLayout() const; On 2016/04/11 21:12:11, ananta wrote: > Do we need this function?. It just calls into the base helper I'm adding this to make it clear why we're branching on RTL layout, and because I'm going to make the logic more complicated when I custom draw the titlebar since I can make the caption buttons draw on the left like they're supposed to.
https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:29: // draw, in DIPs. Only used in Windows versions pre-10. Nit: "Only used pre-Win 10." https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:106: // into our client area in ::UpdateDWMFrame(). Nit: No :: (this is not a global method) https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:275: if (!GetWidget()->ShouldUseNativeFrame() || GetWidget()->IsFullscreen()) Could GetWidget()->IsFullscreen() and browser_view_->IsFullscreen() disagree? The old code checked both (for different pieces). https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:282: gfx::Point pixel_margin = gfx::win::DIPToScreenPoint(dip_margin); This is a behavior change, albeit one that looks correct to me. Can you demonstrate (e.g. in a screenshot) the effect of this change? https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:283: margins.cxLeftWidth = pixel_margin.x(); These lines no longer add 1 to the value like they used to. Why not? https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:289: // blackness doesn't show up on our rounded toolbar corners. Hmm. This comment feels a bit misleading. This is really about whether the toolbar has rounded corners, which I'm not convinced is the same as whether we're pre-Win10. Did I make the glass frame draw square toolbar corners in MD? If so, we would want to put this inside an "if (!md)" conditional. Maybe the comment should be something like: "Since the toolbar has rounded corners that show the glass frame behind, the extension of the glass area to the bottom of the tabstrip below isn't sufficient. We need to go down further to the bottom of the rounded corner region." Also, this implies that we may not want the same value for this constant as for the other three (as indeed the old code ended up using 3 for this and 4 for the others). https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:295: margins.cxLeftWidth = 0; These lines are unnecessary since |margins| has already been zero-initted. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:49: const int kClientBorderThicknessWin10 = 1; Nit: Maybe we should have a TODO to get rid of the Win10 constant? https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:50: // Extra empty space to draw in restored mode to make the titlebar thicker. This seems misleading. I'm pretty sure we only use this for tabbed windows, i.e. when there is no titlebar, and its purpose is indeed to provide a drag handle. The old comment seemed clearer and more accurate. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:52: // The size of the corners of the titlebar that trigger diagonal resizing. Again, the old comment seemed clearer and more accurate here, since this implies the corners are actually 16 px square, instead of being L-shaped. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:67: // buttons but doesn't change for 10 where the caption buttons are much bigger. This seems like a behavior change? The old code used the system metrics instead of hardcoding this, and in general that's what we want -- we want the profile switcher button, in principle, to look like another caption button. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:315: // screen. Nit: Maybe "Distinct from NonClientBorderThickness() because the system gives maximized windows an invisible CYSIZEFRAME-thick nonclient region above the top of the screen, which we need to keep UI elements below." (At least I assume it's the system doing this, and not our own code elsewhere. I didn't see anything relevant in browser_desktop_window_tree_host_win.cc.) https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:360: // border. This is the same positioning as the Windows caption buttons. Nit: Simpler: "Windows draws its caption buttons at the top of the screen for maximized windows, and 1 px from the top of the window otherwise." This suggests maybe this function should be named CaptionButtonY(). https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:58: int NonClientBorderThickness() const; Note: For all these functions you're renaming, consider whether we have the same function names in OpaqueBrowserFrameView[Layout] and {Ash,Mus}BrowserFrameView. We probably want to rename such things in sync across all files to keep it as clear as possible what the parallels are. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:69: int ClientBorderThickness(bool restored) const; Nit: I might place this above the NonClientXXX() functions, so it's not in the middle of them and so it's next to NonClientBorderThickness(), which it seems related to. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:74: int NonClientTopHeight(bool restored) const; Nit: I might place this just below NonClientTopBorderThickness(), since it seems related to that. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: bool RTLSpecialLayout() const; "Special layout" is kind of vague. If this is specifically about where the caption buttons are, maybe call it CaptionButtonsOnLeft() or UseRTLCaptionButtons() or something. Perhaps better, since you indicated you intended to make this more complicated later, might be to leave this out for now, having callers call the underlying i18n function, and then add it in a subsequent patch set where its greater functionality would make the correct name more obvious. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:85: int WindowTopY() const; Nit: I would place this next to the other XXXTopXXX() functions above. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:99: void LayoutAvatar(); Nit: If we're going to rename this, maybe we should rename it LayoutProfileSwitcher(), since that's more accurate at this point (we no longer show graphical avatars).
https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:29: // draw, in DIPs. Only used in Windows versions pre-10. On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: "Only used pre-Win 10." Done. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:106: // into our client area in ::UpdateDWMFrame(). On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: No :: (this is not a global method) Done. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:275: if (!GetWidget()->ShouldUseNativeFrame() || GetWidget()->IsFullscreen()) On 2016/04/12 01:49:31, Peter Kasting wrote: > Could GetWidget()->IsFullscreen() and browser_view_->IsFullscreen() disagree? > The old code checked both (for different pieces). Wow I HOPE they don't disagree... I double-checked this and they end up delegating to the same place (FullscreenHandler::fullscreen()). And I can't think of a reason anything that could be fullscreen would even want to delegate somewhere else, because that class is used for all top-level windows. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:282: gfx::Point pixel_margin = gfx::win::DIPToScreenPoint(dip_margin); On 2016/04/12 01:49:31, Peter Kasting wrote: > This is a behavior change, albeit one that looks correct to me. Can you > demonstrate (e.g. in a screenshot) the effect of this change? Here are some screenshots of this change: http://imgur.com/a/8Wd7L Best I can tell there's no visual difference. There is one small visual difference shown in this pair of screenshots: http://imgur.com/a/uQ9yt There's a bug where in hidpi mode if our resolution is odd the client area will "pull away" from the frame and leave 1 pixel of blackness due to rounding. But after this change we're now keeping the amount of frame relative to the scale, so the pixel of blackness is replaced by a pixel of frame color. I think it looks better. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:283: margins.cxLeftWidth = pixel_margin.x(); On 2016/04/12 01:49:31, Peter Kasting wrote: > These lines no longer add 1 to the value like they used to. Why not? I was confused by why were adding 1 to this value so I played around with it. As long as this border is sufficiently large to cover the areas we don't draw its exact value doesn't matter. It's hard to discover the reason the original committer was adding 1, but I assume based on my testing that 3 pixels ended up not being sufficient in the hidpi case so it was padded 1 extra pixel. It seems more correct to just say the value is in DIPs and scale it correctly. Hence this change. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:289: // blackness doesn't show up on our rounded toolbar corners. On 2016/04/12 01:49:31, Peter Kasting wrote: > Hmm. This comment feels a bit misleading. This is really about whether the > toolbar has rounded corners, which I'm not convinced is the same as whether > we're pre-Win10. Did I make the glass frame draw square toolbar corners in MD? > If so, we would want to put this inside an "if (!md)" conditional. > > Maybe the comment should be something like: > > "Since the toolbar has rounded corners that show the glass frame behind, the > extension of the glass area to the bottom of the tabstrip below isn't > sufficient. We need to go down further to the bottom of the rounded corner > region." > > Also, this implies that we may not want the same value for this constant as for > the other three (as indeed the old code ended up using 3 for this and 4 for the > others). Ok I like your comment better, replaced. I can gate this behind "md" if you think it's very important, but it wasn't gated before and it won't make a visible difference. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:295: margins.cxLeftWidth = 0; On 2016/04/12 01:49:31, Peter Kasting wrote: > These lines are unnecessary since |margins| has already been zero-initted. Done. I kept the comment though. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:49: const int kClientBorderThicknessWin10 = 1; On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: Maybe we should have a TODO to get rid of the Win10 constant? I tried this but it's actually a substantial visual difference between the Windows versions, not a weird compatibility hack. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:50: // Extra empty space to draw in restored mode to make the titlebar thicker. On 2016/04/12 01:49:31, Peter Kasting wrote: > This seems misleading. I'm pretty sure we only use this for tabbed windows, > i.e. when there is no titlebar, and its purpose is indeed to provide a drag > handle. The old comment seemed clearer and more accurate. Okay you're right. Reverted. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:52: // The size of the corners of the titlebar that trigger diagonal resizing. On 2016/04/12 01:49:31, Peter Kasting wrote: > Again, the old comment seemed clearer and more accurate here, since this implies > the corners are actually 16 px square, instead of being L-shaped. Hmm okay. I found the original comment confusing so I've edited it a bit, but it should be substantially the same. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:67: // buttons but doesn't change for 10 where the caption buttons are much bigger. On 2016/04/12 01:49:31, Peter Kasting wrote: > This seems like a behavior change? The old code used the system metrics instead > of hardcoding this, and in general that's what we want -- we want the profile > switcher button, in principle, to look like another caption button. This one is an odd duck. The old code was using SM_CYMENUSIZE which isn't the caption button height. Reading the API documentation I can't actually tell what it is exactly (it's at least some kind of button height), it just happens to return 19 for all Windows versions we support. The caption button height is SM_CYSIZE ostensibly, but that doesn't return 19 (iirc it returns 22), so it's wrong. It seems like the way you're supposed to get the caption button sizing is to handle WM_GETTITLEBARINFOEX and save the information... but on Windows 10 we don't actually want it to be the same height as the caption buttons because that would destroy our layout. So I decided the only sensible thing to do was not even pretend we're using a system metric anymore. When I traced the actual calculation it was resulting in 19 pixels maximized and 20 pixels restored, so this isn't a behavior change. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:315: // screen. On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: Maybe "Distinct from NonClientBorderThickness() because the system gives > maximized windows an invisible CYSIZEFRAME-thick nonclient region above the top > of the screen, which we need to keep UI elements below." > > (At least I assume it's the system doing this, and not our own code elsewhere. > I didn't see anything relevant in browser_desktop_window_tree_host_win.cc.) Confusingly this is actually our fault. Updated the comment to be more specific. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:360: // border. This is the same positioning as the Windows caption buttons. On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: Simpler: "Windows draws its caption buttons at the top of the screen for > maximized windows, and 1 px from the top of the window otherwise." > > This suggests maybe this function should be named CaptionButtonY(). Yeah you're right that's clearer. Done. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:58: int NonClientBorderThickness() const; On 2016/04/12 01:49:31, Peter Kasting wrote: > Note: For all these functions you're renaming, consider whether we have the same > function names in OpaqueBrowserFrameView[Layout] and {Ash,Mus}BrowserFrameView. > We probably want to rename such things in sync across all files to keep it as > clear as possible what the parallels are. I took a look at these. It seems like Ash/Mus are architected a bit differently so I didn't find any common functions. As for opaque, it has some commonly named functions but they seem to actually be what they're named, which is probably why glass has those names even though they ended up doing something else. I did end up renaming OpaqueBrowserFrameViewLayout::NonClientTopBorderHeight to NonClientTopBorderThickness to be consistent. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:69: int ClientBorderThickness(bool restored) const; On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: I might place this above the NonClientXXX() functions, so it's not in the > middle of them and so it's next to NonClientBorderThickness(), which it seems > related to. ClientBorderThickness is only sort of related to NonClientBorderThickness... you're right that it SOUNDS very related which means it probably needs a different name. What do you think about "ContentBorderThickness"? Or maybe it needs to be something very specific like "ClientNonClientBorderThickness"? Opaque uses "ClientEdgeThickness," which I also kind of like, but it could get confusing with kClientEdgeThickness from NonClientFrameView since it isn't the same value. At any rate I moved it up for now like you suggested. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:74: int NonClientTopHeight(bool restored) const; On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: I might place this just below NonClientTopBorderThickness(), since it seems > related to that. Done. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: bool RTLSpecialLayout() const; On 2016/04/12 01:49:31, Peter Kasting wrote: > "Special layout" is kind of vague. If this is specifically about where the > caption buttons are, maybe call it CaptionButtonsOnLeft() or > UseRTLCaptionButtons() or something. > > Perhaps better, since you indicated you intended to make this more complicated > later, might be to leave this out for now, having callers call the underlying > i18n function, and then add it in a subsequent patch set where its greater > functionality would make the correct name more obvious. Hmm I like having a comment on it even if it's a simple delegation because it's not obvious what the purpose of all those branches are otherwise. Renamed to "RTLModeAndCaptionButtonsOnRight". https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:85: int WindowTopY() const; On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: I would place this next to the other XXXTopXXX() functions above. Done. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:99: void LayoutAvatar(); On 2016/04/12 01:49:31, Peter Kasting wrote: > Nit: If we're going to rename this, maybe we should rename it > LayoutProfileSwitcher(), since that's more accurate at this point (we no longer > show graphical avatars). Done for references in this class. There are a bunch more "avatar" references from other classes... it's inconsistent already so I'm a bit unsure how much further I should go down the renaming rabbit hole.
https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:289: // blackness doesn't show up on our rounded toolbar corners. On 2016/04/12 23:19:38, Bret Sepulveda wrote: > On 2016/04/12 01:49:31, Peter Kasting wrote: > > Hmm. This comment feels a bit misleading. This is really about whether the > > toolbar has rounded corners, which I'm not convinced is the same as whether > > we're pre-Win10. Did I make the glass frame draw square toolbar corners in > MD? > > If so, we would want to put this inside an "if (!md)" conditional. > > > > Maybe the comment should be something like: > > > > "Since the toolbar has rounded corners that show the glass frame behind, the > > extension of the glass area to the bottom of the tabstrip below isn't > > sufficient. We need to go down further to the bottom of the rounded corner > > region." > > > > Also, this implies that we may not want the same value for this constant as > for > > the other three (as indeed the old code ended up using 3 for this and 4 for > the > > others). > > Ok I like your comment better, replaced. I can gate this behind "md" if you > think it's very important, but it wasn't gated before and it won't make a > visible difference. If we find blocks that are only relevant in a non-MD world, we should definitely add "if (!md)" gates to them; that way after we ship MD, when we go back and rip out all non-MD code, we'll know to rip this stuff out too. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:49: const int kClientBorderThicknessWin10 = 1; On 2016/04/12 23:19:38, Bret Sepulveda wrote: > On 2016/04/12 01:49:31, Peter Kasting wrote: > > Nit: Maybe we should have a TODO to get rid of the Win10 constant? > > I tried this but it's actually a substantial visual difference between the > Windows versions, not a weird compatibility hack. Sorry, I think I wasn't clear. I don't mean use the non-Win10 value on Win10; I mean use "0" on Win10. That will be a somewhat involved change, I believe. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:67: // buttons but doesn't change for 10 where the caption buttons are much bigger. On 2016/04/12 23:19:38, Bret Sepulveda wrote: > On 2016/04/12 01:49:31, Peter Kasting wrote: > > This seems like a behavior change? The old code used the system metrics > instead > > of hardcoding this, and in general that's what we want -- we want the profile > > switcher button, in principle, to look like another caption button. > > This one is an odd duck. The old code was using SM_CYMENUSIZE which isn't the > caption button height. Reading the API documentation I can't actually tell what > it is exactly (it's at least some kind of button height), it just happens to > return 19 for all Windows versions we support. The caption button height is > SM_CYSIZE ostensibly, but that doesn't return 19 (iirc it returns 22), so it's > wrong. It seems like the way you're supposed to get the caption button sizing is > to handle WM_GETTITLEBARINFOEX and save the information... but on Windows 10 we > don't actually want it to be the same height as the caption buttons because that > would destroy our layout. So I decided the only sensible thing to do was not > even pretend we're using a system metric anymore. My point was that I think we _do_ actually want the Win10 profile switcher button to look like another caption button. I think right now its appearance is more like a Win8 caption button? (Don't have Win 8, can't check.) Yes, this will affect layout; no, we shouldn't try to fix that in this patch. But I don't think forcing to 19/20 px is what we actually want; I think the current Win10 behavior is just wrong. Should check with the UI designers though. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:69: int ClientBorderThickness(bool restored) const; On 2016/04/12 23:19:38, Bret Sepulveda wrote: > On 2016/04/12 01:49:31, Peter Kasting wrote: > > Nit: I might place this above the NonClientXXX() functions, so it's not in the > > middle of them and so it's next to NonClientBorderThickness(), which it seems > > related to. > > ClientBorderThickness is only sort of related to NonClientBorderThickness... > you're right that it SOUNDS very related which means it probably needs a > different name. What do you think about "ContentBorderThickness"? Or maybe it > needs to be something very specific like "ClientNonClientBorderThickness"? > Opaque uses "ClientEdgeThickness," which I also kind of like, but it could get > confusing with kClientEdgeThickness from NonClientFrameView since it isn't the > same value. > > At any rate I moved it up for now like you suggested. Maybe it's the NonClient functions which have poor names. Maybe NonClientBorderThickness should be NonClientThickness/NonClientAreaThickness/NonClientEdgeThickness, or perhaps WindowBorderThickness. Similarly for NonClientTopBorderThickness. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: bool RTLSpecialLayout() const; On 2016/04/12 23:19:38, Bret Sepulveda wrote: > On 2016/04/12 01:49:31, Peter Kasting wrote: > > "Special layout" is kind of vague. If this is specifically about where the > > caption buttons are, maybe call it CaptionButtonsOnLeft() or > > UseRTLCaptionButtons() or something. > > > > Perhaps better, since you indicated you intended to make this more complicated > > later, might be to leave this out for now, having callers call the underlying > > i18n function, and then add it in a subsequent patch set where its greater > > functionality would make the correct name more obvious. > > Hmm I like having a comment on it even if it's a simple delegation because it's > not obvious what the purpose of all those branches are otherwise. Renamed to > "RTLModeAndCaptionButtonsOnRight". That implies we have two RTL mode states: caption buttons on right, and caption buttons on left. Is that true? I thought today they were always on the right, which was a bug, and we wanted them to be always on the left. This means it's a bit of a mess to figure out what we want here because we really want to change the behavior and make the buttons on the left, at which point the functions we need, and where they're called from, will probably differ. For now, perhaps this should be CaptionButtonsOnLeadingEdge() or something. https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:333: // of the screen. Huh. The wording here suggests that this is not actually returning the thickness of the nonclient border region, but rather the thickness of the client region that we want to pretend is nonclient region. If that were true of all these functions, we'd probably want to either name them differently or at least comment differently in them. But that would mean that in a restored window, the window resize border is client area. I thought that was nonclient area. Am I confused? https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h (right): https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:78: // true, acts as if the window is restored regardless of the real mode. Nit: We should probably update the comments on all matching functions here so that they're the same as the comments in the glass frame header file. https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:79: int NonClientTopBorderThickness(bool restored) const; I'm of two minds regarding this name switch. On one hand, using "Thickness" makes it clear this parallels NonClientBorderThickness(). OTOH, we normally use "width" or "height" in preference to "thickness" when the value refers only to the horizontal or vertical dimension, because it seems clearer. So I'm not sure whether this function's name should change, or the one in the glass frame should.
https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:289: // blackness doesn't show up on our rounded toolbar corners. On 2016/04/13 01:00:22, Peter Kasting wrote: > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > Hmm. This comment feels a bit misleading. This is really about whether the > > > toolbar has rounded corners, which I'm not convinced is the same as whether > > > we're pre-Win10. Did I make the glass frame draw square toolbar corners in > > MD? > > > If so, we would want to put this inside an "if (!md)" conditional. > > > > > > Maybe the comment should be something like: > > > > > > "Since the toolbar has rounded corners that show the glass frame behind, the > > > extension of the glass area to the bottom of the tabstrip below isn't > > > sufficient. We need to go down further to the bottom of the rounded corner > > > region." > > > > > > Also, this implies that we may not want the same value for this constant as > > for > > > the other three (as indeed the old code ended up using 3 for this and 4 for > > the > > > others). > > > > Ok I like your comment better, replaced. I can gate this behind "md" if you > > think it's very important, but it wasn't gated before and it won't make a > > visible difference. > > If we find blocks that are only relevant in a non-MD world, we should definitely > add "if (!md)" gates to them; that way after we ship MD, when we go back and rip > out all non-MD code, we'll know to rip this stuff out too. Good point. Done. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:49: const int kClientBorderThicknessWin10 = 1; On 2016/04/13 01:00:22, Peter Kasting wrote: > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > Nit: Maybe we should have a TODO to get rid of the Win10 constant? > > > > I tried this but it's actually a substantial visual difference between the > > Windows versions, not a weird compatibility hack. > > Sorry, I think I wasn't clear. I don't mean use the non-Win10 value on Win10; I > mean use "0" on Win10. > > That will be a somewhat involved change, I believe. I'm a bit confused because that would be a visual change that I think looks fine how it is. Are you saying we want the web content to be flush with the Windows border? https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:67: // buttons but doesn't change for 10 where the caption buttons are much bigger. On 2016/04/13 01:00:22, Peter Kasting wrote: > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > This seems like a behavior change? The old code used the system metrics > > instead > > > of hardcoding this, and in general that's what we want -- we want the > profile > > > switcher button, in principle, to look like another caption button. > > > > This one is an odd duck. The old code was using SM_CYMENUSIZE which isn't the > > caption button height. Reading the API documentation I can't actually tell > what > > it is exactly (it's at least some kind of button height), it just happens to > > return 19 for all Windows versions we support. The caption button height is > > SM_CYSIZE ostensibly, but that doesn't return 19 (iirc it returns 22), so it's > > wrong. It seems like the way you're supposed to get the caption button sizing > is > > to handle WM_GETTITLEBARINFOEX and save the information... but on Windows 10 > we > > don't actually want it to be the same height as the caption buttons because > that > > would destroy our layout. So I decided the only sensible thing to do was not > > even pretend we're using a system metric anymore. > > My point was that I think we _do_ actually want the Win10 profile switcher > button to look like another caption button. I think right now its appearance is > more like a Win8 caption button? (Don't have Win 8, can't check.) Yes, this > will affect layout; no, we shouldn't try to fix that in this patch. But I don't > think forcing to 19/20 px is what we actually want; I think the current Win10 > behavior is just wrong. > > Should check with the UI designers though. In Win7 it does a decent job of pretending to be a caption button, but in Win8 I think it looks fairly different, since it has a unique background color and isn't the same width as any of the real caption buttons. Right now we are forcing 19/20 pixels, it just doesn't look like it from the code. I'd rather just be clear about the heights we're using until we decide to do something cleverer. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:69: int ClientBorderThickness(bool restored) const; On 2016/04/13 01:00:22, Peter Kasting wrote: > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > Nit: I might place this above the NonClientXXX() functions, so it's not in > the > > > middle of them and so it's next to NonClientBorderThickness(), which it > seems > > > related to. > > > > ClientBorderThickness is only sort of related to NonClientBorderThickness... > > you're right that it SOUNDS very related which means it probably needs a > > different name. What do you think about "ContentBorderThickness"? Or maybe it > > needs to be something very specific like "ClientNonClientBorderThickness"? > > Opaque uses "ClientEdgeThickness," which I also kind of like, but it could get > > confusing with kClientEdgeThickness from NonClientFrameView since it isn't the > > same value. > > > > At any rate I moved it up for now like you suggested. > > Maybe it's the NonClient functions which have poor names. Maybe > NonClientBorderThickness should be > NonClientThickness/NonClientAreaThickness/NonClientEdgeThickness, or perhaps > WindowBorderThickness. Similarly for NonClientTopBorderThickness. Before I spend a bunch of time thinking about these names see my comment in glass_[etc].cc and let's figure out if we actually want to use Client/NonClient terminology at all. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: bool RTLSpecialLayout() const; On 2016/04/13 01:00:22, Peter Kasting wrote: > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > "Special layout" is kind of vague. If this is specifically about where the > > > caption buttons are, maybe call it CaptionButtonsOnLeft() or > > > UseRTLCaptionButtons() or something. > > > > > > Perhaps better, since you indicated you intended to make this more > complicated > > > later, might be to leave this out for now, having callers call the > underlying > > > i18n function, and then add it in a subsequent patch set where its greater > > > functionality would make the correct name more obvious. > > > > Hmm I like having a comment on it even if it's a simple delegation because > it's > > not obvious what the purpose of all those branches are otherwise. Renamed to > > "RTLModeAndCaptionButtonsOnRight". > > That implies we have two RTL mode states: caption buttons on right, and caption > buttons on left. Is that true? I thought today they were always on the right, > which was a bug, and we wanted them to be always on the left. > > This means it's a bit of a mess to figure out what we want here because we > really want to change the behavior and make the buttons on the left, at which > point the functions we need, and where they're called from, will probably > differ. > > For now, perhaps this should be CaptionButtonsOnLeadingEdge() or something. You're correct that at this moment they're on always on the right on Windows, but when I custom draw the titlebar I'll draw them on the left in RTL, so there will be two modes. If someone does figure out how to make them always on the left then all of this can just go away, but I don't foresee that happening soon. Anyway that name seems fine. https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:333: // of the screen. On 2016/04/13 01:00:22, Peter Kasting wrote: > Huh. The wording here suggests that this is not actually returning the > thickness of the nonclient border region, but rather the thickness of the client > region that we want to pretend is nonclient region. > > If that were true of all these functions, we'd probably want to either name them > differently or at least comment differently in them. But that would mean that > in a restored window, the window resize border is client area. I thought that > was nonclient area. Am I confused? Oh hmm you're right, for this comment I'm mixing up Windows non-client area with our visual non-client area (which are indeed different, our window border is 5/8 non-client and 3/8 client). It's pretty difficult to explain why the top of the screen isn't y=0 without talking about Windows non-client area though. My intention was that anything "non-client" in this class would be VISUAL non-client (i.e. the window resize border), whereas in BrowserDesktopTreeHostWin it refers to the WINDOWS non-client. Now that I'm saying it outloud though it doesn't sound like a good idea. Maybe I should just avoid all reference to "client/non-client" inside this class? It would be a departure from opaque frame though; it likes to talk about visual client/non-client because it doesn't have Windows non-client area. For now I've edited the comment to try to make sure the reader knows which kind of "non-client" I'm talking about. https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h (right): https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:78: // true, acts as if the window is restored regardless of the real mode. On 2016/04/13 01:00:22, Peter Kasting wrote: > Nit: We should probably update the comments on all matching functions here so > that they're the same as the comments in the glass frame header file. I need to be careful because some of these comments are accurate where they were not in the glass frame, but I'll update them where it makes sense to. https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:79: int NonClientTopBorderThickness(bool restored) const; On 2016/04/13 01:00:22, Peter Kasting wrote: > I'm of two minds regarding this name switch. On one hand, using "Thickness" > makes it clear this parallels NonClientBorderThickness(). OTOH, we normally use > "width" or "height" in preference to "thickness" when the value refers only to > the horizontal or vertical dimension, because it seems clearer. So I'm not sure > whether this function's name should change, or the one in the glass frame > should. I made a mistake with this one, its equivalent is actually NonClientTopHeight in glass. Since opaque doesn't do any trickery with Windows non-client it doesn't have a separate NonClientTopBorderThickness.
https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:49: const int kClientBorderThicknessWin10 = 1; On 2016/04/13 22:19:47, Bret Sepulveda wrote: > On 2016/04/13 01:00:22, Peter Kasting wrote: > > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > > Nit: Maybe we should have a TODO to get rid of the Win10 constant? > > > > > > I tried this but it's actually a substantial visual difference between the > > > Windows versions, not a weird compatibility hack. > > > > Sorry, I think I wasn't clear. I don't mean use the non-Win10 value on Win10; > I > > mean use "0" on Win10. > > > > That will be a somewhat involved change, I believe. > > I'm a bit confused because that would be a visual change that I think looks fine > how it is. Are you saying we want the web content to be flush with the Windows > border? Eventually, yes. We don't want the current 1 DP border of our own around the content, just the 1 px Windows border. This is also what Edge does AFAICT. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:67: // buttons but doesn't change for 10 where the caption buttons are much bigger. On 2016/04/13 22:19:47, Bret Sepulveda wrote: > On 2016/04/13 01:00:22, Peter Kasting wrote: > > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > > This seems like a behavior change? The old code used the system metrics > > > instead > > > > of hardcoding this, and in general that's what we want -- we want the > > profile > > > > switcher button, in principle, to look like another caption button. > > > > > > This one is an odd duck. The old code was using SM_CYMENUSIZE which isn't > the > > > caption button height. Reading the API documentation I can't actually tell > > what > > > it is exactly (it's at least some kind of button height), it just happens to > > > return 19 for all Windows versions we support. The caption button height is > > > SM_CYSIZE ostensibly, but that doesn't return 19 (iirc it returns 22), so > it's > > > wrong. It seems like the way you're supposed to get the caption button > sizing > > is > > > to handle WM_GETTITLEBARINFOEX and save the information... but on Windows 10 > > we > > > don't actually want it to be the same height as the caption buttons because > > that > > > would destroy our layout. So I decided the only sensible thing to do was not > > > even pretend we're using a system metric anymore. > > > > My point was that I think we _do_ actually want the Win10 profile switcher > > button to look like another caption button. I think right now its appearance > is > > more like a Win8 caption button? (Don't have Win 8, can't check.) Yes, this > > will affect layout; no, we shouldn't try to fix that in this patch. But I > don't > > think forcing to 19/20 px is what we actually want; I think the current Win10 > > behavior is just wrong. > > > > Should check with the UI designers though. > > In Win7 it does a decent job of pretending to be a caption button, but in Win8 I > think it looks fairly different, since it has a unique background color and > isn't the same width as any of the real caption buttons. Yeah, we never really tested anything on Win 8 desktop :( > Right now we are > forcing 19/20 pixels, it just doesn't look like it from the code. I'd rather > just be clear about the heights we're using until we decide to do something > cleverer. OK, but this is still a minor behavior change from the old code, I think. The old code always used the same height, and changed the Y coordinate to be above the window top in maximized mode. The new code uses a more consistent Y coordinate and changes the height instead. This has ramifications for the precise vertical positioning of the content inside the button. I think the old code, while it seems strange, is closer to what Windows actually does when positioning caption buttons, and I'd stick with that. https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: bool RTLSpecialLayout() const; On 2016/04/13 22:19:47, Bret Sepulveda wrote: > On 2016/04/13 01:00:22, Peter Kasting wrote: > > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > > "Special layout" is kind of vague. If this is specifically about where > the > > > > caption buttons are, maybe call it CaptionButtonsOnLeft() or > > > > UseRTLCaptionButtons() or something. > > > > > > > > Perhaps better, since you indicated you intended to make this more > > complicated > > > > later, might be to leave this out for now, having callers call the > > underlying > > > > i18n function, and then add it in a subsequent patch set where its greater > > > > functionality would make the correct name more obvious. > > > > > > Hmm I like having a comment on it even if it's a simple delegation because > > it's > > > not obvious what the purpose of all those branches are otherwise. Renamed to > > > "RTLModeAndCaptionButtonsOnRight". > > > > That implies we have two RTL mode states: caption buttons on right, and > caption > > buttons on left. Is that true? I thought today they were always on the > right, > > which was a bug, and we wanted them to be always on the left. > > > > This means it's a bit of a mess to figure out what we want here because we > > really want to change the behavior and make the buttons on the left, at which > > point the functions we need, and where they're called from, will probably > > differ. > > > > For now, perhaps this should be CaptionButtonsOnLeadingEdge() or something. > > You're correct that at this moment they're on always on the right on Windows, > but when I custom draw the titlebar I'll draw them on the left in RTL, so there > will be two modes. If someone does figure out how to make them always on the > left then all of this can just go away, but I don't foresee that happening soon. Note that once you're custom-drawing them on the left, you actually probably don't need any special logic to position them, as long as you use the views auto-mirroring machinery and coordinate calculation stuff correctly, since they'll logically always be on the trailing edge of the window. We only need two modes for now, since they're not always in the same logical position as long as Windows is drawing them but we don't have the magic bit set that flips the whole window (which would screw us over). https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:333: // of the screen. On 2016/04/13 22:19:47, Bret Sepulveda wrote: > On 2016/04/13 01:00:22, Peter Kasting wrote: > > Huh. The wording here suggests that this is not actually returning the > > thickness of the nonclient border region, but rather the thickness of the > client > > region that we want to pretend is nonclient region. > > > > If that were true of all these functions, we'd probably want to either name > them > > differently or at least comment differently in them. But that would mean that > > in a restored window, the window resize border is client area. I thought that > > was nonclient area. Am I confused? > > Oh hmm you're right, for this comment I'm mixing up Windows non-client area with > our visual non-client area (which are indeed different, our window border is 5/8 > non-client and 3/8 client). It's pretty difficult to explain why the top of the > screen isn't y=0 without talking about Windows non-client area though. > > My intention was that anything "non-client" in this class would be VISUAL > non-client (i.e. the window resize border), whereas in BrowserDesktopTreeHostWin > it refers to the WINDOWS non-client. Now that I'm saying it outloud though it > doesn't sound like a good idea. Maybe I should just avoid all reference to > "client/non-client" inside this class? It would be a departure from opaque frame > though; it likes to talk about visual client/non-client because it doesn't have > Windows non-client area. > > For now I've edited the comment to try to make sure the reader knows which kind > of "non-client" I'm talking about. Yeah, the opaque frame was written first, and the glass frame code copied its terminology, even though that meant a confusing meaning overload. There are a couple ways around this. One is to use something other than "nonclient" to talk about visual nonclient area, e.g. "window border" or something, as long as a unique such phrase exists. Another is just to put a big disclaimer in the header file above any functions using these terms talking about what they mean, and then if anyone ever needs to refer to Windows' concept of nonclient area in this class, that comment should explicitly say that. Maybe this problem gets less bad in the future when we custom draw just about everything? https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:274: // If the opaque frame is visible or we're fullscreen, we don't extend the Nit: the opaque frame is visible -> we're using the opaque frame (Because otherwise it sounds like the opaque frame is some subelement that gets toggled on and off.) https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:47: // thickness of the border between the web content and our frame border. Is the 1 DP "client edge thickness" at least _included_ in this value? Because it doesn't seem (from my memory of the layout here) that we draw a separate 1 DP for that and 1 or 3 DP for this adjacent to each other. If so, the comment should probably make that more clear... https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:54: // pixels at the end of the top edges trigger diagonal resizing. Why did you remove "and bottom"? We still say "edges", and we still talk about this applying to the bottom edge down where this constant is used. https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:367: // Windows always draws the buttons on the right, even in RTL mode. Nit: Maybe add more detail, e.g.: http://crbug.com/560619 - Because we don't set WS_EX_LAYOUTRTL (which would conflict with Chrome's own RTL layout logic), Windows always draws the caption buttons on the right, even when we want to be RTL. TODO(bsep): Fix this by drawing the caption buttons ourselves. https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/opaque_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view.h:131: // the titlebar/tabstrip area. Now I'm really confused -- I don't know what "the entire nonclient top border, but excluding the titlebar/tabstrip area" actually includes. And I don't think this is ever used for a top border height, nor would it be accurate for that (since, even excluding the titlebar/tabstrip, there's no client edge on the top border). So it seems like the old comment was more correct.
https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:49: const int kClientBorderThicknessWin10 = 1; On 2016/04/15 00:20:51, Peter Kasting wrote: > On 2016/04/13 22:19:47, Bret Sepulveda wrote: > > On 2016/04/13 01:00:22, Peter Kasting wrote: > > > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > > > Nit: Maybe we should have a TODO to get rid of the Win10 constant? > > > > > > > > I tried this but it's actually a substantial visual difference between the > > > > Windows versions, not a weird compatibility hack. > > > > > > Sorry, I think I wasn't clear. I don't mean use the non-Win10 value on > Win10; > > I > > > mean use "0" on Win10. > > > > > > That will be a somewhat involved change, I believe. > > > > I'm a bit confused because that would be a visual change that I think looks > fine > > how it is. Are you saying we want the web content to be flush with the Windows > > border? > > Eventually, yes. We don't want the current 1 DP border of our own around the > content, just the 1 px Windows border. This is also what Edge does AFAICT. Oh, okay. I think it would actually be pretty easy. I'll make a note to do it later, maybe for md only? https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:67: // buttons but doesn't change for 10 where the caption buttons are much bigger. On 2016/04/15 00:20:51, Peter Kasting wrote: > On 2016/04/13 22:19:47, Bret Sepulveda wrote: > > On 2016/04/13 01:00:22, Peter Kasting wrote: > > > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > > > This seems like a behavior change? The old code used the system metrics > > > > instead > > > > > of hardcoding this, and in general that's what we want -- we want the > > > profile > > > > > switcher button, in principle, to look like another caption button. > > > > > > > > This one is an odd duck. The old code was using SM_CYMENUSIZE which isn't > > the > > > > caption button height. Reading the API documentation I can't actually tell > > > what > > > > it is exactly (it's at least some kind of button height), it just happens > to > > > > return 19 for all Windows versions we support. The caption button height > is > > > > SM_CYSIZE ostensibly, but that doesn't return 19 (iirc it returns 22), so > > it's > > > > wrong. It seems like the way you're supposed to get the caption button > > sizing > > > is > > > > to handle WM_GETTITLEBARINFOEX and save the information... but on Windows > 10 > > > we > > > > don't actually want it to be the same height as the caption buttons > because > > > that > > > > would destroy our layout. So I decided the only sensible thing to do was > not > > > > even pretend we're using a system metric anymore. > > > > > > My point was that I think we _do_ actually want the Win10 profile switcher > > > button to look like another caption button. I think right now its > appearance > > is > > > more like a Win8 caption button? (Don't have Win 8, can't check.) Yes, > this > > > will affect layout; no, we shouldn't try to fix that in this patch. But I > > don't > > > think forcing to 19/20 px is what we actually want; I think the current > Win10 > > > behavior is just wrong. > > > > > > Should check with the UI designers though. > > > > In Win7 it does a decent job of pretending to be a caption button, but in Win8 > I > > think it looks fairly different, since it has a unique background color and > > isn't the same width as any of the real caption buttons. > > Yeah, we never really tested anything on Win 8 desktop :( > > > Right now we are > > forcing 19/20 pixels, it just doesn't look like it from the code. I'd rather > > just be clear about the heights we're using until we decide to do something > > cleverer. > > OK, but this is still a minor behavior change from the old code, I think. > > The old code always used the same height, and changed the Y coordinate to be > above the window top in maximized mode. The new code uses a more consistent Y > coordinate and changes the height instead. This has ramifications for the > precise vertical positioning of the content inside the button. I think the old > code, while it seems strange, is closer to what Windows actually does when > positioning caption buttons, and I'd stick with that. Okay you're right. I'll keep the same position logic, and add a TODO for making this look like a Win10 caption button. https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:333: // of the screen. On 2016/04/15 00:20:51, Peter Kasting wrote: > On 2016/04/13 22:19:47, Bret Sepulveda wrote: > > On 2016/04/13 01:00:22, Peter Kasting wrote: > > > Huh. The wording here suggests that this is not actually returning the > > > thickness of the nonclient border region, but rather the thickness of the > > client > > > region that we want to pretend is nonclient region. > > > > > > If that were true of all these functions, we'd probably want to either name > > them > > > differently or at least comment differently in them. But that would mean > that > > > in a restored window, the window resize border is client area. I thought > that > > > was nonclient area. Am I confused? > > > > Oh hmm you're right, for this comment I'm mixing up Windows non-client area > with > > our visual non-client area (which are indeed different, our window border is > 5/8 > > non-client and 3/8 client). It's pretty difficult to explain why the top of > the > > screen isn't y=0 without talking about Windows non-client area though. > > > > My intention was that anything "non-client" in this class would be VISUAL > > non-client (i.e. the window resize border), whereas in > BrowserDesktopTreeHostWin > > it refers to the WINDOWS non-client. Now that I'm saying it outloud though it > > doesn't sound like a good idea. Maybe I should just avoid all reference to > > "client/non-client" inside this class? It would be a departure from opaque > frame > > though; it likes to talk about visual client/non-client because it doesn't > have > > Windows non-client area. > > > > For now I've edited the comment to try to make sure the reader knows which > kind > > of "non-client" I'm talking about. > > Yeah, the opaque frame was written first, and the glass frame code copied its > terminology, even though that meant a confusing meaning overload. > > There are a couple ways around this. One is to use something other than > "nonclient" to talk about visual nonclient area, e.g. "window border" or > something, as long as a unique such phrase exists. Another is just to put a big > disclaimer in the header file above any functions using these terms talking > about what they mean, and then if anyone ever needs to refer to Windows' concept > of nonclient area in this class, that comment should explicitly say that. > > Maybe this problem gets less bad in the future when we custom draw just about > everything? Okay I went back to what opaque was doing and calling it the "frame border," which I didn't like originally but I'm seeing the advantage of now as long as I avoid any references to "non-client." I left ClientBorderThickness because I don't think anyone is getting confusing about what the client area is for the purposes of this class, even if it's slightly different from the Windows client area. In general we're never going to be able to custom draw everything. Windows likes to throw transparent areas and window shadows around that'd be a ton of work to reproduce. But with the custom titlebar in Win10 we'll get pretty close. https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_win.cc:274: // If the opaque frame is visible or we're fullscreen, we don't extend the On 2016/04/15 00:20:51, Peter Kasting wrote: > Nit: the opaque frame is visible -> we're using the opaque frame > > (Because otherwise it sounds like the opaque frame is some subelement that gets > toggled on and off.) Done. https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:47: // thickness of the border between the web content and our frame border. On 2016/04/15 00:20:52, Peter Kasting wrote: > Is the 1 DP "client edge thickness" at least _included_ in this value? Because > it doesn't seem (from my memory of the layout here) that we draw a separate 1 DP > for that and 1 or 3 DP for this adjacent to each other. > > If so, the comment should probably make that more clear... It does. Comment edited. https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:54: // pixels at the end of the top edges trigger diagonal resizing. On 2016/04/15 00:20:52, Peter Kasting wrote: > Why did you remove "and bottom"? We still say "edges", and we still talk about > this applying to the bottom edge down where this constant is used. Oops, I actually thought it wasn't used at the bottom edge but I double-checked and you're right. https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:367: // Windows always draws the buttons on the right, even in RTL mode. On 2016/04/15 00:20:51, Peter Kasting wrote: > Nit: Maybe add more detail, e.g.: > > http://crbug.com/560619 - Because we don't set WS_EX_LAYOUTRTL (which would > conflict with Chrome's own RTL layout logic), Windows always draws the caption > buttons on the right, even when we want to be RTL. TODO(bsep): Fix this by > drawing the caption buttons ourselves. Okay I like your comment but since this situation will probably partially persist as long as we support Windows 7 I don't want to tether myself to a TODO like that. https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/opaque_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view.h:131: // the titlebar/tabstrip area. On 2016/04/15 00:20:52, Peter Kasting wrote: > Now I'm really confused -- I don't know what "the entire nonclient top border, > but excluding the titlebar/tabstrip area" actually includes. > > And I don't think this is ever used for a top border height, nor would it be > accurate for that (since, even excluding the titlebar/tabstrip, there's no > client edge on the top border). > > So it seems like the old comment was more correct. Hmmm okay you're right, this was the same mistake as the corner width comment edit I made in the other file, I mixed up how the parameters of GetHTComponentForFrame are used. Anyway, reverted.
LGTM https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:49: const int kClientBorderThicknessWin10 = 1; On 2016/04/15 23:07:08, Bret Sepulveda wrote: > On 2016/04/15 00:20:51, Peter Kasting wrote: > > On 2016/04/13 22:19:47, Bret Sepulveda wrote: > > > On 2016/04/13 01:00:22, Peter Kasting wrote: > > > > On 2016/04/12 23:19:38, Bret Sepulveda wrote: > > > > > On 2016/04/12 01:49:31, Peter Kasting wrote: > > > > > > Nit: Maybe we should have a TODO to get rid of the Win10 constant? > > > > > > > > > > I tried this but it's actually a substantial visual difference between > the > > > > > Windows versions, not a weird compatibility hack. > > > > > > > > Sorry, I think I wasn't clear. I don't mean use the non-Win10 value on > > Win10; > > > I > > > > mean use "0" on Win10. > > > > > > > > That will be a somewhat involved change, I believe. > > > > > > I'm a bit confused because that would be a visual change that I think looks > > fine > > > how it is. Are you saying we want the web content to be flush with the > Windows > > > border? > > > > Eventually, yes. We don't want the current 1 DP border of our own around the > > content, just the 1 px Windows border. This is also what Edge does AFAICT. > > Oh, okay. I think it would actually be pretty easy. I'll make a note to do it > later, maybe for md only? Sure. https://codereview.chromium.org/1869163003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:541: button_y -= 1; Shouldn't this code/comment be in CaptionButtonY()? Otherwise, it's not really the caption button y. https://codereview.chromium.org/1869163003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.h:73: // the top of the tabs. If |restored| is true, this is calculated as if the Nit: Maybe "Returns the height of everything above the tabstrip's hit-test region, including both the window border (i.e. FrameTopBorderThickness()) and any additional draggable area that's considered part of the window frame rather than the tabstrip. ..."
https://codereview.chromium.org/1869163003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/1869163003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:541: button_y -= 1; On 2016/04/19 21:00:00, Peter Kasting wrote: > Shouldn't this code/comment be in CaptionButtonY()? Otherwise, it's not really > the caption button y. Oh yeah... this is why I originally called it "WindowTopY." I'll go back to that name because I need it without the specific caption button behavior for custom drawn titlebar. It's also better to have the weird behavior close to where it's needed, I think. https://codereview.chromium.org/1869163003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/1869163003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.h:73: // the top of the tabs. If |restored| is true, this is calculated as if the On 2016/04/19 21:00:00, Peter Kasting wrote: > Nit: Maybe "Returns the height of everything above the tabstrip's hit-test > region, including both the window border (i.e. FrameTopBorderThickness()) and > any additional draggable area that's considered part of the window frame rather > than the tabstrip. ..." Ok, I like your comment. Done.
The CQ bit was checked by bsep@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/1869163003/#ps140001 (title: "final edits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869163003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869163003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by bsep@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/1869163003/#ps160001 (title: "fix merge error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869163003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869163003/160001
Message was sent while issue was closed.
Description was changed from ========== Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. Renamed many methods and constants. Updated their associated comments to be consistent with how they're actually used and to hopefully be less confusing to future readers. Also did some minor code cleanup. No visible differences are intended as a result of this change. ========== to ========== Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. Renamed many methods and constants. Updated their associated comments to be consistent with how they're actually used and to hopefully be less confusing to future readers. Also did some minor code cleanup. No visible differences are intended as a result of this change. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. Renamed many methods and constants. Updated their associated comments to be consistent with how they're actually used and to hopefully be less confusing to future readers. Also did some minor code cleanup. No visible differences are intended as a result of this change. ========== to ========== Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin. Renamed many methods and constants. Updated their associated comments to be consistent with how they're actually used and to hopefully be less confusing to future readers. Also did some minor code cleanup. No visible differences are intended as a result of this change. Committed: https://crrev.com/05e4802e0ac10a178c9718ad21091d6e8a6c4a74 Cr-Commit-Position: refs/heads/master@{#388542} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/05e4802e0ac10a178c9718ad21091d6e8a6c4a74 Cr-Commit-Position: refs/heads/master@{#388542} |
