Bug 79212

Created attachment 128163 [details] patch

Comment on attachment 128163 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=128163&action=review Still some stuff to improve. > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:2 > + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved. Needs 2012. > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:30 > + String iconName(name); // a.k.a. "enable string comparisons with ==" I dont see a need for this, why not just use strcmp? > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:40 > + context->fillRect(FloatRect(0, 0, 16, 16), Color::white, ColorSpaceDeviceRGB); There may be a variant that flood fills the entire surface, then you dont need to specify the FloatRect. > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:59 > + // FIXME: Have a setting for the resource path Is the FIXME still accurate?

(In reply to comment #2) > (From update of attachment 128163 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128163&action=review > > Still some stuff to improve. > > > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:2 > > + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved. > > Needs 2012. Will add. > > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:30 > > + String iconName(name); // a.k.a. "enable string comparisons with ==" > > I dont see a need for this, why not just use strcmp? > Good point. > > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:40 > > + context->fillRect(FloatRect(0, 0, 16, 16), Color::white, ColorSpaceDeviceRGB); > > There may be a variant that flood fills the entire surface, then you dont need to specify the FloatRect. > GraphicsContext does not have functions that flood fills the entire surface, though SkCanvas have such functions. We can change it something like this: SkCanvas* canvas = context->platformContext()->canvas(); canvas->drawColor(SK_ColorWHITE); Both are ok for me, but I prefer not to change it. > > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:59 > > + // FIXME: Have a setting for the resource path > > Is the FIXME still accurate? I don't think we need to add a public setting for the resource path. And seems no other ports did this. I will remove the FIXME.

Created attachment 128375 [details] updated patch

Comment on attachment 128375 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=128375&action=review > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:52 > + context->fillRect(FloatRect(0, 0, 16, 16), Color::white, ColorSpaceDeviceRGB); > + > + if (!strcmp(name, "searchCancel")) > + context->setFillColor(Color(128, 128, 128), ColorSpaceDeviceRGB); > + else > + context->setFillColor(Color(64, 64, 64), ColorSpaceDeviceRGB); > + > + context->translate(8, 8); > + > + context->rotate(piDouble / 4.0); > + context->fillRect(FloatRect(-1, -7, 2, 14)); > + > + context->rotate(-piDouble / 2.0); > + context->fillRect(FloatRect(-1, -7, 2, 14)); some magic numbers here. Anyway we could improve it? > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:70 > + img->setData(buffer.release(), true); what is true here?

(In reply to comment #5) > (From update of attachment 128375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128375&action=review > > > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:52 > > + context->fillRect(FloatRect(0, 0, 16, 16), Color::white, ColorSpaceDeviceRGB); > > + > > + if (!strcmp(name, "searchCancel")) > > + context->setFillColor(Color(128, 128, 128), ColorSpaceDeviceRGB); > > + else > > + context->setFillColor(Color(64, 64, 64), ColorSpaceDeviceRGB); > > + > > + context->translate(8, 8); > > + > > + context->rotate(piDouble / 4.0); > > + context->fillRect(FloatRect(-1, -7, 2, 14)); > > + > > + context->rotate(-piDouble / 2.0); > > + context->fillRect(FloatRect(-1, -7, 2, 14)); > > some magic numbers here. Anyway we could improve it? > We just want to draw a "x" here. Maybe i should add a comment? > > Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:70 > > + img->setData(buffer.release(), true); > > what is true here? true == allDataReceived bool Image::setData(PassRefPtr<SharedBuffer> data, bool allDataReceived);

Comment on attachment 128375 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=128375&action=review >>> Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:52 >>> + context->fillRect(FloatRect(-1, -7, 2, 14)); >> >> some magic numbers here. Anyway we could improve it? > > We just want to draw a "x" here. Maybe i should add a comment? please add. >>> Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:70 >>> + img->setData(buffer.release(), true); >> >> what is true here? > > true == allDataReceived > bool Image::setData(PassRefPtr<SharedBuffer> data, bool allDataReceived); lets add a /*allDataReceived*/ comment

Created attachment 128647 [details] Updated patch for landing

Comment on attachment 128647 [details] Updated patch for landing Sending to cq...

Comment on attachment 128647 [details] Updated patch for landing Clearing flags on attachment: 128647 Committed r108774: <http://trac.webkit.org/changeset/108774>

All reviewed patches have been landed. Closing bug.