108406 2013-01-30 16:59:53 -0800 Mac: Cmd-w should close full screen window. 2013-03-11 14:53:30 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jer.noble jer.noble oldest_to_newest 820829 0 jer.noble 2013-01-30 16:59:53 -0800 Mac: Cmd-w should close full screen window. 820835 1 185629 jer.noble 2013-01-30 17:06:22 -0800 Created attachment 185629 Patch 841066 2 185629 darin 2013-02-25 10:09:33 -0800 Comment on attachment 185629 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185629&action=review Patch is OK. I’d like to see a clearer comment. > Source/WebKit2/ChangeLog:15 > +2013-01-30 Jer Noble <jer.noble@apple.com> > + > + Full screen mode should not exit when application resigns active state. > + https://bugs.webkit.org/show_bug.cgi?id=106129 > + > + Reviewed by NOBODY (OOPS!). > + > + Allow the user to close the full screen window with Cmd-w by making the full screen window > + closable, and by intercepting performClose:. > + > + * UIProcess/mac/WKFullScreenWindowController.mm: > + (-[WKFullScreenWindowController init]): Create a closable full screen window. > + (-[WKFullScreenWindowController performClose:]): When we receive a close request in full screen mode, > + animate out of full screen. > + You put this patch into a separate bug. Please don’t land this extra change log entry. > Source/WebKit/mac/WebView/WebFullScreenController.mm:99 > + NSWindow *window = [[WebCoreFullScreenWindow alloc] initWithContentRect:NSZeroRect styleMask:NSClosableWindowMask backing:NSBackingStoreBuffered defer:NO]; Why no longer borderless? Won’t we potentially see the borders overlapping onto adjacent screens? > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:404 > +- (BOOL)performClose:(id)sender > +{ > + // Don't immediately close. Rather, animate out of full screen mode first. > + if (_isFullScreen) { > + [self cancelOperation:sender]; > + return false; > + } > + return true; > +} Comment says what but not why. Further it does not make it clear why it’s right to return NO here rather than YES when we start the “animate out of full screen” process. 841091 3 jer.noble 2013-02-25 10:31:38 -0800 (In reply to comment #2) > (From update of attachment 185629 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185629&action=review > > Patch is OK. I’d like to see a clearer comment. Okay, I"ll clean things up before landing. > > Source/WebKit2/ChangeLog:15 > > +2013-01-30 Jer Noble <jer.noble@apple.com> > > + > > + Full screen mode should not exit when application resigns active state. > > + https://bugs.webkit.org/show_bug.cgi?id=106129 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + Allow the user to close the full screen window with Cmd-w by making the full screen window > > + closable, and by intercepting performClose:. > > + > > + * UIProcess/mac/WKFullScreenWindowController.mm: > > + (-[WKFullScreenWindowController init]): Create a closable full screen window. > > + (-[WKFullScreenWindowController performClose:]): When we receive a close request in full screen mode, > > + animate out of full screen. > > + > > You put this patch into a separate bug. Please don’t land this extra change log entry. Whoops. Will do. > > Source/WebKit/mac/WebView/WebFullScreenController.mm:99 > > + NSWindow *window = [[WebCoreFullScreenWindow alloc] initWithContentRect:NSZeroRect styleMask:NSClosableWindowMask backing:NSBackingStoreBuffered defer:NO]; > > Why no longer borderless? Won’t we potentially see the borders overlapping onto adjacent screens? Borderless is defined as a 0 in the bit mask, so ORing it together with Closable is a no-op. But since it still lacks the Titled mask, it won't have a border. > > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:404 > > +- (BOOL)performClose:(id)sender > > +{ > > + // Don't immediately close. Rather, animate out of full screen mode first. > > + if (_isFullScreen) { > > + [self cancelOperation:sender]; > > + return false; > > + } > > + return true; > > +} > > Comment says what but not why. Further it does not make it clear why it’s right to return NO here rather than YES when we start the “animate out of full screen” process. Will do. 852759 4 jer.noble 2013-03-11 14:53:30 -0700 Committed r145409: <http://trac.webkit.org/changeset/145409> 185629 2013-01-30 17:06:22 -0800 2013-02-25 10:09:33 -0800 Patch bug-108406-20130130170306.patch text/plain 6306 jer.noble U3VidmVyc2lvbiBSZXZpc2lvbjogMTQxMzMzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNGJhMjIwYmZkNjljN2I0 ZGE1MDEyNzI0ZGIyNzY1NzIyMGM0ZDQ0Zi4uNDViYjk0NmM2MGYwOGFmOGZiMjQ5MDMwMWI4NGI1 ODQxZDc1MzAxZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTAxLTMwICBKZXIg Tm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgorCisgICAgICAgIE1hYzogQ21kLXcgc2hvdWxk IGNsb3NlIGZ1bGwgc2NyZWVuIHdpbmRvdy4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTEwODQwNgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFBhc3MgcGVyZm9ybUNsb3NlOiByZXF1ZXN0cyBvbiB0byB0aGUg b3duaW5nIHdpbmRvdyBjb250cm9sbGVyLgorCisgICAgICAgICogcGxhdGZvcm0vbWFjL1dlYkNv cmVGdWxsU2NyZWVuV2luZG93Lm1tOgorICAgICAgICAoLVtXZWJDb3JlRnVsbFNjcmVlbldpbmRv dyBwZXJmb3JtQ2xvc2U6XSk6CisKIDIwMTMtMDEtMzAgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hv cnRvbkBhcHBsZS5jb20+CiAKICAgICAgICAgW21hY10gSW1hZ2VCdWZmZXIgc2hvdWxkIGNyZWF0 ZSBhY2NlbGVyYXRlZCBidWZmZXJzIGZvciBzbWFsbCBjYW52YXNlcywgYnV0IHdlIHNob3VsZG4n dCBmb3JjZSB0aGVtIHRvIGNyZWF0ZSBjb21wb3NpdGluZyBsYXllcnMKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJLaXQvbWFjL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvbWFjL0NoYW5nZUxvZwpp bmRleCA3MWUyYzdiOTQ0MjU1OTcyNzliNzM0NzdkMWU0NWFiMGVhOTg0YjExLi42MDg2MTRlMzg2 ZmVjNzZhNmZmODdiOTVmOTRiMTNhNTBmNDNmMjE4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L21hYy9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9tYWMvQ2hhbmdlTG9nCkBAIC0xLDMg KzEsMTggQEAKKzIwMTMtMDEtMzAgIEplciBOb2JsZSAgPGplci5ub2JsZUBhcHBsZS5jb20+CisK KyAgICAgICAgTWFjOiBDbWQtdyBzaG91bGQgY2xvc2UgZnVsbCBzY3JlZW4gd2luZG93LgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA4NDA2CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWxsb3cgdGhlIHVz ZXIgdG8gY2xvc2UgdGhlIGZ1bGwgc2NyZWVuIHdpbmRvdyB3aXRoIENtZC13IGJ5IG1ha2luZyB0 aGUgZnVsbCBzY3JlZW4gd2luZG93CisgICAgICAgIGNsb3NhYmxlLCBhbmQgYnkgaW50ZXJjZXB0 aW5nIHBlcmZvcm1DbG9zZTouCisKKyAgICAgICAgKiBXZWJWaWV3L1dlYkZ1bGxTY3JlZW5Db250 cm9sbGVyLm1tOgorICAgICAgICAoLVtXZWJGdWxsU2NyZWVuQ29udHJvbGxlciBpbml0XSk6IENy ZWF0ZSBhIGNsb3NhYmxlIGZ1bGwgc2NyZWVuIHdpbmRvdy4KKyAgICAgICAgKC1bV2ViRnVsbFNj cmVlbkNvbnRyb2xsZXIgcGVyZm9ybUNsb3NlOl0pOiBXaGVuIHdlIHJlY2VpdmUgYSBjbG9zZSBy ZXF1ZXN0IGluIGZ1bGwgc2NyZWVuIG1vZGUsCisgICAgICAgICAgICBhbmltYXRlIG91dCBvZiBm dWxsIHNjcmVlbi4KKwogMjAxMy0wMS0yOSAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJv dEBnbWFpbC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjE0MDk4My4K ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA0ZmI0MTgzOGYyMGUxNzQxMDMyYjAzNDExNTY1ZWZkMzIyOTMzOGI1Li4w NjE4MWRjMjY2YzQ2NDg1MTIwMGEwYmIyMDJiMTdmYzE1YmVlNDFiIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViS2l0Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMTggQEAKKzIwMTMtMDEtMzAgIEplciBOb2JsZSAgPGplci5ub2JsZUBhcHBsZS5jb20+ CisKKyAgICAgICAgRnVsbCBzY3JlZW4gbW9kZSBzaG91bGQgbm90IGV4aXQgd2hlbiBhcHBsaWNh dGlvbiByZXNpZ25zIGFjdGl2ZSBzdGF0ZS4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTEwNjEyOQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIEFsbG93IHRoZSB1c2VyIHRvIGNsb3NlIHRoZSBmdWxsIHNjcmVl biB3aW5kb3cgd2l0aCBDbWQtdyBieSBtYWtpbmcgdGhlIGZ1bGwgc2NyZWVuIHdpbmRvdworICAg ICAgICBjbG9zYWJsZSwgYW5kIGJ5IGludGVyY2VwdGluZyBwZXJmb3JtQ2xvc2U6LgorCisgICAg ICAgICogVUlQcm9jZXNzL21hYy9XS0Z1bGxTY3JlZW5XaW5kb3dDb250cm9sbGVyLm1tOgorICAg ICAgICAoLVtXS0Z1bGxTY3JlZW5XaW5kb3dDb250cm9sbGVyIGluaXRdKTogQ3JlYXRlIGEgY2xv c2FibGUgZnVsbCBzY3JlZW4gd2luZG93LgorICAgICAgICAoLVtXS0Z1bGxTY3JlZW5XaW5kb3dD b250cm9sbGVyIHBlcmZvcm1DbG9zZTpdKTogV2hlbiB3ZSByZWNlaXZlIGEgY2xvc2UgcmVxdWVz dCBpbiBmdWxsIHNjcmVlbiBtb2RlLAorICAgICAgICAgICAgYW5pbWF0ZSBvdXQgb2YgZnVsbCBz Y3JlZW4uCisKIDIwMTMtMDEtMDQgIEplciBOb2JsZSAgPGplci5ub2JsZUBhcHBsZS5jb20+CiAK ICAgICAgICAgRnVsbCBzY3JlZW4gbW9kZSBzaG91bGQgbm90IGV4aXQgd2hlbiBhcHBsaWNhdGlv biByZXNpZ25zIGFjdGl2ZSBzdGF0ZS4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL21hYy9XZWJDb3JlRnVsbFNjcmVlbldpbmRvdy5tbSBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL21hYy9XZWJDb3JlRnVsbFNjcmVlbldpbmRvdy5tbQppbmRleCBjYzZlYTM0ZDFlZmMwNGZh OTA5NjRkMzI5NWJkZDNkYWMzY2I3ZDQ2Li40Zjk2MjQ1MTQ5MmUwYjE4YjgyMmIxZTI2YTRjMDlk NDE2MmViNjJhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9tYWMvV2ViQ29y ZUZ1bGxTY3JlZW5XaW5kb3cubW0KKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbWFjL1dl YkNvcmVGdWxsU2NyZWVuV2luZG93Lm1tCkBAIC02Nyw2ICs2NywxMSBAQAogewogICAgIFtbc2Vs ZiB3aW5kb3dDb250cm9sbGVyXSBjYW5jZWxPcGVyYXRpb246c2VuZGVyXTsKIH0KKworLSAodm9p ZClwZXJmb3JtQ2xvc2U6KGlkKXNlbmRlcgoreworICAgIFtbc2VsZiB3aW5kb3dDb250cm9sbGVy XSBwZXJmb3JtQ2xvc2U6c2VuZGVyXTsKK30KIEBlbmQKIAogI2VuZGlmIC8vIEVOQUJMRShGVUxM U0NSRUVOX0FQSSkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvbWFjL1dlYlZpZXcvV2ViRnVs bFNjcmVlbkNvbnRyb2xsZXIubW0gYi9Tb3VyY2UvV2ViS2l0L21hYy9XZWJWaWV3L1dlYkZ1bGxT Y3JlZW5Db250cm9sbGVyLm1tCmluZGV4IGI0ZDQ0Y2FjMDMxNzNhOWMzYzlmNDE2OGZlYWU4NjVm NTk3M2U3NGUuLjRmZTEzMzBlZmU2NTgzYTdkNWM0YjJjZGU5NzNlNTAyNGE2NmM4MzMgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQvbWFjL1dlYlZpZXcvV2ViRnVsbFNjcmVlbkNvbnRyb2xsZXIu bW0KKysrIGIvU291cmNlL1dlYktpdC9tYWMvV2ViVmlldy9XZWJGdWxsU2NyZWVuQ29udHJvbGxl ci5tbQpAQCAtOTYsNyArOTYsNyBAQCBzdGF0aWMgTlNSZWN0IGNvbnZlcnRSZWN0VG9TY3JlZW4o TlNXaW5kb3cgKndpbmRvdywgTlNSZWN0IHJlY3QpCiAtIChpZClpbml0CiB7CiAgICAgLy8gRG8g bm90IGRlZmVyIHdpbmRvdyBjcmVhdGlvbiwgdG8gbWFrZSBzdXJlIC13aW5kb3dOdW1iZXIgaXMg Y3JlYXRlZCAobmVlZGVkIGJ5IFdlYldpbmRvd1NjYWxlQW5pbWF0aW9uKS4KLSAgICBOU1dpbmRv dyAqd2luZG93ID0gW1tXZWJDb3JlRnVsbFNjcmVlbldpbmRvdyBhbGxvY10gaW5pdFdpdGhDb250 ZW50UmVjdDpOU1plcm9SZWN0IHN0eWxlTWFzazpOU0JvcmRlcmxlc3NXaW5kb3dNYXNrIGJhY2tp bmc6TlNCYWNraW5nU3RvcmVCdWZmZXJlZCBkZWZlcjpOT107CisgICAgTlNXaW5kb3cgKndpbmRv dyA9IFtbV2ViQ29yZUZ1bGxTY3JlZW5XaW5kb3cgYWxsb2NdIGluaXRXaXRoQ29udGVudFJlY3Q6 TlNaZXJvUmVjdCBzdHlsZU1hc2s6TlNDbG9zYWJsZVdpbmRvd01hc2sgYmFja2luZzpOU0JhY2tp bmdTdG9yZUJ1ZmZlcmVkIGRlZmVyOk5PXTsKICAgICBzZWxmID0gW3N1cGVyIGluaXRXaXRoV2lu ZG93OndpbmRvd107CiAgICAgW3dpbmRvdyByZWxlYXNlXTsKICAgICBpZiAoIXNlbGYpCkBAIC0z ODYsNiArMzg2LDE2IEBAIHN0YXRpYyBOU1JlY3QgY29udmVydFJlY3RUb1NjcmVlbihOU1dpbmRv dyAqd2luZG93LCBOU1JlY3QgcmVjdCkKICAgICBOU0VuYWJsZVNjcmVlblVwZGF0ZXMoKTsKIH0K IAorLSAoQk9PTClwZXJmb3JtQ2xvc2U6KGlkKXNlbmRlcgoreworICAgIC8vIERvbid0IGltbWVk aWF0ZWx5IGNsb3NlLiAgUmF0aGVyLCBhbmltYXRlIG91dCBvZiBmdWxsIHNjcmVlbiBtb2RlIGZp cnN0LgorICAgIGlmIChfaXNGdWxsU2NyZWVuKSB7CisgICAgICAgIFtzZWxmIGNhbmNlbE9wZXJh dGlvbjpzZW5kZXJdOworICAgICAgICByZXR1cm4gZmFsc2U7CisgICAgfQorICAgIHJldHVybiB0 cnVlOworfQorCiAtICh2b2lkKWNsb3NlCiB7CiAgICAgLy8gV2UgYXJlIGJlaW5nIGFza2VkIHRv IGNsb3NlIHJhcGlkbHksIG1vc3QgbGlrZWx5IGJlY2F1c2UgdGhlIHBhZ2UgCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvbWFjL1dLRnVsbFNjcmVlbldpbmRvd0NvbnRyb2xs ZXIubW0gYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvbWFjL1dLRnVsbFNjcmVlbldpbmRvd0Nv bnRyb2xsZXIubW0KaW5kZXggMGY0ZThjYTdkMTc5YTM1YTU5YWVhYTZhNTU1ZjQ2NDE0YmFlYzY0 MC4uY2FjZjdjMTk0OTRhOTc3MjExMzY5N2Q2ZmVjMzEwYzlkMjQ4MmM5MyAxMDA2NDQKLS0tIGEv U291cmNlL1dlYktpdDIvVUlQcm9jZXNzL21hYy9XS0Z1bGxTY3JlZW5XaW5kb3dDb250cm9sbGVy Lm1tCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9tYWMvV0tGdWxsU2NyZWVuV2luZG93 Q29udHJvbGxlci5tbQpAQCAtODMsNyArODMsNyBAQCBzdGF0aWMgTlNSZWN0IGNvbnZlcnRSZWN0 VG9TY3JlZW4oTlNXaW5kb3cgKndpbmRvdywgTlNSZWN0IHJlY3QpCiAjcHJhZ21hIG1hcmsgSW5p dGlhbGl6YXRpb24KIC0gKGlkKWluaXQKIHsKLSAgICBSZXRhaW5QdHI8TlNXaW5kb3c+IHdpbmRv dyA9IGFkb3B0TlMoW1tXZWJDb3JlRnVsbFNjcmVlbldpbmRvdyBhbGxvY10gaW5pdFdpdGhDb250 ZW50UmVjdDpOU1plcm9SZWN0IHN0eWxlTWFzazpOU0JvcmRlcmxlc3NXaW5kb3dNYXNrIGJhY2tp bmc6TlNCYWNraW5nU3RvcmVCdWZmZXJlZCBkZWZlcjpOT10pOworICAgIFJldGFpblB0cjxOU1dp bmRvdz4gd2luZG93ID0gYWRvcHROUyhbW1dlYkNvcmVGdWxsU2NyZWVuV2luZG93IGFsbG9jXSBp bml0V2l0aENvbnRlbnRSZWN0Ok5TWmVyb1JlY3Qgc3R5bGVNYXNrOk5TQ2xvc2FibGVXaW5kb3dN YXNrIGJhY2tpbmc6TlNCYWNraW5nU3RvcmVCdWZmZXJlZCBkZWZlcjpOT10pOwogICAgIHNlbGYg PSBbc3VwZXIgaW5pdFdpdGhXaW5kb3c6d2luZG93LmdldCgpXTsKICAgICBpZiAoIXNlbGYpCiAg ICAgICAgIHJldHVybiBuaWw7CkBAIC0zOTMsNiArMzkzLDE2IEBAIHN0YXRpYyB2b2lkIGNvbXBs ZXRlRmluaXNoRXhpdEZ1bGxTY3JlZW5BbmltYXRpb25BZnRlclJlcGFpbnQoV0tFcnJvclJlZiwg dm9pZCoKICAgICBbKFdLRnVsbFNjcmVlbldpbmRvd0NvbnRyb2xsZXIqKV9zZWxmIGNvbXBsZXRl RmluaXNoRXhpdEZ1bGxTY3JlZW5BbmltYXRpb25BZnRlclJlcGFpbnRdOwogfQogCistIChCT09M KXBlcmZvcm1DbG9zZTooaWQpc2VuZGVyCit7CisgICAgLy8gRG9uJ3QgaW1tZWRpYXRlbHkgY2xv c2UuICBSYXRoZXIsIGFuaW1hdGUgb3V0IG9mIGZ1bGwgc2NyZWVuIG1vZGUgZmlyc3QuCisgICAg aWYgKF9pc0Z1bGxTY3JlZW4pIHsKKyAgICAgICAgW3NlbGYgY2FuY2VsT3BlcmF0aW9uOnNlbmRl cl07CisgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICB9CisgICAgcmV0dXJuIHRydWU7Cit9CisK IC0gKHZvaWQpY2xvc2UKIHsKICAgICAvLyBXZSBhcmUgYmVpbmcgYXNrZWQgdG8gY2xvc2UgcmFw aWRseSwgbW9zdCBsaWtlbHkgYmVjYXVzZSB0aGUgcGFnZSAK