[Webkit-unassigned] [Bug 54874] Frame opener is not reset between tests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54874





--- Comment #8 from Vsevolod Vlasov <vsevik at chromium.org>  2011-02-22 14:04:58 PST ---
(From update of attachment 83322)
View in context: https://bugs.webkit.org/attachment.cgi?id=83322&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (no code affected)
> 
> You could as well explain what has been done, not what hasn't. E.g.: "no behavior change, just exporting a method for DumpRenderTree use."

Done

>> Source/WebKit/chromium/public/WebFrame.h:173
>> +    virtual void resetOpener() = 0;
> 
> Here an in all other ports: it's arguable whether this method resets opener or not (resets to what?). I suggest naming it clearOpener() or forgetOpener().

Done (clearOpener).

>> Source/WebKit/gtk/webkit/webkitwebframe.h:112
>> +webkit_web_frame_reset_opener       (WebKitWebFrame       *frame);
> 
> This should probably be in webkitwebframeprivate.h - you could ask Gtk folks to be sure.

Done (DumpRenderTreeSupportGTK).

>> Source/WebKit/mac/WebView/WebFrame.h:195
>> +- (void)resetOpener;
> 
> Please don't change anything in WebFrame.h - it's public Apple API, and any changes need to be vetted in an internal Apple process.
> 
> New methods can be added to WebFramePrivate.h.

Done.

>> Source/WebKit/qt/Api/qwebframe.h:149
>> +    void resetOpener();
> 
> Again, not sure if it's OK to add an API method. The private interface is in qwebframe_p.h.

Done (DumpRenderTreeSupportQT).

>>> Source/WebKit/win/WebFrame.cpp:752
>>> +HRESULT STDMETHODCALLTYPE WebFrame::resetOpener( void)
>> 
>> Extra space after ( in function call  [whitespace/parens] [4]
> 
> Why does this need a (void) at all? That only sometimes makes a difference in plain C.

Some methods there were written in this weird form.
I changed it to clearOpener().

>>> Source/WebKit/win/WebFrame.h:146
>>> +    virtual HRESULT STDMETHODCALLTYPE resetOpener( void);
>> 
>> Extra space after ( in function call  [whitespace/parens] [4]
> 
> Ditto - probably doesn't need (void).

Done.

>> Source/WebKit2/ChangeLog:9
>> +        (WKBundleFrameCopyChildFrames):
> 
> Please add a ChangeLog comment explaining what you did.

Done.

>> Source/WebKit2/ChangeLog:10
>> +        (WKBundleFrameResetOpener):
> 
> WebKit2 isn't an API, so we are more relaxed about adding functions there at the moment. But WKBundleFrameResetOpener doesn't look like they would ever become APIs, so it's best to put them in WKBundleFramePrivate.h right away.

Done.

>> Tools/ChangeLog:6
>> +        https://bugs.webkit.org/show_bug.cgi?id=54874
> 
> Please explain what issue you are fixing. You are saying that opener needs to be cleared, but what to and when is it set, in the first place?

Done.

>> Tools/DumpRenderTree/win/DumpRenderTree.cpp:895
>> +        return;
> 
> Your implementation of resetOpener can never FAIL, as far as I can tell. Perhaps it's best to ASSERT the result - returning right before exiting the function makes no sense anyway.

Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.