Bug 35288 – [Haiku] Implement ImageBuffer support

• Component: Platform
• Status: RESOLVED FIXED

[Stephan Aßmus]

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

[Stephan Aßmus]

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.

[Stephan Aßmus]

Created attachment 49274 [details]
Patch implementing all ImageBuffer methods.

Second patch which completes the implementation of ImageBuffer. Needs 49273 to be commited first.

[David Levin]

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.

[Stephan Aßmus]

Created attachment 50676 [details]
Patch implementing StillImage class

Thanks for the review. I've edited the patch and made the requested changes.

[Stephan Aßmus]

Created attachment 50677 [details]
Patch implementing StillImage class

[Stephan Aßmus]

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.

[Oliver Hunt]

Comment on attachment 50677 [details]
Patch implementing StillImage class

r=me

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

--Oliver

[Stephan Aßmus]

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.

[Oliver Hunt]

Comment on attachment 50678 [details]
Patch implementing all ImageBuffer methods.

You can't remove other peoples copyright notices

[Maxime Simon]

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

[Stephan Aßmus]

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.

[Stephan Aßmus]

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.

[WebKit Commit Bot]

Comment on attachment 50677 [details]
Patch implementing StillImage class

Clearing flags on attachment: 50677

Committed r56149: <http://trac.webkit.org/changeset/56149>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Stephan Aßmus]

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.

[Stephan Aßmus]

Comment on attachment 50678 [details]
Patch implementing all ImageBuffer methods.

Flipping review flag back to ?, as suggested on mailing list.

[David Levin]

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

[Stephan Aßmus]

Created attachment 53266 [details]
Patch implementing all ImageBuffer methods.

Revised patch after review.

[Stephan Aßmus]

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

[WebKit Review Bot]

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.

[WebKit Commit Bot]

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)

[Eric Seidel (no email)]

This is a commit-queue bug.  I'll look into it.

[Eric Seidel (no email)]

Comment on attachment 53266 [details]
Patch implementing all ImageBuffer methods.

Hopefully fixed now.

[WebKit Commit Bot]

Comment on attachment 53266 [details]
Patch implementing all ImageBuffer methods.

Clearing flags on attachment: 53266

Committed r58078: <http://trac.webkit.org/changeset/58078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.