[webkit-changes] [69097] trunk/WebCore

[Sanjeev Radhakrishnan]

I uploaded a new patch with an updated implementation.

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

> 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/f2382108/attachment-0001.html>