Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(285)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 124943003: Remove 'String::append' from some of the blink source. (Closed)

Created:
6 years, 11 months ago by gnana
Modified:
6 years, 11 months ago
CC:
blink-reviews, eae+blinkwatch, apavlov+blink_chromium.org, adamk+blink_chromium.org, aandrey+blink_chromium.org, rwlbuis, jamesr, caseq+blink_chromium.org, krit, dsinclair, yurys+blink_chromium.org, abarth-chromium, danakj, dglazkov+blink, Rik, gavinp+loader_chromium.org, devtools-reviews_chromium.org, pdr., kenneth.christiansen, loislo+blink_chromium.org, sof, jbroman, lushnikov+blink_chromium.org, kinuko, eustas+blink_chromium.org, paulirish+reviews_chromium.org, darktears, Nate Chapin, alph+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, Stephen Chennney
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Remove 'String::append' from some of the blink source. Replaced with StringBuilder whereever possible for (marginally) better performance. BUG=268281 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164851

Patch Set 1 : #

Total comments: 1

Patch Set 2 : Incorporated review comments #

Total comments: 3

Patch Set 3 : Merge to trunk and incorporated comments #

Total comments: 4

Patch Set 4 : Incorporated review comments #

Total comments: 2

Patch Set 5 : incorporated review comments #

Patch Set 6 : Removing the CustomFilterValidatedProgram.cpp from the patch(File is deleted in r164847) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -27 lines) Patch
M Source/core/editing/markup.cpp View 1 2 3 chunks +4 lines, -4 lines 0 comments Download
M Source/core/fetch/CSSStyleSheetResource.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/fetch/FontResource.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/fetch/ScriptResource.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/fetch/XSLStyleSheetResource.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/inspector/InspectorDOMAgent.cpp View 1 2 1 chunk +2 lines, -4 lines 0 comments Download
M Source/core/inspector/NetworkResourcesData.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/WorkerThreadableLoader.cpp View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M Source/modules/filesystem/DOMFilePath.cpp View 1 2 3 4 3 chunks +5 lines, -7 lines 0 comments Download
M Source/modules/websockets/WebSocketChannel.cpp View 1 chunk +1 line, -2 lines 0 comments Download
M Source/platform/network/HTTPParsers.cpp View 1 2 3 4 1 chunk +2 lines, -3 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
gnana
Please have a look.
6 years, 11 months ago (2014-01-07 15:22:10 UTC) #1
abarth-chromium
not lgtm https://chromiumcodereview.appspot.com/124943003/diff/60001/Source/core/fetch/CSSStyleSheetResource.cpp File Source/core/fetch/CSSStyleSheetResource.cpp (right): https://chromiumcodereview.appspot.com/124943003/diff/60001/Source/core/fetch/CSSStyleSheetResource.cpp#newcode91 Source/core/fetch/CSSStyleSheetResource.cpp:91: String sheetText = m_decoder->decode(m_data->data(), m_data->size()) + m_decoder->flush(); ...
6 years, 11 months ago (2014-01-07 16:10:31 UTC) #2
gnana
Thanks abarth for review. Please have a look. I have made some modifications. https://chromiumcodereview.appspot.com/124943003/diff/160001/Source/core/fetch/CSSStyleSheetResource.cpp File ...
6 years, 11 months ago (2014-01-08 16:01:41 UTC) #3
abarth-chromium
https://chromiumcodereview.appspot.com/124943003/diff/160001/Source/core/fetch/CSSStyleSheetResource.cpp File Source/core/fetch/CSSStyleSheetResource.cpp (right): https://chromiumcodereview.appspot.com/124943003/diff/160001/Source/core/fetch/CSSStyleSheetResource.cpp#newcode94 Source/core/fetch/CSSStyleSheetResource.cpp:94: sheetText.append(m_decoder->flush()); On 2014/01/08 16:01:42, gnana wrote: > I m ...
6 years, 11 months ago (2014-01-08 16:35:22 UTC) #4
gnana
Thanks For the suggestion. Please have a look. https://chromiumcodereview.appspot.com/124943003/diff/160001/Source/core/fetch/CSSStyleSheetResource.cpp File Source/core/fetch/CSSStyleSheetResource.cpp (right): https://chromiumcodereview.appspot.com/124943003/diff/160001/Source/core/fetch/CSSStyleSheetResource.cpp#newcode94 Source/core/fetch/CSSStyleSheetResource.cpp:94: sheetText.append(m_decoder->flush()); ...
6 years, 11 months ago (2014-01-09 08:06:43 UTC) #5
aandrey
https://chromiumcodereview.appspot.com/124943003/diff/290001/Source/platform/network/HTTPParsers.cpp File Source/platform/network/HTTPParsers.cpp (right): https://chromiumcodereview.appspot.com/124943003/diff/290001/Source/platform/network/HTTPParsers.cpp#newcode133 Source/platform/network/HTTPParsers.cpp:133: String s = String(p, std::min<size_t>(length, maxInputSampleSize)) + horizontalEllipsis; std::min(...) ...
6 years, 11 months ago (2014-01-09 08:25:38 UTC) #6
gnana
Thanks aandrey. https://chromiumcodereview.appspot.com/124943003/diff/290001/Source/platform/network/HTTPParsers.cpp File Source/platform/network/HTTPParsers.cpp (right): https://chromiumcodereview.appspot.com/124943003/diff/290001/Source/platform/network/HTTPParsers.cpp#newcode133 Source/platform/network/HTTPParsers.cpp:133: String s = String(p, std::min<size_t>(length, maxInputSampleSize)) + ...
6 years, 11 months ago (2014-01-09 10:59:11 UTC) #7
abarth-chromium
LGTM This looks much better. Thanks! https://chromiumcodereview.appspot.com/124943003/diff/340001/Source/modules/filesystem/DOMFilePath.cpp File Source/modules/filesystem/DOMFilePath.cpp (right): https://chromiumcodereview.appspot.com/124943003/diff/340001/Source/modules/filesystem/DOMFilePath.cpp#newcode52 Source/modules/filesystem/DOMFilePath.cpp:52: return newPath; I'd ...
6 years, 11 months ago (2014-01-09 15:48:23 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/124943003/390001
6 years, 11 months ago (2014-01-10 06:15:51 UTC) #9
commit-bot: I haz the power
Retried try job too often on mac_blink_rel for step(s) blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests, webkit_unit_tests, wtf_unittests ...
6 years, 11 months ago (2014-01-10 06:38:58 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/124943003/550001
6 years, 11 months ago (2014-01-10 06:50:20 UTC) #11
commit-bot: I haz the power
6 years, 11 months ago (2014-01-10 07:55:10 UTC) #12
Message was sent while issue was closed. 
Change committed as 164851

Powered by Google App Engine
This is Rietveld 408576698