86658 2012-05-16 11:16:18 -0700 ImageLoader can still dispatch beforeload events for ImageDocuments 2012-05-16 13:56:18 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jeffrey+webkit jeffrey+webkit beidson japhet mjs webkit.review.bot oldest_to_newest 625656 0 jeffrey+webkit 2012-05-16 11:16:18 -0700 When loading images, some of the logic regarding making the images appear on the page seems to occur somewhere within the beforeload logic (somewhere within code called by ImageLoader::dispatchPendingBeforeLoadEvent()). This means that a beforeload event needs to be dispatched on all loaded images, even within some contexts like ImageDocuments. While calling dispatchPendingBeforeLoadEvent while the image is being first updated (ImageLoader::updateFromElement), instead of being deferred (as happens when the page has a beforeload handler) ensures that listeners never actually sees the event, it still shouldn't fire the event in the first place. This means that the logic for connecting the images so they appear needs to be moved out of whatever dispatchPendingBeforeLoadEvent calls and into a separate function. I've tried to find where this logic occurs, but I have as of yet been unsuccessful. <rdar://problem/11465863> 625733 1 142322 jeffrey+webkit 2012-05-16 12:26:15 -0700 Created attachment 142322 Patch 625737 2 142322 beidson 2012-05-16 12:30:06 -0700 Comment on attachment 142322 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142322&action=review > Source/WebCore/loader/ImageLoader.cpp:198 > - m_hasPendingBeforeLoadEvent = newImage; > + if (!m_element->document()->isImageDocument()) > + m_hasPendingBeforeLoadEvent = newImage; I would consider forcing m_hasPendingBeforeLoadEvent to false for image documents, and make sure whatever other bits of related data are set appropriately (if any) 625748 3 jeffrey+webkit 2012-05-16 12:35:02 -0700 (In reply to comment #2) > (From update of attachment 142322 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142322&action=review > > > Source/WebCore/loader/ImageLoader.cpp:198 > > - m_hasPendingBeforeLoadEvent = newImage; > > + if (!m_element->document()->isImageDocument()) > > + m_hasPendingBeforeLoadEvent = newImage; > > I would consider forcing m_hasPendingBeforeLoadEvent to false for image documents, and make sure whatever other bits of related data are set appropriately (if any) I'm not sure that's necessary, but I suppose it is nice to be explicit. The only associated bit seems to be regarding queuing of events in the event sender--which we don't do in this case anyway, so that's fine. I'll upload a version with the flag set explicitly. 625793 4 142332 jeffrey+webkit 2012-05-16 13:25:40 -0700 Created attachment 142332 Patch 625797 5 142332 beidson 2012-05-16 13:28:57 -0700 Comment on attachment 142332 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142332&action=review > Source/WebCore/ChangeLog:12 > + > + Prevent flags regarding sending beforeload events from being set on ImageDocuments. > + > + No new tests. > + Might want to reiterate the same reason why not from the last change. 625832 6 jeffrey+webkit 2012-05-16 13:56:18 -0700 Committed r117336: <http://trac.webkit.org/changeset/117336> 142322 2012-05-16 12:26:15 -0700 2012-05-16 13:25:36 -0700 Patch bug-86658-20120516122621.patch text/plain 2330 jeffrey+webkit U3VidmVyc2lvbiBSZXZpc2lvbjogMTE3MzE0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjA2MjExNzA1Y2FkOTM5 YzUzYmIzZDJlZmMwYjQyMzc5ODQyNjM1ZC4uNmI4YTIyMmRiYTA1ZmZkMzA1MDU4MjYzOGE4YWEw YmMwMzUyMzQ5OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTA1LTE2ICBKZWZm cmV5IFBmYXUgIDxqcGZhdUBhcHBsZS5jb20+CisKKyAgICAgICAgU29tZSBpbWFnZSBsb2FkaW5n IGxvZ2ljIGhhcHBlbnMgaW4gYW4gdW5pbnR1aXRpdmUgYW5kIGluY29ycmVjdCBsb2NhdGlvbgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODY2NTgKKyAg ICAgICAgPHJkYXI6Ly9wcm9ibGVtLzExNDY1ODYzPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIFByZXZlbnQgZmxhZ3MgcmVnYXJkaW5nIHNlbmRpbmcg YmVmb3JlbG9hZCBldmVudHMgZnJvbSBiZWluZyBzZXQgb24gSW1hZ2VEb2N1bWVudHMuCisKKyAg ICAgICAgTm8gbmV3IHRlc3RzLgorCisgICAgICAgICogbG9hZGVyL0ltYWdlTG9hZGVyLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkltYWdlTG9hZGVyOjp1cGRhdGVGcm9tRWxlbWVudCk6CisKIDIw MTItMDUtMTYgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CiAKICAgICAg ICAgU2Nyb2xsYmFyIGxheWVycyBzaG91bGQgcmVzcGVjdCBhY2NlbGVyYXRlZCBkcmF3aW5nIHNl dHRpbmcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9JbWFnZUxvYWRlci5jcHAg Yi9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvSW1hZ2VMb2FkZXIuY3BwCmluZGV4IGQ0YWY2ZWY4NmMw MThiZTQ5MjNmY2FmZTY0MDc3NTUyNDM2ZTk1YjMuLjkyOTUyNTYzYWJlYmVjZGNhZGY3NTc5NzZj YzQ0NzNlODBlNjM5NjMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9JbWFnZUxv YWRlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL0ltYWdlTG9hZGVyLmNwcApAQCAt MTk0LDE1ICsxOTQsMTkgQEAgdm9pZCBJbWFnZUxvYWRlcjo6dXBkYXRlRnJvbUVsZW1lbnQoKQog ICAgICAgICAgICAgZXJyb3JFdmVudFNlbmRlcigpLmNhbmNlbEV2ZW50KHRoaXMpOwogCiAgICAg ICAgIG1faW1hZ2UgPSBuZXdJbWFnZTsKLSAgICAgICAgbV9oYXNQZW5kaW5nQmVmb3JlTG9hZEV2 ZW50ID0gbmV3SW1hZ2U7CisgICAgICAgIGlmICghbV9lbGVtZW50LT5kb2N1bWVudCgpLT5pc0lt YWdlRG9jdW1lbnQoKSkKKyAgICAgICAgICAgIG1faGFzUGVuZGluZ0JlZm9yZUxvYWRFdmVudCA9 IG5ld0ltYWdlOwogICAgICAgICBtX2hhc1BlbmRpbmdMb2FkRXZlbnQgPSBuZXdJbWFnZTsKICAg ICAgICAgbV9pbWFnZUNvbXBsZXRlID0gIW5ld0ltYWdlOwogCiAgICAgICAgIGlmIChuZXdJbWFn ZSkgewotICAgICAgICAgICAgaWYgKCFtX2VsZW1lbnQtPmRvY3VtZW50KCktPmhhc0xpc3RlbmVy VHlwZShEb2N1bWVudDo6QkVGT1JFTE9BRF9MSVNURU5FUikgfHwgbV9lbGVtZW50LT5kb2N1bWVu dCgpLT5pc0ltYWdlRG9jdW1lbnQoKSkKLSAgICAgICAgICAgICAgICBkaXNwYXRjaFBlbmRpbmdC ZWZvcmVMb2FkRXZlbnQoKTsKLSAgICAgICAgICAgIGVsc2UKLSAgICAgICAgICAgICAgICBiZWZv cmVMb2FkRXZlbnRTZW5kZXIoKS5kaXNwYXRjaEV2ZW50U29vbih0aGlzKTsKKyAgICAgICAgICAg IGlmICghbV9lbGVtZW50LT5kb2N1bWVudCgpLT5pc0ltYWdlRG9jdW1lbnQoKSkgeworICAgICAg ICAgICAgICAgIGlmICghbV9lbGVtZW50LT5kb2N1bWVudCgpLT5oYXNMaXN0ZW5lclR5cGUoRG9j dW1lbnQ6OkJFRk9SRUxPQURfTElTVEVORVIpKQorICAgICAgICAgICAgICAgICAgICBkaXNwYXRj aFBlbmRpbmdCZWZvcmVMb2FkRXZlbnQoKTsKKyAgICAgICAgICAgICAgICBlbHNlCisgICAgICAg ICAgICAgICAgICAgIGJlZm9yZUxvYWRFdmVudFNlbmRlcigpLmRpc3BhdGNoRXZlbnRTb29uKHRo aXMpOworICAgICAgICAgICAgfSBlbHNlCisgICAgICAgICAgICAgICAgdXBkYXRlUmVuZGVyZXIo KTsKIAogICAgICAgICAgICAgLy8gSWYgbmV3SW1hZ2UgaXMgY2FjaGVkLCBhZGRDbGllbnQoKSB3 aWxsIHJlc3VsdCBpbiB0aGUgbG9hZCBldmVudAogICAgICAgICAgICAgLy8gYmVpbmcgcXVldWVk IHRvIGZpcmUuIEVuc3VyZSB0aGlzIGhhcHBlbnMgYWZ0ZXIgYmVmb3JlbG9hZCBpcwo= 142332 2012-05-16 13:25:40 -0700 2012-05-16 13:28:57 -0700 Patch bug-86658-20120516132546.patch text/plain 2315 jeffrey+webkit U3VidmVyc2lvbiBSZXZpc2lvbjogMTE3MzE0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjA2MjExNzA1Y2FkOTM5 YzUzYmIzZDJlZmMwYjQyMzc5ODQyNjM1ZC4uNmI4YTIyMmRiYTA1ZmZkMzA1MDU4MjYzOGE4YWEw YmMwMzUyMzQ5OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTA1LTE2ICBKZWZm cmV5IFBmYXUgIDxqcGZhdUBhcHBsZS5jb20+CisKKyAgICAgICAgU29tZSBpbWFnZSBsb2FkaW5n IGxvZ2ljIGhhcHBlbnMgaW4gYW4gdW5pbnR1aXRpdmUgYW5kIGluY29ycmVjdCBsb2NhdGlvbgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODY2NTgKKyAg ICAgICAgPHJkYXI6Ly9wcm9ibGVtLzExNDY1ODYzPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIFByZXZlbnQgZmxhZ3MgcmVnYXJkaW5nIHNlbmRpbmcg YmVmb3JlbG9hZCBldmVudHMgZnJvbSBiZWluZyBzZXQgb24gSW1hZ2VEb2N1bWVudHMuCisKKyAg ICAgICAgTm8gbmV3IHRlc3RzLgorCisgICAgICAgICogbG9hZGVyL0ltYWdlTG9hZGVyLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkltYWdlTG9hZGVyOjp1cGRhdGVGcm9tRWxlbWVudCk6CisKIDIw MTItMDUtMTYgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CiAKICAgICAg ICAgU2Nyb2xsYmFyIGxheWVycyBzaG91bGQgcmVzcGVjdCBhY2NlbGVyYXRlZCBkcmF3aW5nIHNl dHRpbmcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9JbWFnZUxvYWRlci5jcHAg Yi9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvSW1hZ2VMb2FkZXIuY3BwCmluZGV4IGQ0YWY2ZWY4NmMw MThiZTQ5MjNmY2FmZTY0MDc3NTUyNDM2ZTk1YjMuLmY0YTlmZjUzNTEzMmY0Y2QzYTUyYWZkZmUy YTU0MThiYjMyNDIxMTUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9JbWFnZUxv YWRlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL0ltYWdlTG9hZGVyLmNwcApAQCAt MTk0LDE1ICsxOTQsMTggQEAgdm9pZCBJbWFnZUxvYWRlcjo6dXBkYXRlRnJvbUVsZW1lbnQoKQog ICAgICAgICAgICAgZXJyb3JFdmVudFNlbmRlcigpLmNhbmNlbEV2ZW50KHRoaXMpOwogCiAgICAg ICAgIG1faW1hZ2UgPSBuZXdJbWFnZTsKLSAgICAgICAgbV9oYXNQZW5kaW5nQmVmb3JlTG9hZEV2 ZW50ID0gbmV3SW1hZ2U7CisgICAgICAgIG1faGFzUGVuZGluZ0JlZm9yZUxvYWRFdmVudCA9ICFt X2VsZW1lbnQtPmRvY3VtZW50KCktPmlzSW1hZ2VEb2N1bWVudCgpICYmIG5ld0ltYWdlOwogICAg ICAgICBtX2hhc1BlbmRpbmdMb2FkRXZlbnQgPSBuZXdJbWFnZTsKICAgICAgICAgbV9pbWFnZUNv bXBsZXRlID0gIW5ld0ltYWdlOwogCiAgICAgICAgIGlmIChuZXdJbWFnZSkgewotICAgICAgICAg ICAgaWYgKCFtX2VsZW1lbnQtPmRvY3VtZW50KCktPmhhc0xpc3RlbmVyVHlwZShEb2N1bWVudDo6 QkVGT1JFTE9BRF9MSVNURU5FUikgfHwgbV9lbGVtZW50LT5kb2N1bWVudCgpLT5pc0ltYWdlRG9j dW1lbnQoKSkKLSAgICAgICAgICAgICAgICBkaXNwYXRjaFBlbmRpbmdCZWZvcmVMb2FkRXZlbnQo KTsKLSAgICAgICAgICAgIGVsc2UKLSAgICAgICAgICAgICAgICBiZWZvcmVMb2FkRXZlbnRTZW5k ZXIoKS5kaXNwYXRjaEV2ZW50U29vbih0aGlzKTsKKyAgICAgICAgICAgIGlmICghbV9lbGVtZW50 LT5kb2N1bWVudCgpLT5pc0ltYWdlRG9jdW1lbnQoKSkgeworICAgICAgICAgICAgICAgIGlmICgh bV9lbGVtZW50LT5kb2N1bWVudCgpLT5oYXNMaXN0ZW5lclR5cGUoRG9jdW1lbnQ6OkJFRk9SRUxP QURfTElTVEVORVIpKQorICAgICAgICAgICAgICAgICAgICBkaXNwYXRjaFBlbmRpbmdCZWZvcmVM b2FkRXZlbnQoKTsKKyAgICAgICAgICAgICAgICBlbHNlCisgICAgICAgICAgICAgICAgICAgIGJl Zm9yZUxvYWRFdmVudFNlbmRlcigpLmRpc3BhdGNoRXZlbnRTb29uKHRoaXMpOworICAgICAgICAg ICAgfSBlbHNlCisgICAgICAgICAgICAgICAgdXBkYXRlUmVuZGVyZXIoKTsKIAogICAgICAgICAg ICAgLy8gSWYgbmV3SW1hZ2UgaXMgY2FjaGVkLCBhZGRDbGllbnQoKSB3aWxsIHJlc3VsdCBpbiB0 aGUgbG9hZCBldmVudAogICAgICAgICAgICAgLy8gYmVpbmcgcXVldWVkIHRvIGZpcmUuIEVuc3Vy ZSB0aGlzIGhhcHBlbnMgYWZ0ZXIgYmVmb3JlbG9hZCBpcwo=