103702 2012-11-29 18:30:48 -0800 PluginDocument fires didFinishDocumentLoadForFrame upon receiving initial bytes instead of when load completes 2012-11-30 15:31:47 -0800 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 thorton thorton andersca ap bdakin beidson ojan webkit.review.bot oldest_to_newest 779727 0 thorton 2012-11-29 18:30:48 -0800 PluginDocumentParser::appendBytes() was made to call finish() in http://trac.webkit.org/changeset/14838. This is a bit odd because it basically means that onload and all the other load-y events get called immediately when the first bytes arrive, instead of when the document's main resource is actually finished loading. However, DocumentWriter::end() already calls finish() at a more appropriate time. Reading http://trac.webkit.org/changeset/114062 makes me assume that at some point in the past (and likely in the case of r14838), finish() would not get called in the case of a main resource load, but it is now. I'm having trouble doing sufficient archaeology to determine when that change occurred, but I don't think it matters. I'd like to remove that call to finish() if there are no objections. <rdar://problem/12762534> 779831 1 beidson 2012-11-29 22:50:32 -0800 (In reply to comment #0) > PluginDocumentParser::appendBytes() was made to call finish() in http://trac.webkit.org/changeset/14838. This is a bit odd because it basically means that onload and all the other load-y events get called immediately when the first bytes arrive, instead of when the document's main resource is actually finished loading. > > However, DocumentWriter::end() already calls finish() at a more appropriate time. Reading http://trac.webkit.org/changeset/114062 makes me assume that at some point in the past (and likely in the case of r14838), finish() would not get called in the case of a main resource load, but it is now. > > I'm having trouble doing sufficient archaeology to determine when that change occurred, but I don't think it matters. > > I'd like to remove that call to finish() if there are no objections. Here's why I hesitate to support this change - I'm not sure it matters! Is there any difficulty we're running in to as a result here? Do PluginDocuments even *have* script that observes this? Thinking logically about what a PluginDocument is... it's a template of HTML built in to the engine which instantiates a Plugin with the main resource data. Especially in a world of asynchronous plugin initialization I'm not yet convinced this is *wrong*. Hearing any symptom it causes could quickly change my mind. 779832 2 thorton 2012-11-29 22:51:18 -0800 (In reply to comment #1) > (In reply to comment #0) > > PluginDocumentParser::appendBytes() was made to call finish() in http://trac.webkit.org/changeset/14838. This is a bit odd because it basically means that onload and all the other load-y events get called immediately when the first bytes arrive, instead of when the document's main resource is actually finished loading. > > > > However, DocumentWriter::end() already calls finish() at a more appropriate time. Reading http://trac.webkit.org/changeset/114062 makes me assume that at some point in the past (and likely in the case of r14838), finish() would not get called in the case of a main resource load, but it is now. > > > > I'm having trouble doing sufficient archaeology to determine when that change occurred, but I don't think it matters. > > > > I'd like to remove that call to finish() if there are no objections. > > Here's why I hesitate to support this change - I'm not sure it matters! Is there any difficulty we're running in to as a result here? Do PluginDocuments even *have* script that observes this? > > Thinking logically about what a PluginDocument is... it's a template of HTML built in to the engine which instantiates a Plugin with the main resource data. Especially in a world of asynchronous plugin initialization I'm not yet convinced this is *wrong*. > > Hearing any symptom it causes could quickly change my mind. Please read the Radar. 779838 3 thorton 2012-11-29 22:56:52 -0800 (In reply to comment #2) > > Hearing any symptom it causes could quickly change my mind. > > Please read the Radar. For those who can't, the symptom is basically that clients who listen to didFinishDocumentLoadForFrame get the notification way too early. 779849 4 176905 thorton 2012-11-29 23:34:24 -0800 Created attachment 176905 patch 780279 5 ap 2012-11-30 11:58:42 -0800 The patch looks good to me, but I'd like to better understand whether this only affects DOMContentLoaded, or also the load event. 780461 6 thorton 2012-11-30 15:09:53 -0800 By experiment, both DOMContentLoaded and onload now fire when the main resource finishes loading instead of immediately (I confirmed they both fired immediately before, too). 780472 7 thorton 2012-11-30 15:27:08 -0800 Technically, this is about didFinishDocumentLoadForFrame, not DOMContentLoaded. Also, we really ideally wouldn't move DOMContentLoaded - the DOM is actually ready immediately upon PluginDocument construction. However, we're not convinced there's actually a way to observe DOMContentLoaded on a PluginDocument, so it likely doesn't matter. 780473 8 176905 ap 2012-11-30 15:27:55 -0800 Comment on attachment 176905 patch r=me I think that the new behavior is correct for the load event, but not logical for DOMContentLoaded. However, the purpose of this bug is to get client delegates at the right time, and timing of DOM events doesn't matter nearly as much. Also, I'm not sure if the change of behavior for DOMContentLoaded is even observable. Please update the bug title to track what we actually want to fix in Bugzilla and in ChangeLog. 780477 9 thorton 2012-11-30 15:31:47 -0800 (In reply to comment #8) > (From update of attachment 176905 [details]) > r=me > > I think that the new behavior is correct for the load event, but not logical for DOMContentLoaded. > > However, the purpose of this bug is to get client delegates at the right time, and timing of DOM events doesn't matter nearly as much. Also, I'm not sure if the change of behavior for DOMContentLoaded is even observable. Indeed. Thanks, Alexey! http://trac.webkit.org/changeset/136289 176905 2012-11-29 23:34:24 -0800 2012-11-30 15:27:55 -0800 patch plugindocument.diff text/plain 1891 thorton ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBiOGRlN2Q5Li41MWEzYWQxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjQg QEAKKzIwMTItMTEtMjkgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisK KyAgICAgICAgUGx1Z2luRG9jdW1lbnQgZmlyZXMgRE9NQ29udGVudExvYWRlZCB1cG9uIHJlY2Vp dmluZyBpbml0aWFsIGJ5dGVzIGluc3RlYWQgb2Ygd2hlbiBsb2FkIGNvbXBsZXRlcworICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAzNzAyCisgICAgICAg IDxyZGFyOi8vcHJvYmxlbS8xMjc2MjUzND4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBSZW1vdmUgdGhlIGNhbGwgdG8gZmluaXNoKCkgZnJvbSBQbHVn aW5Eb2N1bWVudFBhcnNlcjo6YXBwZW5kQnl0ZXMoKS4KKworICAgICAgICBUaGlzIHdhcyBpbnRy b2R1Y2VkIGluIGh0dHA6Ly90cmFjLndlYmtpdC5vcmcvY2hhbmdlc2V0LzE0ODM4LCB3aGVuIGZp bmlzaCgpIHdvdWxkbid0IGdldAorICAgICAgICBjYWxsZWQgZm9yIFBsdWdpbkRvY3VtZW50cyBh bnkgb3RoZXIgd2F5LiBJbiB0aGUgdGltZSBzaW5jZSwgRG9jdW1lbnRXcml0ZXI6OmVuZCgpIHdh cyBtYWRlCisgICAgICAgIHRvIGNhbGwgZmluaXNoKCkgYW55d2F5LCBhdCB0aGUgY29ycmVjdCB0 aW1lICh0aGUgY2FsbCBmcm9tIGFwcGVuZEJ5dGVzIG1ha2VzIGEgUGx1Z2luRG9jdW1lbnQKKyAg ICAgICAgYXBwZWFyIHRvIGJlIGZpbmlzaGVkIGxvYWRpbmcgYWZ0ZXIgdGhlIGZpcnN0IGJ5dGVz IGFyZSByZWNlaXZlZCkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBiZWNhdXNlIHRoZXJlIGRv ZXNuJ3Qgc2VlbSB0byBiZSBhbnkgc3RhdGUgaW4gYSBQbHVnaW5Eb2N1bWVudCB0aGF0IGNhbiBi ZSBhY2Nlc3NlZAorICAgICAgICBmcm9tIEphdmFTY3JpcHQgdG8gZGV0ZXJtaW5lIHdoZXRoZXIg b3Igbm90IGl0IGhhcyBmaW5pc2hlZCBsb2FkaW5nIGluIERPTUNvbnRlbnRMb2FkZWQuCisKKyAg ICAgICAgKiBodG1sL1BsdWdpbkRvY3VtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBsdWdp bkRvY3VtZW50UGFyc2VyOjphcHBlbmRCeXRlcyk6CisKIDIwMTItMTEtMjkgIE1hcnRpbiBSb2Jp bnNvbiAgPG1yb2JpbnNvbkBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtHVEtdIFtXZWJLaXQyXSBF bWJlZCB0aGUgSFRUUCBhdXRoZW50aWNhdGlvbiBkaWFsb2cgaW50byB0aGUgV2ViVmlldwpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9QbHVnaW5Eb2N1bWVudC5jcHAgYi9Tb3VyY2Uv V2ViQ29yZS9odG1sL1BsdWdpbkRvY3VtZW50LmNwcAppbmRleCA0OTY0NDkyLi4xYWQwMzJhIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9odG1sL1BsdWdpbkRvY3VtZW50LmNwcAorKysgYi9T b3VyY2UvV2ViQ29yZS9odG1sL1BsdWdpbkRvY3VtZW50LmNwcApAQCAtMTMzLDggKzEzMyw2IEBA IHZvaWQgUGx1Z2luRG9jdW1lbnRQYXJzZXI6OmFwcGVuZEJ5dGVzKERvY3VtZW50V3JpdGVyKiwg Y29uc3QgY2hhciosIHNpemVfdCkKICAgICAgICAgICAgIGZyYW1lLT5sb2FkZXIoKS0+YWN0aXZl RG9jdW1lbnRMb2FkZXIoKS0+bWFpblJlc291cmNlTG9hZGVyKCktPnNldFNob3VsZEJ1ZmZlckRh dGEoRG9Ob3RCdWZmZXJEYXRhKTsKICAgICAgICAgfQogICAgIH0KLQotICAgIGZpbmlzaCgpOwog fQogCiBQbHVnaW5Eb2N1bWVudDo6UGx1Z2luRG9jdW1lbnQoRnJhbWUqIGZyYW1lLCBjb25zdCBL VVJMJiB1cmwpCg==