Bug 103702 - PluginDocument fires didFinishDocumentLoadForFrame upon receiving initial bytes instead of when load completes
Summary: PluginDocument fires didFinishDocumentLoadForFrame upon receiving initial byt...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-11-29 18:30 PST by Tim Horton
Modified: 2012-11-30 15:31 PST (History)
6 users (show)

See Also:


Attachments
patch (1.85 KB, patch)
2012-11-29 23:34 PST, Tim Horton
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2012-11-29 18:30:48 PST
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>
Comment 1 Brady Eidson 2012-11-29 22:50:32 PST
(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.
Comment 2 Tim Horton 2012-11-29 22:51:18 PST
(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.
Comment 3 Tim Horton 2012-11-29 22:56:52 PST
(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.
Comment 4 Tim Horton 2012-11-29 23:34:24 PST
Created attachment 176905 [details]
patch
Comment 5 Alexey Proskuryakov 2012-11-30 11:58:42 PST
The patch looks good to me, but I'd like to better understand whether this only affects DOMContentLoaded, or also the load event.
Comment 6 Tim Horton 2012-11-30 15:09:53 PST
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).
Comment 7 Tim Horton 2012-11-30 15:27:08 PST
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.
Comment 8 Alexey Proskuryakov 2012-11-30 15:27:55 PST
Comment on attachment 176905 [details]
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.
Comment 9 Tim Horton 2012-11-30 15:31:47 PST
(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