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

Issue 73813002: Simplify state handling in WebVTTTokenizer::nextToken (Closed)

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

Description

Simplify state handling in WebVTTTokenizer::nextToken When the WebVTT tokenizer returns a new token, it always returns to the 'Data' state - except for when encountering an opening '<' while in the 'Data' state, and having collected a non-empty text-node. In that particular case though, we can avoid the transition to the 'Tag' state by not advancing the input, and simply return indicating a new token. With the above change, and the change to always set the FSM state to 'Data', the 'state' argument to emitAndResumeIn() becomes redundant - so it can be removed. With the argument gone, rename the method to advanceAndEmitToken (since it no longer needs to set a state to resume in). Also add a new method emitToken which only returns true, and is currently only used as a marker. Add a (currently unused) argument to the various emitter methods in WebVTTTokenizer. Use this new argument to indicate what type of token each return statement should produce. With the annotations in place it becomes clear that - after adding additional "ensurance" to one of the instances - that the emitEndOfFile emitter is redundant (always emits "Character" tokens and always has a token "buffered" - i.e. haveBufferedCharacterToken() will return true), and can be replaced by a call to emitToken instead. This also means that WebVTTToken::makeEndOfFile and the corresponding state can be removed. BUG=319391 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162198

Patch Set 1 #

Patch Set 2 : Rebased after files moved #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -30 lines) Patch
M Source/core/html/track/vtt/VTTToken.h View 1 2 chunks +0 lines, -7 lines 0 comments Download
M Source/core/html/track/vtt/VTTTokenizer.h View 1 2 chunks +7 lines, -10 lines 0 comments Download
M Source/core/html/track/vtt/VTTTokenizer.cpp View 1 11 chunks +17 lines, -13 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
fs
7 years, 1 month ago (2013-11-15 08:06:07 UTC) #1
fs
7 years, 1 month ago (2013-11-18 11:30:36 UTC) #2
jochen (gone - plz use gerrit)
lgtm
7 years, 1 month ago (2013-11-18 11:45:26 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fs@opera.com/73813002/40001
7 years, 1 month ago (2013-11-18 11:55:04 UTC) #4
commit-bot: I haz the power
7 years, 1 month ago (2013-11-18 13:27:37 UTC) #5
Message was sent while issue was closed.
Change committed as 162198

Powered by Google App Engine
This is Rietveld 408576698