Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23494065:
Fix restoring a window after maximizing it. (Mac) (Closed)
|
Created:
7 years, 3 months ago by jackhou1 Modified:
7 years, 2 months ago CC:
chromium-reviews, scheib+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix restoring a window after maximizing it. (Mac)
Currently the window does not restore to its original size because we
save the current size of the window whenever it's resized. However,
during a zoom (maximize), the windowDidResize notification is fired on
every animation frame.
In this CL, the current size is only saved after the animation finishes.
|is_maximized_| and |is_fullscreen_| are also updated when a resize
finishes, and when going in and out of fullscreen.
BUG=168080, 230778, 302053
TEST=Maximize and restore an app window, it should return to its original size.
TEST=Do the following sequence on an app window:
Maximize, fullscreen, exit fullscreen, then restore.
The app should return to its original size.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226401
Patch Set 1 #
Total comments: 2
Patch Set 2 : Sync and rebase #Patch Set 3 : Update is_fullscreen_ #
Total comments: 4
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Call OnNativeWindowChanged after WindowDidFinishResize #Patch Set 6 : Revert to patch 4. Call WindowDidFinishResize after SetBounds. #
Total comments: 2
Patch Set 7 : Add comment #
Messages
Total messages: 22 (0 generated)
+chrome-apps-syd-reviews https://codereview.chromium.org/23494065/diff/1/chrome/browser/ui/cocoa/apps/... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/1/chrome/browser/ui/cocoa/apps/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:93: - (void)windowDidExitFullScreen:(NSNotification*)notification { What about windowDidEnterFullScreen - should that also call WindowDidFinishResize()? (also the fullscreen goo is 10.7 only, but I don't think there's any issue having them here in 10.6)
https://codereview.chromium.org/23494065/diff/1/chrome/browser/ui/cocoa/apps/... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/1/chrome/browser/ui/cocoa/apps/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:93: - (void)windowDidExitFullScreen:(NSNotification*)notification { On 2013/09/24 03:11:44, tapted wrote: > What about windowDidEnterFullScreen - should that also call > WindowDidFinishResize()? Yup, and WindowDidFinishResize needs to update is_fullscreen_ too. > (also the fullscreen goo is 10.7 only, but I don't > think there's any issue having them here in 10.6) Yeah, I think they're just not called.
lgtm. But check if there's anything useful you could add (and there are not regressions) in src/chrome/test/data/extensions/platform_apps/windows_api/test.js . Parts are disabled for flakiness, but they currently work and all pass (at least on Mac), with `./out/Debug/browser_tests --gtest_also_run_disabled_tests --gtest_filter='*WindowsApi*'` -- ensure that still is the case after this change. Other parts are just commented out wholesale in the test.js (specifically maximize and restore :|). I think with these animating and manipulating windows they are doomed to flakiness until moved to an interactive ui test and the animations are disabled. Disabling maximize/resotre animations can be done on Mac, but it would need a separate .mm test file to call into Cocoa... app_window_apitest.cc is already littered with bug numbers, so probably not worth adding more :/ And, the fix for is_fullscreen should get a mention in the CL description, since it's a bit orthogonal. Also, forgot some description nits: > Currently the window does not restore to it's original size because we "it's" -> "its" > TEST=Maximize and restore and app window, it should return to its original size. "and" -> "an" On 2013/09/24 04:52:02, jackhou1 wrote: > https://codereview.chromium.org/23494065/diff/1/chrome/browser/ui/cocoa/apps/... > File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): > https://codereview.chromium.org/23494065/diff/1/chrome/browser/ui/cocoa/apps/... > Yup, and WindowDidFinishResize needs to update is_fullscreen_ too. Ah, good find. https://codereview.chromium.org/23494065/diff/11001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/11001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:103: nit: extra blank line crept in here https://codereview.chromium.org/23494065/diff/11001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:872: is_fullscreen_ = [window() styleMask] & NSFullScreenWindowMask; nit: maybe `([window() styleMask] & NSFullScreenWindowMask) != 0` I think C++'s `bool` does the right thing w.r.t. overflow, but Objective-C `BOOL` does not. (i.e. BOOL is_fullscreen_ = [window() styleMask] & NSFullScreenWindowMask; is always `false` because NSFullScreenWindowMask > 256 and BOOL is a typedef, but since is_fullscreen_ is actually `bool` it's fine -- the compiler does something smarter than dumb assignment.
https://codereview.chromium.org/23494065/diff/11001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/11001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:103: On 2013/09/24 05:59:24, tapted wrote: > nit: extra blank line crept in here Done. https://codereview.chromium.org/23494065/diff/11001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:872: is_fullscreen_ = [window() styleMask] & NSFullScreenWindowMask; On 2013/09/24 05:59:24, tapted wrote: > nit: maybe `([window() styleMask] & NSFullScreenWindowMask) != 0` > > I think C++'s `bool` does the right thing w.r.t. overflow, but Objective-C > `BOOL` does not. (i.e. > > BOOL is_fullscreen_ = [window() styleMask] & NSFullScreenWindowMask; > > is always `false` because NSFullScreenWindowMask > 256 and BOOL is a typedef, > but since is_fullscreen_ is actually `bool` it's fine -- the compiler does > something smarter than dumb assignment. Done.
benwells, please review for OWNERS
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/23494065/22001
Retried try job too often on linux_aura for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/23494065/22001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
https://codereview.chromium.org/23494065/diff/22001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/22001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:876: void NativeAppWindowCocoa::WindowDidResize() { Certain you don't need UpdaterestoredBounds() in this method? OnNativeWindowChanged will attempt to save to geometry cache stale data otherwise I think.
https://codereview.chromium.org/23494065/diff/22001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/22001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:876: void NativeAppWindowCocoa::WindowDidResize() { On 2013/09/24 16:04:15, scheib (out of office) wrote: > Certain you don't need UpdaterestoredBounds() in this method? > OnNativeWindowChanged will attempt to save to geometry cache stale data > otherwise I think. WindowDidResize is called repeatedly while a window is being resized or animating. During that time, we want the restored_bounds_ to contain the bounds of the window before the resize/animation started. Once the resize/animation is finished, WindowDidFinishResize will update the new bounds. It should probably call OnNativeWindowChanged as well, to ensure the new bounds are saved.
https://codereview.chromium.org/23494065/diff/22001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/22001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:876: void NativeAppWindowCocoa::WindowDidResize() { On 2013/09/24 21:34:25, jackhou1 wrote: > On 2013/09/24 16:04:15, scheib (out of office) wrote: > > Certain you don't need UpdaterestoredBounds() in this method? > > OnNativeWindowChanged will attempt to save to geometry cache stale data > > otherwise I think. > > WindowDidResize is called repeatedly while a window is being resized or > animating. During that time, we want the restored_bounds_ to contain the bounds > of the window before the resize/animation started. Once the resize/animation is > finished, WindowDidFinishResize will update the new bounds. It should probably > call OnNativeWindowChanged as well, to ensure the new bounds are saved. I got the impression from the Apple docs (i.e. https://developer.apple.com/library/mac/documentation/cocoa/reference/Applica... and https://developer.apple.com/library/mac/documentation/cocoa/conceptual/CocoaP... ) that WindowDidFinishResize is guaranteed to be sent after a resize (since 10.1) - so the geometry data should always get a refresh once the size is stable. I also got the impression that the size couldn't change between the last WindowDidResize() and the WindowDidFinishResize(), so adding OnNativeWindowChanged to WindowDidFinishResize might be redundant, and cause a re-layout for some apps -- can you check the frame size never changes between the last WindowDidResize and the WindowDidFinishResize? However, one thing to check is whether WindowDidFinishResize is sent for API-initiated resizes (i.e. non-user). A re-read of the wording in the documentation suggests: "maybe" :/. But it would be pretty crap for Cocoa to change this behaviour depending on the 'animate' argument to [NSWindow setFrame:]
https://codereview.chromium.org/23494065/diff/22001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/22001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:876: void NativeAppWindowCocoa::WindowDidResize() { > can you check the frame size never changes between > the last WindowDidResize and the WindowDidFinishResize? You're right, the frame size is the same. > However, one thing to check is whether WindowDidFinishResize is sent for > API-initiated resizes (i.e. non-user). A re-read of the wording in the > documentation suggests: "maybe" :/. But it would be pretty crap for Cocoa to > change this behaviour depending on the 'animate' argument to [NSWindow > setFrame:] Good catch, it does not call WindowDidFinishResize. The API-initiated resizes use SetBounds, so added WindowDidFinishResize to that.
scheib, how does Patch 6 look to you?
lgtm https://codereview.chromium.org/23494065/diff/57001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/57001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:585: WindowDidFinishResize(); Place explanation summary from patch discussion in a comment.
https://codereview.chromium.org/23494065/diff/57001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/23494065/diff/57001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:585: WindowDidFinishResize(); On 2013/09/30 14:11:01, scheib wrote: > Place explanation summary from patch discussion in a comment. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/23494065/68001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/23494065/68001
Message was sent while issue was closed.
Change committed as 226401 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
