Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1636703002:
Implement MD specs for non-tabbed windows in Ash (Closed)
|
Created:
4 years, 11 months ago by tdanderson Modified:
4 years, 10 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement MD specs for non-tabbed windows in Ash
For material design in Ash, non-tabbed windows
draw a 1px (regardless of device scale factor)
separator line drawn between the frame header,
location bar (if one is visible), and the window
contents. Furthermore, remove extra vertical
spacing between these elements so that no toolbar
theming is visible.
BUG=580302, 585856
TEST=manual, BrowserTest.TestPopupBounds
Committed: https://crrev.com/f55088d33f76bb51e066e322d54fceabd18b463c
Cr-Commit-Position: refs/heads/master@{#374923}
Patch Set 1 #Patch Set 2 : fix tabstrip bottom edge #
Total comments: 6
Patch Set 3 : refactor #
Total comments: 10
Patch Set 4 : avoid double-painting separator line #Patch Set 5 : test changed, ready for review #
Total comments: 6
Patch Set 6 : latest comments #
Total comments: 30
Patch Set 7 : more feedback addressed #
Total comments: 15
Patch Set 8 : test changed, TODO added #Patch Set 9 : re-enabled test, changed TODO #
Total comments: 6
Patch Set 10 : for landing #
Messages
Total messages: 33 (8 generated)
tdanderson@chromium.org changed reviewers: + pkasting@chromium.org
Peter, can you please take a look? See the screenshots at https://code.google.com/p/chromium/issues/detail?id=580302 (I anticipate that my change to ToolbarView::PopupTopSpacing() as-is will cause a regression on Windows.)
https://codereview.chromium.org/1636703002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:551: (ui::MaterialDesignController::IsModeMaterial() ? Why do you need to do an MD check here? The other frame views' versions of this function don't. https://codereview.chromium.org/1636703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:555: void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) { I'd like to see you refactor these next two functions to be of the same basic structure/order as the current glass and opaque implementations. In the process of doing this, you may want to revert your change to ToolbarView::PopupTopSpacing(); I haven't debugged locally on Windows yet (checkout out of date) to be certain. In particular you'll want to look at https://codereview.chromium.org/1455193003 , though that's not the only difference between this code and those files. With these changes, the ordering in this function makes a check for "is the tabstrip visible" first. This results in not drawing a bunch of stuff for popup windows, e.g. the toolbar background. How much you'll need in PaintClientEdge() isn't totally clear to me as I haven't looked closely at how those other implementations do things; from glancing at your current implementation here, it seems like you don't need the side and bottom edges, just the top, so maybe something close to the simplicity of your current implementation works. But note that without doing any of this refactoring, the structure you have set up in this implementation draws the top separator in both these two functions for MD, which is a clear sign something isn't right. Doing this should fix the problem you're having with 2x popup windows in customized themes, since those implementations explicitly check if the tabstrip is visible, and if not, don't paint the toolbar background (among other things). https://codereview.chromium.org/1636703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:588: gfx::ScopedCanvas scoped_canvas(canvas); Why did you hoist this statement?
Peter, here is an updated WIP with inline comments from me about the remaining problems. https://codereview.chromium.org/1636703002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:551: (ui::MaterialDesignController::IsModeMaterial() ? On 2016/01/26 03:04:39, Peter Kasting wrote: > Why do you need to do an MD check here? The other frame views' versions of this > function don't. Changed in next patch set. https://codereview.chromium.org/1636703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:555: void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) { On 2016/01/26 03:04:39, Peter Kasting wrote: > I'd like to see you refactor these next two functions to be of the same basic > structure/order as the current glass and opaque implementations. In the process > of doing this, you may want to revert your change to > ToolbarView::PopupTopSpacing(); I haven't debugged locally on Windows yet > (checkout out of date) to be certain. > > In particular you'll want to look at https://codereview.chromium.org/1455193003 > , though that's not the only difference between this code and those files. > > With these changes, the ordering in this function makes a check for "is the > tabstrip visible" first. This results in not drawing a bunch of stuff for popup > windows, e.g. the toolbar background. How much you'll need in PaintClientEdge() > isn't totally clear to me as I haven't looked closely at how those other > implementations do things; from glancing at your current implementation here, it > seems like you don't need the side and bottom edges, just the top, so maybe > something close to the simplicity of your current implementation works. But > note that without doing any of this refactoring, the structure you have set up > in this implementation draws the top separator in both these two functions for > MD, which is a clear sign something isn't right. > > Doing this should fix the problem you're having with 2x popup windows in > customized themes, since those implementations explicitly check if the tabstrip > is visible, and if not, don't paint the toolbar background (among other things). I have made an effort to do so in the next patch set, but some problems remain (see my inline comments). https://codereview.chromium.org/1636703002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:588: gfx::ScopedCanvas scoped_canvas(canvas); On 2016/01/26 03:04:39, Peter Kasting wrote: > Why did you hoist this statement? Changed in next patch set. https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:656: canvas->FillRect(toolbar_bounds, SK_ColorWHITE); Without line 656, at 2x device scale factor we will have a transparent row of pixels between the toolbar and the separator line. By filling in with white here it gives the illusion that the bottom of the toolbar is actually flush with the separator line. https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:670: BrowserView::Paint1pxHorizontalLine( DefaultHeaderPainter::PaintTitleBar() already paints this separator line (DefaultHeaderPainter is used when UsePackagedAppHeaderStyle() || UseWebAppHeaderStyle() is true). So line 670 looks to be redundant in such cases, but without it I am sometimes noticing incomplete rendering of the line (until I force a repaint by maximizing/unmaximizing or resizing the window). https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:754: if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) This change makes the frame header flush with the contents. But as a consequence, if you hover over one of the caption buttons and then move the cursor into the web contents by crossing the bottom edge of the button, the button's hover state persists.
https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:656: canvas->FillRect(toolbar_bounds, SK_ColorWHITE); On 2016/01/29 22:28:43, tdanderson wrote: > Without line 656, at 2x device scale factor we will have a transparent row of > pixels between the toolbar and the separator line. By filling in with white here > it gives the illusion that the bottom of the toolbar is actually flush with the > separator line. This is actually a bug on non-Ash as well; I must have regressed this. Let me fix this for other platforms, which should likely fix for Ash as well. https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:670: BrowserView::Paint1pxHorizontalLine( On 2016/01/29 22:28:43, tdanderson wrote: > DefaultHeaderPainter::PaintTitleBar() already paints this separator line > (DefaultHeaderPainter is used when UsePackagedAppHeaderStyle() || > UseWebAppHeaderStyle() is true). So line 670 looks to be redundant in such > cases, but without it I am sometimes noticing incomplete rendering of the line > (until I force a repaint by maximizing/unmaximizing or resizing the window). I would try to figure out why this isn't the case before your change, where that was the only thing to render this line and there was no redundancy. https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:754: if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) On 2016/01/29 22:28:43, tdanderson wrote: > This change makes the frame header flush with the contents. But as a > consequence, if you hover over one of the caption buttons and then move the > cursor into the web contents by crossing the bottom edge of the button, the > button's hover state persists. That sounds like a bug where views is not sending a mouse out event correctly. I would debug to find the call stack that sends the mouse out event in the normal case, then see if you can debug with this layout and find out where in that callstack things are getting interrupted.
https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:656: canvas->FillRect(toolbar_bounds, SK_ColorWHITE); On 2016/01/30 02:53:37, Peter Kasting wrote: > On 2016/01/29 22:28:43, tdanderson wrote: > > Without line 656, at 2x device scale factor we will have a transparent row of > > pixels between the toolbar and the separator line. By filling in with white > here > > it gives the illusion that the bottom of the toolbar is actually flush with > the > > separator line. > > This is actually a bug on non-Ash as well; I must have regressed this. Let me > fix this for other platforms, which should likely fix for Ash as well. Thanks, I've added a TODO in the next patch set. https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:670: BrowserView::Paint1pxHorizontalLine( On 2016/01/30 02:53:37, Peter Kasting wrote: > On 2016/01/29 22:28:43, tdanderson wrote: > > DefaultHeaderPainter::PaintTitleBar() already paints this separator line > > (DefaultHeaderPainter is used when UsePackagedAppHeaderStyle() || > > UseWebAppHeaderStyle() is true). So line 670 looks to be redundant in such > > cases, but without it I am sometimes noticing incomplete rendering of the line > > (until I force a repaint by maximizing/unmaximizing or resizing the window). > > I would try to figure out why this isn't the case before your change, where that > was the only thing to render this line and there was no redundancy. Fixed this in the next patch set. https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:754: if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) On 2016/01/30 02:53:37, Peter Kasting wrote: > On 2016/01/29 22:28:43, tdanderson wrote: > > This change makes the frame header flush with the contents. But as a > > consequence, if you hover over one of the caption buttons and then move the > > cursor into the web contents by crossing the bottom edge of the button, the > > button's hover state persists. > > That sounds like a bug where views is not sending a mouse out event correctly. > I would debug to find the call stack that sends the mouse out event in the > normal case, then see if you can debug with this layout and find out where in > that callstack things are getting interrupted. Still looking into this.
Description was changed from ========== Implement MD specs for non-tabbed windows in Ash For material design in Ash, non-tabbed windows draw a 1px (regardless of device scale factor) separator line drawn between the frame header, location bar (if one is visible), and the window contents. Furthermore, remove extra vertical spacing between these elements so that no toolbar theming is visible. BUG=580302 TEST=manual ========== to ========== Implement MD specs for non-tabbed windows in Ash For material design in Ash, non-tabbed windows draw a 1px (regardless of device scale factor) separator line drawn between the frame header, location bar (if one is visible), and the window contents. Furthermore, remove extra vertical spacing between these elements so that no toolbar theming is visible. BUG=580302 TEST=manual, BrowserTest.TestPopupBounds ==========
Peter, this is ready for formal review. Can you please take another look? https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/1636703002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:754: if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) On 2016/02/01 22:17:51, tdanderson wrote: > On 2016/01/30 02:53:37, Peter Kasting wrote: > > On 2016/01/29 22:28:43, tdanderson wrote: > > > This change makes the frame header flush with the contents. But as a > > > consequence, if you hover over one of the caption buttons and then move the > > > cursor into the web contents by crossing the bottom edge of the button, the > > > button's hover state persists. > > > > That sounds like a bug where views is not sending a mouse out event correctly. > > > I would debug to find the call stack that sends the mouse out event in the > > normal case, then see if you can debug with this layout and find out where in > > that callstack things are getting interrupted. > > Still looking into this. Turns out this was a recent regression unrelated to my CL (crbug.com/583389).
https://codereview.chromium.org/1636703002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:568: // Paint the main toolbar image. Since this image is also used to draw It would be nice to make this line up even more closely with the other frames' implementations in terms of comments, variable names, etc., but I guess that could be done as a separate pass. https://codereview.chromium.org/1636703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:656: canvas->FillRect(toolbar_bounds, SK_ColorWHITE); Doesn't this look gruesomely bad in incognito? I would probably just leave this out. The transparent line is present on all platforms and I'm fixing that, so working around it for now doesn't seem very useful anyway. https://codereview.chromium.org/1636703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:678: separator_rect, true); It seems like this is drawing the same bottom separator that PaintToolbar() is drawing. One of these should be drawing the popup-mode top separator instead.
Peter, please take another look. https://codereview.chromium.org/1636703002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:568: // Paint the main toolbar image. Since this image is also used to draw On 2016/02/03 01:09:23, Peter Kasting wrote: > It would be nice to make this line up even more closely with the other frames' > implementations in terms of comments, variable names, etc., but I guess that > could be done as a separate pass. I think the variable names here are already identical to the ones used for the opaque frame class (at least for the MD case - I didn't make any additional modifications to the pre-MD code). https://codereview.chromium.org/1636703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:656: canvas->FillRect(toolbar_bounds, SK_ColorWHITE); On 2016/02/03 01:09:23, Peter Kasting wrote: > Doesn't this look gruesomely bad in incognito? > > I would probably just leave this out. The transparent line is present on all > platforms and I'm fixing that, so working around it for now doesn't seem very > useful anyway. OK, removed. Is there a bug for that? https://codereview.chromium.org/1636703002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:678: separator_rect, true); On 2016/02/03 01:09:23, Peter Kasting wrote: > It seems like this is drawing the same bottom separator that PaintToolbar() is > drawing. One of these should be drawing the popup-mode top separator instead. Thanks for catching that, fixed.
https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3231: #endif // defined(OS_CHROMEOS) Rather than ifdef this, let's either convert this hardcoded constant to some kind of calculation that reflects what the real layout code actually does, or remove it entirely. Better yet, since the test is trying to ensure that the window is sized such that the content area size is correct, maybe instead of trying to calculate the total window height, we should modify the test to just ask for the actual content area size within the window, and ensure it matches exactly. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:572: // it lines up across the tab and toolbar. Nit: Use same comments as in glass/opaque frames https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:581: // of the tab strip. Nit: Use same comments as in glass/opaque frames https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:599: // must be changed as well. Nit: Use same comments as in glass/opaque frames https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:600: int split_point = kFrameShadowThickness * 2; Nit: Do like the glass and opaque frames and define a kContentEdgeShadowThickness value (= 2) and use that here. It doesn't really make sense to multiply kFrameShadowThickness by anything. (The code actually already defines a "kContentShadowHeight" value below that I think is supposed to be the same thing.) You also need to do std::min(kContentEdgeShadowThickness, h) instead of just using this directly, I think. Once that's done, you can wrap everything up to and including the next TileImageInt() call in a: if (h > split_point) { ... } https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:601: int bottom_y = y + split_point; Nit: Call this split_y to match other frames https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:605: tp->GetColor(ThemeProperties::COLOR_TOOLBAR)); Why is this statement needed? https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:611: // lines up across the tab and toolbar. This comment is really about code we did much earlier; the other frames omit it and I would here as well. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:627: w, split_point + kContentShadowHeight + 1); What does the +1 here mean? In fact, this whole height calculation seems extremely suspect; I don't know why we'd be adding kContentShadowHeight to split_point, which already has that value built in. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:641: w - toolbar_right->width() - 2*kClientEdgeThickness, Nit: Spaces around operators https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:659: if (browser_view()->IsTabStripVisible()) Nit: Go ahead and hoist this to the caller side; I'm doing something similar in my patch to fix popup mode drawing. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:669: ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR); What sort of window do we draw here that has no tabstrip, has no toolbar, and is not a packaged or web app? I ask because it seems like this should probably be COLOR_TOOLBAR_TOP_SEPARATOR always, but I'm not sure. I have to figure out a similar case before sending my patch for review, so if you know how to trigger this I'm curious. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:128: bool IsToolbarVisible() const; Nit: Swap with function above to keep the PaintXXX methods together.
Hi Peter, please take another look. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3231: #endif // defined(OS_CHROMEOS) On 2016/02/05 22:10:24, Peter Kasting wrote: > Rather than ifdef this, let's either convert this hardcoded constant to some > kind of calculation that reflects what the real layout code actually does, or > remove it entirely. > > Better yet, since the test is trying to ensure that the window is sized such > that the content area size is correct, maybe instead of trying to calculate the > total window height, we should modify the test to just ask for the actual > content area size within the window, and ensure it matches exactly. I'm not sure how to do this. Querying browser->window()->GetContentsSize() inside this test gives me 126*86, which doesn't make sense given that we've set the initial bounds to 100*122. Based on the comment "initial height is content height" I would have expected GetContentsSize() to give me 100*122. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:572: // it lines up across the tab and toolbar. On 2016/02/05 22:10:24, Peter Kasting wrote: > Nit: Use same comments as in glass/opaque frames Done. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:581: // of the tab strip. On 2016/02/05 22:10:24, Peter Kasting wrote: > Nit: Use same comments as in glass/opaque frames Changed to just "Top stroke." https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:599: // must be changed as well. On 2016/02/05 22:10:24, Peter Kasting wrote: > Nit: Use same comments as in glass/opaque frames Changed to just "Background." The extra comments in opaque/glass (creating a separate layer + using IDR_CONTENT_TOP_XXX) don't apply to Ash. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:600: int split_point = kFrameShadowThickness * 2; On 2016/02/05 22:10:24, Peter Kasting wrote: > Nit: Do like the glass and opaque frames and define a > kContentEdgeShadowThickness value (= 2) and use that here. It doesn't really > make sense to multiply kFrameShadowThickness by anything. (The code actually > already defines a "kContentShadowHeight" value below that I think is supposed to > be the same thing.) Done. > > You also need to do std::min(kContentEdgeShadowThickness, h) instead of just > using this directly, I think. Once that's done, you can wrap everything up to > and including the next TileImageInt() call in a: > > if (h > split_point) { > ... > } I don't understand the need to define |split_point| using min(kContentEdgeShadowThickness, h) and then have if (h > split_point). The only time this would make any difference in the existing logic is when h == 1, and I don't think that is possible if we have reached here in the code. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:601: int bottom_y = y + split_point; On 2016/02/05 22:10:24, Peter Kasting wrote: > Nit: Call this split_y to match other frames Done. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:605: tp->GetColor(ThemeProperties::COLOR_TOOLBAR)); On 2016/02/05 22:10:24, Peter Kasting wrote: > Why is this statement needed? It definitely isn't needed now that we have restructured this method. Removed. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:611: // lines up across the tab and toolbar. On 2016/02/05 22:10:24, Peter Kasting wrote: > This comment is really about code we did much earlier; the other frames omit it > and I would here as well. Done. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:627: w, split_point + kContentShadowHeight + 1); On 2016/02/05 22:10:24, Peter Kasting wrote: > What does the +1 here mean? In fact, this whole height calculation seems > extremely suspect; I don't know why we'd be adding kContentShadowHeight to > split_point, which already has that value built in. It's due to the fact that the IDR_TOOLBAR_SHADE_TOP asset is 5px tall, with the "shadow" occupying the bottom pixel (this is the +1). I have removed the height calculation and replaced it with toolbar_top->height(). https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:641: w - toolbar_right->width() - 2*kClientEdgeThickness, On 2016/02/05 22:10:25, Peter Kasting wrote: > Nit: Spaces around operators Done. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:659: if (browser_view()->IsTabStripVisible()) On 2016/02/05 22:10:25, Peter Kasting wrote: > Nit: Go ahead and hoist this to the caller side; I'm doing something similar in > my patch to fix popup mode drawing. Done. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:669: ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR); On 2016/02/05 22:10:24, Peter Kasting wrote: > What sort of window do we draw here that has no tabstrip, has no toolbar, and is > not a packaged or web app? Sounds like a riddle :) > > I ask because it seems like this should probably be COLOR_TOOLBAR_TOP_SEPARATOR > always, but I'm not sure. I have to figure out a similar case before sending my > patch for review, so if you know how to trigger this I'm curious. I thought there was such a window, but after checking every case I know of it turns out that there isn't one. I've changed this to always use the top separator color. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:128: bool IsToolbarVisible() const; On 2016/02/05 22:10:25, Peter Kasting wrote: > Nit: Swap with function above to keep the PaintXXX methods together. Done.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (left): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:193: return caption_buttons_bottom - kContentShadowHeight; would this break non-MD?
https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (left): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:193: return caption_buttons_bottom - kContentShadowHeight; On 2016/02/08 20:09:39, Evan Stade wrote: > would this break non-MD? No it doesn't, since I've changed BrowserNonClientFrameViewAsh::PaintContentEdge() to draw a 1px separator line (regardless of scale factor) at the bottom of the popup frame header (which is now flush against the top of the toolbar for both MD and non-MD).
https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3231: #endif // defined(OS_CHROMEOS) On 2016/02/08 19:36:44, tdanderson wrote: > On 2016/02/05 22:10:24, Peter Kasting wrote: > > Rather than ifdef this, let's either convert this hardcoded constant to some > > kind of calculation that reflects what the real layout code actually does, or > > remove it entirely. > > > > Better yet, since the test is trying to ensure that the window is sized such > > that the content area size is correct, maybe instead of trying to calculate > the > > total window height, we should modify the test to just ask for the actual > > content area size within the window, and ensure it matches exactly. > > I'm not sure how to do this. Querying browser->window()->GetContentsSize() > inside this test gives me 126*86, which doesn't make sense given that we've set > the initial bounds to 100*122. Based on the comment "initial height is content > height" I would have expected GetContentsSize() to give me 100*122. You might want to check that this isn't a legitimate bug. Perhaps we're not actually creating popup windows with the correct contents size on request. Another possibility is that we changed the meaning of the initial bounds param to be the outer bounds instead of the inner bounds and the test is out of date. Regardless, it sounds like we should understand why this is happening so we can fix correctly. https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:600: int split_point = kFrameShadowThickness * 2; On 2016/02/08 19:36:45, tdanderson wrote: > On 2016/02/05 22:10:24, Peter Kasting wrote: > > Nit: Do like the glass and opaque frames and define a > > kContentEdgeShadowThickness value (= 2) and use that here. It doesn't really > > make sense to multiply kFrameShadowThickness by anything. (The code actually > > already defines a "kContentShadowHeight" value below that I think is supposed > to > > be the same thing.) > > Done. > > > > > You also need to do std::min(kContentEdgeShadowThickness, h) instead of just > > using this directly, I think. Once that's done, you can wrap everything up to > > and including the next TileImageInt() call in a: > > > > if (h > split_point) { > > ... > > } > > I don't understand the need to define |split_point| using > min(kContentEdgeShadowThickness, h) and then have if (h > split_point). The only > time this would make any difference in the existing logic is when h == 1, and I > don't think that is possible if we have reached here in the code. At least for other frames, I believe that's possible while the location bar is animating to/from zero height. Save reddit.com as shortcut that opens a standalone window (so it will have no toolbar by default) and then click a link to imgur, which should animate the toolbar into view. (Hit back to animate the toolbar closed again.) If there's a reason that case can't reach here with h = 1, let me know.
https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3231: #endif // defined(OS_CHROMEOS) On 2016/02/08 23:18:19, Peter Kasting wrote: > On 2016/02/08 19:36:44, tdanderson wrote: > > On 2016/02/05 22:10:24, Peter Kasting wrote: > > > Rather than ifdef this, let's either convert this hardcoded constant to some > > > kind of calculation that reflects what the real layout code actually does, > or > > > remove it entirely. > > > > > > Better yet, since the test is trying to ensure that the window is sized such > > > that the content area size is correct, maybe instead of trying to calculate > > the > > > total window height, we should modify the test to just ask for the actual > > > content area size within the window, and ensure it matches exactly. > > > > I'm not sure how to do this. Querying browser->window()->GetContentsSize() > > inside this test gives me 126*86, which doesn't make sense given that we've > set > > the initial bounds to 100*122. Based on the comment "initial height is content > > height" I would have expected GetContentsSize() to give me 100*122. > > You might want to check that this isn't a legitimate bug. Perhaps we're not > actually creating popup windows with the correct contents size on request. > > Another possibility is that we changed the meaning of the initial bounds param > to be the outer bounds instead of the inner bounds and the test is out of date. > > Regardless, it sounds like we should understand why this is happening so we can > fix correctly. During layout, BrowserView is reporting its own bounds as 126*149 and then determines that the bounds of its contents is 126*86. So you're right that it looks as though there is a legitimate bug here when params.initial_bounds is used. After spending some more time I have not yet been successful in finding the root cause - can we keep this test as-is for now and investigate the bug as a separate issue? https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:600: int split_point = kFrameShadowThickness * 2; On 2016/02/08 23:18:19, Peter Kasting wrote: > On 2016/02/08 19:36:45, tdanderson wrote: > > On 2016/02/05 22:10:24, Peter Kasting wrote: > > > Nit: Do like the glass and opaque frames and define a > > > kContentEdgeShadowThickness value (= 2) and use that here. It doesn't > really > > > make sense to multiply kFrameShadowThickness by anything. (The code > actually > > > already defines a "kContentShadowHeight" value below that I think is > supposed > > to > > > be the same thing.) > > > > Done. > > > > > > > > You also need to do std::min(kContentEdgeShadowThickness, h) instead of just > > > using this directly, I think. Once that's done, you can wrap everything up > to > > > and including the next TileImageInt() call in a: > > > > > > if (h > split_point) { > > > ... > > > } > > > > I don't understand the need to define |split_point| using > > min(kContentEdgeShadowThickness, h) and then have if (h > split_point). The > only > > time this would make any difference in the existing logic is when h == 1, and > I > > don't think that is possible if we have reached here in the code. > > At least for other frames, I believe that's possible while the location bar is > animating to/from zero height. Save http://reddit.com as shortcut that opens a > standalone window (so it will have no toolbar by default) and then click a link > to imgur, which should animate the toolbar into view. (Hit back to animate the > toolbar closed again.) > > If there's a reason that case can't reach here with h = 1, let me know. At least on Ash, the tab strip is not visible in the case you describe, so we won't reach this point in the code. I don't think there is any case where the toolbar animates into view and the tab strip is visible, but please correct me if I am mistaken.
LGTM https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3231: #endif // defined(OS_CHROMEOS) For now, let's either disable this test, or change to check the content height but check the erroneous value you found, and in either case add a TODO + file a bug to do the right thing. https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:601: // couple of pixels above the toolbar bounds. The code in this section still doesn't make sense, but you're not making anything worse, so let's not worry about it in this change. I've sent you a separate CL to fix the problems I see in here.
https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:691: if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) Oh, BTW, HOST_DESKTOP_TYPE_ASH is going away as we remove Metro mode Ash. Instead, use #if defined(OS_CHROMEOS) here. It turns out I have to do this anyway: https://codereview.chromium.org/1682373002 You should just modify my code so that instead of returning kCLientEdgeThickness, you return 0.
https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:691: if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) On 2016/02/10 07:23:40, Peter Kasting wrote: > Oh, BTW, HOST_DESKTOP_TYPE_ASH is going away as we remove Metro mode Ash. > Instead, use #if defined(OS_CHROMEOS) here. > > It turns out I have to do this anyway: > https://codereview.chromium.org/1682373002 > > You should just modify my code so that instead of returning > kCLientEdgeThickness, you return 0. (Which is in fact precisely what https://codereview.chromium.org/1685763004/ does. However, I planned to let you land this patch before I landed that one, since otherwise I'll break Ash between the two patches; instead, after you land, I'll add the Ash changes necessary to that CR.)
https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:692: return 0; Actually, the more I think about this, the less sure I am that it's correct. You do, in fact, reserve 2 px atop the toolbar, inside its bounds, for "shadow above the location bar", which is what the reservation below here is for. This is also what's leading to your popup mode unittest size changing versus the expectation. Doesn't this cause the location bar itself to be 2 px too short? Why do we not need to reserve any space here?
Peter, please take another look and let me know if this still LGTY to land. https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3231: #endif // defined(OS_CHROMEOS) On 2016/02/10 02:43:58, Peter Kasting wrote: > For now, let's either disable this test, or change to check the content height > but check the erroneous value you found, and in either case add a TODO + file a > bug to do the right thing. Done (crbug.com/585856). I have a funny feeling that the reported contents bounds may vary by platform, but we'll see what the trybots say when running the new test. https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:601: // couple of pixels above the toolbar bounds. On 2016/02/10 02:43:58, Peter Kasting wrote: > The code in this section still doesn't make sense, but you're not making > anything worse, so let's not worry about it in this change. I've sent you a > separate CL to fix the problems I see in here. OK, once this CL lands I will build the other CL and address your questions there. https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:691: if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) On 2016/02/10 07:46:04, Peter Kasting wrote: > On 2016/02/10 07:23:40, Peter Kasting wrote: > > Oh, BTW, HOST_DESKTOP_TYPE_ASH is going away as we remove Metro mode Ash. > > Instead, use #if defined(OS_CHROMEOS) here. > > > > It turns out I have to do this anyway: > > https://codereview.chromium.org/1682373002 > > > > You should just modify my code so that instead of returning > > kCLientEdgeThickness, you return 0. > > (Which is in fact precisely what https://codereview.chromium.org/1685763004/ > does. However, I planned to let you land this patch before I landed that one, > since otherwise I'll break Ash between the two patches; instead, after you land, > I'll add the Ash changes necessary to that CR.) Acknowledged. https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:692: return 0; On 2016/02/10 09:12:57, Peter Kasting wrote: > Actually, the more I think about this, the less sure I am that it's correct. > > You do, in fact, reserve 2 px atop the toolbar, inside its bounds, for "shadow > above the location bar", which is what the reservation below here is for. This > is also what's leading to your popup mode unittest size changing versus the > expectation. Doesn't this cause the location bar itself to be 2 px too short? > Why do we not need to reserve any space here? In my understanding, we're not (and don't want to) reserve this extra padding for a shadow in the case of a popup/app window (at least on Ash). The header, toolbar, and contents should all be flush on top of one another, and adding in the extra padding just creates a gap between the header and the toolbar. I'm attaching screenshots on crbug.com/580302 to show you what each window type looks like with no extra top spacing (like what is done here) and what it would look like if I were to delete lines 691-692. With the CL as-is, the toolbar (less borders) is the same height for tabbed browser windows, popups, and app windows (25px for non-material, 26px for material, and 30px for hybrid).
Description was changed from ========== Implement MD specs for non-tabbed windows in Ash For material design in Ash, non-tabbed windows draw a 1px (regardless of device scale factor) separator line drawn between the frame header, location bar (if one is visible), and the window contents. Furthermore, remove extra vertical spacing between these elements so that no toolbar theming is visible. BUG=580302 TEST=manual, BrowserTest.TestPopupBounds ========== to ========== Implement MD specs for non-tabbed windows in Ash For material design in Ash, non-tabbed windows draw a 1px (regardless of device scale factor) separator line drawn between the frame header, location bar (if one is visible), and the window contents. Furthermore, remove extra vertical spacing between these elements so that no toolbar theming is visible. BUG=580302,585856 TEST=manual, BrowserTest.TestPopupBounds ==========
Looks like the reported contents size does vary by platform (see trybot runs from patchset 8). I propose keeping the test enabled and ifdef-ed on ChromeOS while adding a TODO. This way at least we get some sort of regression test coverage in the meanwhile (i.e., it will detect if the actual dimensions of a popup window shrinks on any platform).
LGTM https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:601: // couple of pixels above the toolbar bounds. On 2016/02/10 18:22:30, tdanderson wrote: > On 2016/02/10 02:43:58, Peter Kasting wrote: > > The code in this section still doesn't make sense, but you're not making > > anything worse, so let's not worry about it in this change. I've sent you a > > separate CL to fix the problems I see in here. > > OK, once this CL lands I will build the other CL and address your questions > there. I wouldn't wait for this CL to land -- that CL is independent of this and only affects non-MD so you should go ahead and look. https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:692: return 0; On 2016/02/10 18:22:30, tdanderson wrote: > On 2016/02/10 09:12:57, Peter Kasting wrote: > > Actually, the more I think about this, the less sure I am that it's correct. > > > > You do, in fact, reserve 2 px atop the toolbar, inside its bounds, for "shadow > > above the location bar", which is what the reservation below here is for. > This > > is also what's leading to your popup mode unittest size changing versus the > > expectation. Doesn't this cause the location bar itself to be 2 px too short? > > > Why do we not need to reserve any space here? > > In my understanding, we're not (and don't want to) reserve this extra padding > for a shadow in the case of a popup/app window (at least on Ash). The header, > toolbar, and contents should all be flush on top of one another, and adding in > the extra padding just creates a gap between the header and the toolbar. > > I'm attaching screenshots on crbug.com/580302 to show you what each window type > looks like with no extra top spacing (like what is done here) and what it would > look like if I were to delete lines 691-692. > > With the CL as-is, the toolbar (less borders) is the same height for tabbed > browser windows, popups, and app windows (25px for non-material, 26px for > material, and 30px for hybrid). OK, reading your code again, I understand what's happening here. The gap here is because of the code you deleted in BrowserNonClientFrameViewAsh::Layout(). We could use the 2 px gap here and restore the code there, then draw the 2 px of shadow in the popup case. But that would be inconsistent with how the other frames handle popups, where the location bar doesn't have a shadow above it. So I think the changes you've made are correct. This suggests that I need to refactor this code to do something similar for opaque frames: don't add the 2 px here, and then in the opaque frame layout code, ensure we expand the above-toolbar height by 2 px for popups. https://codereview.chromium.org/1636703002/diff/160001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1636703002/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3216: // TODO(tdanderson|pkasting): Change the first test to verify that the Nit: "the first test" -> "this", since there are actually three EXPECTs below and we'd need to change both first and second. https://codereview.chromium.org/1636703002/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3217: // contents bounds set by params.initial_bounds Nit: Don't indent the text on these lines https://codereview.chromium.org/1636703002/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3224: #if defined(OS_CHROMEOS) Nit: I would eliminate the ifdef here, use the CrOS value in all cases (since the 29 case is already only applicable to Windows, not Mac, so it's already not a tight bound), and change the comment above -- which is inaccurate anyway -- to read: Minimum height a popup window should have added to the supplied content bounds when drawn. This accommodates the browser toolbar.
https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:601: // couple of pixels above the toolbar bounds. On 2016/02/10 23:46:21, Peter Kasting wrote: > On 2016/02/10 18:22:30, tdanderson wrote: > > On 2016/02/10 02:43:58, Peter Kasting wrote: > > > The code in this section still doesn't make sense, but you're not making > > > anything worse, so let's not worry about it in this change. I've sent you a > > > separate CL to fix the problems I see in here. > > > > OK, once this CL lands I will build the other CL and address your questions > > there. > > I wouldn't wait for this CL to land -- that CL is independent of this and only > affects non-MD so you should go ahead and look. Acknowledged. https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/1636703002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view.cc:692: return 0; On 2016/02/10 23:46:21, Peter Kasting wrote: > On 2016/02/10 18:22:30, tdanderson wrote: > > On 2016/02/10 09:12:57, Peter Kasting wrote: > > > Actually, the more I think about this, the less sure I am that it's correct. > > > > > > You do, in fact, reserve 2 px atop the toolbar, inside its bounds, for > "shadow > > > above the location bar", which is what the reservation below here is for. > > This > > > is also what's leading to your popup mode unittest size changing versus the > > > expectation. Doesn't this cause the location bar itself to be 2 px too > short? > > > > > Why do we not need to reserve any space here? > > > > In my understanding, we're not (and don't want to) reserve this extra padding > > for a shadow in the case of a popup/app window (at least on Ash). The header, > > toolbar, and contents should all be flush on top of one another, and adding in > > the extra padding just creates a gap between the header and the toolbar. > > > > I'm attaching screenshots on crbug.com/580302 to show you what each window > type > > looks like with no extra top spacing (like what is done here) and what it > would > > look like if I were to delete lines 691-692. > > > > With the CL as-is, the toolbar (less borders) is the same height for tabbed > > browser windows, popups, and app windows (25px for non-material, 26px for > > material, and 30px for hybrid). > > OK, reading your code again, I understand what's happening here. The gap here is > because of the code you deleted in BrowserNonClientFrameViewAsh::Layout(). > > We could use the 2 px gap here and restore the code there, then draw the 2 px of > shadow in the popup case. But that would be inconsistent with how the other > frames handle popups, where the location bar doesn't have a shadow above it. So > I think the changes you've made are correct. > > This suggests that I need to refactor this code to do something similar for > opaque frames: don't add the 2 px here, and then in the opaque frame layout > code, ensure we expand the above-toolbar height by 2 px for popups. Acknowledged. https://codereview.chromium.org/1636703002/diff/160001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1636703002/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3216: // TODO(tdanderson|pkasting): Change the first test to verify that the On 2016/02/10 23:46:21, Peter Kasting wrote: > Nit: "the first test" -> "this", since there are actually three EXPECTs below > and we'd need to change both first and second. Done. https://codereview.chromium.org/1636703002/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3217: // contents bounds set by params.initial_bounds On 2016/02/10 23:46:21, Peter Kasting wrote: > Nit: Don't indent the text on these lines Done. https://codereview.chromium.org/1636703002/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3224: #if defined(OS_CHROMEOS) On 2016/02/10 23:46:21, Peter Kasting wrote: > Nit: I would eliminate the ifdef here, use the CrOS value in all cases (since > the 29 case is already only applicable to Windows, not Mac, so it's already not > a tight bound), and change the comment above -- which is inaccurate anyway -- to > read: > > Minimum height a popup window should have added to the supplied content bounds > when drawn. This accommodates the browser toolbar. Done.
The CQ bit was checked by tdanderson@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/1636703002/#ps180001 (title: "for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1636703002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1636703002/180001
Message was sent while issue was closed.
Description was changed from ========== Implement MD specs for non-tabbed windows in Ash For material design in Ash, non-tabbed windows draw a 1px (regardless of device scale factor) separator line drawn between the frame header, location bar (if one is visible), and the window contents. Furthermore, remove extra vertical spacing between these elements so that no toolbar theming is visible. BUG=580302,585856 TEST=manual, BrowserTest.TestPopupBounds ========== to ========== Implement MD specs for non-tabbed windows in Ash For material design in Ash, non-tabbed windows draw a 1px (regardless of device scale factor) separator line drawn between the frame header, location bar (if one is visible), and the window contents. Furthermore, remove extra vertical spacing between these elements so that no toolbar theming is visible. BUG=580302,585856 TEST=manual, BrowserTest.TestPopupBounds ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Implement MD specs for non-tabbed windows in Ash For material design in Ash, non-tabbed windows draw a 1px (regardless of device scale factor) separator line drawn between the frame header, location bar (if one is visible), and the window contents. Furthermore, remove extra vertical spacing between these elements so that no toolbar theming is visible. BUG=580302,585856 TEST=manual, BrowserTest.TestPopupBounds ========== to ========== Implement MD specs for non-tabbed windows in Ash For material design in Ash, non-tabbed windows draw a 1px (regardless of device scale factor) separator line drawn between the frame header, location bar (if one is visible), and the window contents. Furthermore, remove extra vertical spacing between these elements so that no toolbar theming is visible. BUG=580302,585856 TEST=manual, BrowserTest.TestPopupBounds Committed: https://crrev.com/f55088d33f76bb51e066e322d54fceabd18b463c Cr-Commit-Position: refs/heads/master@{#374923} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f55088d33f76bb51e066e322d54fceabd18b463c Cr-Commit-Position: refs/heads/master@{#374923} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
