[webkit-changes] [69097] trunk/WebCore

[Sanjeev Radhakrishnan]

Thanks for the comments. Replies inline.

On Tue, Oct 5, 2010 at 8:51 AM, Dan Bernstein <mitz at apple.com> wrote:

> 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 <http://trac.webkit.org/projects/webkit/changeset/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.
>

Yes, I believe this is the case.


>
>         * 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?
>

It could. I thought about this but I thought that is an actual error
condition since this code is invoked for a PluginDocument only (I should
probably change the imput argument to PluginDocument*) and a PluginDocument
should contain the plugin widget. I could ASSERT and just return NULL
instead of dereferencing emdedNodes.


>
> +        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?
>

I initially thought this is also an error condition like the one above but I
perhaps in this case we should search for the specific named "embed" tag?

Thoughts?


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