Bug 136711 – Merge some Blink webkitpy Changes

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Brent Fulgham]

The Windows port has experienced numerous failures in the webkitpy system due to improper mixing of unicode and ascii data in the string handling (and elsewhere). In an effort to clear this up, I am bringing our webkitpy libraries in closer sync with Blink's implementation.

This is a first patch that covers a variety of subsections. Subsequent patches will deal with specific subsystems.

[Brent Fulgham]

Created attachment 237904 [details]
Patch

[Brent Fulgham]

Created attachment 237907 [details]
Correct merge error.

[Build Bot]

Comment on attachment 237907 [details]
Correct merge error.

Attachment 237907 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5578578913656832

New failing tests:
http/tests/media/video-throttled-load-metadata.html
http/tests/media/video-served-as-text.html
http/tests/media/video-error-does-not-exist.html

[Build Bot]

Created attachment 237917 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Brent Fulgham]

I don't see these http video failures locally. According to the logs, these are all "notifyDone" failures, which means the page probably got hung or crashed somewhere.

It doesn't seem likely that these webkitpy changes would cause just these video HTTP tests to fail; I would expect all HTTP tests to fail if there was a new bug.

[Alexey Proskuryakov]

Yes, these failures are unrelated, as they happened on multiple patches today.

[Csaba Osztrogonác]

Comment on attachment 237907 [details]
Correct merge error.

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

Merging many different fixes in one huge patch isn't a good idea.
Additionally we should add references to the original changes
and describe the reason why we need a fix to be merged.

> Tools/Scripts/webkitpy/tool/grammar.py:41
> -def pluralize(count, noun, showCount=True):
> +def pluralize(noun, count, showCount=True):

I don't agree with this change. It was modified intentionally 
to be be more understandable, please don't change it back.

To get "2 patches" now, you can simple do it with pluralize(2, "patch").
Why do you think if pluralize("patch", 2) would be better< To be closer to
Blink's webkitpy isn't a good reason.

[Csaba Osztrogonác]

Comment on attachment 237907 [details]
Correct merge error.

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

> Tools/Scripts/webkitpy/layout_tests/views/printing.py:316
> -            summary = "%s ran as expected, %d didn't%s:" % (grammar.pluralize(expected, "test"), unexpected, incomplete_str)
> +            summary = "%s ran as expected%s, %d didn't%s%s:" % (grammar.pluralize('test', expected), expected_summary_str, unexpected, incomplete_str, timing_summary)

Changing the output of the run-webkit-tests would break the buildbots.

The result parser of run-webkit-test in master.cfg should be changed to 
handle the new format properly. And the buildmaster should be restarted too.

[Brent Fulgham]

This seems like the wrong approach. Closing!