Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738623002:
[Mac] Turn on Auto Layout for browser windows. (Closed)
|
Created:
3 years, 9 months ago by Sidney San Martín Modified:
3 years, 9 months ago Reviewers:
tapted CC:
chromium-reviews, mac-reviews_chromium.org, erikchen, shrike, Eugene But (OOO till 7-30) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Turn on Auto Layout for browser windows.
Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and
NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout.
This change makes browser windows use Auto Layout but doesn't create any
dependence on it, so that it has time to bake while being easy to disable.
BUG=695577
Review-Url: https://codereview.chromium.org/2738623002
Cr-Commit-Position: refs/heads/master@{#458945}
Committed: https://chromium.googlesource.com/chromium/src/+/4f561865ff1add5ad5367d3c15f98a8fba1dff2a
Patch Set 1 #Patch Set 2 : Add a comment. #Patch Set 3 : Add a test. #
Total comments: 13
Patch Set 4 : Trigger Auto Layout from chromeContentView, which covers all browser windows. #
Total comments: 8
Patch Set 5 : Nits. #
Messages
Total messages: 33 (26 generated)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [NOT FOR COMMIT] Turn on Auto Layout BUG=695577 ========== to ========== [Mac] Turn on Auto Layout for browser windows. This change shouldn't have any observable effect, but gives us a chance to catch Auto Layout-related bugs before we make any changes that depend on it. A window uses Auto Layout if any of its views require it, and returning YES from +[FullSizeContentView requiresConstraintBasedLayout] is a simple way to accomplish that without actually using any Auto Layout features. BUG=695577 ==========
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Mac] Turn on Auto Layout for browser windows. This change shouldn't have any observable effect, but gives us a chance to catch Auto Layout-related bugs before we make any changes that depend on it. A window uses Auto Layout if any of its views require it, and returning YES from +[FullSizeContentView requiresConstraintBasedLayout] is a simple way to accomplish that without actually using any Auto Layout features. BUG=695577 ========== to ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. Things broke the last time Auto Layout was on, so this change makes browser windows use Auto Layout but doesn't add anything that depends on it, to leave a buffer where it can be disabled easily if anything else breaks. BUG=695577 ==========
sdy@chromium.org changed reviewers: + tapted@chromium.org
Description was changed from ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. Things broke the last time Auto Layout was on, so this change makes browser windows use Auto Layout but doesn't add anything that depends on it, to leave a buffer where it can be disabled easily if anything else breaks. BUG=695577 ========== to ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. Things broke the last time Auto Layout was on, so this change makes browser windows use Auto Layout but doesn't add anything that depends on it, giving us a buffer where it can be disabled easily if anything else breaks. BUG=695577 ==========
Description was changed from ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. Things broke the last time Auto Layout was on, so this change makes browser windows use Auto Layout but doesn't add anything that depends on it, giving us a buffer where it can be disabled easily if anything else breaks. BUG=695577 ========== to ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. This change makes browser windows use Auto Layout but doesn't add anything that depends on it, giving us a buffer where it can be disabled easily if anything breaks. BUG=695577 ==========
Description was changed from ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. This change makes browser windows use Auto Layout but doesn't add anything that depends on it, giving us a buffer where it can be disabled easily if anything breaks. BUG=695577 ========== to ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. This change makes browser windows use Auto Layout but doesn't create any dependence on it, so that it has time to bake while being easy to disable. BUG=695577 ==========
PTAL. As far as I can tell, there are no known issues left with turning on Auto Layout. Let me know if I missed any! cc: everyone from the "Did we accidentally opt into auto-layout?" thread a few months ago.
https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm (right): https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:27: wantsViewsOverTitlebar:YES]; Is this needed? It suggests that passing `NO` will result in a window that does not want auto layout. We pass `NO` when there is no tab strip, which can happen for popups, devtools, packaged apps. I think for the reasons you point out, we wouldn't want these windows to behave differently in aspects of layout versus regular browser windows. E.g. packaged apps want to do similar things; drawing over the titlebar sometimes (and don't use BorderlessWindowMask) -- see NativeAppWindowCocoa::InstallView() (I think we just mash a WebContentsView into the contentView in that case, but it's the altered codepaths for resizing the WebContentsView that required the fix in http://crbug.com/655112#c16 -- I think we'd want the code touched there to be able to make the same assumptions about the layout codepaths. https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:49: // Verify that browser windows use Auto Layout. Since frame-based layout and This is super nit-picky, but I'm not sure if this test is verifying that browser windows use AutoLayout so much as verifying that the ChromeBrowserWindowTest test harness creates a window that uses autolayout. Perhaps just a re-wording is appropriate here. E.g. "Verify that passing wantsViewsOverTitlebar:YES to the ChromeBrowserWindow initializer results in a window that uses Auto Layout." https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:54: base::scoped_nsobject<NSView> view( Comment why the extra subview is necessary for the test? https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:57: [window_.contentView addSubview:view]; nit: [[window contentView] addSubview: view] https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:60: EXPECT_TRUE([[[window_ contentView] constraints] count] > 0); nit: EXPECT_LT(0u, [[[window_ contentView] constraints] count]); https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:35: nit: Maybe we want #pragma mark - NSView Overrides and #pragma mark - Private Methods before forceFrame So that it's clear where the methods are declared .(#pragma is consistent with the rest of the file, but a comment works too and is probably how we'd do it in a new file)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm (right): https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:27: wantsViewsOverTitlebar:YES]; On 2017/03/21 02:16:51, tapted wrote: > Is this needed? It suggests that passing `NO` will result in a window that does > not want auto layout. We pass `NO` when there is no tab strip, which can happen > for popups, devtools, packaged apps. I think for the reasons you point out, we > wouldn't want these windows to behave differently in aspects of layout versus > regular browser windows. E.g. packaged apps want to do similar things; drawing > over the titlebar sometimes (and don't use BorderlessWindowMask) -- see > NativeAppWindowCocoa::InstallView() (I think we just mash a WebContentsView > into the contentView in that case, but it's the altered codepaths for resizing > the WebContentsView that required the fix in http://crbug.com/655112#c16 -- I > think we'd want the code touched there to be able to make the same assumptions > about the layout codepaths. Where we force Auto Layout is somewhat arbitrary. Not *every* window should be opted in, since that includes bubbles, dialogs, and menus, but you're right that every kind of browser window should be. I moved +requiresConstraintBasedLayout to the window controller's chromeContentView (which turns out to be created by TabWindowController, which even non-tabbed browser windows use, which is still odd but outside the scope of this CL…). Let me know how that sounds. https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:49: // Verify that browser windows use Auto Layout. Since frame-based layout and On 2017/03/21 02:16:51, tapted wrote: > This is super nit-picky, but I'm not sure if this test is verifying that browser > windows use AutoLayout so much as verifying that the ChromeBrowserWindowTest > test harness creates a window that uses autolayout. > > Perhaps just a re-wording is appropriate here. E.g. "Verify that passing > wantsViewsOverTitlebar:YES to the ChromeBrowserWindow initializer results in a > window that uses Auto Layout." Heh, great point :). Re-wording the comment like that would have shown that the test wasn't meaningful. I moved it to BrowserWindowController's unit test, even though the change is in its superclass, to honor the original intent: browser windows should use Auto Layout. https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:54: base::scoped_nsobject<NSView> view( On 2017/03/21 02:16:51, tapted wrote: > Comment why the extra subview is necessary for the test? Gone. https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:57: [window_.contentView addSubview:view]; On 2017/03/21 02:16:51, tapted wrote: > nit: [[window contentView] addSubview: view] Gone. https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:60: EXPECT_TRUE([[[window_ contentView] constraints] count] > 0); On 2017/03/21 02:16:50, tapted wrote: > nit: > > EXPECT_LT(0u, [[[window_ contentView] constraints] count]); > Oof, `EXPECT_LT(0u, count)` seems harder to understand. Since it's unsigned, how about `EXPECT_NE(0u, count)`? https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.mm (right): https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.mm:35: On 2017/03/21 02:16:51, tapted wrote: > nit: Maybe we want > > #pragma mark - NSView Overrides > > and > > #pragma mark - Private Methods > > before forceFrame > > > So that it's clear where the methods are declared .(#pragma is consistent with > the rest of the file, but a comment works too and is probably how we'd do it in > a new file) Done.
lgtm % nits https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm (right): https://codereview.chromium.org/2738623002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm:60: EXPECT_TRUE([[[window_ contentView] constraints] count] > 0); On 2017/03/22 21:40:21, sdy wrote: > On 2017/03/21 02:16:50, tapted wrote: > > nit: > > > > EXPECT_LT(0u, [[[window_ contentView] constraints] count]); > > > > Oof, `EXPECT_LT(0u, count)` seems harder to understand. Since it's unsigned, how > about `EXPECT_NE(0u, count)`? looks good! https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right): https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:755: // If Auto Layout is on, there will at least be synthesized constraints based nit: remove `at least`? (or "..there will be at least one synthesized constraint..") https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:66: @interface ChromeContentView : NSView comment like // Subview of the NSWindow's contentView containing everything except the tab strip. https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:71: #pragma mark - NSView Overrides optional: a comment here like // NSView overrides. or // NSView implementation. would be fine. (I've never been that enamoured with `#pragma mark`, but maybe there's some IDE integration I've never noticed that gives it meaning). This file doesn't have a prevailing style one way or the other - maybe Robert has opinions about this :). (I'm pretty indifferent). https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:100: chromeContentView_.reset([[ChromeContentView alloc] I think this will capture popups, but not packaged app windows. I'm ok with this. I think WebContentsViewCocoa is the only part of the view hierarchy that they share, which sits in the content layer. It would probably be weird to put requiresConstraintBasedLayout on that, since it will affect other embedders (also WebContentsViewCocoa does an explicit layout of subviews). (although NativeAppWindowCocoa is guilty of mashing things into [[window contentView] superview] as well, so if packaged apps don't disappear sooner, be aware that NativeAppWindowCocoa may switch to autolayout if that gets updated to use FullsizeContentView the way it should).
https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right): https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:755: // If Auto Layout is on, there will at least be synthesized constraints based On 2017/03/22 22:40:00, tapted wrote: > nit: remove `at least`? (or "..there will be at least one synthesized > constraint..") Done. https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:66: @interface ChromeContentView : NSView On 2017/03/22 22:40:00, tapted wrote: > comment like > > // Subview of the NSWindow's contentView containing everything except the tab > strip. Done. https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:71: #pragma mark - NSView Overrides On 2017/03/22 22:40:00, tapted wrote: > optional: a comment here like > > // NSView overrides. > > or > > // NSView implementation. > > would be fine. (I've never been that enamoured with `#pragma mark`, but maybe > there's some IDE integration I've never noticed that gives it meaning). > > > This file doesn't have a prevailing style one way or the other - maybe Robert > has opinions about this :). (I'm pretty indifferent). Done. Xcode likes `#pragma mark -` (there's a popup menu to browse the current file, and this makes a nice little divider and section heading in that menu), but we're not an Xcode-heavy team so it's not super important. https://codereview.chromium.org/2738623002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:100: chromeContentView_.reset([[ChromeContentView alloc] On 2017/03/22 22:40:00, tapted wrote: > I think this will capture popups, but not packaged app windows. I'm ok with > this. I think WebContentsViewCocoa is the only part of the view hierarchy that > they share, which sits in the content layer. It would probably be weird to put > requiresConstraintBasedLayout on that, since it will affect other embedders > (also WebContentsViewCocoa does an explicit layout of subviews). > > (although NativeAppWindowCocoa is guilty of mashing things into [[window > contentView] superview] as well, so if packaged apps don't disappear sooner, be > aware that NativeAppWindowCocoa may switch to autolayout if that gets updated to > use FullsizeContentView the way it should). Oh, good to know. I may be working on that change in the near-ish future. Do you know off the top of your head why they do that instead of using borderless windows? It looks like it just does this for windows with AppWindow::FRAME_NONE.
The CQ bit was checked by sdy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2738623002/#ps80001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490225431746820,
"parent_rev": "8cde3db06edbce905c1eef92dc1c7962c1e8693c", "commit_rev":
"4f561865ff1add5ad5367d3c15f98a8fba1dff2a"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. This change makes browser windows use Auto Layout but doesn't create any dependence on it, so that it has time to bake while being easy to disable. BUG=695577 ========== to ========== [Mac] Turn on Auto Layout for browser windows. Some newer AppKit features like NSWindowStyleMaskFullSizeContentView and NSStackView use Auto Layout, which opts the whole NSWindow into Auto Layout. This change makes browser windows use Auto Layout but doesn't create any dependence on it, so that it has time to bake while being easy to disable. BUG=695577 Review-Url: https://codereview.chromium.org/2738623002 Cr-Commit-Position: refs/heads/master@{#458945} Committed: https://chromium.googlesource.com/chromium/src/+/4f561865ff1add5ad5367d3c15f9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4f561865ff1add5ad5367d3c15f9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
