[webkit-changes] [69097] trunk/WebCore

[Sanjeev Radhakrishnan]

Oh, by the way, I just noticed that PluginDocument::pluginNode also uses the
old firstChild logic. I presume this should also be changed? Another option
is to disallow content scripts from modifying the plugin document at all.

On Tue, Oct 5, 2010 at 9:20 AM, Sanjeev Radhakrishnan <sanjeevr at chromium.org
> wrote:

> 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/5ad5b281/attachment.html>