Bug 79094 – [BlackBerry] Upstream the BlackBerry change to platform/graphics/IntPoint.h

• Component: New Bugs
• Status: RESOLVED FIXED

[Leo Yang]

This is to add the blackberry specific change to platform/graphics/IntPoint.h.

[Leo Yang]

Created attachment 127948 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 127948 [details]
Patch

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

Looks fine, I have a suggestion though:

> Source/WebCore/ChangeLog:11
> +        The porting can be built now, no new tests.

I think this sentence is obsolete, at least I don't understand it. Do you mean the port can now be build for the first time?

> Source/WebCore/platform/graphics/IntPoint.h:127
> +        return String::format("(%i,%i)", m_x, m_y);

String::format should absolutely be avoided in new code. It's deprecated.
You can either make use of Strings operator+ optimization, and juse use:
return String("(") + String::number(m_x) + ...
That will result in one malloc, for the final string size, and no intermediate <any>->String conversions, and thus should be at least as efficient as using String::format directly.

[Antonio Gomes]

Comment on attachment 127948 [details]
Patch

Can not this code go to IntPointBlackBerry?

[Leo Yang]

Created attachment 128097 [details]
Patch v2

[Leo Yang]

(In reply to comment #2)
> (From update of attachment 127948 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127948&action=review
> 
> Looks fine, I have a suggestion though:
> 
> > Source/WebCore/ChangeLog:11
> > +        The porting can be built now, no new tests.
> 
> I think this sentence is obsolete, at least I don't understand it. Do you mean the port can now be build for the first time?
Oh, silly. The porting can't be built yet, now new tests.

> 
> > Source/WebCore/platform/graphics/IntPoint.h:127
> > +        return String::format("(%i,%i)", m_x, m_y);
> 
> String::format should absolutely be avoided in new code. It's deprecated.
> You can either make use of Strings operator+ optimization, and juse use:
> return String("(") + String::number(m_x) + ...
> That will result in one malloc, for the final string size, and no intermediate <any>->String conversions, and thus should be at least as efficient as using String::format directly.
OK.

[Leo Yang]

(In reply to comment #3)
> (From update of attachment 127948 [details])
> Can not this code go to IntPointBlackBerry?

I think it should go in the header because we need to alter the declaration.

[Antonio Gomes]

Comment on attachment 128097 [details]
Patch v2

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

> Source/WebCore/platform/graphics/IntPoint.h:127
> +#if PLATFORM(BLACKBERRY)
> +    String toString() const
> +    {
> +        return String("(") + String::number(m_x) + String(",") + String::number(m_y) + String(")");

I would suggest making it non bb-specific

[Leo Yang]

(In reply to comment #7)
> (From update of attachment 128097 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128097&action=review
> 
> > Source/WebCore/platform/graphics/IntPoint.h:127
> > +#if PLATFORM(BLACKBERRY)
> > +    String toString() const
> > +    {
> > +        return String("(") + String::number(m_x) + String(",") + String::number(m_y) + String(")");
> 
> I would suggest making it non bb-specific

good suggestion.

[Leo Yang]

Created attachment 128421 [details]
Patch v3

[Leo Yang]

toString has been removed internally.

[Leo Yang]

Comment on attachment 128421 [details]
Patch v3

Sending it to cq...

[WebKit Review Bot]

Comment on attachment 128421 [details]
Patch v3

Clearing flags on attachment: 128421

Committed r108715: <http://trac.webkit.org/changeset/108715>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.