Bug 62227 – [Qt] fast/dom/HTMLScriptElement/nested-execution.html failed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[qi]

layoutTestController.setCacheModel need implement.

[qi]

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.

[Eric Seidel (no email)]

Comment on attachment 96416 [details]
patch

Huh?  How could this cause tests to pass?  Certainly not for valid reasons.

[Laszlo Gombos]

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.

[Yael]

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.

[qi]

(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.

[Yael]

(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.

[qi]

Created attachment 97115 [details]
patch2

Based on Laszlo and Yael's comments renew the patch.

[Yael]

(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.

[Laszlo Gombos]

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.

[qi]

Created attachment 97132 [details]
patch3

Repatch.

[Laszlo Gombos]

Comment on attachment 97132 [details]
patch3

LGTM, r+.

[WebKit Review Bot]

Comment on attachment 97132 [details]
patch3

Clearing flags on attachment: 97132

Committed r88838: <http://trac.webkit.org/changeset/88838>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.