Summary: | [Haiku] Implement ImageBuffer support | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephan Aßmus <superstippi> | ||||||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, eric, simon.maxime, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Other | ||||||||||||||||
OS: | Other | ||||||||||||||||
Bug Depends on: | 37765 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Stephan Aßmus
2010-02-23 03:04:57 PST
Created attachment 49273 [details]
Patch implementing StillImage class
The StillImage class wraps a native BBitmap in a WebCore::Image derived class.
A reviewer could check the destroyDecodedData() and decodedSize() methods. Since StillImage will be used only for ImageBuffer implementation, and appear to be short-lived, I don't believe image data needs to be pruned while keeping the StillImage instance itself alive, but it would be nice to get a confirmation.
Created attachment 49274 [details]
Patch implementing all ImageBuffer methods.
Second patch which completes the implementation of ImageBuffer. Needs 49273 to be commited first.
Comment on attachment 49273 [details] Patch implementing StillImage class Just some minor things to address. > Index: WebCore/platform/graphics/haiku/StillImageHaiku.cpp > +unsigned StillImage::decodedSize() const > +{ > + // TODO: It could be wise to return 0 here, since we don't want WebCore Use FIXME instead of TODO. > Index: WebCore/platform/graphics/haiku/StillImageHaiku.h > + virtual void destroyDecodedData(bool = true); You should put in the parameter name here. I had no idea what the bool for was. > + StillImage(const BBitmap& bitmap); You should remove the param name "bitmap" here. as it adds no information. Created attachment 50676 [details]
Patch implementing StillImage class
Thanks for the review. I've edited the patch and made the requested changes.
Created attachment 50677 [details]
Patch implementing StillImage class
Created attachment 50678 [details]
Patch implementing all ImageBuffer methods.
Fixed a possible division by zero in the patch and moved a comment into it's appropriate scope.
Comment on attachment 50677 [details]
Patch implementing StillImage class
r=me
Did you mean these patches to be "commit-queue?" ?
--Oliver
Yes. I wasn't aware that I am supposed to mark patches for commit-queue right away. Is it also possible to indicate that patches do not really require review? I don't mean that review wouldn't help, but there are patches were I am more confident and there are patches were I am less confident. As long as the patches do not affect other ports, I could mark the ones I am quite confident with for commit-queue only, if that is appropriate. Comment on attachment 50678 [details]
Patch implementing all ImageBuffer methods.
You can't remove other peoples copyright notices
(In reply to comment #9) > (From update of attachment 50678 [details]) > You can't remove other peoples copyright notices Stephan removed/changed almost entirely my code. I'm not even sure that there is still a piece of code which is mine. So I think it's fair to remove my copyright here. The file was previous copied and contained only empty stubs. This was obviously done only to make the port compile. Unless Maxime has copyright on the design of the ImageBuffer interface, which didn't seem to be the case, I understood from reading comments in other Haiku patch reviews, that it was desired to remove copyright "left-overs". Naturally, I am very careful and double check before I remove any peoples copyright. Maybe you stopped reading the patch after you saw I removed someone's copyright, but I just checked again and there is not a single line of the previous code left. It consisted entirely of "notImplemented();" or "notImplemented(); return 0;" in the function bodies. Comment on attachment 50677 [details] Patch implementing StillImage class Clearing flags on attachment: 50677 Committed r56149: <http://trac.webkit.org/changeset/56149> All reviewed patches have been landed. Closing bug. The ticket has been closed prematurely. The second patch received a negative review, but Maxim and I have tried to explain the situation and why we believe the reasons for the rejection are not valid. Comment on attachment 50678 [details]
Patch implementing all ImageBuffer methods.
Flipping review flag back to ?, as suggested on mailing list.
Comment on attachment 50678 [details] Patch implementing all ImageBuffer methods. > Index: WebCore/platform/graphics/haiku/ImageBufferData.h > +#include "OwnPtr.h" It doesn't look like OwnPtr.h is used. Also I believe it is typically included as <wtf/OwnPtr.h> > Index: WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp > +static inline void convertFromInternalData(const uint8* sourceRows, unsigned sourceBytesPerRow, > + uint8* destRows, unsigned destBytesPerRow, > + unsigned rows, unsigned columns, > + bool premultiplied) > +{ > + if (premultiplied) { > + // Internal storage is not pre-multiplied, pre-multiply on the fly. > + for (unsigned y = 0; y < rows; y++) { > + const uint8* s = sourceRows; Please avoid single letter variables (well "i" is ok as a loop vairable, etc. and x, y are ok, but in this case "s" is an abbreviation of ? > + unsigned char* destRows = data; > + // Offset the destination pointer to point at the first pixel of the > + // intersection rect. > + destRows += (rect.x() - (int)sourceRect.left) * 4 + (rect.y() - (int)sourceRect.top) * destBytesPerRow; Use c++ style casting instead of c style casting. > +static void putImageData(ImageData* source, const IntRect& sourceRect, const IntPoint& destPoint, ImageBufferData& imageData, const IntSize& size, bool premultiplied) > +{ > + // If the source image is outside the destination image, we can return at > + // this point. > + // TODO: Check if this isn't already done in WebCore. Use FIXME instead of TODO in WebKit code. > + roster->GetOutputFormats(translators[i], &outputFormats, &formatCount); > + for (int32 j = 0; j < formatCount; j++) { > + if (outputFormats[j].group == B_TRANSLATOR_BITMAP > + && mimeTypeString == outputFormats[j].MIME) { No need to wrap this line (unless you really want to). Created attachment 53266 [details]
Patch implementing all ImageBuffer methods.
Revised patch after review.
Forgot to mention that I didn't change the line wrapping, since I personally find the long lines in WebCore code really awkward. For example, the code was badly wrapped in the notification email sent by Bugzilla. This is just one example. Even in the HTML mails sent to webkit-changes, text runs out of the box elements. Since the style guide does not require me to write long lines, I prefer not to. :-) Attachment 53266 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 53266 [details]
Patch implementing all ImageBuffer methods.
Rejecting patch 53266 from commit-queue.
Unexpected failure when landing patch! Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '53266', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
', 'commit', '--all', '-F', '-'], input=message)
File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/system/executive.py", line 85, in run_command
return Executive().run_command(*args, **kwargs)
File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/system/executive.py", line 186, in run_command
string_to_communicate = str(input)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xdf' in position 21: ordinal not in range(128)
This is a commit-queue bug. I'll look into it. Comment on attachment 53266 [details]
Patch implementing all ImageBuffer methods.
Hopefully fixed now.
Comment on attachment 53266 [details] Patch implementing all ImageBuffer methods. Clearing flags on attachment: 53266 Committed r58078: <http://trac.webkit.org/changeset/58078> All reviewed patches have been landed. Closing bug. |