204721 2019-11-30 14:54:15 -0800 Disable LocalCurrentGraphicsContext when doing DisplayList recording 2019-12-01 19:33:19 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 thorton thorton commit-queue dino graouts jonlee sam simon.fraser webkit-bug-importer wenson_hsieh oldest_to_newest 1594347 0 thorton 2019-11-30 14:54:15 -0800 Disable LocalCurrentGraphicsContext when doing DisplayList recording 1594348 1 384549 thorton 2019-11-30 14:54:25 -0800 Created attachment 384549 Patch 1594349 2 thorton 2019-11-30 14:55:25 -0800 Also fixed some deprecated API usage while I was there 1594352 3 384549 thorton 2019-11-30 15:20:13 -0800 Comment on attachment 384549 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384549&action=review > Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36 > + m_savedNSGraphicsContext = 0; Uh... don't know why 0, I just copied from below. I'll make it `nil`. 1594353 4 thorton 2019-11-30 15:20:24 -0800 Also not sure why this doesn't use RetainPtr 1594425 5 384549 simon.fraser 2019-12-01 08:40:17 -0800 Comment on attachment 384549 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384549&action=review >> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36 >> + m_savedNSGraphicsContext = 0; > > Uh... don't know why 0, I just copied from below. I'll make it `nil`. Should probably WTFLogAlways() or LOG(DisplayLists, ) here to tell us we've done something wrong. 1594441 6 thorton 2019-12-01 14:45:07 -0800 We’re not doing anything wrong. Ideally we would log when someone /reads/ it, but that’s code outside of our control. 1594442 7 thorton 2019-12-01 14:46:03 -0800 Unless you think we should change every callsite to not create a LocalCurrentGraphicsContext in the display list case, but that seems like too much. 1594451 8 384549 sam 2019-12-01 15:46:13 -0800 Comment on attachment 384549 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384549&action=review >>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36 >>> + m_savedNSGraphicsContext = 0; >> >> Uh... don't know why 0, I just copied from below. I'll make it `nil`. > > Should probably WTFLogAlways() or LOG(DisplayLists, ) here to tell us we've done something wrong. Wouldn't we want to ASSERT() when using DisplayList, so as to not miss subtle issues? If we are going to have to implement functionality that depends on a global graphics context another way, is there really any point in supporting both ways? 1594453 9 sam 2019-12-01 15:48:31 -0800 (In reply to Tim Horton from comment #7) > Unless you think we should change every callsite to not create a > LocalCurrentGraphicsContext in the display list case, but that seems like > too much. What is the point(In reply to Sam Weinig from comment #8) > Comment on attachment 384549 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384549&action=review > > >>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36 > >>> + m_savedNSGraphicsContext = 0; > >> > >> Uh... don't know why 0, I just copied from below. I'll make it `nil`. > > > > Should probably WTFLogAlways() or LOG(DisplayLists, ) here to tell us we've done something wrong. > > Wouldn't we want to ASSERT() when using DisplayList, so as to not miss > subtle issues? If we are going to have to implement functionality that > depends on a global graphics context another way, is there really any point > in supporting both ways? Or rather, what is the point of keeping LocalCurrentGraphicsContext if we will have to find alternate implementations for anything that uses it? 1594481 10 thorton 2019-12-01 19:06:50 -0800 (In reply to Sam Weinig from comment #8) > Comment on attachment 384549 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384549&action=review > > >>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36 > >>> + m_savedNSGraphicsContext = 0; > >> > >> Uh... don't know why 0, I just copied from below. I'll make it `nil`. > > > > Should probably WTFLogAlways() or LOG(DisplayLists, ) here to tell us we've done something wrong. > > Wouldn't we want to ASSERT() when using DisplayList, so as to not miss > subtle issues? If we are going to have to implement functionality that > depends on a global graphics context another way, is there really any point > in supporting both ways? For the time being, we definitely need to support both ways, since we don't have a solution for things (e.g. form controls) that depend on the global context. Later, I agree! But for now, we can't just rip out the LocalCurrentGraphicsContext invocations, nor can we leave them in unconditionally (because the DL GraphicsContext asserts when you try to get the platformContext). Since I can't do the ideal thing (log when the bad thing actually happens -- when we read), I'll do the thing that smfr suggested so that we don't forget to address this later. 1594482 11 384590 thorton 2019-12-01 19:10:25 -0800 Created attachment 384590 Patch 1594490 12 384590 commit-queue 2019-12-01 19:32:33 -0800 Comment on attachment 384590 Patch Clearing flags on attachment: 384590 Committed r252977: <https://trac.webkit.org/changeset/252977> 1594491 13 commit-queue 2019-12-01 19:32:35 -0800 All reviewed patches have been landed. Closing bug. 1594492 14 webkit-bug-importer 2019-12-01 19:33:19 -0800 <rdar://problem/57548595> 384549 2019-11-30 14:54:25 -0800 2019-12-01 19:10:24 -0800 Patch bug-204721-20191130145425.patch text/plain 2372 thorton U3VidmVyc2lvbiBSZXZpc2lvbjogMjUyOTM2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjZlNjQ1MTZjOGI4NzEz ZmExMWM1ODVjZDE1YjdkNjcxMTA5YzMyOC4uNzQzY2JhMjgzNjhlYjFiMGZkNWJmYjNiZDZjZmYx MDQyYWM5OWFlNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDE5LTExLTMwICBUaW0g SG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgorCisgICAgICAgIERpc2FibGUgTG9j YWxDdXJyZW50R3JhcGhpY3NDb250ZXh0IHdoZW4gZG9pbmcgRGlzcGxheUxpc3QgcmVjb3JkaW5n CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDQ3MjEK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBsYXRm b3JtL21hYy9Mb2NhbEN1cnJlbnRHcmFwaGljc0NvbnRleHQubW06CisgICAgICAgIChXZWJDb3Jl OjpMb2NhbEN1cnJlbnRHcmFwaGljc0NvbnRleHQ6OkxvY2FsQ3VycmVudEdyYXBoaWNzQ29udGV4 dCk6CisgICAgICAgIFNpbmNlIHdlIGRvbid0IGhhdmUgYSBwbGF0Zm9ybSBjb250ZXh0LCB3ZSBj YW4ndCBzZXQgdGhlIGdsb2JhbAorICAgICAgICBncmFwaGljcyBjb250ZXh0LiBUaGlzIG1lYW5z IHRoYXQgYW55dGhpbmcgZGVwZW5kaW5nIG9uIHRoaXMgZG93bnN0cmVhbQorICAgICAgICB3aWxs IGJyZWFrIGFuZCBuZWVkIHRvIGJlIGltcGxlbWVudGVkIGEgZGlmZmVyZW50IHdheSBpbgorICAg ICAgICB0aGUgRGlzcGxheUxpc3QgY2FzZS4KKwogMjAxOS0xMS0yOCAgRnVqaWkgSGlyb25vcmkg IDxIaXJvbm9yaS5GdWppaUBzb255LmNvbT4KIAogICAgICAgICBSZW1vdmUgRU5BQkxFX0tFWUJP QVJEX0NPREVfQVRUUklCVVRFIGFuZCBFTkFCTEVfS0VZQk9BUkRfS0VZX0FUVFJJQlVURSBtYWNy b3MKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL21hYy9Mb2NhbEN1cnJlbnRH cmFwaGljc0NvbnRleHQubW0gYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9tYWMvTG9jYWxDdXJy ZW50R3JhcGhpY3NDb250ZXh0Lm1tCmluZGV4IDQxZTQyNWJjZjkyNmVhNmU2MjljMDEwZDM3N2Iz NTAwNTYwMzFmODkuLmM5OWI4OTkxNzQyMDdlMDgzZTY0MTJkODc0MGM2ZTViODlhZWYxMTEgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL21hYy9Mb2NhbEN1cnJlbnRHcmFwaGlj c0NvbnRleHQubW0KKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbWFjL0xvY2FsQ3VycmVu dEdyYXBoaWNzQ29udGV4dC5tbQpAQCAtMzIsMTYgKzMyLDE5IEBAIExvY2FsQ3VycmVudEdyYXBo aWNzQ29udGV4dDo6TG9jYWxDdXJyZW50R3JhcGhpY3NDb250ZXh0KEdyYXBoaWNzQ29udGV4dCYg Z3JhcGhpCiB7CiAgICAgZ3JhcGhpY3NDb250ZXh0LnNhdmUoKTsKIAotICAgIEFMTE9XX0RFUFJF Q0FURURfREVDTEFSQVRJT05TX0JFR0lOCisgICAgaWYgKCFncmFwaGljc0NvbnRleHQuaGFzUGxh dGZvcm1Db250ZXh0KCkpIHsKKyAgICAgICAgbV9zYXZlZE5TR3JhcGhpY3NDb250ZXh0ID0gMDsK KyAgICAgICAgcmV0dXJuOworICAgIH0KKwogICAgIENHQ29udGV4dFJlZiBjZ0NvbnRleHQgPSB0 aGlzLT5jZ0NvbnRleHQoKTsKLSAgICBpZiAoY2dDb250ZXh0ID09IFtbTlNHcmFwaGljc0NvbnRl eHQgY3VycmVudENvbnRleHRdIGdyYXBoaWNzUG9ydF0pIHsKKyAgICBpZiAoY2dDb250ZXh0ID09 IFtbTlNHcmFwaGljc0NvbnRleHQgY3VycmVudENvbnRleHRdIENHQ29udGV4dF0pIHsKICAgICAg ICAgbV9zYXZlZE5TR3JhcGhpY3NDb250ZXh0ID0gMDsKICAgICAgICAgcmV0dXJuOwogICAgIH0K IAogICAgIG1fc2F2ZWROU0dyYXBoaWNzQ29udGV4dCA9IFtbTlNHcmFwaGljc0NvbnRleHQgY3Vy cmVudENvbnRleHRdIHJldGFpbl07Ci0gICAgTlNHcmFwaGljc0NvbnRleHQqIG5ld0NvbnRleHQg PSBbTlNHcmFwaGljc0NvbnRleHQgZ3JhcGhpY3NDb250ZXh0V2l0aEdyYXBoaWNzUG9ydDpjZ0Nv bnRleHQgZmxpcHBlZDpZRVNdOwotICAgIEFMTE9XX0RFUFJFQ0FURURfREVDTEFSQVRJT05TX0VO RAorICAgIE5TR3JhcGhpY3NDb250ZXh0KiBuZXdDb250ZXh0ID0gW05TR3JhcGhpY3NDb250ZXh0 IGdyYXBoaWNzQ29udGV4dFdpdGhDR0NvbnRleHQ6Y2dDb250ZXh0IGZsaXBwZWQ6WUVTXTsKICAg ICBbTlNHcmFwaGljc0NvbnRleHQgc2V0Q3VycmVudENvbnRleHQ6bmV3Q29udGV4dF07CiAgICAg bV9kaWRTZXRHcmFwaGljc0NvbnRleHQgPSB0cnVlOwogfQo= 384590 2019-12-01 19:10:25 -0800 2019-12-01 19:32:33 -0800 Patch bug-204721-20191201191025.patch text/plain 2565 thorton U3VidmVyc2lvbiBSZXZpc2lvbjogMjUyOTYxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZGZhOTQzODEyNDE3NmJk NzI3YzY1OTgwZjFhZmQ4OGNiY2VhMTVmNC4uNzQ3Y2UyYTg2YzBkYzU5Njc2NzQxYmIyNzM5OWUz MDlmYTI1NmRiMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDE5LTEyLTAxICBUaW0g SG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgorCisgICAgICAgIERpc2FibGUgTG9j YWxDdXJyZW50R3JhcGhpY3NDb250ZXh0IHdoZW4gZG9pbmcgRGlzcGxheUxpc3QgcmVjb3JkaW5n CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDQ3MjEK KworICAgICAgICBSZXZpZXdlZCBieSBTaW1vbiBGcmFzZXIuCisKKyAgICAgICAgKiBwbGF0Zm9y bS9tYWMvTG9jYWxDdXJyZW50R3JhcGhpY3NDb250ZXh0Lm1tOgorICAgICAgICAoV2ViQ29yZTo6 TG9jYWxDdXJyZW50R3JhcGhpY3NDb250ZXh0OjpMb2NhbEN1cnJlbnRHcmFwaGljc0NvbnRleHQp OgorICAgICAgICBTaW5jZSB3ZSBkb24ndCBoYXZlIGEgcGxhdGZvcm0gY29udGV4dCwgd2UgY2Fu J3Qgc2V0IHRoZSBnbG9iYWwKKyAgICAgICAgZ3JhcGhpY3MgY29udGV4dC4gVGhpcyBtZWFucyB0 aGF0IGFueXRoaW5nIGRlcGVuZGluZyBvbiB0aGlzIGRvd25zdHJlYW0KKyAgICAgICAgd2lsbCBi cmVhayBhbmQgbmVlZCB0byBiZSBpbXBsZW1lbnRlZCBhIGRpZmZlcmVudCB3YXkgaW4KKyAgICAg ICAgdGhlIERpc3BsYXlMaXN0IGNhc2UuCisKIDIwMTktMTEtMzAgIHlvdWVubiBmYWJsZXQgIDx5 b3Vlbm5AYXBwbGUuY29tPgogCiAgICAgICAgIFNpbXBsaWZ5IENvcmVBdWRpb0NhcHR1cmVTb3Vy Y2Ugc3VzcGVuc2lvbiBsb2dpYwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v bWFjL0xvY2FsQ3VycmVudEdyYXBoaWNzQ29udGV4dC5tbSBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL21hYy9Mb2NhbEN1cnJlbnRHcmFwaGljc0NvbnRleHQubW0KaW5kZXggNDFlNDI1YmNmOTI2 ZWE2ZTYyOWMwMTBkMzc3YjM1MDA1NjAzMWY4OS4uOTgwODQ0YmQ2OGZhMTA1NTMxZWQ4OTg5ZjQz ZGIwZjYyYzIzMWU3MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbWFjL0xv Y2FsQ3VycmVudEdyYXBoaWNzQ29udGV4dC5tbQorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS9tYWMvTG9jYWxDdXJyZW50R3JhcGhpY3NDb250ZXh0Lm1tCkBAIC0zMiwxNiArMzIsMjAgQEAg TG9jYWxDdXJyZW50R3JhcGhpY3NDb250ZXh0OjpMb2NhbEN1cnJlbnRHcmFwaGljc0NvbnRleHQo R3JhcGhpY3NDb250ZXh0JiBncmFwaGkKIHsKICAgICBncmFwaGljc0NvbnRleHQuc2F2ZSgpOwog Ci0gICAgQUxMT1dfREVQUkVDQVRFRF9ERUNMQVJBVElPTlNfQkVHSU4KKyAgICBpZiAoIWdyYXBo aWNzQ29udGV4dC5oYXNQbGF0Zm9ybUNvbnRleHQoKSkgeworICAgICAgICBXVEZMb2dBbHdheXMo IkxvY2FsQ3VycmVudEdyYXBoaWNzQ29udGV4dCBpcyBub3Qgc2V0dGluZyB0aGUgZ2xvYmFsIGNv bnRleHQgYmVjYXVzZSB0aGUgcHJvdmlkZWQgR3JhcGhpY3NDb250ZXh0IGRvZXMgbm90IGhhdmUg YSBwbGF0Zm9ybSBjb250ZXh0IChsaWtlbHkgZGlzcGxheSBsaXN0IHJlY29yZGluZykiKTsKKyAg ICAgICAgbV9zYXZlZE5TR3JhcGhpY3NDb250ZXh0ID0gbmlsOworICAgICAgICByZXR1cm47Cisg ICAgfQorCiAgICAgQ0dDb250ZXh0UmVmIGNnQ29udGV4dCA9IHRoaXMtPmNnQ29udGV4dCgpOwot ICAgIGlmIChjZ0NvbnRleHQgPT0gW1tOU0dyYXBoaWNzQ29udGV4dCBjdXJyZW50Q29udGV4dF0g Z3JhcGhpY3NQb3J0XSkgewotICAgICAgICBtX3NhdmVkTlNHcmFwaGljc0NvbnRleHQgPSAwOwor ICAgIGlmIChjZ0NvbnRleHQgPT0gW1tOU0dyYXBoaWNzQ29udGV4dCBjdXJyZW50Q29udGV4dF0g Q0dDb250ZXh0XSkgeworICAgICAgICBtX3NhdmVkTlNHcmFwaGljc0NvbnRleHQgPSBuaWw7CiAg ICAgICAgIHJldHVybjsKICAgICB9CiAKICAgICBtX3NhdmVkTlNHcmFwaGljc0NvbnRleHQgPSBb W05TR3JhcGhpY3NDb250ZXh0IGN1cnJlbnRDb250ZXh0XSByZXRhaW5dOwotICAgIE5TR3JhcGhp Y3NDb250ZXh0KiBuZXdDb250ZXh0ID0gW05TR3JhcGhpY3NDb250ZXh0IGdyYXBoaWNzQ29udGV4 dFdpdGhHcmFwaGljc1BvcnQ6Y2dDb250ZXh0IGZsaXBwZWQ6WUVTXTsKLSAgICBBTExPV19ERVBS RUNBVEVEX0RFQ0xBUkFUSU9OU19FTkQKKyAgICBOU0dyYXBoaWNzQ29udGV4dCogbmV3Q29udGV4 dCA9IFtOU0dyYXBoaWNzQ29udGV4dCBncmFwaGljc0NvbnRleHRXaXRoQ0dDb250ZXh0OmNnQ29u dGV4dCBmbGlwcGVkOllFU107CiAgICAgW05TR3JhcGhpY3NDb250ZXh0IHNldEN1cnJlbnRDb250 ZXh0Om5ld0NvbnRleHRdOwogICAgIG1fZGlkU2V0R3JhcGhpY3NDb250ZXh0ID0gdHJ1ZTsKIH0K