Bug 79094

Summary: [BlackBerry] Upstream the BlackBerry change to platform/graphics/IntPoint.h
Product: WebKit Reporter: Leo Yang <leo.yang>
Component: New BugsAssignee: Leo Yang <leo.yang>
Status: RESOLVED FIXED    
Severity: Normal CC: charles.wei, rwlbuis, staikos, tonikitoo, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73144    
Attachments:
Description Flags
Patch
none
Patch v2
none
Patch v3 none

Leo Yang
Reported 2012-02-21 02:21:06 PST
This is to add the blackberry specific change to platform/graphics/IntPoint.h.
Attachments
Patch (2.81 KB, patch)
2012-02-21 02:29 PST, Leo Yang 		no flags
DetailsFormatted DiffDiff
Patch v2 (2.86 KB, patch)
2012-02-21 17:59 PST, Leo Yang 		no flags
DetailsFormatted DiffDiff
Patch v3 (2.28 KB, patch)
2012-02-23 00:32 PST, Leo Yang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Leo Yang
Comment 1 2012-02-21 02:29:29 PST
Created attachment 127948 [details] Patch
Nikolas Zimmermann
Comment 2 2012-02-21 03:15:30 PST
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 3 2012-02-21 05:05:27 PST
Comment on attachment 127948 [details] Patch Can not this code go to IntPointBlackBerry?
Leo Yang
Comment 4 2012-02-21 17:59:51 PST
Created attachment 128097 [details] Patch v2
Leo Yang
Comment 5 2012-02-21 18:01:20 PST
(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
Comment 6 2012-02-21 18:02:25 PST
(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 7 2012-02-21 18:08:50 PST
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
Comment 8 2012-02-21 18:13:37 PST
(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
Comment 9 2012-02-23 00:32:42 PST
Created attachment 128421 [details] Patch v3
Leo Yang
Comment 10 2012-02-23 00:33:27 PST
toString has been removed internally.
Leo Yang
Comment 11 2012-02-23 18:04:13 PST
Comment on attachment 128421 [details] Patch v3 Sending it to cq...
WebKit Review Bot
Comment 12 2012-02-23 19:05:40 PST
Comment on attachment 128421 [details] Patch v3 Clearing flags on attachment: 128421 Committed r108715: <http://trac.webkit.org/changeset/108715>
WebKit Review Bot
Comment 13 2012-02-23 19:05:45 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.