Bug 62227 - [Qt] fast/dom/HTMLScriptElement/nested-execution.html failed
Summary: [Qt] fast/dom/HTMLScriptElement/nested-execution.html failed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: qi
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-06-07 13:22 PDT by qi
Modified: 2011-06-14 11:44 PDT (History)
4 users (show)

See Also:


Attachments
patch (3.03 KB, patch)
2011-06-08 07:10 PDT, qi
no flags Details | Formatted Diff | Diff
patch2 (3.50 KB, patch)
2011-06-14 07:27 PDT, qi
no flags Details | Formatted Diff | Diff
patch3 (3.16 KB, patch)
2011-06-14 10:15 PDT, qi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description qi 2011-06-07 13:22:04 PDT
layoutTestController.setCacheModel need implement.
Comment 1 qi 2011-06-08 07:10:15 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 2 Eric Seidel (no email) 2011-06-13 21:18:54 PDT
Comment on attachment 96416 [details]
patch

Huh?  How could this cause tests to pass?  Certainly not for valid reasons.
Comment 3 Laszlo Gombos 2011-06-13 22:03:09 PDT
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 4 Yael 2011-06-14 05:52:30 PDT
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.
Comment 5 qi 2011-06-14 05:58:20 PDT
(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.
Comment 6 Yael 2011-06-14 06:35:04 PDT
(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.
Comment 7 qi 2011-06-14 07:27:03 PDT
Created attachment 97115 [details]
patch2

Based on Laszlo and Yael's comments renew the patch.
Comment 8 Yael 2011-06-14 08:52:39 PDT
(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 9 Laszlo Gombos 2011-06-14 08:54:19 PDT
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.
Comment 10 qi 2011-06-14 10:15:06 PDT
Created attachment 97132 [details]
patch3

Repatch.
Comment 11 Laszlo Gombos 2011-06-14 11:01:55 PDT
Comment on attachment 97132 [details]
patch3

LGTM, r+.
Comment 12 WebKit Review Bot 2011-06-14 11:44:38 PDT
Comment on attachment 97132 [details]
patch3

Clearing flags on attachment: 97132

Committed r88838: <http://trac.webkit.org/changeset/88838>
Comment 13 WebKit Review Bot 2011-06-14 11:44:43 PDT
All reviewed patches have been landed.  Closing bug.