Bug 86658 - ImageLoader can still dispatch beforeload events for ImageDocuments
Summary: ImageLoader can still dispatch beforeload events for ImageDocuments
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vicki Pfau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-05-16 11:16 PDT by Vicki Pfau
Modified: 2012-05-16 13:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.28 KB, patch)
2012-05-16 12:26 PDT, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (2.26 KB, patch)
2012-05-16 13:25 PDT, Vicki Pfau
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vicki Pfau 2012-05-16 11:16:18 PDT
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>
Comment 1 Vicki Pfau 2012-05-16 12:26:15 PDT
Created attachment 142322 [details]
Patch
Comment 2 Brady Eidson 2012-05-16 12:30:06 PDT
Comment on attachment 142322 [details]
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)
Comment 3 Vicki Pfau 2012-05-16 12:35:02 PDT
(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.
Comment 4 Vicki Pfau 2012-05-16 13:25:40 PDT
Created attachment 142332 [details]
Patch
Comment 5 Brady Eidson 2012-05-16 13:28:57 PDT
Comment on attachment 142332 [details]
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.
Comment 6 Vicki Pfau 2012-05-16 13:56:18 PDT
Committed r117336: <http://trac.webkit.org/changeset/117336>