Bug 19472 - test for dynamic changes to the "data" attribute of an OBJECT element fails
Summary: test for dynamic changes to the "data" attribute of an OBJECT element fails
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://www.hixie.ch/tests/adhoc/dom/h...
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2008-06-10 17:40 PDT by Calvin Anderson
Modified: 2012-02-01 14:46 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (19.56 KB, patch)
2011-10-21 03:09 PDT, Sameer Patil
no flags Details | Formatted Diff | Diff
Proposed patch (21.08 KB, patch)
2011-10-21 03:26 PDT, Sameer Patil
no flags Details | Formatted Diff | Diff
Proposed patch (20.82 KB, patch)
2011-10-21 05:09 PDT, Sameer Patil
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Calvin Anderson 2008-06-10 17:40:34 PDT
I Steps:
 Go to http://www.hixie.ch/tests/adhoc/dom/html/flow/object/001.html
 
 II Issue:
 Test fails for dynamic changes to the "data" attribute of an OBJECT element.
 
 III Other browsers:
 IE7: not ok
 FF3: ok
 Opera: ok
 
 IV Nightly tested: 34380
Comment 1 Mark Rowe (bdash) 2008-06-11 00:30:12 PDT
<rdar://problem/5999635>
Comment 2 Robert Blaut 2008-06-12 00:53:55 PDT
The test works correctly if it's loaded on background tab.

Steps to reproduce:
1) Uncheck in Preferences, Tabs option: "Select tabs and windows as they are created"
2) Middle click on URL: http://www.hixie.ch/tests/adhoc/dom/html/flow/object/001.html
3) Test reports "PASSED".

I can confirm the problem.
Comment 3 Christopher Vickery 2010-01-14 12:25:47 PST
Is there a workaround for this, or is AJAX/innerHTML the only solution?
Comment 4 Sameer Patil 2011-10-21 03:09:23 PDT
Created attachment 111930 [details]
Proposed patch

Even if setUpdateWidget() set to true in HTMLObjectElement after a data attribute change, scheduling a call to updateWidget() is required to update the element. Same is true for HTMLEmbed element if src attribute changes dynamically.
Comment 5 Sameer Patil 2011-10-21 03:26:05 PDT
Created attachment 111931 [details]
Proposed patch

Please ignore previous patch. Forgot to add some test files.
Comment 6 Sameer Patil 2011-10-21 05:09:41 PDT
Created attachment 111948 [details]
Proposed patch
Comment 7 Sameer Patil 2011-11-16 04:50:24 PST
Can anyone please review my patch.
Comment 8 Eric Seidel (no email) 2012-01-09 13:24:19 PST
Comment on attachment 111948 [details]
Proposed patch

OK.  I'm glad we have this fix, but why do we need to use a custom timer to make the widget update?
Comment 9 Darin Adler 2012-01-10 10:25:57 PST
Comment on attachment 111948 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111948&action=review

> Source/WebCore/html/HTMLPlugInImageElement.cpp:203
> +void HTMLPlugInImageElement::updateWidgetTimerFired(Timer<HTMLPlugInImageElement> *)

Incorrect formatting here. Space before * should be omitted.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:205
> +    m_updateWidgetTimer.stop();

No need to stop the timer; it’s a one-shot.

> Source/WebCore/html/HTMLPlugInImageElement.h:61
> +    Timer<HTMLPlugInImageElement> m_updateWidgetTimer;

A timer is a large object. This means we’re consuming a lot more memory. Is there a way to do this without the timer?
Comment 10 Eric Seidel (no email) 2012-01-30 16:09:17 PST
Comment on attachment 111948 [details]
Proposed patch

r- based on Darin and my comments.  Just need some justification about the timer or thoughts on how we'd do this without one.
Comment 11 Sameer Patil 2012-01-31 21:59:51 PST
(In reply to comment #10)
> (From update of attachment 111948 [details])
> r- based on Darin and my comments.  Just need some justification about the timer or thoughts on how we'd do this without one.
I had tried this doing without timer by calling updateWidgetIfNecessary()(making it protected in base class) directly from parseMappedAttribute() of object and embed element.

But the main issue here is from updateWidget() call go to SubframeLoader::loadOrRedirectSubframe() in this function instead of loadSubframe(), scheduleLocationChange() gets called. I guess loadSubFrame() needs to be get called to load new data src.

The reason looks likes updateWidget() come from the same stack of previous subframeload(original data src url).

I used timer to accomplish this so previous subframe load will complete first and next load will come on new stack and it will load it as subframe.

Please correct me if my understanding is wrong.
Comment 12 Darin Adler 2012-02-01 14:46:29 PST
(In reply to comment #11)
> But the main issue here is from updateWidget() call go to SubframeLoader::loadOrRedirectSubframe() in this function instead of loadSubframe(), scheduleLocationChange() gets called. I guess loadSubFrame() needs to be get called to load new data src.

We need to find a clear way to say this in a comment in the code. The answer to “why is a timer needed” should be something stated rather than something people just have to figure out.