Bug 131799

Summary: [Mac] implement WebKitDataCue
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: 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 Flags
Patch for the bots to chew on
none
Rebased patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Another patch.
none
Updated patch
none
YAP
none
Another attempt
dino: review+
Patch for landing
none
Patch for landing, rev 2 none

Description Eric Carlson 2014-04-17 12:15:00 PDT
The HTML working group is discussing a proposal to improve the usability of DataCue [1]. 

Implement the proposed changes behind a build flag (ENABLE_DATACUE_VALUE) so that we can experiment with the proposal. Also change DataCue to WebKitDataCue when the flag is defined because the proposal is still being discussed.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=25352
Comment 1 Eric Carlson 2014-04-17 13:04:41 PDT
Created attachment 229568 [details]
Patch for the bots to chew on
Comment 2 Eric Carlson 2014-04-17 18:23:07 PDT
Created attachment 229608 [details]
Rebased patch
Comment 3 WebKit Commit Bot 2014-04-17 18:24:42 PDT
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 4 Build Bot 2014-04-17 19:57:32 PDT
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
Comment 5 Build Bot 2014-04-17 19:57:37 PDT
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 6 Build Bot 2014-04-17 22:06:25 PDT
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
Comment 7 Build Bot 2014-04-17 22:06:33 PDT
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 8 Build Bot 2014-04-17 22:58:17 PDT
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
Comment 9 Build Bot 2014-04-17 22:58:24 PDT
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 10 Build Bot 2014-04-17 23:57:00 PDT
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
Comment 11 Build Bot 2014-04-17 23:57:07 PDT
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
Comment 12 Eric Carlson 2014-04-18 09:55:48 PDT
Created attachment 229650 [details]
Another patch.
Comment 13 WebKit Commit Bot 2014-04-18 09:57:03 PDT
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.
Comment 14 Eric Carlson 2014-04-18 10:44:23 PDT
Created attachment 229652 [details]
Updated patch
Comment 15 WebKit Commit Bot 2014-04-18 10:45:42 PDT
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.
Comment 16 Eric Carlson 2014-04-18 11:34:45 PDT
Created attachment 229658 [details]
YAP
Comment 17 Philippe Normand 2014-04-18 12:11:59 PDT
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
Comment 18 Eric Carlson 2014-04-18 12:28:55 PDT
(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!
Comment 19 Eric Carlson 2014-04-18 12:59:42 PDT
Created attachment 229667 [details]
Another attempt
Comment 20 Dean Jackson 2014-04-18 14:56:23 PDT
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.
Comment 21 Eric Carlson 2014-04-18 15:12:47 PDT
(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!
Comment 22 Eric Carlson 2014-04-18 19:40:51 PDT
Created attachment 229704 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2014-04-18 20:47:14 PDT
Comment on attachment 229704 [details]
Patch for landing

Clearing flags on attachment: 229704

Committed r167527: <http://trac.webkit.org/changeset/167527>
Comment 24 Alexey Proskuryakov 2014-04-18 22:15:39 PDT
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.
Comment 25 Eric Carlson 2014-04-21 08:31:58 PDT
Created attachment 229802 [details]
Patch for landing, rev 2
Comment 26 Eric Carlson 2014-04-21 16:00:19 PDT
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
Comment 27 Eric Carlson 2014-04-21 16:00:40 PDT
<rdar://problem/7694831>
Comment 28 Eric Carlson 2014-04-21 16:19:38 PDT
Committed r167632: https://trac.webkit.org/r167632