Bug 115819

Summary: unskip webarchive tests on mac
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, thorton
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.8   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2013-05-08 12:27:09 PDT
Because dumpDOMAsWebArchive was implemented on osx (https://bugs.webkit.org/show_bug.cgi?id=42324) the webarchive tests can be unskipped if they are not being skipped for another reason.  If they are being skipped for another reason, the reasons should be noted.
Comment 1 Alex Christensen 2013-05-08 12:32:37 PDT
Created attachment 201097 [details]
Patch
Comment 2 Tim Horton 2013-05-08 12:49:03 PDT
Comment on attachment 201097 [details]
Patch

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

> LayoutTests/platform/mac-wk2/TestExpectations:328
> +### START OF (5) Features that are disabled elsewhere re-enabled here

add an "and" after elsewhere.

> LayoutTests/platform/mac-wk2/TestExpectations:334
> +# dumpDOMAsWebArchive only supported for mac

"is", "Mac", and perhaps this should be "Override global WebKit2 skip of dumpDOMAsWebArchive because it's implemented for Mac" or something.

> LayoutTests/platform/wk2/TestExpectations:97
> +# dumpDOMAsWebArchive only implemented for mac

add an "is" before only and capitalize Mac. Also, should these be in the global TestExpectations? Are they also skipped on WK1 for non-Mac platforms?
Comment 3 Alex Christensen 2013-05-08 13:20:40 PDT
Created attachment 201102 [details]
Patch
Comment 4 Tim Horton 2013-05-08 13:25:19 PDT
Comment on attachment 201102 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        Replaced list of webarchive tests with webarchive test directories.
> +        * platform/wk2/TestExpectations:
> +        Unskip webarchive tests for Mac, note which webarchive tests still fail with bug numbers.

I think these comments are under the wrong filenames (swap them?)

> LayoutTests/platform/mac-wk2/TestExpectations:371
> +# setIconDatabaseEnabled not implemented in WebKitTestRunner
> +# https://bugs.webkit.org/show_bug.cgi?id=115809
> +webarchive/test-link-rel-icon-beforeload.html [ Skip ]
> +
> +# https://bugs.webkit.org/show_bug.cgi?id=82665
> +http/tests/webarchive/test-css-url-encoding.html [ Skip ]
> +http/tests/webarchive/test-css-url-encoding-shift-jis.html [ Skip ]
> +http/tests/webarchive/test-css-url-encoding-utf-8.html [ Skip ]
> +webarchive/test-css-url-resources-in-stylesheets.html [ Skip ]
> +webarchive/test-css-url-resources-inline-styles.html [ Skip ]

Does it make sense for these to be in this section?
Comment 5 Alex Christensen 2013-05-08 13:30:58 PDT
I'll do the swap, but I think it does make sense to put the skipped webarchive tests into the section called "Features that are disabled elsewhere and re-enabled here" because when they are fixed, they will only be enabled for Mac WK2
Comment 6 Alex Christensen 2013-05-08 13:34:04 PDT
Created attachment 201103 [details]
Patch
Comment 7 Benjamin Poulain 2013-05-09 01:57:14 PDT
How much would would it be to just support that on WebKit1?
Comment 8 Alex Christensen 2013-05-09 09:35:35 PDT
(In reply to comment #7)
> How much would would it be to just support that on WebKit1?

webarchives are already supported on Mac WebKit and I just added support for testing them with WK2, but they're a Mac-only feature that use CoreFoundation
Comment 9 Benjamin Poulain 2013-05-09 14:19:00 PDT
Comment on attachment 201103 [details]
Patch

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

> LayoutTests/platform/wk2/TestExpectations:99
> +# non-Mac ports don't support webarchives
> +webarchive
> +svg/webarchive

I would prefer if you add those skip in EFL/QT/GTK's TestExpectations.

Otherwise, it would be really easy to add a test skipped on WK2 by accident.
Comment 10 Alex Christensen 2013-05-09 15:55:28 PDT
Created attachment 201296 [details]
Patch
Comment 11 Benjamin Poulain 2013-05-09 17:03:29 PDT
Comment on attachment 201296 [details]
Patch

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

> LayoutTests/platform/efl-wk2/TestExpectations:289
> +# non-Mac ports don't support webarchives

Webkit comment style:
comments are complete sentences with Upper case for the first character and a period.

non-Mac port don't -> EFL doesn't.

> LayoutTests/platform/gtk-wk2/TestExpectations:199
> +# non-Mac ports don't support webarchives

ditto.

> LayoutTests/platform/qt-5.0-mac-wk2/TestExpectations:86
> +# QT Mac port doesn't support webarchives

QT -> Qt.

> LayoutTests/platform/qt-5.0-wk2/TestExpectations:104
> +# non-Mac ports don't support webarchives

ditto.

> LayoutTests/platform/wk2/TestExpectations:97
> +# setIconDatabaseEnabled not implemented in WebKitTestRunner

Comment style.
Comment 12 Alex Christensen 2013-05-09 17:17:20 PDT
Created attachment 201305 [details]
Patch
Comment 13 WebKit Commit Bot 2013-05-09 18:45:48 PDT
Comment on attachment 201305 [details]
Patch

Clearing flags on attachment: 201305

Committed r149859: <http://trac.webkit.org/changeset/149859>
Comment 14 WebKit Commit Bot 2013-05-09 18:45:51 PDT
All reviewed patches have been landed.  Closing bug.