Bug 51811 – Merge m_height and m_width members of ImageData into a common m_size member

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Renata Hodovan]

Merge m_height and m_width members of ImageData into a common m_size member

[Renata Hodovan]

Created attachment 77799 [details]
Patch

[Andreas Kling]

Comment on attachment 77799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77799&action=review

> WebCore/ChangeLog:6
> +        Merge m_height and m_width memebers of ImageData into the new m_size member.

s/memebers/members/

> WebCore/html/ImageData.h:53
> -    unsigned m_width;
> -    unsigned m_height;
> +    IntSize m_size;

Width and height are stored as signed ints rather than unsigned after this change.
Is this intentional?

[Renata Hodovan]

(In reply to comment #2)
> (From update of attachment 77799 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77799&action=review
> 
> > WebCore/ChangeLog:6
> > +        Merge m_height and m_width memebers of ImageData into the new m_size member.
> 
> s/memebers/members/
> 
> > WebCore/html/ImageData.h:53
> > -    unsigned m_width;
> > -    unsigned m_height;
> > +    IntSize m_size;
> 
> Width and height are stored as signed ints rather than unsigned after this change.
> Is this intentional?

There is no advantage using unsigned int here since this is the since of a memory block. Bigger image sizes than INT_MAX is unlikely.

[Andreas Kling]

Comment on attachment 77799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77799&action=review

> WebCore/html/ImageData.h:46
> +    unsigned width() const { return m_size.width(); }
> +    unsigned height() const { return m_size.height(); }

The getter return type should at the very least match the member variable's type.
Setting r- for this, but please explain why the overall change is correct before (or as part of) the next iteration.

[Balazs Kelemen]

> > Width and height are stored as signed ints rather than unsigned after this change.
> > Is this intentional?
> 
> There is no advantage using unsigned int here since this is the since of a memory block. Bigger image sizes than INT_MAX is unlikely.

Using a signed type for something that has no valid negative value is wrong in my point of view.
I think this is an unwritten rule in WebKit.

[Renata Hodovan]

> Using a signed type for something that has no valid negative value is wrong in my point of view.
> I think this is an unwritten rule in WebKit.
WebKit also has a rule: avoid code duplications. IntSize has a lot of member functions, and can handle various things. See class Image  (graphics/Image.h) Although no image can have a negative size, the core Image base class (and its descendants) use it for storing the dimensions. (BitmapImage or ImageSource)

> The getter return type should at the very least match the member variable's type.
It's done.

> please explain why the overall change is correct
Because this way of storing size is the uniform in WebKit.

[Renata Hodovan]

Created attachment 77873 [details]
Patch

[Andreas Kling]

Comment on attachment 77873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77873&action=review

> WebCore/ChangeLog:6
> +        Merge m_height and m_width memebers of ImageData into the new m_size member.

Typo, memebers -> members.

> WebCore/bindings/js/SerializedScriptValue.cpp:407
> -                write(data->width());
> -                write(data->height());
> +                write(static_cast<uint32_t>(data->width()));
> +                write(static_cast<uint32_t>(data->height()));

This is ugly.
We should change the format of serialized ImageData to "ImageDataTag <width:int32_t><height:int32_t><length:uint32_t><data:uint8_t{length}>" instead.

> WebCore/html/ImageData.h:51
> -    ImageData(unsigned width, unsigned height);
> -    ImageData(unsigned width, unsigned height, PassRefPtr<ByteArray>);
> +    ImageData(int width, int height);
> +    ImageData(int width, int height, PassRefPtr<ByteArray>);

The constructors could take an IntSize instead now (just like ImageBuffer.)

[Andreas Kling]

(In reply to comment #6)
> WebKit also has a rule: avoid code duplications. IntSize has a lot of member functions, and can handle various things. See class Image  (graphics/Image.h) Although no image can have a negative size, the core Image base class (and its descendants) use it for storing the dimensions. (BitmapImage or ImageSource)

Fair point, so ImageData::width() and ImageData::height() should be replaced by ImageData::size()

Actually, we might want to keep them for convenience, but a size() should be added, which would make some call sites slightly nicer, for example CanvasRenderingContext2D::createImageData(imageData, ec)

[Balazs Kelemen]

(In reply to comment #6)
> > Using a signed type for something that has no valid negative value is wrong in my point of view.
> > I think this is an unwritten rule in WebKit.
> WebKit also has a rule: avoid code duplications. IntSize has a lot of member functions, and can handle various things. See class Image  (graphics/Image.h) Although no image can have a negative size, the core Image base class (and its descendants) use it for storing the dimensions. (BitmapImage or ImageSource)
 
Accepted your argument :)

[Renata Hodovan]

Created attachment 77885 [details]
Patch

[WebKit Review Bot]

Attachment 77885 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7233414

[Andreas Kling]

Comment on attachment 77885 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77885&action=review

Getting better, but not quite there yet.

There are a bunch of ImageData::create() calls in WebCore, for example in FilterEffect. They should be updated to pass an IntSize now (does this patch even build?)

> WebCore/html/ImageData.h:43
> +    static PassRefPtr<ImageData> create(IntSize);
> +    static PassRefPtr<ImageData> create(IntSize, PassRefPtr<ByteArray>);

IntSize -> const IntSize&

> WebCore/html/ImageData.h:52
> +    ImageData(IntSize);
> +    ImageData(IntSize, PassRefPtr<ByteArray>);

IntSize -> const IntSize&

> WebCore/html/canvas/CanvasRenderingContext2D.cpp:1532
> +    IntSize size(imageData->size());
>      return createEmptyImageData(size);

No need for the temporary "size" variable here.

[Andreas Kling]

Comment on attachment 77885 [details]
Patch

Ehm, r- not r+ :)

[Dirk Schulze]

(In reply to comment #13)
> (From update of attachment 77885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77885&action=review
> 
> Getting better, but not quite there yet.
> 
> There are a bunch of ImageData::create() calls in WebCore, for example in FilterEffect. They should be updated to pass an IntSize now (does this patch even build?)
You should update your working directory ;-) I killed all calls of ImageData, as far as possible.

[Andreas Kling]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 77885 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77885&action=review
> > 
> > Getting better, but not quite there yet.
> > 
> > There are a bunch of ImageData::create() calls in WebCore, for example in FilterEffect. They should be updated to pass an IntSize now (does this patch even build?)
> You should update your working directory ;-) I killed all calls of ImageData, as far as possible.

Derp! Right you are ;)

[WebKit Review Bot]

Attachment 77885 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7300366

[Renata Hodovan]

Created attachment 78000 [details]
Patch

It'll break the style bot, since it keeps a style error, which should be fixed in a followup patch.

[WebKit Review Bot]

Attachment 78000 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/bindings/js/SerializedScriptValue.cpp', u'WebCore/bindings/v8/SerializedScriptValue.cpp', u'WebCore/html/ImageData.cpp', u'WebCore/html/ImageData.h', u'WebCore/html/canvas/CanvasRenderingContext2D.cpp', u'WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp']" exit_code: 1
WebCore/bindings/v8/SerializedScriptValue.cpp:882:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp:212:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Comment on attachment 78000 [details]
Patch

The rule is, that we always fix style errors within a patch, sorry I can't r+ this patch as is. Please fix the PassRefPtr before uploading a new patch. Just switch to RefPtr<ImageData>on both files. Make sure that every return value on ImageBufferHaiku::getImageData() has a release() call: return result.release();

[Renata Hodovan]

Created attachment 78112 [details]
Patch

[WebKit Review Bot]

Attachment 78112 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/bindings/js/SerializedScriptValue.cpp', u'WebCore/bindings/v8/SerializedScriptValue.cpp', u'WebCore/html/ImageData.cpp', u'WebCore/html/ImageData.h', u'WebCore/html/canvas/CanvasRenderingContext2D.cpp', u'WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp']" exit_code: 1
WebCore/bindings/v8/SerializedScriptValue.cpp:882:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp:212:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Renata Hodovan]

Created attachment 78113 [details]
Patch

[WebKit Review Bot]

Attachment 78113 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7266464

[Dirk Schulze]

Comment on attachment 78113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78113&action=review

Nearly done, just some snippets.

> WebCore/bindings/js/SerializedScriptValue.cpp:503
> +    void write(int32_t i)
> +    {
> +        writeLittleEndian(m_buffer, i);
> +    }
> +

Do we still need void write(uint32_t i)? I guess it is still useful :-/

> WebCore/html/ImageData.h:47
> +    int width() const { return m_size.width(); }
> +    int height() const { return m_size.height(); }

I read the comment of Andreas (https://bugs.webkit.org/show_bug.cgi?id=51811#c9), but I don't think that these lines should still be included. Are we doing something similar somewhere else? I don't think so. This should be removed, and all callers replaced by size().width()/height().

> WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp:249
> +    return result.release();

Looks like you forgot one early return above this line:

if (rect.x() > size.width() || rect.y() > size.height() || rect.right() < 0 || rect.bottom() < 0)
	        return result;

should be return result.release(); as well.

> WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp:257
> +    return getImageData(rect, m_data, m_size, false).release();

The release is wrong here.

> WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp:264
> +    return getImageData(rect, m_data, m_size, true).release();

The release is wrong here.

[WebKit Review Bot]

Attachment 78113 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7317433

[David Levin]

(In reply to comment #24)
> Attachment 78113 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/7266464

Changing 
 *value = toV8(imageData);
to
 *value = toV8(imageData.release());
on line 888 of WebCore/bindings/v8/SerializedScriptValue.cpp should fix the cr-linux/mac build break.

[Dirk Schulze]

(In reply to comment #27)
> (In reply to comment #24)
> > Attachment 78113 [details] [details] did not build on chromium:
> > Build output: http://queues.webkit.org/results/7266464
> 
> Changing 
>  *value = toV8(imageData);
> to
>  *value = toV8(imageData.release());
> on line 888 of WebCore/bindings/v8/SerializedScriptValue.cpp should fix the cr-linux/mac build break.

Yes :-)

[Andreas Kling]

(In reply to comment #25)
> I read the comment of Andreas (https://bugs.webkit.org/show_bug.cgi?id=51811#c9), but I don't think that these lines should still be included. Are we doing something similar somewhere else? I don't think so. This should be removed, and all callers replaced by size().width()/height().

Yeah, you're right. No need for width() and height().

[Andreas Kling]

(In reply to comment #29)
> (In reply to comment #25)
> > I read the comment of Andreas (https://bugs.webkit.org/show_bug.cgi?id=51811#c9), but I don't think that these lines should still be included. Are we doing something similar somewhere else? I don't think so. This should be removed, and all callers replaced by size().width()/height().
> 
> Yeah, you're right. No need for width() and height().

Eh, strike that, width and height are exposed in ImageData.idl ;)

[Dirk Schulze]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #25)
> > > I read the comment of Andreas (https://bugs.webkit.org/show_bug.cgi?id=51811#c9), but I don't think that these lines should still be included. Are we doing something similar somewhere else? I don't think so. This should be removed, and all callers replaced by size().width()/height().
> > 
> > Yeah, you're right. No need for width() and height().
> 
> Eh, strike that, width and height are exposed in ImageData.idl ;)

Haha, I forgot the IDL ;-)

[Renata Hodovan]

Created attachment 78121 [details]
Patch

[Renata Hodovan]

> Do we still need void write(uint32_t i)? I guess it is still useful :-/
Yeah, it is called from different places.

> Changing 
>  *value = toV8(imageData);
> to
>  *value = toV8(imageData.release());
> on line 888 of WebCore/bindings/v8/SerializedScriptValue.cpp should fix the cr-linux/mac build break.
Thanks :)

> Eh, strike that, width and height are exposed in ImageData.idl ;)
In this case I don't change the call of width and height.

[Andreas Kling]

Comment on attachment 78121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78121&action=review

Looks good, r=me

> WebCore/ChangeLog:8
> +        Image (BitMapImage, ImageSource) types in WebKit use IntSize to store their

s/BitMapImage/BitmapImage/

[Renata Hodovan]

Closing bug. It's landed in: http://trac.webkit.org/changeset/75160