Updating <object> upon changing "data", "classid", "type" attributes.

Updating <object> upon changing "data", "classid", "type" attributes.

If an HTMLObjectElement's "type", "data", or "classid" attributes are
dynamically modified, trigger a refresh of the element. Following the
rules given in

  http://www.whatwg.org/specs/web-apps/current-work/#dom-object-data

(cf. "Whenever one of the following conditions occur:")

R=abarth,eseidel,esprehn
BUG=123536
TEST=fast/dom/HTMLObjectElement/update-data

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165298

[sof, 2014-01-14 12:56:36]

Please take a look when you next have a moment.

[abarth-chromium, 2014-01-14 18:11:44]

Maybe eseidel should review this CL?

https://codereview.chromium.org/135103003/diff/120001/LayoutTests/svg/as-obje...
File LayoutTests/svg/as-object/embedded-svg-immediate-offsetWidth-query.html
(right):

https://codereview.chromium.org/135103003/diff/120001/LayoutTests/svg/as-obje...
LayoutTests/svg/as-object/embedded-svg-immediate-offsetWidth-query.html:30: },
50);
This looks like it's going to be flaky.

[eseidel, 2014-01-15 00:09:41]

https://codereview.chromium.org/135103003/diff/120001/LayoutTests/svg/as-obje...
File LayoutTests/svg/as-object/embedded-svg-immediate-offsetWidth-query.html
(right):

https://codereview.chromium.org/135103003/diff/120001/LayoutTests/svg/as-obje...
LayoutTests/svg/as-object/embedded-svg-immediate-offsetWidth-query.html:30: },
50);
On 2014/01/14 18:11:45, abarth wrote:
> This looks like it's going to be flaky.

Yeah, please find a non-time-based way.  newobject.onload?

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:103: // Per spec, only re-determine
representation iff 'classid' and 'data' aren't present.
setNeedsWidgetUpdate is going to cause a recalc, no?

[esprehn, 2014-01-15 00:16:37]

https://codereview.chromium.org/135103003/diff/120001/LayoutTests/fast/dom/HT...
File LayoutTests/fast/dom/HTMLObjectElement/resources/message-ping-on-load.html
(right):

https://codereview.chromium.org/135103003/diff/120001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLObjectElement/resources/message-ping-on-load.html:3:
window.parent.postMessage('got it', '*');
This is really cute.

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:101: if (renderer()) {
Blah, this isn't right, if the thing was display: none we still need to do the
setNeedsWidgetUpdate and setNeedsStyleRecalc. I guess we can't load plugins
without a renderer today so it doesn't matter, but we'll have to fix this
someday.

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:103: // Per spec, only re-determine
representation iff 'classid' and 'data' aren't present.
On 2014/01/15 00:09:41, eseidel wrote:
> setNeedsWidgetUpdate is going to cause a recalc, no?

no, it just sets a boolean, that whole system is busted. :(

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:104: if (inDocument() &&
!fastHasAttribute(classidAttr) && !fastHasAttribute(dataAttr))
You don't need the inDocument() check, setNeedsStyleRecalc checks it for you.

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:115: } else if (inDocument() &&
!fastHasAttribute(classidAttr)) {
Same, don't check inDocument()

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:124: if (inDocument())
Remove inDocument()

[eseidel, 2014-01-15 01:35:50]

Renderless plugins will be landed *any day now*:
https://codereview.chromium.org/23618022/

We need to assume that we operate in a world wherby we can have Wigets which
will be kept alive past their RenderWidget.

[sof, 2014-01-15 06:25:33]

On 2014/01/15 01:35:50, eseidel wrote:
> Renderless plugins will be landed *any day now*:
> https://codereview.chromium.org/23618022/
> 
> We need to assume that we operate in a world wherby we can have Wigets which
> will be kept alive past their RenderWidget.

Cool, don't want to upset its final approach to 'master'.

Does it offer a mechanism for revalidating/invalidating its state via its
associated plugin element? Could see anything at first peek.

[eseidel, 2014-01-15 06:59:19]

No, but that sounds like very valid feedback to James.  I suspect that we'll
still want to "restart" persistent plugins when their attributes change.

[sof, 2014-01-15 15:26:57]

https://codereview.chromium.org/135103003/diff/120001/LayoutTests/svg/as-obje...
File LayoutTests/svg/as-object/embedded-svg-immediate-offsetWidth-query.html
(right):

https://codereview.chromium.org/135103003/diff/120001/LayoutTests/svg/as-obje...
LayoutTests/svg/as-object/embedded-svg-immediate-offsetWidth-query.html:30: },
50);
On 2014/01/15 00:09:41, eseidel wrote:
> On 2014/01/14 18:11:45, abarth wrote:
> > This looks like it's going to be flaky.
> 
> Yeah, please find a non-time-based way.  newobject.onload?

I used postMessage().

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:101: if (renderer()) {
On 2014/01/15 00:16:38, esprehn wrote:
> Blah, this isn't right, if the thing was display: none we still need to do the
> setNeedsWidgetUpdate and setNeedsStyleRecalc. I guess we can't load plugins
> without a renderer today so it doesn't matter, but we'll have to fix this
> someday.

ok, I went ahead & did it here/now.

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:104: if (inDocument() &&
!fastHasAttribute(classidAttr) && !fastHasAttribute(dataAttr))
On 2014/01/15 00:16:38, esprehn wrote:
> You don't need the inDocument() check, setNeedsStyleRecalc checks it for you.

Done.

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:115: } else if (inDocument() &&
!fastHasAttribute(classidAttr)) {
On 2014/01/15 00:16:38, esprehn wrote:
> Same, don't check inDocument()

Done.

https://codereview.chromium.org/135103003/diff/120001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:124: if (inDocument())
On 2014/01/15 00:16:38, esprehn wrote:
> Remove inDocument()

Done.

[eseidel, 2014-01-15 18:40:08]

https://codereview.chromium.org/135103003/diff/310001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/310001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:110: m_imageLoader = adoptPtr(new
HTMLImageLoader(this));
Lame that the original author didn't just add an ensureImageLoader() function.
:)

https://codereview.chromium.org/135103003/diff/310001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:112: } else if
(!fastHasAttribute(classidAttr)) {
I don't understand this bit?  Maybe we should abstract the "!classid && !data
into a helper function which has a nice name?  shouldReloadPluginOnAttrChange()

[sof, 2014-01-15 19:52:53]

https://codereview.chromium.org/135103003/diff/310001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/310001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:112: } else if
(!fastHasAttribute(classidAttr)) {
On 2014/01/15 18:40:09, eseidel wrote:
> I don't understand this bit?  Maybe we should abstract the "!classid && !data
> into a helper function which has a nice name? 
shouldReloadPluginOnAttrChange()

That's a good suggestion; now done (taking care not to accidentally upset the
handling of images.)

[sof, 2014-01-16 06:58:27]

Another rebase needed to avoid yet more TestExpectations conflicts.

Is this close to ready?

[sof, 2014-01-16 23:19:11]

(ping)

[esprehn, 2014-01-16 23:23:04]

lgtm, please fix the name before landing.

https://codereview.chromium.org/135103003/diff/430001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/430001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:263: void
HTMLObjectElement::shouldReloadPluginOnAttributeChange(const QualifiedName&
name)
should methods usually return a boolean, I think you want:

reloadPluginIfNeeded(name) or something like that

[sof, 2014-01-17 07:14:53]

https://codereview.chromium.org/135103003/diff/430001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/430001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:263: void
HTMLObjectElement::shouldReloadPluginOnAttributeChange(const QualifiedName&
name)
On 2014/01/16 23:23:05, esprehn wrote:
> should methods usually return a boolean, I think you want:
> 
> reloadPluginIfNeeded(name) or something like that

right, done.

[commit-bot: I haz the power, 2014-01-17 07:15:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/135103003/500001

[commit-bot: I haz the power, 2014-01-17 10:36:55]

Change committed as 165298

[rune, 2014-01-17 21:12:20]

https://codereview.chromium.org/135103003/diff/500001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/500001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:284: setNeedsStyleRecalc();
What's the recalc for? Is it the right tool here?

[sof, 2014-01-17 21:20:35]

https://codereview.chromium.org/135103003/diff/500001/Source/core/html/HTMLOb...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/135103003/diff/500001/Source/core/html/HTMLOb...
Source/core/html/HTMLObjectElement.cpp:284: setNeedsStyleRecalc();
On 2014/01/17 21:12:21, rune - CET wrote:
> What's the recalc for? Is it the right tool here?

That's the prevalent mechanism for elements to trigger repaint/invalidation. Do
you have a smaller hammer?

[rune, 2014-01-17 22:03:13]

On 2014/01/17 21:20:35, sof wrote:
>
https://codereview.chromium.org/135103003/diff/500001/Source/core/html/HTMLOb...
> File Source/core/html/HTMLObjectElement.cpp (right):
> 
>
https://codereview.chromium.org/135103003/diff/500001/Source/core/html/HTMLOb...
> Source/core/html/HTMLObjectElement.cpp:284: setNeedsStyleRecalc();
> On 2014/01/17 21:12:21, rune - CET wrote:
> > What's the recalc for? Is it the right tool here?
> 
> That's the prevalent mechanism for elements to trigger repaint/invalidation.
Do
> you have a smaller hammer?

setNeedsRecalcStyle will cause recalculation of computed styles and changes in
computed styles will cause repaint, reflow, reattach. I was wondering if any of
these attributes actually cause a style diff or if there is some other side
effect of triggering style recalc that will cause the necessary invalidation for
you.

The default parameter to setNeedsRecalcStyle is SubtreeStyleChange which could
cause unnecessary recalcs of sibling trees of <object> if there are adjacent
selectors present.