[webkit-changes] [69097] trunk/WebCore

[Dan Bernstein]

On Oct 5, 2010, at 9:29 AM, Sanjeev Radhakrishnan wrote:

> 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.

Thanks for calling my attention to PluginDocument::pluginNode(). It needs to be fixed too. But now that I am aware of it, I don’t understand why PluginDocument::pluginWidget() doesn’t just use pluginNode().

More comments below.

> 
> 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
>> 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.

But a content script could remove it. According to the bug, this entire change is about content scripts.

> I could ASSERT and just return NULL instead of dereferencing emdedNodes.

I don’t think asserting is appropriate here. Assertions are made about the internal logic of the code. It is never okay to assert something about the behavior of the client or the content.

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

This goes back to what the API is supposed to do. If it is supposed to return the embed element that was created by the PluginDocumentParser, then I think it could just do that (the parser holds a reference to that element). If it is supposed to do something different, and that something different can result in a null return value from this API, then clients of the API should be ready to handle it.

> 
> Thoughts?
>  
> 
> —Dan
> 
> 

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