| Summary: | Fix TimeRanges layering violations | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bunhere, cgarcia, commit-queue, esprehn+autocc, glenn, gustavo, gyuyoung.kim, jer.noble, menard, mrobinson, philipj, pnormand, rakuco, sergio | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Eric Carlson
2014-02-12 19:15:16 PST
Created attachment 224681 [details]
Patch for the bots to chew on.
Attachment 224681 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:773: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:774: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/PlatformTimeRanges.cpp:28: You should not add a blank line before implementation file's own header. [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/PlatformTimeRanges.cpp:144: Omit int when using unsigned [runtime/unsigned] [1]
ERROR: Source/WebCore/platform/graphics/PlatformTimeRanges.cpp:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/PlatformTimeRanges.cpp:171: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 6 in 41 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 224694 [details]
Updated patch
Attachment 224694 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:772: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:774: More than one command on the same line [whitespace/newline] [4]
Total errors found: 2 in 43 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 224694 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=224694&action=review r=me, with nits. > Source/WebCore/Modules/mediasource/MediaSource.cpp:180 > - return intersectionRanges.release(); > + auto result = PlatformTimeRanges::create(); > + unsigned size = intersectionRanges->length(); > + for (unsigned i = 0; i < size; i++) > + result->add(intersectionRanges->start(i, ASSERT_NO_EXCEPTION), intersectionRanges->end(i, ASSERT_NO_EXCEPTION)); > + > + return result; This is weird. Can't we just do: return PlatformTimeRanges::create(intersectionRanges->ranges()) ? > Source/WebCore/html/HTMLMediaElement.cpp:3079 > + auto timeRanges = m_player->buffered(); I don't believe we're supposed to use 'auto' in this way; only when the type is implied by the method called. > Source/WebCore/html/HTMLMediaElement.cpp:3082 > - double start = timeRanges->start(i, IGNORE_EXCEPTION); > - double end = timeRanges->end(i, IGNORE_EXCEPTION); > + double start = timeRanges->start(i, ignored); > + double end = timeRanges->end(i, ignored); Why are we removing IGNORE_EXCEPTION? > Source/WebCore/html/TimeRanges.h:67 > + const PlatformTimeRanges* ranges() const { return &m_ranges; } Lets make this public, and have it return a 'const PlatformTimeRanges&'. Comment on attachment 224694 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=224694&action=review >> Source/WebCore/Modules/mediasource/MediaSource.cpp:180 >> + return result; > > This is weird. Can't we just do: > > return PlatformTimeRanges::create(intersectionRanges->ranges()) ? Good idea, I will try that. >> Source/WebCore/html/HTMLMediaElement.cpp:3079 >> + auto timeRanges = m_player->buffered(); > > I don't believe we're supposed to use 'auto' in this way; only when the type is implied by the method called. OK >> Source/WebCore/html/HTMLMediaElement.cpp:3082 >> + double end = timeRanges->end(i, ignored); > > Why are we removing IGNORE_EXCEPTION? Exception is an HTML concept, so PlatformTimeRanges returns a bool to indicate success/failure. Committed r164498: https://trac.webkit.org/r164498 |