172883 2017-06-02 17:05:02 -0700 REGRESSION(r216212): RenderReplaced::paint() should not save and restore the context unless it has to 2017-06-03 15:01:01 -0700 1 1 1 Unclassified WebKit Layout and Rendering WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 sabouhallawa wenson_hsieh bfulgham commit-queue simon.fraser thorton webkit-bug-importer wenson_hsieh zalan oldest_to_newest 1315297 0 sabouhallawa 2017-06-02 17:05:02 -0700 The change r216212 made RenderReplaced::paint() saves and restores the context every time it is called. However the context should be saved and restored only if the renderer is dragged and it is painted in a low alpha. Restoring the context is an expensive operation and should be called only when we have to. Actually with the low alpha painting case, we may not need the context to be saved and restored. All we need: At the beginning of the function: float alpha = paintInfo.context().alpha(); And before it exits: paintInfo.context().setAlpha(alpha); This affects the MotionMark scores. 1315298 1 webkit-bug-importer 2017-06-02 17:05:35 -0700 <rdar://problem/32548614> 1315317 2 311893 wenson_hsieh 2017-06-02 18:02:23 -0700 Created attachment 311893 Patch 1315337 3 311893 wenson_hsieh 2017-06-02 18:59:27 -0700 Comment on attachment 311893 Patch Thanks Tim! 1315342 4 311893 sabouhallawa 2017-06-02 19:27:09 -0700 Comment on attachment 311893 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311893&action=review > Source/WebCore/ChangeLog:18 > + graphics context. The whole context will be saved and restored because we just changed its alpha. This is still expensive. This might be okay because dragging the renderer is not the common case. 1315343 5 311893 commit-queue 2017-06-02 19:27:31 -0700 Comment on attachment 311893 Patch Clearing flags on attachment: 311893 Committed r217748: <http://trac.webkit.org/changeset/217748> 1315344 6 commit-queue 2017-06-02 19:27:33 -0700 All reviewed patches have been landed. Closing bug. 1315464 7 311893 zalan 2017-06-03 12:48:27 -0700 Comment on attachment 311893 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311893&action=review > Source/WebCore/rendering/RenderReplaced.cpp:166 > if (element() && element()->parentOrShadowHostElement()) { > auto* parentContainer = element()->parentOrShadowHostElement(); > - if (draggedContentContainsReplacedElement(document().markers().markersFor(parentContainer, DocumentMarker::DraggedContent), *element())) > + if (draggedContentContainsReplacedElement(document().markers().markersFor(parentContainer, DocumentMarker::DraggedContent), *element())) { > + savedGraphicsContext.save(); I'd rather have this as a state somewhere like we have it with selection (since dragging has a start and an end). We make all these calls on every paint when "paints with dragging/all paints --> 0". 1315484 8 311893 wenson_hsieh 2017-06-03 15:01:01 -0700 Comment on attachment 311893 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311893&action=review >> Source/WebCore/ChangeLog:18 >> + graphics context. > > The whole context will be saved and restored because we just changed its alpha. This is still expensive. This might be okay because dragging the renderer is not the common case. The renderer being dragged is a very rare case -- we should be okay here. >> Source/WebCore/rendering/RenderReplaced.cpp:166 >> + savedGraphicsContext.save(); > > I'd rather have this as a state somewhere like we have it with selection (since dragging has a start and an end). We make all these calls on every paint when "paints with dragging/all paints --> 0". I see. I considered an approach like this, but didn't want to introduce new state to RenderObject or RenderText. What do you think about the following? 1. Make a new DraggedRange struct that has start/end offsets and make it an optional member of RenderObject. 2. When beginning a drag, use TextIterator to find text/images in the dragged range and set their renderers' dragged ranges. 3. When painting a RenderText or RenderReplaced, inspect this dragged content range to determine whether we need to fade before painting. 4. When dragging concludes, iterate through all nodes in the dragged range and clear their renderers' dragged ranges. Alternately, we could remember exactly which nodes we set dragged ranges for at the beginning of the drag, and unset those. 311893 2017-06-02 18:02:23 -0700 2017-06-02 19:27:31 -0700 Patch bug-172883-20170602180222.patch text/plain 2919 wenson_hsieh U3VidmVyc2lvbiBSZXZpc2lvbjogMjE3Njg4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGRjOGMzZTcyNjNlN2I4 MTQ1NWYzOTRlOTM3NWY0MTZkYTU0ZjQwYi4uNzAxM2MyMzY2NTMxOGQwY2NjY2ZmNjYyMzI5Mzli OWE4MTM1ZTNiOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDE3LTA2LTAyICBXZW5z b24gSHNpZWggIDx3ZW5zb25faHNpZWhAYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04o cjIxNjIxMik6IFJlbmRlclJlcGxhY2VkOjpwYWludCgpIHNob3VsZCBub3Qgc2F2ZSBhbmQgcmVz dG9yZSB0aGUgY29udGV4dCB1bmxlc3MgaXQgaGFzIHRvCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzI4ODMKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVt LzMyNTQ4NjE0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEFmdGVyIGltcGxlbWVudGluZyBkcmFnZ2VkIGNvbnRlbnQgZmFkaW5nLCBSZW5kZXJSZXBs YWNlOjpwYWludCBpcyBub3cgYWx3YXlzIGd1YXJkZWQgYnkgdW5uZWNlc3NhcnkgY2FsbHMgdG8K KyAgICAgICAgR3JhcGhpY3NDb250ZXh0OjpzYXZlIGFuZCBHcmFwaGljc0NvbnRleHQ6OnJlc3Rv cmUsIGV2ZW4gd2hlbiB0aGVyZSBpcyBubyBkcmFnZ2VkIGNvbnRlbnQgYmVpbmcgcmVuZGVyZWQu IFRvCisgICAgICAgIGFkZHJlc3MgdGhpcywgd2UgaW5pdGlhbGl6ZSBvdXIgR3JhcGhpY3NDb250 ZXh0U3RhdGVTYXZlciB3aXRoIHNhdmVBbmRSZXN0b3JlID0gZmFsc2UsIGluZGljYXRpbmcgdGhh dCB3ZSBkb24ndAorICAgICAgICB3YW50IHRvIGltbWVkaWF0ZWx5IHRyeSBhbmQgc2F2ZSB0aGUg Y29udGV4dC4KKworICAgICAgICBJZiB3ZSBhcmUgaW4gYSBkcmFnZ2VkIGNvbnRlbnQgcmFuZ2Us IHdlIHdpbGwgdGhlbiBjYWxsIEdyYXBoaWNzQ29udGV4dFN0YXRlU2F2ZXI6OnNhdmUsIHdoaWNo IHNhdmVzIHRoZQorICAgICAgICBncmFwaGljcyBjb250ZXh0IGFuZCBhbHNvIGNhdXNlcyB0aGUg R3JhcGhpY3NDb250ZXh0U3RhdGVTYXZlciB0byBldmVudHVhbGx5IHRyeSBhbmQgcmVzdG9yZSgp IHdoZW4gaXQgaXMKKyAgICAgICAgZGVzdHJveWVkLiBPdGhlcndpc2UsIGluIHRoZSBjb21tb24g Y29kZXBhdGggd2hlcmUgdGhlIHJlbmRlcmVyIGlzIG5vdCBpbiBhIGRyYWdnZWQgY29udGVudCBy YW5nZSwgdGhlCisgICAgICAgIGNvbnN0cnVjdG9yIGFuZCBkZXN0cnVjdG9yIG9mIEdyYXBoaWNz Q29udGV4dFN0YXRlU2F2ZXIgd2lsbCBiZSBuby1vcHMgd2l0aCByZXNwZWN0IHRvIHNhdmluZyBh bmQgcmVzdG9yaW5nIHRoZQorICAgICAgICBncmFwaGljcyBjb250ZXh0LgorCisgICAgICAgICog cmVuZGVyaW5nL1JlbmRlclJlcGxhY2VkLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlclJl cGxhY2VkOjpwYWludCk6CisKIDIwMTctMDYtMDEgIFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNl ckBhcHBsZS5jb20+CiAKICAgICAgICAgUkVHUkVTU0lPTiAocjIxNzI5Nik6IERyYWdnaW5nIGEg dmlkZW8gb25seSBzaG93cyBjb250cm9scyBkdXJpbmcgZHJhZyBldmVudApkaWZmIC0tZ2l0IGEv U291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclJlcGxhY2VkLmNwcCBiL1NvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9SZW5kZXJSZXBsYWNlZC5jcHAKaW5kZXggMzM5ZGJjYjI5ODM3MDIxNDFj NjU3NjJkZDIwYjIxY2QzNTBkNzY1Mi4uMDI2MDY1YTQ2NmI5ODMxYjFkOTU4YzNiM2U0ZTVkY2Rl ZTMxMGM0MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclJlcGxh Y2VkLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyUmVwbGFjZWQuY3Bw CkBAIC0xNTksMTEgKzE1OSwxMyBAQCB2b2lkIFJlbmRlclJlcGxhY2VkOjpwYWludChQYWludElu Zm8mIHBhaW50SW5mbywgY29uc3QgTGF5b3V0UG9pbnQmIHBhaW50T2Zmc2V0KQogICAgIFNldExh eW91dE5lZWRlZEZvcmJpZGRlblNjb3BlIHNjb3BlKHRoaXMpOwogI2VuZGlmCiAKLSAgICBHcmFw aGljc0NvbnRleHRTdGF0ZVNhdmVyIHNhdmVkR3JhcGhpY3NDb250ZXh0KHBhaW50SW5mby5jb250 ZXh0KCkpOworICAgIEdyYXBoaWNzQ29udGV4dFN0YXRlU2F2ZXIgc2F2ZWRHcmFwaGljc0NvbnRl eHQocGFpbnRJbmZvLmNvbnRleHQoKSwgZmFsc2UpOwogICAgIGlmIChlbGVtZW50KCkgJiYgZWxl bWVudCgpLT5wYXJlbnRPclNoYWRvd0hvc3RFbGVtZW50KCkpIHsKICAgICAgICAgYXV0byogcGFy ZW50Q29udGFpbmVyID0gZWxlbWVudCgpLT5wYXJlbnRPclNoYWRvd0hvc3RFbGVtZW50KCk7Ci0g ICAgICAgIGlmIChkcmFnZ2VkQ29udGVudENvbnRhaW5zUmVwbGFjZWRFbGVtZW50KGRvY3VtZW50 KCkubWFya2VycygpLm1hcmtlcnNGb3IocGFyZW50Q29udGFpbmVyLCBEb2N1bWVudE1hcmtlcjo6 RHJhZ2dlZENvbnRlbnQpLCAqZWxlbWVudCgpKSkKKyAgICAgICAgaWYgKGRyYWdnZWRDb250ZW50 Q29udGFpbnNSZXBsYWNlZEVsZW1lbnQoZG9jdW1lbnQoKS5tYXJrZXJzKCkubWFya2Vyc0Zvcihw YXJlbnRDb250YWluZXIsIERvY3VtZW50TWFya2VyOjpEcmFnZ2VkQ29udGVudCksICplbGVtZW50 KCkpKSB7CisgICAgICAgICAgICBzYXZlZEdyYXBoaWNzQ29udGV4dC5zYXZlKCk7CiAgICAgICAg ICAgICBwYWludEluZm8uY29udGV4dCgpLnNldEFscGhhKDAuMjUpOworICAgICAgICB9CiAgICAg fQogCiAgICAgTGF5b3V0UG9pbnQgYWRqdXN0ZWRQYWludE9mZnNldCA9IHBhaW50T2Zmc2V0ICsg bG9jYXRpb24oKTsK