Remove testRunner.handleErrorPages()

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Fast/history/back-forward-reset-after-error-handling.html calls handleErrorPages(), which was not supported by DumpRenderTree and is not supported by TestRunner.  Given that this test was added for QT support, it should be removed.

[Jonathan Bedard]

Created attachment 285331 [details]
Patch

[Daniel Bates]

(In reply to comment #0)
> Fast/history/back-forward-reset-after-error-handling.html calls
> handleErrorPages(), which was not supported by DumpRenderTree and is not
> supported by TestRunner.  Given that this test was added for QT support, it
> should be removed.

This description is disingenuous. This test LayoutTests/fast/history/back-forward-reset-after-error-handling.html was added for all ports that implemented testRunner.handleErrorPages(). As it turns out testRunner.handleErrorPages() was only implemented on the Qt WebKit port.

For completeness, the motivation for adding testRunner.handleErrorPages() is described in bug #31509, comment 0. And the bug #31555 was filed to implement this feature for Mac though it was resolved as WONTFIX on 05/01/2012.

[Daniel Bates]

Comment on attachment 285331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285331&action=review

The motivation for this change seems reasonable. We need to also update file LayoutTests/platform/ios-simulator/TestExpectations. Also we should update the ChangeLog entry such that it list the files modified in this patch as well as clarify the description as per my remarks above.

> LayoutTests/ChangeLog:10
> +
> +        Removed fast/history/back-forward-reset-after-error-handling.html and all
> +        references since handleErrorPages() was never supported in TestRunners
> +

How are you generating this ChangeLog entry? Please use prepare-ChangeLog to generate the ChangeLog message.

It is disingenuous to write "was never supported in TestRunners". We should explain that window.testRunner.handleErrorPages() was only implement for the Qt WebKit port and that there has not been interest to implement this feature in any other port in the over 7 years since this feature was added.

[Jonathan Bedard]

Created attachment 285431 [details]
Patch

[Jonathan Bedard]

I think the format/comments on this patch should be better.

[Daniel Bates]

Comment on attachment 285431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285431&action=review

> LayoutTests/ChangeLog:3
> +        handleErrorPages() is not supported by TestRunner, remove dependent test

Please update the title in the ChangeLog to match the Bugzilla title.

> LayoutTests/ChangeLog:16
> +        * platform/gtk/TestExpectations: Dito.

Dito => Ditto.

> LayoutTests/ChangeLog:17
> +        * platform/ios-simulator/TestExpectations: Dito.

Ditto.

> LayoutTests/ChangeLog:18
> +        * platform/mac/TestExpectations: Dito.

Ditto.

> LayoutTests/ChangeLog:19
> +        * platform/win/TestExpectations: Dito.

Ditto.

[Jonathan Bedard]

Created attachment 285440 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 285440 [details]
Patch

Clearing flags on attachment: 285440

Committed r204191: <http://trac.webkit.org/changeset/204191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.