Bug 27965 - ImageSourceFoo.cpp contains lots of Copy+Paste code and should be refactored
Summary: ImageSourceFoo.cpp contains lots of Copy+Paste code and should be refactored
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks: 26467
  Show dependency treegraph
 
Reported: 2009-08-03 17:17 PDT by Adam Treat
Modified: 2012-11-20 00:30 PST (History)
9 users (show)

See Also:


Attachments
Patch to merge ImageSourceCairo.cpp and ImageSourceSkia.cpp (33.42 KB, patch)
2009-08-03 17:21 PDT, Adam Treat
no flags Details | Formatted Diff | Diff
Move ImageSourceCairo.cpp to ImageSource.cpp (19.48 KB, patch)
2009-08-11 13:10 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Merge in ImageSourceSkia.cpp (12.31 KB, patch)
2009-08-11 17:56 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Merge in ImageSourceWx.cpp (16.76 KB, patch)
2009-08-11 18:55 PDT, Peter Kasting
manyoso: review-
Details | Formatted Diff | Diff
Merge in ImageSourceWx.cpp (8.87 KB, patch)
2009-08-12 10:34 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Move createDecoder() onto ImageDecoder (9.25 KB, patch)
2009-08-27 17:08 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Remove most of ImageSourceQt.cpp (5.90 KB, patch)
2009-08-28 11:47 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Treat 2009-08-03 17:17:33 PDT
This first patch is an attempt to combine and refactor two files that are largely a result of copy+paste code.  The files in question are ImageSourceCairo.cpp and ImageSourceSkia.cpp.  A later patch will attempt to merge the third such file, ImageSourceWx.cpp.
Comment 1 Adam Treat 2009-08-03 17:21:36 PDT
Created attachment 34027 [details]
Patch to merge ImageSourceCairo.cpp and ImageSourceSkia.cpp

Some notes:

1) Please read the ChangeLog in the patch carefully for a detailed list of the changes.
2) I am not able to build either Skia or Cairo as I'm not set up for it, however I've been very careful in the refactoring and gone through it line-by-line.
Comment 2 Adam Treat 2009-08-03 17:41:04 PDT
Seems pkasting and I have clashed with efforts.  Assigning to him to deal with...
Comment 3 Adam Treat 2009-08-03 17:41:34 PDT
Comment on attachment 34027 [details]
Patch to merge ImageSourceCairo.cpp and ImageSourceSkia.cpp

Obsoleted by pkasting's next patch.
Comment 4 Peter Kasting 2009-08-11 13:10:18 PDT
Created attachment 34586 [details]
Move ImageSourceCairo.cpp to ImageSource.cpp

This moves ImageSourceCairo.cpp to ImageSource.cpp.

There are a couple questionable bits here:

* I have excluded ImageSource.cpp from the WebKit Windows CG builds, like the old ImageSourceCairo.cpp was.  Perhaps instead I should have included it but put a "!PLATFORM(CG)" atop it?  It seems uncommon to exclude "cross-platform" files from the Windows CG build...

* I removed "#include <cairo.h>".  The only potential use is for the NativeImagePtr return type from createFrameAtIndex().  Since this is being returned directly from another function, I don't think the compiler actually needs the underlying type here.  At least for Skia this type of change compiles (I tried removing "#include SkBitmap.h" from ImageSourceSkia.cpp and building); I'd like to be able to test whether it works on Cairo too.  If so, it means we can avoid pulling in any platform-specific headers, which would be great.
Comment 5 Adam Treat 2009-08-11 13:22:47 PDT
(In reply to comment #4)
> Created an attachment (id=34586) [details]
> Move ImageSourceCairo.cpp to ImageSource.cpp
> 
> This moves ImageSourceCairo.cpp to ImageSource.cpp.
> 
> There are a couple questionable bits here:
> 
> * I have excluded ImageSource.cpp from the WebKit Windows CG builds, like the
> old ImageSourceCairo.cpp was.  Perhaps instead I should have included it but
> put a "!PLATFORM(CG)" atop it?  It seems uncommon to exclude "cross-platform"
> files from the Windows CG build...

ImageSource.cpp is no more or less cross-platform than the image-decoders I would think.  If Windows CG doesn't build with image-decoders why would it build with this ImageSource.cpp?

> * I removed "#include <cairo.h>".  The only potential use is for the
> NativeImagePtr return type from createFrameAtIndex().  Since this is being
> returned directly from another function, I don't think the compiler actually
> needs the underlying type here.  At least for Skia this type of change compiles
> (I tried removing "#include SkBitmap.h" from ImageSourceSkia.cpp and building);
> I'd like to be able to test whether it works on Cairo too.  If so, it means we
> can avoid pulling in any platform-specific headers, which would be great.

Ok.  Why did you keep the 'PLATFORM(CAIRO)' call?  Because you added to Skia build, but haven't unforked it yet?

Also, it seems the patch is weirdly formatted.  The hunk to remove cairo is not included in the rest of it.  How'd that happen?
Comment 6 Peter Kasting 2009-08-11 13:50:46 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > * I have excluded ImageSource.cpp from the WebKit Windows CG builds, like the
> > old ImageSourceCairo.cpp was.
> 
> ImageSource.cpp is no more or less cross-platform than the image-decoders I
> would think.  If Windows CG doesn't build with image-decoders why would it
> build with this ImageSource.cpp?

Fair enough.  I'll leave it the way I've done it here, then.

> Why did you keep the 'PLATFORM(CAIRO)' call?  Because you added to Skia
> build, but haven't unforked it yet?

Precisely.  The WebCore.gypi file that the Chromium build uses contains all the files for all the ports, so I had to update it, but if I removed this guard I'd have gotten double symbols.  I'm trying to decide whether when merging Skia in I should remove this, change to !PLATFORM(CG), or change to PLATFORM(CAIRO) || PLATFORM(SKIA).  If I did the last one, I'd change to one of the first two once everything was merged.

> Also, it seems the patch is weirdly formatted.  The hunk to remove cairo is not
> included in the rest of it.  How'd that happen?

That was the output of svn-create-patch.  I think it purposefully splits a "move with modifications" into two pieces, the "add/delete" and "modify" parts, for ease of reviewing.  Or else it has some bug on my machine.  I don't know.
Comment 7 Adam Treat 2009-08-11 14:02:34 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > * I have excluded ImageSource.cpp from the WebKit Windows CG builds, like the
> > > old ImageSourceCairo.cpp was.
> > 
> > ImageSource.cpp is no more or less cross-platform than the image-decoders I
> > would think.  If Windows CG doesn't build with image-decoders why would it
> > build with this ImageSource.cpp?
> 
> Fair enough.  I'll leave it the way I've done it here, then.
> 
> > Why did you keep the 'PLATFORM(CAIRO)' call?  Because you added to Skia
> > build, but haven't unforked it yet?
> 
> Precisely.  The WebCore.gypi file that the Chromium build uses contains all the
> files for all the ports, so I had to update it, but if I removed this guard I'd
> have gotten double symbols.  I'm trying to decide whether when merging Skia in
> I should remove this, change to !PLATFORM(CG), or change to PLATFORM(CAIRO) ||
> PLATFORM(SKIA).  If I did the last one, I'd change to one of the first two once
> everything was merged.

I think you should remove all checks for PLATFORM... if a port doesn't use the file, they don't add it to their build.  Again, just like image-decoders :)
Comment 8 Adam Treat 2009-08-11 14:03:18 PDT
Comment on attachment 34586 [details]
Move ImageSourceCairo.cpp to ImageSource.cpp

Please watch the buildbots...
Comment 9 Peter Kasting 2009-08-11 16:20:31 PDT
Comment on attachment 34586 [details]
Move ImageSourceCairo.cpp to ImageSource.cpp

Landed in r47073, clearing flags.
Comment 10 Peter Kasting 2009-08-11 17:56:49 PDT
Created attachment 34625 [details]
Merge in ImageSourceSkia.cpp

Merge in ImageSourceSkia.cpp.

There are only a few issues of note:

* As with Cairo, Skia doesn't really need the "#include SkBitmap.h" it had, so I didn't port that over.

* Didn't bother adding the Skia check for if the decoder had failed before calling setData(), since every decode but XBM already checks that, and XBM is going away soon.

* Expanded the Cairo rejection of 0-height files to include 0-width and negative-dimension files as well.  I am told these aren't valid for any popular image formats anyway, and at least the BMP/ICO decoders I wrote already reject them.  Even though Skia didn't have this, it seemed like a safe and appropriate thing to do.

* Didn't bother porting over the Skia check for ImageDecoder::supportsAlpha() in frameHasAlphaAtIndex().  Only the JPEG decoder returns false in that function, and if the frame is complete none of the pixels should have alpha anyway.  Once all the copies of this file are merged together I'm just going to delete that function because no one else ever cares.
Comment 11 Adam Treat 2009-08-11 18:26:44 PDT
Comment on attachment 34625 [details]
Merge in ImageSourceSkia.cpp

> -
> -    return m_decoder->frameBufferAtIndex(index)->hasAlpha();
> +    // When a frame has not finished decoding, always mark it as having alpha.
> +    // Ports that check the result of this function to determine their
> +    // compositing op need this in order to not draw the undecoded portion as
> +    // black.
> +    // TODO: Perhaps we should ensure that each individual decoder returns true
> +    // in this case.
> +  return frameIsCompleteAtIndex(index) ?
> +      m_decoder->frameBufferAtIndex(index)->hasAlpha() : true;

Looks like an indentation problem.  Otherwise looks good!  r+ and you can fix indentation on landing.
Comment 12 Peter Kasting 2009-08-11 18:31:20 PDT
Comment on attachment 34625 [details]
Merge in ImageSourceSkia.cpp

Landed in r47081, clearing flags.
Comment 13 Peter Kasting 2009-08-11 18:47:34 PDT
CCing kmccullough as I want him to notice the Wx patch I'm going to post, in case I screw something up.
Comment 14 Peter Kasting 2009-08-11 18:55:33 PDT
Created attachment 34632 [details]
Merge in ImageSourceWx.cpp 

After reviewing the functions in ImageSourceWx.cpp, I believe everything in ImageSource.cpp is suitable, and no changes are needed; so I'm simply deleting ImageSourceWx.cpp and updating the Bakefiles.

I remember that Wx was going to move away from Bakefiles, but there are no other checked-in files which reference ImageSourceWx.cpp besides the ones I've modified, so I assume either that hasn't happened yet or it's still private.
Comment 15 Adam Treat 2009-08-11 19:01:56 PDT
Comment on attachment 34632 [details]
Merge in ImageSourceWx.cpp 

I think the approach is sound, but indentation problems prevent r+.  Looks like ImageSourceFoo* is close to complete unforking though :)
Comment 16 Peter Kasting 2009-08-12 10:34:52 PDT
Created attachment 34672 [details]
Merge in ImageSourceWx.cpp 

Fixed the modified indentation.
Comment 17 Eric Seidel (no email) 2009-08-12 11:07:54 PDT
Comment on attachment 34672 [details]
Merge in ImageSourceWx.cpp 

Looks fine to me.  Kevin, please scream if there is something wrong about this from a wx perspective.
Comment 18 Peter Kasting 2009-08-12 11:29:54 PDT
Comment on attachment 34672 [details]
Merge in ImageSourceWx.cpp 

Landed in r47127, clearing flags.
Comment 19 Kevin Ollivier 2009-08-12 11:52:38 PDT
(In reply to comment #17)
> (From update of attachment 34672 [details])
> Looks fine to me.  Kevin, please scream if there is something wrong about this
> from a wx perspective.

Looks fine to me, although the Bakefile build is completely broken right now thanks to a nasty hardcoded string character limit in Bakefile, so I'm not sure there's much point really in updating it. It's not a big deal either way, just a heads up for future patches that might touch wx. I'd like to remove those files to avoid confusion but I need that last waf patch landed first... ;-)

BTW, I don't know why wx sources are in WebCore.gypi? I suppose you're free to try maintaining it if you want, but I won't be supporting / using it myself. The generated project files weren't the draw I thought they would be; in fact, the read only nature of them became frustrating to contributors. It *looked* like they could just work in the IDE, which was the big draw, but then they discovered that they couldn't make any changes to it. Worse, in XCode, even swapping the wx build you used would require a project regeneration step. I even had a MSVC user petitioning me to allow a hand-maintained project file in the tree. They even agreed to maintain it, just so they could make changes in the IDE.

Also, one big reason I chose waf was so that I don't have to  make changes to the build every time a source file is added / moved / removed. For wx/waf this patch would have just been the ChangeLog entry and the removed ImageSourceWx.cpp. :) I still have to add new dirs manually, unfortunately, but I'll see if I can hack something up to auto-determine those as well soon. Shared maintenance is better than maintaining something yourself, but having the computer do the work of figuring out what needs built for you is even better IMHO. ;)
Comment 20 Peter Kasting 2009-08-12 11:56:56 PDT
(In reply to comment #19)
> BTW, I don't know why wx sources are in WebCore.gypi?

WebCore.gypi contains all the sources in the entire tree, I believe.  You'd have to ask dglazkov about why; I suspect to make it as easy as possible for people touching the tree to maintain it (no need to worry about whether or not to add a file).

I just do my best to try not to break things...
Comment 21 Dimitri Glazkov (Google) 2009-08-12 11:58:51 PDT
> BTW, I don't know why wx sources are in WebCore.gypi? I suppose you're free to
> try maintaining it if you want, but I won't be supporting / using it myself.

The idea is that you don't have to worry about whether to add files or not to WebCore.gypi. The rule is "always add".

> The generated project files weren't the draw I thought they would be; in fact,
> the read only nature of them became frustrating to contributors. It *looked*
> like they could just work in the IDE, which was the big draw, but then they
> discovered that they couldn't make any changes to it. Worse, in XCode, even
> swapping the wx build you used would require a project regeneration step. I
> even had a MSVC user petitioning me to allow a hand-maintained project file in
> the tree. They even agreed to maintain it, just so they could make changes in
> the IDE.

Not sure why it's not working well for you guys. gyp has been a huge time-saver for us, and we all use XCode and MSVC generated projects for everyday use.
Comment 22 Kevin Ollivier 2009-08-12 14:23:11 PDT
(In reply to comment #21)
> > BTW, I don't know why wx sources are in WebCore.gypi? I suppose you're free to
> > try maintaining it if you want, but I won't be supporting / using it myself.
> 
> The idea is that you don't have to worry about whether to add files or not to
> WebCore.gypi. The rule is "always add".

Ah, I see. Kinda like what I'm doing with waf, except that I don't have an explicit file list anywhere.

> > The generated project files weren't the draw I thought they would be; in fact,
> > the read only nature of them became frustrating to contributors. It *looked*
> > like they could just work in the IDE, which was the big draw, but then they
> > discovered that they couldn't make any changes to it. Worse, in XCode, even
> > swapping the wx build you used would require a project regeneration step. I
> > even had a MSVC user petitioning me to allow a hand-maintained project file in
> > the tree. They even agreed to maintain it, just so they could make changes in
> > the IDE.
> 
> Not sure why it's not working well for you guys. gyp has been a huge time-saver
> for us, and we all use XCode and MSVC generated projects for everyday use.

Well, in that respect, waf is a huge time-saver too and so is Bakefile, so obviously no tool has a monopoly on being better than manually maintaining several different project files. ;) Currently, I mostly just have to maintain the list of project directories, and with any luck, soon I won't even need to do that. 

Basically, for wx hardly anyone was really using the IDE projects that were being generated for wxWebKit, and those that did tended not to be happy with them being read-only, so for me it raised the logical question of why I had chosen an automatic project generator for my build tool. ;) And honestly, now that I've moved away from that, I'm much happier with waf. I never have to think about how to make things like Python bindings generation, disabling optional build steps if deps are missing, etc., work within several different IDEs, many of which are black boxes that I can't debug or extend. I can now do a lot of "on the fly configuration" that I couldn't do with the IDE project files, and the users like that too because the build system is smarter about letting them know what might be wrong when their config is off. The way waf works just feels like a natural fit for this sort of build process to me.
Comment 23 Peter Kasting 2009-08-27 17:08:43 PDT
Created attachment 38702 [details]
Move createDecoder() onto ImageDecoder

This is prep for factoring common code out of ImageSourceQt.
Comment 24 Eric Seidel (no email) 2009-08-27 18:06:38 PDT
Comment on attachment 38702 [details]
Move createDecoder() onto ImageDecoder

What about Haiku?  Don't they turn off JPEG support?  Seems we might need some sort of JPEGDecoder::supported() call for the various image decoders if we're gonna share this code.
Comment 25 Peter Kasting 2009-08-28 10:40:59 PDT
(In reply to comment #24)
> (From update of attachment 38702 [details])
> What about Haiku?  Don't they turn off JPEG support?

According to bug 28252 comment 8, Haiku is working on native libjpeg support and isn't going to add an exclusion for this.
Comment 26 Peter Kasting 2009-08-28 11:09:02 PDT
Comment on attachment 38702 [details]
Move createDecoder() onto ImageDecoder

Landed in r47868, clearing flags.
Comment 27 Peter Kasting 2009-08-28 11:47:22 PDT
Created attachment 38745 [details]
Remove most of ImageSourceQt.cpp

I think it might be possible (eventually) to remove the rest of ImageSourceQt by defining suitable abstractions on ImageDecoder, but that's something to ponder separately.
Comment 28 Kenneth Rohde Christiansen 2009-08-30 19:58:12 PDT
Hi there,

I wonder how all your image source/decoder changes relate to https://bugs.webkit.org/show_bug.cgi?id=27538 . Maybe you can give some input on the patches there. At least it would be nice to know if the other patches are still valid.

Thanks!
Comment 29 Eric Seidel (no email) 2009-09-01 00:35:24 PDT
Comment on attachment 38745 [details]
Remove most of ImageSourceQt.cpp

Assuming this doesn't hose Qt, LGTM.  Setting cq-.  Since you're a committer you're welcome to change it to cq+ if you'd rather have the bot land it for you.
Comment 30 Peter Kasting 2009-09-01 08:33:21 PDT
Comment on attachment 38745 [details]
Remove most of ImageSourceQt.cpp

I'll watch the tree when the bot commits.
Comment 31 Eric Seidel (no email) 2009-09-01 08:44:12 PDT
Comment on attachment 38745 [details]
Remove most of ImageSourceQt.cpp

Rejecting patch 38745 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 32 Peter Kasting 2009-09-01 08:51:30 PDT
Comment on attachment 38745 [details]
Remove most of ImageSourceQt.cpp

Landed in r47935, clearing flags.
Comment 33 Peter Kasting 2009-09-01 09:21:28 PDT
QT build fixes landed in r47936, r47938 and r47939.
Comment 34 Eric Seidel (no email) 2009-09-01 16:14:28 PDT
Sorry.  Layout test got left around from a previously failed patch:
svg/dom/smil-methods.svg -> failed

This type of error is tracked by bug 28603.
Comment 35 Brent Fulgham 2012-11-19 22:35:23 PST
Should this bug be closed?
Comment 36 Peter Kasting 2012-11-19 23:04:20 PST
Not unless the files have been cleaned up more since I last touched them, or I would have closed the bug.  Unless I royally screwed up.
Comment 37 noel gordon 2012-11-19 23:49:44 PST
% find . | grep ImageSource
./Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp
./Source/WebCore/platform/graphics/cg/ImageSourceCG.h
./Source/WebCore/platform/graphics/cg/ImageSourceCGMac.mm
./Source/WebCore/platform/graphics/cg/ImageSourceCGWin.cpp
./Source/WebCore/platform/graphics/ImageSource.cpp
./Source/WebCore/platform/graphics/ImageSource.h

These ImageSourceFoo.cpp no longer exist.
Comment 38 Peter Kasting 2012-11-20 00:30:59 PST
Looks like the last bits of ImageSourceQt.cpp were cleaned up in r49185.  Let's go ahead and close, then.