Summary: | [Qt] fast/dom/HTMLScriptElement/nested-execution.html failed | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | qi <qi.2.zhang> | ||||||||
Component: | New Bugs | Assignee: | qi <qi.2.zhang> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, laszlo.gombos, webkit.review.bot, yael | ||||||||
Priority: | P2 | Keywords: | Qt | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
qi
2011-06-07 13:22:04 PDT
Created attachment 96416 [details]
patch
Created an empty interface for setCacheModel since QWebSetting doesn't have matched "cacheModel". Without this empty interface, layoutTest will complain "undefined" method.
Comment on attachment 96416 [details]
patch
Huh? How could this cause tests to pass? Certainly not for valid reasons.
Comment on attachment 96416 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=96416&action=review > Tools/ChangeLog:8 > + Created setCacheModel interface in LayoutTestController. It seems that http://trac.webkit.org/changeset/44591 missed the Qt implementation. > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:605 > + // WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER It seems to me that the test calls this function with 0. This seems to deserve a "// FIXME: Implement" comment. Comment on attachment 96416 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=96416&action=review I am not a reviewer, here s my comment. > LayoutTests/platform/qt/Skipped:-1395 > -fast/dom/HTMLScriptElement/nested-execution.html Is this test passing as-is? If so, you should unskip it separately from introducing the new cache interface. (In reply to comment #2) > (From update of attachment 96416 [details]) > Huh? How could this cause tests to pass? Certainly not for valid reasons. The test case call setCacheModel(0), the "0" is the default value (so, even we do nothing we will pass). But for us, we didn't have the interface that will get "undefined" error in javascript. (In reply to comment #5) > (In reply to comment #2) > > (From update of attachment 96416 [details] [details]) > > Huh? How could this cause tests to pass? Certainly not for valid reasons. > The test case call setCacheModel(0), the "0" is the default value (so, even we do nothing we will pass). But for us, we didn't have the interface that will get "undefined" error in javascript. Thanks for the explanation. Please add it also to the Changelog. Created attachment 97115 [details]
patch2
Based on Laszlo and Yael's comments renew the patch.
(In reply to comment #7) > Created an attachment (id=97115) [details] > patch2 > Based on Laszlo and Yael's comments renew the patch. Please remove the extra entry in the Changelog. Comment on attachment 97115 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=97115&action=review > Tools/ChangeLog:9 > + Currently, we don't have matched cache model in qt, but without this I would suggest better wording here... QtWebkit does not yet support different CacheModels. This change will expose setCacheModel() with a stub implementation, which is enough to pass the LayoutTest. > Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:609 > + // FIXME: Implement to supprt setCacheModel. Typo - support. I think simply "FIXME: Implement." is less confusing. Created attachment 97132 [details]
patch3
Repatch.
Comment on attachment 97132 [details]
patch3
LGTM, r+.
Comment on attachment 97132 [details] patch3 Clearing flags on attachment: 97132 Committed r88838: <http://trac.webkit.org/changeset/88838> All reviewed patches have been landed. Closing bug. |