Bug 35288

Summary: [Haiku] Implement ImageBuffer support
Product: WebKit Reporter: Stephan Aßmus <superstippi>
Component: PlatformAssignee: 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 Flags
Patch implementing StillImage class
levin: review-
Patch implementing all ImageBuffer methods.
none
Patch implementing StillImage class
none
Patch implementing StillImage class
none
Patch implementing all ImageBuffer methods.
levin: review-
Patch implementing all ImageBuffer methods. none

Description Stephan Aßmus 2010-02-23 03:04:57 PST
There is going to be two patches.

The first adds a StillImage class inheriting from WebCore::Image just like it's done for the Qt port. This class wraps a native BBitmap and provides access to the image data from within WebCore code.

The second patch actually implements ImageBuffer using StillImage for context() and image().
Comment 1 Stephan Aßmus 2010-02-23 03:25:58 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.
Comment 2 Stephan Aßmus 2010-02-23 03:39:53 PST
Created attachment 49274 [details]
Patch implementing all ImageBuffer methods.

Second patch which completes the implementation of ImageBuffer. Needs 49273 to be commited first.
Comment 3 David Levin 2010-03-11 14:52:11 PST
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.
Comment 4 Stephan Aßmus 2010-03-14 14:56:07 PDT
Created attachment 50676 [details]
Patch implementing StillImage class

Thanks for the review. I've edited the patch and made the requested changes.
Comment 5 Stephan Aßmus 2010-03-14 14:58:49 PDT
Created attachment 50677 [details]
Patch implementing StillImage class
Comment 6 Stephan Aßmus 2010-03-14 15:22:35 PDT
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 7 Oliver Hunt 2010-03-17 02:10:39 PDT
Comment on attachment 50677 [details]
Patch implementing StillImage class

r=me

Did you mean these patches to be "commit-queue?" ?

--Oliver
Comment 8 Stephan Aßmus 2010-03-17 02:38:36 PDT
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 9 Oliver Hunt 2010-03-17 02:47:12 PDT
Comment on attachment 50678 [details]
Patch implementing all ImageBuffer methods.

You can't remove other peoples copyright notices
Comment 10 Maxime Simon 2010-03-17 02:54:09 PDT
(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.
Comment 11 Stephan Aßmus 2010-03-17 04:34:06 PDT
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.
Comment 12 Stephan Aßmus 2010-03-17 04:37:58 PDT
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 13 WebKit Commit Bot 2010-03-17 21:56:46 PDT
Comment on attachment 50677 [details]
Patch implementing StillImage class

Clearing flags on attachment: 50677

Committed r56149: <http://trac.webkit.org/changeset/56149>
Comment 14 WebKit Commit Bot 2010-03-17 21:56:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Stephan Aßmus 2010-03-18 03:59:59 PDT
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 16 Stephan Aßmus 2010-04-11 13:06:42 PDT
Comment on attachment 50678 [details]
Patch implementing all ImageBuffer methods.

Flipping review flag back to ?, as suggested on mailing list.
Comment 17 David Levin 2010-04-13 09:37:35 PDT
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).
Comment 18 Stephan Aßmus 2010-04-13 11:29:54 PDT
Created attachment 53266 [details]
Patch implementing all ImageBuffer methods.

Revised patch after review.
Comment 19 Stephan Aßmus 2010-04-13 11:33:16 PDT
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. :-)
Comment 20 WebKit Review Bot 2010-04-13 11:34:15 PDT
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 21 WebKit Commit Bot 2010-04-14 08:52:05 PDT
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)
Comment 22 Eric Seidel (no email) 2010-04-14 12:23:31 PDT
This is a commit-queue bug.  I'll look into it.
Comment 23 Eric Seidel (no email) 2010-04-21 18:35:20 PDT
Comment on attachment 53266 [details]
Patch implementing all ImageBuffer methods.

Hopefully fixed now.
Comment 24 WebKit Commit Bot 2010-04-22 02:38:40 PDT
Comment on attachment 53266 [details]
Patch implementing all ImageBuffer methods.

Clearing flags on attachment: 53266

Committed r58078: <http://trac.webkit.org/changeset/58078>
Comment 25 WebKit Commit Bot 2010-04-22 02:38:46 PDT
All reviewed patches have been landed.  Closing bug.