200528 2019-08-07 19:31:32 -0700 Set WKWebView opaque based on drawsBackground in PageConfiguration 2019-08-15 17:43:23 -0700 1 1 1 Unclassified WebKit WebKit Misc. Safari Technology Preview Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 timothy timothy bdakin commit-queue darin thorton webkit-bug-importer wenson_hsieh oldest_to_newest 1559417 0 timothy 2019-08-07 19:31:32 -0700 Setting opaque early puts the transparent backgroundColor in the WebPageCreationParameters and avoids a page message later, and some extra transparency/layout thrash. <rdar://problem/53859380> 1559422 1 375781 timothy 2019-08-07 19:40:22 -0700 Created attachment 375781 Patch 1559615 2 375781 commit-queue 2019-08-08 12:04:47 -0700 Comment on attachment 375781 Patch Clearing flags on attachment: 375781 Committed r248436: <https://trac.webkit.org/changeset/248436> 1559616 3 commit-queue 2019-08-08 12:04:49 -0700 All reviewed patches have been landed. Closing bug. 1559629 4 375781 darin 2019-08-08 12:51:24 -0700 Comment on attachment 375781 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375781&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715 > + if (!self.opaque || !pageConfiguration->drawsBackground()) > + self.opaque = NO; Is checking the existing value of self.opaque an important optimization? 1559631 5 timothy 2019-08-08 12:55:35 -0700 (In reply to Darin Adler from comment #4) > Comment on attachment 375781 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375781&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715 > > + if (!self.opaque || !pageConfiguration->drawsBackground()) > > + self.opaque = NO; > > Is checking the existing value of self.opaque an important optimization? This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.) 1559634 6 375781 darin 2019-08-08 12:59:29 -0700 Comment on attachment 375781 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375781&action=review >>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715 >>> + self.opaque = NO; >> >> Is checking the existing value of self.opaque an important optimization? > > This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.) I don’t understand. This code skips calling the setter in the case where the getter is overridden to return NO. So how does that ensure that NO will get propagated to our setter? Sounds backwards. 1559640 7 375781 timothy 2019-08-08 13:10:53 -0700 Comment on attachment 375781 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375781&action=review >>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715 >>>> + self.opaque = NO; >>> >>> Is checking the existing value of self.opaque an important optimization? >> >> This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.) > > I don’t understand. This code skips calling the setter in the case where the getter is overridden to return NO. So how does that ensure that NO will get propagated to our setter? Sounds backwards. The check is !self.opaque || !pageConfiguration->drawsBackground(). 1559696 8 375781 darin 2019-08-08 16:01:51 -0700 Comment on attachment 375781 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375781&action=review >>>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715 >>>>> + self.opaque = NO; >>>> >>>> Is checking the existing value of self.opaque an important optimization? >>> >>> This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.) >> >> I don’t understand. This code skips calling the setter in the case where the getter is overridden to return NO. So how does that ensure that NO will get propagated to our setter? Sounds backwards. > > The check is !self.opaque || !pageConfiguration->drawsBackground(). Got it! Thanks. I was the one who had it backwards. Now makes total sense to me. I think we could follow up and make this better in three ways: 1a) Adding a comment could be helpful since the setter here is "impure" and someone might not understand that later. Maybe making the same mistake I made. // There is valuable code in the setter for "opaque" that we want to call explicitly if opaque is overridden to return NO. 1b) Move the valuable code in the setter for opaque into a different method, and call that method unconditionally instead of relying on the side effect of the setter. 2) Add a test case that overrides opaque to return NO to TestWebKitAPI to catch when this is broken by a future well-meaning programmers. 1561861 9 timothy 2019-08-15 17:43:23 -0700 (In reply to Darin Adler from comment #8) > Comment on attachment 375781 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375781&action=review > > >>>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715 > >>>>> + self.opaque = NO; > >>>> > >>>> Is checking the existing value of self.opaque an important optimization? > >>> > >>> This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.) > >> > >> I don’t understand. This code skips calling the setter in the case where the getter is overridden to return NO. So how does that ensure that NO will get propagated to our setter? Sounds backwards. > > > > The check is !self.opaque || !pageConfiguration->drawsBackground(). > > Got it! Thanks. I was the one who had it backwards. Now makes total sense to > me. I think we could follow up and make this better in three ways: > > 1a) Adding a comment could be helpful since the setter here is "impure" and > someone might not understand that later. Maybe making the same mistake I > made. > > // There is valuable code in the setter for "opaque" that we want to > call explicitly if opaque is overridden to return NO. > > 1b) Move the valuable code in the setter for opaque into a different method, > and call that method unconditionally instead of relying on the side effect > of the setter. > > 2) Add a test case that overrides opaque to return NO to TestWebKitAPI to > catch when this is broken by a future well-meaning programmers. Addressed in bug 200802. 375781 2019-08-07 19:40:22 -0700 2019-08-08 12:04:47 -0700 Patch bug-200528-20190807194021.patch text/plain 2432 timothy U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ4MzI2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGRmZTIwOTA5OTI3NGQ1NjI4 MTEzOTBlN2Y5MzBiNTdlMGE3ZjNlODYuLjBlY2JkNjhiNDQ0YmU1ZDA3ZjRjY2YxZTg3MTllOTRm MDVjZDIwZTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTktMDgtMDcgIFRpbW90aHkg SGF0Y2hlciAgPHRpbW90aHlAYXBwbGUuY29tPgorCisgICAgICAgIFNldCBXS1dlYlZpZXcgb3Bh cXVlIGJhc2VkIG9uIGRyYXdzQmFja2dyb3VuZCBpbiBQYWdlQ29uZmlndXJhdGlvbi4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIwMDUyOAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogVUlQcm9jZXNzL0FQ SS9Db2NvYS9XS1dlYlZpZXcubW06CisgICAgICAgICgtW1dLV2ViVmlldyBfaW5pdGlhbGl6ZVdp dGhDb25maWd1cmF0aW9uOl0pOiBTZXQgc2VsZi5vcGFxdWUgPSBOTyB3aGVuICFzZWxmLm9wYXF1 ZSB8fCAhcGFnZUNvbmZpZ3VyYXRpb24tPmRyYXdzQmFja2dyb3VuZCgpLgorICAgICAgICBJdCBp cyBhbG1vc3QgaW1wb3NzaWJsZSB0byBoYXZlICFzZWxmLm9wYXF1ZSBiZSBOTyBhdCB0aGlzIHBv aW50LCBzaW5jZSB3ZSBhcmUgc3RpbGwgaW5zaWRlIGluaXRXaXRoRnJhbWU6LiBBIHN1YmNsYXNz IGNvdWxkCisgICAgICAgIG92ZXJyaWRlIG9wYXF1ZSBhbmQgcmV0dXJuIE5PLCBidXQgY2hlY2tp bmcgcGFnZUNvbmZpZ3VyYXRpb24ncyBkcmF3c0JhY2tncm91bmQgaXMgYSBnb29kIGFsdGVybmF0 aXZlLgorICAgICAgICAqIFdlYlByb2Nlc3MvV2ViUGFnZS9XZWJQYWdlLmg6IFJlbW92ZSB1bnVz ZWQgbV9kcmF3c0JhY2tncm91bmQgbWVtYmVyLgorCiAyMDE5LTA4LTA2ICBKaWV3ZW4gVGFuICA8 amlld2VuX3RhbkBhcHBsZS5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgYSBidWlsZCBmaXgg YWZ0ZXIgcjI0ODMxOQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL0Nv Y29hL1dLV2ViVmlldy5tbSBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9Db2NvYS9XS1dl YlZpZXcubW0KaW5kZXggZTIzOGNiNDcyM2IyN2NiMTBiMzc4NGQxYTA4ZTM4MmM3NjMyNjA4OS4u YjBmZTBlZTJjM2UyZWMwY2RhY2VjM2QzNDhhZGM0ZjI4M2QyNzg3ZiAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL0NvY29hL1dLV2ViVmlldy5tbQorKysgYi9Tb3VyY2Uv V2ViS2l0L1VJUHJvY2Vzcy9BUEkvQ29jb2EvV0tXZWJWaWV3Lm1tCkBAIC03MTAsOCArNzEwLDkg QEAgLSAodm9pZClfaW5pdGlhbGl6ZVdpdGhDb25maWd1cmF0aW9uOihXS1dlYlZpZXdDb25maWd1 cmF0aW9uICopY29uZmlndXJhdGlvbgogCiAgICAgX3BhZ2UgPSBbX2NvbnRlbnRWaWV3IHBhZ2Vd OwogICAgIFtzZWxmIF9kaXNwYXRjaFNldERldmljZU9yaWVudGF0aW9uOmRldmljZU9yaWVudGF0 aW9uKCldOwotICAgIGlmICghc2VsZi5vcGFxdWUpCi0gICAgICAgIF9wYWdlLT5zZXRCYWNrZ3Jv dW5kQ29sb3IoV2ViQ29yZTo6Q29sb3IoV2ViQ29yZTo6Q29sb3I6OnRyYW5zcGFyZW50KSk7CisK KyAgICBpZiAoIXNlbGYub3BhcXVlIHx8ICFwYWdlQ29uZmlndXJhdGlvbi0+ZHJhd3NCYWNrZ3Jv dW5kKCkpCisgICAgICAgIHNlbGYub3BhcXVlID0gTk87CiAKICAgICBbX2NvbnRlbnRWaWV3IGxh eWVyXS5hbmNob3JQb2ludCA9IENHUG9pbnRaZXJvOwogICAgIFtfY29udGVudFZpZXcgc2V0RnJh bWU6Ym91bmRzXTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvV2ViUHJvY2Vzcy9XZWJQYWdl L1dlYlBhZ2UuaCBiL1NvdXJjZS9XZWJLaXQvV2ViUHJvY2Vzcy9XZWJQYWdlL1dlYlBhZ2UuaApp bmRleCAyMTgyNmI5NzhhNzY5NGNmM2ZlMTI3ZmFmN2M2NTJmZWE1NDAwNzA1Li4yZDEyODljODBk ZjgyMmFhMmY5ZDA4NTI1ZmFmOWY1NGU5ZDdhNjI3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L1dlYlByb2Nlc3MvV2ViUGFnZS9XZWJQYWdlLmgKKysrIGIvU291cmNlL1dlYktpdC9XZWJQcm9j ZXNzL1dlYlBhZ2UvV2ViUGFnZS5oCkBAIC0xNjMxLDcgKzE2MzEsNiBAQCBwcml2YXRlOgogICAg IEhhc2hNYXA8dWludDY0X3QsIFJlZlB0cjxXZWJDb3JlOjpUZXh0Q2hlY2tpbmdSZXF1ZXN0Pj4g bV9wZW5kaW5nVGV4dENoZWNraW5nUmVxdWVzdE1hcDsKIAogICAgIGJvb2wgbV91c2VGaXhlZExh eW91dCB7IGZhbHNlIH07Ci0gICAgYm9vbCBtX2RyYXdzQmFja2dyb3VuZCB7IHRydWUgfTsKIAog ICAgIFdlYkNvcmU6OkNvbG9yIG1fdW5kZXJsYXlDb2xvcjsKIAo=