Summary: | [Mac] implement WebKitDataCue | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, bunhere, calvaris, cdumez, cmarcelo, commit-queue, eoconnor, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, philipj, rakuco, rniwa, sergio | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | 131883 | ||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||
Attachments: |
|
Description
Eric Carlson
2014-04-17 12:15:00 PDT
Created attachment 229568 [details]
Patch for the bots to chew on
Created attachment 229608 [details]
Rebased patch
Attachment 229608 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/avfoundation/InbandMetadataTextTrackPrivateAVF.cpp:30: You should not add a blank line before implementation file's own header. [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:30: You should not add a blank line before implementation file's own header. [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:57: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:57: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:58: The parameter name "array" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:58: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:59: The parameter name "dictionary" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:59: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:97: Extra space before ) in if [whitespace/parens] [5]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:121: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:207: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/InbandMetadataTextTrackPrivateAVF.h:55: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/InbandMetadataTextTrackPrivateAVF.h:55: The parameter name "cueFormat" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/JSDataCueCustom.cpp:30: You should not add a blank line before implementation file's own header. [build/include_order] [4]
ERROR: Source/WebCore/platform/SerializedPlatformRepresentation.h:32: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 15 in 51 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 229608 [details] Rebased patch Attachment 229608 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5826280943517696 New failing tests: http/tests/media/track-in-band-hls-metadata.html js/dom/global-constructors-attributes.html Created attachment 229616 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 229608 [details] Rebased patch Attachment 229608 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5091306812473344 New failing tests: http/tests/media/track-in-band-hls-metadata.html js/dom/global-constructors-attributes.html Created attachment 229626 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 229608 [details] Rebased patch Attachment 229608 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5328020176896000 New failing tests: http/tests/media/track-in-band-hls-metadata.html js/dom/global-constructors-attributes.html Created attachment 229631 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 229608 [details] Rebased patch Attachment 229608 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5686546866896896 New failing tests: http/tests/media/track-in-band-hls-metadata.html js/dom/global-constructors-attributes.html Created attachment 229633 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 229650 [details]
Another patch.
Attachment 229650 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/avfoundation/InbandMetadataTextTrackPrivateAVF.cpp:30: You should not add a blank line before implementation file's own header. [build/include_order] [4]
ERROR: LayoutTests/platform/mac/TestExpectations:1362: unrecognized bug identifier "Bug(eric.carlson)" [test/expectations] [5]
Total errors found: 2 in 53 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 229652 [details]
Updated patch
Attachment 229652 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/avfoundation/InbandMetadataTextTrackPrivateAVF.cpp:30: You should not add a blank line before implementation file's own header. [build/include_order] [4]
Total errors found: 1 in 54 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 229658 [details]
YAP
Comment on attachment 229658 [details] YAP View in context: https://bugs.webkit.org/attachment.cgi?id=229658&action=review > Source/WebCore/bindings/js/JSDataCueCustom.cpp:65 > +#if ENABLE(DATACUE_VALUE) Why check DATACUE_VALUE here? it should be on already, the whole file is guarded by that test already > Source/WebCore/bindings/js/JSDataCueCustom.cpp:95 > +#if !ENABLE(DATACUE_VALUE) Ditto (In reply to comment #17) > (From update of attachment 229658 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229658&action=review > > > Source/WebCore/bindings/js/JSDataCueCustom.cpp:65 > > +#if ENABLE(DATACUE_VALUE) > > Why check DATACUE_VALUE here? it should be on already, the whole file is guarded by that test already > > > Source/WebCore/bindings/js/JSDataCueCustom.cpp:95 > > +#if !ENABLE(DATACUE_VALUE) > > Ditto Aha, the entire file *shouldn't* be guarded with ENABLE(DATACUE_VALUE) at all - thanks! Created attachment 229667 [details]
Another attempt
Comment on attachment 229667 [details] Another attempt View in context: https://bugs.webkit.org/attachment.cgi?id=229667&action=review > Source/WebCore/html/track/InbandDataTextTrack.h:59 > + virtual void removeCue(TextTrackCue*, ExceptionCode&) override; Is this an override? > Source/WebCore/platform/graphics/avfoundation/InbandMetadataTextTrackPrivateAVF.cpp:33 > +#include "SoftLinking.h" What are you SoftLinking here? > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:32 > +#include "InbandMetadataTextTrackPrivateAVF.h" You seem to include it here... > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:43 > +class InbandMetadataTextTrackPrivateAVF; ... and forward declare it here. (In reply to comment #20) > (From update of attachment 229667 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229667&action=review > > > Source/WebCore/html/track/InbandDataTextTrack.h:59 > > + virtual void removeCue(TextTrackCue*, ExceptionCode&) override; > > Is this an override? > Yes, it overrides TextTrack:: removeCue > > Source/WebCore/platform/graphics/avfoundation/InbandMetadataTextTrackPrivateAVF.cpp:33 > > +#include "SoftLinking.h" > > What are you SoftLinking here? > Oops, nothing any more! > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:32 > > +#include "InbandMetadataTextTrackPrivateAVF.h" > > You seem to include it here... > > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:43 > > +class InbandMetadataTextTrackPrivateAVF; > > ... and forward declare it here. > Fixed. Thanks! Created attachment 229704 [details]
Patch for landing
Comment on attachment 229704 [details] Patch for landing Clearing flags on attachment: 229704 Committed r167527: <http://trac.webkit.org/changeset/167527> This broke 32-bit build. Data cue code uses Objective-C JavaScriptCore API, which is only available on 64 bit. Doesn't seem straightforward to fix, rolling out. Created attachment 229802 [details]
Patch for landing, rev 2
It doesn't look like the efl build failure has anything to do with this patch: Last 500 characters of output: Core.dir/__/__/DerivedSources/WebCore/JSSVGCursorElement.cpp.o c++: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-4.8/README.Bugs> for instructions. make[2]: *** [Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/JSDOMWindow.cpp.o] Error 4 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [Source/WebCore/CMakeFiles/WebCore.dir/all] Error 2 make: *** [all] Error 2 Committed r167632: https://trac.webkit.org/r167632 |