So this is a really large patch. Most of it is splitting up skdiff_main.cpp into ...
12 years, 1 month ago
(2012-11-28 19:07:23 UTC)
#1
So this is a really large patch. Most of it is splitting up skdiff_main.cpp into
more manageable size chunks. These chunks are roughly (1) comparison, (2) file
utilities, (3) summary output, and (4) the main driver. There are also a few
changes to make this split more clean, such as removing the file operations from
the summary output step, and changing the DiffRecord to contain all the
pertinent information required for a proper report.
This patch also contains a new utility which takes advantage of this split
(which was the motivation for actually doing the work). This new utility,
skimagediff, can be used to do a file to file image diff. As such skimagediff
can be used with svn diff --diff-cmd or with git difftool. This allows for the
repository to be the driver of the diff operation, allowing for arbitrary diffs.
There is some work yet to be done (see TODOs in skdiff_image.cpp) but
skimagediff is quite useful already.
Overall I like this change. Sorry for the delayed response. https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp File tools/skdiff.cpp (right): https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp#newcode15 ...
12 years, 1 month ago
(2012-11-29 17:13:05 UTC)
#2
This change also updates the test expectations so that it can be seen what the ...
12 years, 1 month ago
(2012-12-04 17:36:26 UTC)
#6
This change also updates the test expectations so that it can be seen what the
new output will look like. It will also be necessary to update the build bots
(same change as that to run.sh), but that can be done separately (while
unchanged some of them will print a warning).
Hopefully I've also addressed all of the comments on Patch Set 1. Elliot, thanks
for the tests, they found at least one bug and a number of things which needed
improvement or clarification. The new skimagediff should also be tested, but
it's still a work in progress and I'd prefer to get this in first.
https://codereview.appspot.com/6850115/diff/1/gyp/tools.gyp
File gyp/tools.gyp (right):
https://codereview.appspot.com/6850115/diff/1/gyp/tools.gyp#newcode33
gyp/tools.gyp:33: '../tools/skdiff_html.cpp',
On 2012/11/29 17:36:05, epoger wrote:
> Maybe we should create a tools/skdiff directory and put these files in there?
> (As a separate CL; no need to derail this one with such a change.)
Yes, I thought about that as well. This is something I'd like to do, but this CL
is already rather large.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp
File tools/skdiff.cpp (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp#newcode15
tools/skdiff.cpp:15: if (0 == strcmp("EqualBits", name)) {
On 2012/11/29 17:13:05, scroggo-work wrote:
> Should these be declared constants somewhere?
Yes, they are, right here :-) This is just cut-paste moved from skdiff_main.cpp.
The DiffRecord::Resource ones below are new and just followed this existing
pattern. It does seem like a good idea to put these in a static array somewhere.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp#newcode15
tools/skdiff.cpp:15: if (0 == strcmp("EqualBits", name)) {
On 2012/11/29 18:02:09, epoger wrote:
> On 2012/11/29 17:13:05, scroggo-work wrote:
> > Should these be declared constants somewhere?
>
> Maybe a good way to handle this would be to put this string<->enum translation
> code right next to the enum declaration.
>
> As of this CL, the DiffRecord::Result enum declaration is in skdiff.h ; maybe
> you should move it into its own file, along with the translation routines?
This was just cut-paste, but could be done better. Of course, before there was
no .cpp/.h distinction as it was all in the monolithic skdiff_main.cpp. One of
the issues is that the enum needs to be in a header, but any static string array
should be in a cpp to avoid having a copy in every translation unit.
The names here are probably ok to be associated with the enum, but the
descriptions seem like they are actually in the wrong place entirely. The
descriptions are user strings used by skdiff_main and skdiff_image, but have
nothing really to do with skdiff itself. Instead of being moved closer to the
enum they should be moved further away. I'll have to think about where they
should go. For the moment they probably should be in a static string array at
least.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp#newcode55
tools/skdiff.cpp:55: DiffRecord::Resource::Status
DiffRecord::Resource::getStatusByName(const char *name) {
On 2012/11/29 18:02:09, epoger wrote:
> same issue as DiffRecord::Result enum
Moved strings to static array and changed implementation accordingly.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp#newcode140
tools/skdiff.cpp:140: return false;
On 2012/11/29 17:13:05, scroggo-work wrote:
> Can we document the meaning of the return value?
>
> Also, if status == kStatusCount, shouldn't we exit before the loop?
Done. See declaration for comment.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.h
File tools/skdiff.h (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.h#newcode17
tools/skdiff.h:17: #if SK_BUILD_FOR_WIN32
On 2012/11/29 18:02:09, epoger wrote:
> I can't tell how much of this CL is just moving code around vs. adding new
code.
> I would have preferred those to have been done as two separate CLs...
although
> I know that can be quite difficult/time consuming to do in some cases. So it
> may not have been practical in this case.
I thought about that... but I didn't know where I was going until I got there.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.h#newcode32
tools/skdiff.h:32: kCouldNotDecode_Status,
On 2012/11/29 18:02:09, epoger wrote:
> This seems like a collection of orthogonal status bits, not distinct states.
They aren't really orthogonal, it's more like moving up the ladder. For example,
kCouldNotDecode_Status implies the file was specified, exists, and could be
read. Maybe this needs to be better documented. If you can think of a clearer
way to express this, I'm open to suggestions.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.h#newcode41
tools/skdiff.h:41: kUnspecified_Status,
On 2012/11/29 17:13:05, scroggo-work wrote:
> Is this status ever used?
It will be in Patch Set 2 in response to one of your other comments. Also, it's
what will (probably) be used in skdiff_image when it handles the file /dev/null
properly.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.h#newcode58
tools/skdiff.h:58: static bool getMatchingStatuses(const char* selector, bool
statuses[kStatusCount]);
On 2012/11/29 17:13:05, scroggo-work wrote:
> Can we list the valid values for 'selector'?
Added comment.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp
File tools/skdiff_image.cpp (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp#newcode19
tools/skdiff_image.cpp:19: static void create_diff_images (DiffMetricProc dmp,
On 2012/11/29 17:13:05, scroggo-work wrote:
> Can this share code with create_diff_images in skdiff_main?
I was thinking about trying, but while they are similar they have some rather
different assumptions. They are already sharing a lot of code, everything in
skdiff_utils.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp#newcode65
tools/skdiff_image.cpp:65: {
On 2012/11/29 17:13:05, scroggo-work wrote:
> I think the style guide specifies this should go on the end of the line above,
> but in this case (if statement goes onto a second line), I think this is much
> clearer. Perhaps we should recommend this in the style guide?
Heh, yes. I kept getting lost with the brace on the previous line.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp#newcode84
tools/skdiff_image.cpp:84: " %s <baseDir> <comparisonDir> [outputDir] \n" ,
argv0);
On 2012/11/29 18:02:09, epoger wrote:
> please update the usage message; it seems to be only partially correct
Ah yes, I updated the arguments but not the usage line. Done.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp#newcode116
tools/skdiff_image.cpp:116: int tool_main(int argc, char** argv) {
On 2012/11/29 18:02:09, epoger wrote:
> Consider adding the new tool to
> https://code.google.com/p/skia/source/browse/trunk/tools/tests/run.sh ; I have
> found that to be a good way to validate changes to skdiff.
>
> You should also make sure that skdiff still passes those tests!
Hmmm... yes, I think that the test would need to be updated with this change as
well. With the statuses (and removal of kOther) the error messages are more
specific. For example
- [_] 1 file pairs contain different bits and are not parsable images
- [_] 2 file pairs missing from comparisonDir
- [_] 2 file pairs missing from baseDir
---
+ [_] 5 file pairs could not be compared
+ [_] 1 file pairs could not be decoded in baseDir and could not be decoded
in comparisonDir
+ [_] 2 file pairs found in baseDir and not found in comparisonDir
+ [_] 1 file pairs not found in baseDir and decoded in comparisonDir
+ [_] 1 file pairs not found in baseDir and could not be decoded in
comparisonDir
The wording in the generated .html file also differs similarly.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp#newcode179
tools/skdiff_image.cpp:179: for (int comparison = 0; comparison <
DiffRecord::Resource::kStatusCount; ++comparison) {
On 2012/11/29 17:13:05, scroggo-work wrote:
> 100 chars
Done. Note that while I like Resource being scoped to DiffRecord, the lines are
just too long. For this reason I've split it out as DiffResource to bring down
the length of a number of lines.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp#newcode288
tools/skdiff_image.cpp:288: // difftool.<tool>.cmd: skimagediff $LOCAL
$REMOTE -L "$MERGED\t(base)" -L "$MERGED\t(comparison)"
On 2012/11/29 17:13:05, scroggo-work wrote:
> 100 chars
Done.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp#newcode328
tools/skdiff_image.cpp:328: create_diff_images(diffProc, colorThreshold,
baseFile, comparisonFile, outputDir, outputFile, &dr);
On 2012/11/29 17:13:05, scroggo-work wrote:
> 100 chars
Done.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_image.cpp#newcode334
tools/skdiff_image.cpp:334: printf("Comparison %s.\n",
DiffRecord::Resource::getStatusDescription(dr.fComparison.fStatus));
On 2012/11/29 17:13:05, scroggo-work wrote:
> 100 chars
Done.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_main.cpp
File tools/skdiff_main.cpp (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_main.cpp#newcode70
tools/skdiff_main.cpp:70: printf("%d file pairs %s in baseDir and %s in
comparisonDir",
On 2012/11/29 17:13:05, scroggo-work wrote:
> Why printf and not SkDebugf?
Just doing what the rest of the code here is doing. This isn't a debug string,
it is actual output.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_main.cpp#newcode149
tools/skdiff_main.cpp:149: fResultsOfType[drp->fResult].push(new
SkString(drp->fBase.fFilename));
On 2012/11/29 17:13:05, scroggo-work wrote:
> SkNEW_ARGS
Yes, it probably should be, but there are a number of other uses of 'new' in
here, and this is just a utility. Can I push this off to next time around?
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_main.cpp#newcode512
tools/skdiff_main.cpp:512: "\n code if any file
pairs yeilded this status."
On 2012/11/29 17:13:05, scroggo-work wrote:
> yielded*
Done.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_main.cpp#newcode614
tools/skdiff_main.cpp:614: for (int comparison = 0; comparison <
DiffRecord::Resource::kStatusCount; ++comparison) {
On 2012/11/29 17:13:05, scroggo-work wrote:
> 100 chars
Done.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_main.cpp#newcode759
tools/skdiff_main.cpp:759: for (int comparison = 0; comparison <
DiffRecord::Resource::kStatusCount; ++comparison) {
On 2012/11/29 17:13:05, scroggo-work wrote:
> 100 chars
Done.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_utils.cpp
File tools/skdiff_utils.cpp (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_utils.cpp#newcode77
tools/skdiff_utils.cpp:77: static void force_all_opaque(const SkBitmap& bitmap)
{
On 2012/11/29 17:13:05, scroggo-work wrote:
> I think this is also used in PictureRenderer and gmmain. At some point it
might
> be nice to consolidate.
Indeed. One of the nice things about pulling this out into skdiff_utils is that
they're a little less buried.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_utils.cpp#newcode160
tools/skdiff_utils.cpp:160: drp->fDifference.fStatus =
DiffRecord::Resource::kSpecified_Status;
On 2012/11/29 17:13:05, scroggo-work wrote:
> What does specified status mean? It looks to mean we've set up a file name for
> it, but the two files are equal, so we didn't write it to a file. What is this
> information used for?
It is an odd fit. I've put comments on the enums to help explain, but Status was
more written around reading, not writing. Mostly I just wanted to re-use these
Statuses to signal to a reporter if the diff images were requested or if they
were actually written or not. Specifying kDoesNotExist above (as a default) is
actually incorrect, it should be set to kUnspecified if outputDir.isEmpty().
I'll fix that.
Note that resource statuses are for individual files, while diff results are for
the diff.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_utils.h
File tools/skdiff_utils.h (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_utils.h#newcode34
tools/skdiff_utils.h:34: /// Given a image filename, returns the name of the
file containing the
On 2012/11/29 17:13:05, scroggo-work wrote:
> Don't we prefer
> /**
>
> ? I know we prefer 'doxygen-style comments' but I'm not sure whether '///'
> qualifies.
>
> I think these are just copy pastes so it's okay to leave as is.
Yeah, the ones I added are /** but the rest are just cut-paste for the split.
I'll just convert these anyway so they're consistent.
/// is a valid doxygen comment, but doxygen understands too many formats (see
http://www.stack.nl/~dimitri/doxygen/docblocks.html ).
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_utils.h#newcode34
tools/skdiff_utils.h:34: /// Given a image filename, returns the name of the
file containing the
On 2012/11/29 17:13:05, scroggo-work wrote:
> an* image
Done.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff_utils.h#newcode38
tools/skdiff_utils.h:38: /// Given a image filename, returns the name of the
file containing the
On 2012/11/29 17:13:05, scroggo-work wrote:
> an* image
Done.
https://codereview.appspot.com/6850115/diff/9001/tools/tests/run.sh
File tools/tests/run.sh (right):
https://codereview.appspot.com/6850115/diff/9001/tools/tests/run.sh#newcode63
tools/tests/run.sh:63: skdiff_test "--failonresult DifferentPixels
--failonresult DifferentSizes --failonresult Unknown --failonstatus
CouldNotDecode,CouldNotRead any --failonstatus any CouldNotDecode,CouldNotRead
--listfilenames --nodiffs $SKDIFF_TESTDIR/baseDir $SKDIFF_TESTDIR/comparisonDir"
"$SKDIFF_TESTDIR/test2"
I believe these two --failonstatus are the equivalent of the previous (rather
vague) --failonresult DifferentOther.
Some small nits, but overall lgtm. https://codereview.appspot.com/6850115/diff/1/tools/skdiff_main.cpp File tools/skdiff_main.cpp (right): https://codereview.appspot.com/6850115/diff/1/tools/skdiff_main.cpp#newcode149 tools/skdiff_main.cpp:149: fResultsOfType[drp->fResult].push(new SkString(drp->fBase.fFilename)); On ...
12 years, 1 month ago
(2012-12-04 18:14:02 UTC)
#7
LGTM Thanks for this work, Ben! My only substantial beef at this point is the ...
12 years, 1 month ago
(2012-12-05 16:26:35 UTC)
#10
LGTM
Thanks for this work, Ben!
My only substantial beef at this point is the enum declaration stuff (details
below), but I would be OK even if it was committed as-is, if that is your
inclination.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp
File tools/skdiff.cpp (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp#newcode15
tools/skdiff.cpp:15: if (0 == strcmp("EqualBits", name)) {
> This was just cut-paste, but could be done better. Of course, before there was
> no .cpp/.h distinction as it was all in the monolithic skdiff_main.cpp. One of
> the issues is that the enum needs to be in a header, but any static string
array
> should be in a cpp to avoid having a copy in every translation unit.
In this particular case, I think it would be fine to put the static string array
in the header file, because the enum is only used by the skdiff tools (small
number of translation units). It would be different for something within the
skia libraries...
If the copy-of-stringarray-per-translation-unit issue is really a problem, this
might be the cleanest way to do it:
tools/skdiff_result.h:
- declare the DiffResult enum
- declare the functions that convert result names
(e.g. "EqualBits") to/from DiffResult enum values
tools/skdiff_result.cpp:
- define the functions that convert result names
(e.g. "EqualBits") to/from DiffResult enum values
At any rate, please put the string array closer to the enum declaration. As it
stands now, it would just be way too easy for them to get out of sync. (For
that reason, my inclination would be to declare/define the string array within
the header file, duplication-per-translation-unit be damned.)
> The names here are probably ok to be associated with the enum, but the
> descriptions seem like they are actually in the wrong place entirely. The
> descriptions are user strings used by skdiff_main and skdiff_image, but have
> nothing really to do with skdiff itself.
I could see arguments on either side on this one. Yes, the result descriptions
(as opposed to names) have more to do with what a particular application wants
to report to its user. But on the other hand, regardless of which application
is reporting a result, the meaning of each result type does not change!
All in all, I would prefer for the descriptions to be defined as close as
possible to the enum declaration, but (as opposed to the names) I would be
content either way.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.h
File tools/skdiff.h (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.h#newcode17
tools/skdiff.h:17: #if SK_BUILD_FOR_WIN32
On 2012/12/04 17:36:26, bungeman wrote:
> On 2012/11/29 18:02:09, epoger wrote:
> > I can't tell how much of this CL is just moving code around vs. adding new
> code.
> > I would have preferred those to have been done as two separate CLs...
> although
> > I know that can be quite difficult/time consuming to do in some cases. So
it
> > may not have been practical in this case.
>
> I thought about that... but I didn't know where I was going until I got there.
I been there, brother.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.h#newcode32
tools/skdiff.h:32: kCouldNotDecode_Status,
On 2012/12/04 17:36:26, bungeman wrote:
> On 2012/11/29 18:02:09, epoger wrote:
> > This seems like a collection of orthogonal status bits, not distinct states.
>
> They aren't really orthogonal, it's more like moving up the ladder. For
example,
> kCouldNotDecode_Status implies the file was specified, exists, and could be
> read. Maybe this needs to be better documented. If you can think of a clearer
> way to express this, I'm open to suggestions.
I think the descriptions you have in place now are really helpful, thanks.
https://codereview.appspot.com/6850115/diff/14004/tools/skdiff.cpp
File tools/skdiff.cpp (right):
https://codereview.appspot.com/6850115/diff/14004/tools/skdiff.cpp#newcode112
tools/skdiff.cpp:112: char* delimiterPtr = strchr(selector, ',');
please define/use a kComma constant (or kDelimiterChar if you prefer), so we
make sure this stays in sync with the character replacement in line 136.
https://codereview.appspot.com/6850115/diff/14004/tools/tests/run.sh
File tools/tests/run.sh (right):
https://codereview.appspot.com/6850115/diff/14004/tools/tests/run.sh#newcode62
tools/tests/run.sh:62: #skdiff_test "--failonresult DifferentPixels
--failonresult DifferentSizes --failonresult DifferentOther --failonresult
Unknown --listfilenames --nodiffs $SKDIFF_TESTDIR/baseDir
$SKDIFF_TESTDIR/comparisonDir" "$SKDIFF_TESTDIR/test2"
go ahead and delete this line?
P.S. Please prepare a change to https://code.google.com/p/skia/source/browse/buildbot/slave/skia_slave_scripts/compare_gms.py that can be committed at the same time ...
12 years, 1 month ago
(2012-12-05 16:27:50 UTC)
#11
Added change to compare_gms.py, but in so doing had to go up a directory level ...
12 years, 1 month ago
(2012-12-05 18:33:54 UTC)
#12
Added change to compare_gms.py, but in so doing had to go up a directory level
so now the files don't match the Base URL. As a result it's difficult to see the
changes between Patch Sets 3 and 4. However, the only changes made were to
compare_gms.py, and the changes commented on below.
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp
File tools/skdiff.cpp (right):
https://codereview.appspot.com/6850115/diff/1/tools/skdiff.cpp#newcode15
tools/skdiff.cpp:15: if (0 == strcmp("EqualBits", name)) {
On 2012/12/05 16:26:35, epoger wrote:
> > This was just cut-paste, but could be done better. Of course, before there
was
> > no .cpp/.h distinction as it was all in the monolithic skdiff_main.cpp. One
of
> > the issues is that the enum needs to be in a header, but any static string
> array
> > should be in a cpp to avoid having a copy in every translation unit.
>
> In this particular case, I think it would be fine to put the static string
array
> in the header file, because the enum is only used by the skdiff tools (small
> number of translation units). It would be different for something within the
> skia libraries...
>
> If the copy-of-stringarray-per-translation-unit issue is really a problem,
this
> might be the cleanest way to do it:
>
> tools/skdiff_result.h:
> - declare the DiffResult enum
> - declare the functions that convert result names
> (e.g. "EqualBits") to/from DiffResult enum values
> tools/skdiff_result.cpp:
> - define the functions that convert result names
> (e.g. "EqualBits") to/from DiffResult enum values
>
> At any rate, please put the string array closer to the enum declaration. As
it
> stands now, it would just be way too easy for them to get out of sync. (For
> that reason, my inclination would be to declare/define the string array within
> the header file, duplication-per-translation-unit be damned.)
>
> > The names here are probably ok to be associated with the enum, but the
> > descriptions seem like they are actually in the wrong place entirely. The
> > descriptions are user strings used by skdiff_main and skdiff_image, but have
> > nothing really to do with skdiff itself.
>
> I could see arguments on either side on this one. Yes, the result
descriptions
> (as opposed to names) have more to do with what a particular application wants
> to report to its user. But on the other hand, regardless of which application
> is reporting a result, the meaning of each result type does not change!
>
> All in all, I would prefer for the descriptions to be defined as close as
> possible to the enum declaration, but (as opposed to the names) I would be
> content either way.
>
Oh the craziness of ODR and 'static' meaning many things. The 'static' in
DiffRecord's declaration of 'static char const * const
StatusNames[DiffResource::kStatusCount];' means one copy for this class with
external linkage. You actually can't put 'static' on the definition (outside the
class) because that would mean internal linkage and the declaration has external
linkage. I think we could get the string array in the header, but it's
declaration would have to be moved out of DiffRecord to global scope (or put in
a namespace).
On the other hand these are much less likely to get out of sync now that they
are declared to have size DiffResource::kStatusCount / DiffRecord::kResultCount.
If an enum value is added or removed then you get a compile error. (No
protection for re-ordering or changing them, but that seems unlikely.) Note that
C++11 allows for these sort of const arrays to be initialized in the class
declaration, making them 'static' but get defined only in the first translation
unit (like the vtable), so if we can wait a few years this will be easier :-)
For now I'm going to leave it as arrays (as in Patch Set 3) and worry about more
fixing at a latter date. It would be nice to have something like
sfnt/SkTypedEnum.h named SkNamedEnum.h which used macro magic to make all of
this work in one place, but this is a much larger change than I want to make
right now.
https://codereview.appspot.com/6850115/diff/14004/tools/skdiff.cpp
File tools/skdiff.cpp (right):
https://codereview.appspot.com/6850115/diff/14004/tools/skdiff.cpp#newcode112
tools/skdiff.cpp:112: char* delimiterPtr = strchr(selector, ',');
On 2012/12/05 16:26:36, epoger wrote:
> please define/use a kComma constant (or kDelimiterChar if you prefer), so we
> make sure this stays in sync with the character replacement in line 136.
Done.
https://codereview.appspot.com/6850115/diff/14004/tools/tests/run.sh
File tools/tests/run.sh (right):
https://codereview.appspot.com/6850115/diff/14004/tools/tests/run.sh#newcode62
tools/tests/run.sh:62: #skdiff_test "--failonresult DifferentPixels
--failonresult DifferentSizes --failonresult DifferentOther --failonresult
Unknown --listfilenames --nodiffs $SKDIFF_TESTDIR/baseDir
$SKDIFF_TESTDIR/comparisonDir" "$SKDIFF_TESTDIR/test2"
On 2012/12/05 16:26:36, epoger wrote:
> go ahead and delete this line?
Done.
Issue 6850115: Update skdiff.
(Closed)
Created 12 years, 1 month ago by bungeman
Modified 12 years, 1 month ago
Reviewers: epoger, Leon
Base URL: http://skia.googlecode.com/svn/trunk/
Comments: 76
