[webkit-changes] [69097] trunk/WebCore

[Dan Bernstein]

I have a couple of questions and comments about this change. Sorry I didn’t get a chance to comment earlier.

On Oct 5, 2010, at 1:42 AM, commit-queue at webkit.org wrote:

> Revision
> 69097
> Author
> commit-queue at webkit.org
> Date
> 2010-10-05 01:42:47 -0700 (Tue, 05 Oct 2010)
> Log Message
> 
> 2010-10-05  Sanjeev Radhakrishnan  <sanjeevr at chromium.org>
> 
>         Reviewed by Darin Fisher.
> 
>         Fixed implementation of pluginWidgetFromDocument to search for the "embed" element rather than just use the first child.
>         https://bugs.webkit.org/show_bug.cgi?id=47129
It is not immediately obvious why there is no regression test for this change, so a comment in the change log could have helped to clarify this. I think there is a way to test content scripts with DumpRenderTree, so I guess the issue is that pluginWidgetForDocument() is only used by the Chromium WebKit API.

> 
>         * html/PluginDocument.cpp:
>         (WebCore::PluginDocumentParser::pluginWidgetFromDocument):
> Modified Paths
> 
> trunk/WebCore/ChangeLog
> trunk/WebCore/html/PluginDocument.cpp
> Diff
> 
> Modified: trunk/WebCore/ChangeLog (69096 => 69097)
> 
> --- trunk/WebCore/ChangeLog	2010-10-05 08:28:53 UTC (rev 69096)
> +++ trunk/WebCore/ChangeLog	2010-10-05 08:42:47 UTC (rev 69097)
> @@ -1,3 +1,13 @@
> +2010-10-05  Sanjeev Radhakrishnan  <sanjeevr at chromium.org>
> +
> +        Reviewed by Darin Fisher.
> +
> +        Fixed implementation of pluginWidgetFromDocument to search for the "embed" element rather than just use the first child.
> +        https://bugs.webkit.org/show_bug.cgi?id=47129
> +
> +        * html/PluginDocument.cpp:
> +        (WebCore::PluginDocumentParser::pluginWidgetFromDocument):
> +
>  2010-10-05  Chris Rogers  <crogers at google.com>
>  
>          Reviewed by Kenneth Russell.
> Modified: trunk/WebCore/html/PluginDocument.cpp (69096 => 69097)
> 
> --- trunk/WebCore/html/PluginDocument.cpp	2010-10-05 08:28:53 UTC (rev 69096)
> +++ trunk/WebCore/html/PluginDocument.cpp	2010-10-05 08:42:47 UTC (rev 69097)
> @@ -32,6 +32,7 @@
>  #include "HTMLHtmlElement.h"
>  #include "HTMLNames.h"
>  #include "MainResourceLoader.h"
> +#include "NodeList.h"
>  #include "Page.h"
>  #include "RawDataDocumentParser.h"
>  #include "RenderEmbeddedObject.h"
> @@ -70,7 +71,9 @@
>      ASSERT(doc);
>      RefPtr<Element> body = doc->body();
>      if (body) {
> -        RefPtr<Node> node = body->firstChild();
> +        RefPtr<NodeList> embedNodes = body->getElementsByTagName("embed");
> +        ASSERT(embedNodes && embedNodes->length());
Why is this assertion true? If a content script could add nodes before the embed element, couldn’t it also remove all embed elements from the document?

> +        Node* node = embedNodes->item(0);

Even though this kind of logic is used elsewhere in the code, it is not the most efficient way to get the first embed element in the document. querySelector is better because it doesn’t create a dynamic node list (nor does it create a list at all, it just returns the first match). Then again, I wonder if returning the first embed element is really the desired behavior, rather than returning the embed element that was created by the PluginDocumentParser, if it is still present. If the content script introduces an extra embed element before the original one, do you really want to return that one?

—Dan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-changes/attachments/20101005/33c587cd/attachment.html>