Bug 124937 - Move PrettyPatch related code to prettypatch.py
Summary: Move PrettyPatch related code to prettypatch.py
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 124974
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-27 06:42 PST by Dániel Bátyai
Modified: 2013-12-07 06:51 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (13.94 KB, patch)
2013-11-27 06:43 PST, Dániel Bátyai
rniwa: review-
rniwa: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (14.12 KB, patch)
2013-11-28 05:35 PST, Dániel Bátyai
rniwa: review-
rniwa: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (14.03 KB, patch)
2013-11-28 06:31 PST, Dániel Bátyai
no flags Details | Formatted Diff | Diff
Proposed patch (14.04 KB, patch)
2013-12-05 04:50 PST, Dániel Bátyai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dániel Bátyai 2013-11-27 06:42:32 PST
Moved all PrettyPatch related code from base.py to prettypatch.py
Comment 1 Dániel Bátyai 2013-11-27 06:43:28 PST
Created attachment 217947 [details]
Proposed patch
Comment 2 Ryosuke Niwa 2013-11-28 02:37:18 PST
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.
Comment 3 Dániel Bátyai 2013-11-28 05:17:36 PST
> 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.
Comment 4 Ryosuke Niwa 2013-11-28 05:19:16 PST
Please mention that in the change log.
Comment 5 Dániel Bátyai 2013-11-28 05:35:25 PST
Created attachment 217992 [details]
Proposed patch
Comment 6 Ryosuke Niwa 2013-11-28 05:39:54 PST
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.
Comment 7 Dániel Bátyai 2013-11-28 05:56:22 PST
> 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?
Comment 8 Ryosuke Niwa 2013-11-28 06:04:20 PST
(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.
Comment 9 Dániel Bátyai 2013-11-28 06:31:16 PST
Created attachment 217997 [details]
Proposed patch
Comment 10 WebKit Commit Bot 2013-11-28 07:12:04 PST
Comment on attachment 217997 [details]
Proposed patch

Clearing flags on attachment: 217997

Committed r159839: <http://trac.webkit.org/changeset/159839>
Comment 11 WebKit Commit Bot 2013-11-28 07:12:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Commit Bot 2013-11-28 08:50:06 PST
Re-opened since this is blocked by bug 124974
Comment 13 Csaba Osztrogonác 2013-12-04 05:40:06 PST
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?
Comment 14 Dániel Bátyai 2013-12-05 04:50:41 PST
Created attachment 218501 [details]
Proposed patch

run-webkit-tests should correctly generate pretty diffs now
Comment 15 WebKit Commit Bot 2013-12-07 06:51:20 PST
Comment on attachment 218501 [details]
Proposed patch

Clearing flags on attachment: 218501

Committed r160271: <http://trac.webkit.org/changeset/160271>
Comment 16 WebKit Commit Bot 2013-12-07 06:51:22 PST
All reviewed patches have been landed.  Closing bug.