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

Issue 101513002: Add new helper VTTParser::collectDigitsToInt (Closed)

Created:
7 years ago by fs
Modified:
7 years ago
CC:
blink-reviews, nessy, philipj_slow, gasubic, dglazkov+blink, adamk+blink_chromium.org, vcarbune.chromium
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Add new helper VTTParser::collectDigitsToInt Since most uses of VTTParser::collectDigits is followed by a call to String::toInt on the resulting string, add this new helper with the aim of later removing the temporary string construction. Handle overflow in the string-to-int conversion by means of saturation. (The latter is the intended change of semantics.) Replace uses of collectDigits with collectDigitsToInt where applicable. After doing this, there remain no callers to collectDigits outside of VTTParser, so make the function static. Also move VTTParser::skipWhiteSpace "textually" to group it with the other 'scanning' functions, and make it static since it does not depend on any VTTParser state. BUG=305317 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163098

Patch Set 1 #

Patch Set 2 : Rebased #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -43 lines) Patch
M Source/core/html/track/vtt/VTTCue.cpp View 4 chunks +4 lines, -12 lines 0 comments Download
M Source/core/html/track/vtt/VTTParser.h View 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/html/track/vtt/VTTParser.cpp View 1 7 chunks +29 lines, -21 lines 0 comments Download
M Source/core/html/track/vtt/VTTRegion.cpp View 1 2 chunks +4 lines, -8 lines 2 comments Download

Messages

Total messages: 7 (0 generated)
fs
7 years ago (2013-12-03 12:19:41 UTC) #1
Mike West
On 2013/12/03 12:19:41, fs wrote: LGTM. It might be worth merging this parsing style with ...
7 years ago (2013-12-03 13:55:20 UTC) #2
Mike West
There was a nit I should have published with the last comment. https://codereview.chromium.org/101513002/diff/20001/Source/core/html/track/vtt/VTTRegion.cpp File Source/core/html/track/vtt/VTTRegion.cpp ...
7 years ago (2013-12-03 13:55:41 UTC) #3
jochen (gone - plz use gerrit)
lgtm https://codereview.chromium.org/101513002/diff/20001/Source/core/html/track/vtt/VTTRegion.cpp File Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/101513002/diff/20001/Source/core/html/track/vtt/VTTRegion.cpp#newcode262 Source/core/html/track/vtt/VTTRegion.cpp:262: } On 2013/12/03 13:55:41, Mike West wrote: > ...
7 years ago (2013-12-03 14:48:37 UTC) #4
fs
On 2013/12/03 14:48:37, jochen wrote: > lgtm > > https://codereview.chromium.org/101513002/diff/20001/Source/core/html/track/vtt/VTTRegion.cpp > File Source/core/html/track/vtt/VTTRegion.cpp (right): > ...
7 years ago (2013-12-03 15:51:25 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fs@opera.com/101513002/20001
7 years ago (2013-12-03 15:51:41 UTC) #6
commit-bot: I haz the power
7 years ago (2013-12-03 16:55:19 UTC) #7
Message was sent while issue was closed.
Change committed as 163098

Powered by Google App Engine
This is Rietveld 408576698