| Summary: | Frame opener is not reset between tests. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Vsevolod Vlasov <vsevik> | ||||||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | ap, atwilson, buildbot, commit-queue, pfeldman, rniwa, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Bug Depends on: | 55295 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Vsevolod Vlasov
2011-02-21 05:03:03 PST
Created attachment 83319 [details]
Patch
Attachment 83319 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/win/WebFrame.cpp:752: Extra space after ( in function call [whitespace/parens] [4]
Source/WebKit/win/WebFrame.h:147: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebKit/win/WebFrame.h:149: Extra space after ( in function call [whitespace/parens] [4]
Total errors found: 3 in 28 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 83319 [details] did not build on win: Build output: http://queues.webkit.org/results/7943323 Created attachment 83322 [details]
Patch
Attachment 83322 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/win/WebFrame.cpp:752: Extra space after ( in function call [whitespace/parens] [4]
Source/WebKit/win/WebFrame.h:146: Extra space after ( in function call [whitespace/parens] [4]
Total errors found: 2 in 28 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 83322 [details] did not build on win: Build output: http://queues.webkit.org/results/7943332 Comment on attachment 83322 [details] Patch 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." > Source/WebKit/chromium/public/WebFrame.h:173 > + // Reset the frame that opened this frame to 0. > + 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(). > Source/WebKit/gtk/webkit/webkitwebframe.h:112 > +WEBKIT_API void > +webkit_web_frame_reset_opener (WebKitWebFrame *frame); This should probably be in webkitwebframeprivate.h - you could ask Gtk folks to be sure. > Source/WebKit/mac/WebView/WebFrame.h:195 > + @method resetOpener > + @discussion Resets mainFrame opener to 0 > +*/ > +- (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. > 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. >> 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. >> 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). > Source/WebKit2/ChangeLog:9 > + (WKBundleFrameCopyChildFrames): Please add a ChangeLog comment explaining what you did. > 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. > Tools/ChangeLog:6 > + DumpRenderTree should reset frame opener between tests. > + 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? > Tools/DumpRenderTree/win/DumpRenderTree.cpp:895 > + if (FAILED(frame->resetOpener())) > + 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. Comment on attachment 83322 [details] Patch 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. Created attachment 83383 [details]
Patch with fixes
Attachment 83383 [details] did not build on win: Build output: http://queues.webkit.org/results/7946377 Comment on attachment 83383 [details]
Patch with fixes
Looks fine. cq-, because this still breaks Windows.
Created attachment 83536 [details]
Patch with fixes for win build
The only change from previous patch is touching (adding new line) Source/WebKit/win/Interfaces/WebKit.idl file to trigger build scripts.
This change is also mentioned in ChangeLog
The commit-queue encountered the following flaky tests while processing attachment 83536 [details]: http/tests/security/cross-frame-access-port-explicit-domain.html bug 54668 (author: sam@webkit.org) The commit-queue is continuing to process your patch. Comment on attachment 83536 [details] Patch with fixes for win build Clearing flags on attachment: 83536 Committed r79570: <http://trac.webkit.org/changeset/79570> All reviewed patches have been landed. Closing bug. Reverted r79570 for reason: Breaks chromium build because glue/mocks/mock_web_frame.h/cc was not updated Committed r79586: <http://trac.webkit.org/changeset/79586> Comment on attachment 83536 [details] Patch with fixes for win build Clearing flags on attachment: 83536 Committed r79764: <http://trac.webkit.org/changeset/79764> All reviewed patches have been landed. Closing bug. Broke Chromium builds: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/3981/steps/compile/logs/stdio I'm sorry but I don't know how to fix the build so I'm rolling out the patch for now since the author is using @chromium.org address. You should probably add a concrete clearOpener on the chromium side first and then land this patch. Committed r79793: <http://trac.webkit.org/changeset/79793> |