Extract an independent webkit_support_gfx target.

Extract an independent webkit_support_gfx target.

The new webkit_support_gfx, used by DumpRenderTree and ImageDiff,
doesn't depend on most other parts of chromium. This could make building
the individual target of ImageDiff fast. It's implemented by duplicating
most code in ui/gfx/codec/png_codec.cc, after removing code related to
Skia.

This change is required to make ImageDiff for Android, and can benefit
other platforms, too. On Android, most targets are built for
toolsets:target. while ImageDiff needs to be build for toolsets:host.
Currently, building the standalone target of ImageDiff depends on about
85 other targets (try 'make ImageDiff'), which is inefficient. After the
change, ImageDiff only depends on webkit_support_gfx, libpng, and
zlib.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105731

[tony, 2011-10-12 16:50:48]

In general, this approach seems OK.  I think a good follow up would be to move
libwebkit_support_gfx into ui/gfx/codec (rename it to libpng_codec or something)
so we can share the code between libui.a and libwebkit_support.a.  This may be
tricky to get right in the components build, so I think it's OK to do this as a
follow up change.

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support.gypi
File webkit/support/webkit_support.gypi (right):

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support.g...
webkit/support/webkit_support.gypi:138: '../..',
use '<(DEPTH)' instead.

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
File webkit/support/webkit_support_gfx.cc (right):

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
webkit/support/webkit_support_gfx.cc:6: #include "stdlib.h"
Nit: I think we normally use <> for system includes.

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
File webkit/support/webkit_support_gfx.h (right):

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
webkit/support/webkit_support_gfx.h:17: enum ColorFormat {
Can we move this to the .cc file and put it in an anonymous namespace?

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
webkit/support/webkit_support_gfx.h:35: struct Comment {
This too.

[HaoZheng, 2011-10-13 03:16:02]

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support.gypi
File webkit/support/webkit_support.gypi (right):

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support.g...
webkit/support/webkit_support.gypi:138: '../..',
On 2011/10/12 16:50:48, tony wrote:
> use '<(DEPTH)' instead.

Done.

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
File webkit/support/webkit_support_gfx.cc (right):

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
webkit/support/webkit_support_gfx.cc:6: #include "stdlib.h"
On 2011/10/12 16:50:48, tony wrote:
> Nit: I think we normally use <> for system includes.

Done.

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
File webkit/support/webkit_support_gfx.h (right):

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
webkit/support/webkit_support_gfx.h:17: enum ColorFormat {
On 2011/10/12 16:50:48, tony wrote:
> Can we move this to the .cc file and put it in an anonymous namespace?

Done.

http://codereview.chromium.org/8226018/diff/1/webkit/support/webkit_support_g...
webkit/support/webkit_support_gfx.h:35: struct Comment {
On 2011/10/12 16:50:48, tony wrote:
> This too.

Done.

[Satish, 2011-10-13 08:46:32]

+1 to Tony's earlier suggestion of avoiding code duplication. Could we do it as
part of this CL itself and move the common code to a separate .cc file that can
be reused by both webkit_gfx_support.cc and png_codec.cc?

[HaoZheng, 2011-10-13 09:26:37]

On 2011/10/13 08:46:32, Satish wrote:
> +1 to Tony's earlier suggestion of avoiding code duplication. Could we do it
as
> part of this CL itself and move the common code to a separate .cc file that
can
> be reused by both webkit_gfx_support.cc and png_codec.cc?

I've thought about that. The main obstacle to reuse is separation of code
related to Skia. It's not easy to move those code out, and still reuse the
de(en)coding functions. I cannot come to a good solution. Another issue is
logging utils like DCHECK/NOTREACHED, which is replaced by a simple DEFINE in
this change. I think it's acceptable for testing tools like DumpRenderTree and
ImageDiff, but I'm not sure whether other chromium components could use such
stuffs, probably no. So I think leaving these to a future CL is better.

BTW, I think the most ideal way is to sort out the chromium dependencies tree.
Then 'make ImageDiff' would not require to build almost most part of chromium.
At least, components like 'base' should not depend on other components.

[tony, 2011-10-13 17:42:50]

Is the difficulty in moving skia related code into a separate file because of
PngDecoderState?  Maybe we can move that to a header and make the skia version a
subclass of it.

A third idea: For _toolset=="target", add a c define like PNG_CODEC_NO_DEPS or
something.  In pngcodec.*, put all the skia and base specific code behind #ifs.

If there are lots of #ifs, this might be more ugly.

[HaoZheng, 2011-10-14 00:38:44]

On 2011/10/13 17:42:50, tony wrote:
> Is the difficulty in moving skia related code into a separate file because of
> PngDecoderState?  Maybe we can move that to a header and make the skia version
a
> subclass of it.
> 
> A third idea: For _toolset=="target", add a c define like PNG_CODEC_NO_DEPS or
> something.  In pngcodec.*, put all the skia and base specific code behind
#ifs.
> 
> If there are lots of #ifs, this might be more ugly.

Yes. I have considered the possibility and also thought it would be more ugly.
In my estimate, there would be about 10 ifdefs. Is is acceptable? From point of
code reuse, howerver, I would prefer this approach, because even we extract a
subclass of PngDecoderStateSkia, we still need a lot of refactoring to reuse
existing code. I can explore either way further.

[tony, 2011-10-14 17:13:26]

LGTM.

I think the fork is less bad than a bunch of #ifdefs.  This code hasn't changed
in years, so it's unlikely that there will be bugs in it.

http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
File webkit/support/webkit_support_gfx.cc (right):

http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
webkit/support/webkit_support_gfx.cc:6: #include <stdlib.h>
Nit: add a blank line before the system include

http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
webkit/support/webkit_support_gfx.cc:18: 
Nit: Please add a comment explaining why this code is duplicated (similar to the
change description, maybe not as detailed).

[HaoZheng, 2011-10-16 06:02:51]

http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
File webkit/support/webkit_support_gfx.cc (right):

http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
webkit/support/webkit_support_gfx.cc:6: #include <stdlib.h>
On 2011/10/14 17:13:27, tony wrote:
> Nit: add a blank line before the system include

Done.

http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
webkit/support/webkit_support_gfx.cc:18: 
On 2011/10/14 17:13:27, tony wrote:
> Nit: Please add a comment explaining why this code is duplicated (similar to
the
> change description, maybe not as detailed).

Done.

[commit-bot: I haz the power, 2011-10-16 06:03:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenghao@google.com/8226018/13001

[HaoZheng, 2011-10-16 06:04:17]

On 2011/10/14 17:13:26, tony wrote:
> LGTM.
> 
> I think the fork is less bad than a bunch of #ifdefs.  This code hasn't
changed
> in years, so it's unlikely that there will be bugs in it.
> 
>
http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
> File webkit/support/webkit_support_gfx.cc (right):
> 
>
http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
> webkit/support/webkit_support_gfx.cc:6: #include <stdlib.h>
> Nit: add a blank line before the system include
> 
>
http://codereview.chromium.org/8226018/diff/5001/webkit/support/webkit_suppor...
> webkit/support/webkit_support_gfx.cc:18: 
> Nit: Please add a comment explaining why this code is duplicated (similar to
the
> change description, maybe not as detailed).

Thanks!

[commit-bot: I haz the power, 2011-10-16 06:19:59]

Try job failure for 8226018-13001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-10-16 08:50:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenghao@google.com/8226018/17002

[commit-bot: I haz the power, 2011-10-16 09:28:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenghao@google.com/8226018/13003

[commit-bot: I haz the power, 2011-10-16 10:35:42]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 105723, job name
was 8226018-13003 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-10-16 11:22:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenghao@google.com/8226018/13003

[commit-bot: I haz the power, 2011-10-16 12:31:11]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 105727, job name
was 8226018-13003 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-10-16 13:05:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenghao@google.com/8226018/18009

[commit-bot: I haz the power, 2011-10-16 14:47:58]

Change committed as 105731