[webkit-changes] [69097] trunk/WebCore

[Sanjeev Radhakrishnan]

Inline again.

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

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

I just noticed pluginNode() myself. Yes, I agree, pluginWidget() should 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<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.
>>
>
> 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.
>

Fair enough. Again, I wonder if content script should be allowed to modify
the contents of the plugin document at all.


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

The method is a static method on the parser and hence has no access to
element that the parser object holds a reference to. Another option would be
to pass the created element to the PluginDocument when it is created in
the createDocumentStructure method. This might actually be the best option.


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