Bug 124937 – Move PrettyPatch related code to prettypatch.py

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Dániel Bátyai]

Moved all PrettyPatch related code from base.py to prettypatch.py

[Dániel Bátyai]

Created attachment 217947 [details]
Proposed patch

[Ryosuke Niwa]

Comment on attachment 217947 [details]
Proposed patch

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

> Tools/ChangeLog:7
> +

Please explain why we're making this change. r- due to the lack of descriptions.

[Dániel Bátyai]

> Please explain why we're making this change. r- due to the lack of descriptions.

Sorry for not describing the change more clearly. The point of it was to unify the usage of prettypatch, for example tool/steps/confirmdiff.py already used it like this. Plus there was a FIXME in base.py regarding prettypatch path, which stated that prettypatch.py already had this information, so this code seems to have a better place in PrettyPatch, than in Port.

[Ryosuke Niwa]

Please mention that in the change log.

[Dániel Bátyai]

Created attachment 217992 [details]
Proposed patch

[Ryosuke Niwa]

Comment on attachment 217992 [details]
Proposed patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:200
> +        if self._port._pretty_patch.pretty_patch_available():
> +            pretty_patch = self._port._pretty_patch.pretty_patch_text(diff_filename)

We shouldn't be accessing port object's "private" variables like this.

[Dániel Bátyai]

> We shouldn't be accessing port object's "private" variables like this.

Which one would be more preferable: if I change it to public, or if I write a getter function for prettypatch?

[Ryosuke Niwa]

(In reply to comment #7)
> > We shouldn't be accessing port object's "private" variables like this.
> 
> Which one would be more preferable: if I change it to public, or if I write a getter function for prettypatch?

Either.

[Dániel Bátyai]

Created attachment 217997 [details]
Proposed patch

[WebKit Commit Bot]

Comment on attachment 217997 [details]
Proposed patch

Clearing flags on attachment: 217997

Committed r159839: <http://trac.webkit.org/changeset/159839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 124974

[Csaba Osztrogonác]

It was rolled out by http://trac.webkit.org/changeset/159847, 
because it broke run-webkit-tests. After this change run-webkit-tests
didn't generate pretty diffs anymore. Could you check and fix it?

[Dániel Bátyai]

Created attachment 218501 [details]
Proposed patch

run-webkit-tests should correctly generate pretty diffs now

[WebKit Commit Bot]

Comment on attachment 218501 [details]
Proposed patch

Clearing flags on attachment: 218501

Committed r160271: <http://trac.webkit.org/changeset/160271>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.