Bug 27965

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.

Seems pkasting and I have clashed with efforts. Assigning to him to deal with...

Comment on attachment 34027 [details] Patch to merge ImageSourceCairo.cpp and ImageSourceSkia.cpp Obsoleted by pkasting's next patch.

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.

(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?

(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.

(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 on attachment 34586 [details] Move ImageSourceCairo.cpp to ImageSource.cpp Please watch the buildbots...

Comment on attachment 34586 [details] Move ImageSourceCairo.cpp to ImageSource.cpp Landed in r47073, clearing flags.

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 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 on attachment 34625 [details] Merge in ImageSourceSkia.cpp Landed in r47081, clearing flags.

CCing kmccullough as I want him to notice the Wx patch I'm going to post, in case I screw something up.

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 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 :)

Created attachment 34672 [details] Merge in ImageSourceWx.cpp Fixed the modified indentation.

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 on attachment 34672 [details] Merge in ImageSourceWx.cpp Landed in r47127, clearing flags.

(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. ;)

(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...

> 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.

(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.

Created attachment 38702 [details] Move createDecoder() onto ImageDecoder This is prep for factoring common code out of ImageSourceQt.

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.

(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 on attachment 38702 [details] Move createDecoder() onto ImageDecoder Landed in r47868, clearing flags.

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.

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 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 on attachment 38745 [details] Remove most of ImageSourceQt.cpp I'll watch the tree when the bot commits.

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 on attachment 38745 [details] Remove most of ImageSourceQt.cpp Landed in r47935, clearing flags.

QT build fixes landed in r47936, r47938 and r47939.

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.

Should this bug be closed?

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.

% 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.

Looks like the last bits of ImageSourceQt.cpp were cleaned up in r49185. Let's go ahead and close, then.