Bug 93406

Created attachment 157282 [details] Patch

Comment on attachment 157282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157282&action=review I think enough should change here that I am marking it r- for now. > Source/WebCore/Modules/mediasource/MediaSource.cpp:101 > + // Ensure a unique id. Until m_nextSourceBufferId wraps around (very unlikely), > + // this loop will exit after one check. If m_nextSourceBufferId does wrap, > + // this loop may run as many times as the size of m_sourceBuffers, but in most > + // cases that should be less than 10. We may want to investigate a more > + // efficient approach if many more SourceBuffers are allowed in the future. > + size_t oldSourceBufferId = m_nextSourceBufferId; This is specialized and long enough that is should probably be in its own function. Actually, it might make sense to make this a function of SourceBufferList.cpp instead of having some of the logic here and some there. > Source/WebCore/Modules/mediasource/MediaSource.cpp:102 > + while (m_sourceBuffers->contains(String::number(m_nextSourceBufferId++))) { This seems wrong. You are searching for m_nextSourceBufferId, but you use (m_nextSourceBufferId+1) for the new buffer ID even if m_nextSourceBufferId doesn't exist in the vector. > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:89 > +bool SourceBufferList::contains(const String& id) const > +{ > + for (size_t i = 0; i < m_list.size(); ++i) { > + if (m_list[i]->id() == id) > + return true; > + } > + return false; > +} Since you have to implement this, why not pass the ID as a size_t?

Comment on attachment 157282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157282&action=review Thanks Eric, revised patch on the way. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:101 >> + size_t oldSourceBufferId = m_nextSourceBufferId; > > This is specialized and long enough that is should probably be in its own function. > > Actually, it might make sense to make this a function of SourceBufferList.cpp instead of having some of the logic here and some there. OK, I'll give that a go. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:102 >> + while (m_sourceBuffers->contains(String::number(m_nextSourceBufferId++))) { > > This seems wrong. You are searching for m_nextSourceBufferId, but you use (m_nextSourceBufferId+1) for the new buffer ID even if m_nextSourceBufferId doesn't exist in the vector. Good catch, thanks. >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:89 >> +} > > Since you have to implement this, why not pass the ID as a size_t? Done.

Created attachment 157325 [details] revisions based on review

Comment on attachment 157325 [details] revisions based on review Rejecting attachment 157325 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: code: 9 s/ChangeLog Auto-merging LayoutTests/platform/chromium/TestExpectations Failed to merge in the changes. Patch failed at 0001 nested fragment parser test slow on Android When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/13453914

Created attachment 157523 [details] Patch for landing

Comment on attachment 157523 [details] Patch for landing Rejecting attachment 157523 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Websites/webkit.org/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/13473123

Created attachment 157571 [details] no diff Bugger, commit queue ate my patch and lost the r+.

Committed r125424: <http://trac.webkit.org/changeset/125424>