Bug 19472

Summary: test for dynamic changes to the "data" attribute of an OBJECT element fails
Product: WebKit Reporter: Calvin Anderson <anderson.calvin1>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: ahmad.saleem792, andersca, ap, bfulgham, darin, eric, mkrp87, rniwa, vickery, webkit
Priority: P2 Keywords: HasReduction, InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.hixie.ch/tests/adhoc/dom/html/flow/object/001.html
Attachments:
Description Flags
Proposed patch
none
Proposed patch
none
Proposed patch eric: review-

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.
Comment 13 Ahmad Saleem 2022-08-16 16:19:39 PDT
On attached test case, all browser (Safari 15.6, Safari Technology Preview 151, Chrome Canary 105 and Firefox Nightly 105) show "PASS" in white text within "Green" rectangle. Is something else needed here? 

I think it is something fixed along the way and this can be marked as "RESOLVED CONFIGURATION CHANGED"? Please ignore myself, if you think the test case needs to be updated and I am testing wrong but just wanted to share updated results. Thanks!