[webkit-reviews] review granted: [Bug 44567] If an EMBED is part of an OBJECT's fallback content, WebKit should only render the EMBED if the OBJECT fails to load. : [Attachment 65474] Patch (v2)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 25 14:28:51 PDT 2010


Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 44567: If an EMBED is part of an OBJECT's fallback content, WebKit should
only render the EMBED if the OBJECT fails to load.
https://bugs.webkit.org/show_bug.cgi?id=44567

Attachment 65474: Patch (v2)
https://bugs.webkit.org/attachment.cgi?id=65474&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   If an EMBED is part of an OBJECT's fallback content, WebKit should
only
> +	   render the EMBED if the OBJECT fails to load.

Those capitalized tag names are so old school! I think <embed> and <object> are
so much more 2010.

>      Node* p = parentNode();
>      if (p && p->hasTagName(objectTag)) {
>	   ASSERT(p->renderer());
> -	   return false;
> +	   if (!static_cast<HTMLObjectElement*>(p)->useFallbackContent())
> +	       return false;
>      }

A "why" comment would be so great here.

> +    bool useFallbackContent() const { return m_useFallbackContent; }

My only concern about exposing this is that I don't know when the value is set
up. For example, might HTMLEmbedElement::rendererIsNeeded call this function
before the <object> has decided whether to use fallback content?

When is it safe to call this? Maybe only when we have made a renderer? Can we
add some kind of assertion?

> -		       url = p->value();
> +		       url = deprecatedParseURL(p->value());

This looks like a good bug fix, but perhaps one that is separate from the main
change here. Did you add a test case to cover this fix? Should we land this fix
separately, first?

> +	   // Turn the attributes of the OBJECT tag into arrays, but don't
override PARAM values.

I think "OBJECT tag" and "PARAM" should be either "object element" or "<object>
element" and "<param> values".

> +	   * accessibility/plugin.html: Simplified this test by removing the
> +	   unnecessary <object> around the <embed>.

But don't we also want test coverage for the common case where an <object> is
around an <embed>?


More information about the webkit-reviews mailing list