WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
131799
[Mac] implement WebKitDataCue
https://bugs.webkit.org/show_bug.cgi?id=131799
Summary
[Mac] implement WebKitDataCue
Eric Carlson
Reported
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
Attachments
Patch for the bots to chew on
(784.78 KB, patch)
2014-04-17 13:04 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Rebased patch
(784.77 KB, patch)
2014-04-17 18:23 PDT
,
Eric Carlson
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
(510.35 KB, application/zip)
2014-04-17 19:57 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
(668.89 KB, application/zip)
2014-04-17 22:06 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
(664.40 KB, application/zip)
2014-04-17 22:58 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
(666.02 KB, application/zip)
2014-04-17 23:57 PDT
,
Build Bot
no flags
Details
Another patch.
(788.27 KB, patch)
2014-04-18 09:55 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Updated patch
(788.95 KB, patch)
2014-04-18 10:44 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
YAP
(789.71 KB, patch)
2014-04-18 11:34 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Another attempt
(792.25 KB, patch)
2014-04-18 12:59 PDT
,
Eric Carlson
dino
: review+
Details
Formatted Diff
Diff
Patch for landing
(792.57 KB, patch)
2014-04-18 19:40 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Patch for landing, rev 2
(792.60 KB, patch)
2014-04-21 08:31 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Eric Carlson
Comment 1
2014-04-17 13:04:41 PDT
Created
attachment 229568
[details]
Patch for the bots to chew on
Eric Carlson
Comment 2
2014-04-17 18:23:07 PDT
Created
attachment 229608
[details]
Rebased patch
WebKit Commit Bot
Comment 3
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.
Build Bot
Comment 4
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
Build Bot
Comment 5
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
Build Bot
Comment 6
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
Build Bot
Comment 7
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
Build Bot
Comment 8
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
Build Bot
Comment 9
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
Build Bot
Comment 10
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
Build Bot
Comment 11
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
Eric Carlson
Comment 12
2014-04-18 09:55:48 PDT
Created
attachment 229650
[details]
Another patch.
WebKit Commit Bot
Comment 13
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.
Eric Carlson
Comment 14
2014-04-18 10:44:23 PDT
Created
attachment 229652
[details]
Updated patch
WebKit Commit Bot
Comment 15
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.
Eric Carlson
Comment 16
2014-04-18 11:34:45 PDT
Created
attachment 229658
[details]
YAP
Philippe Normand
Comment 17
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
Eric Carlson
Comment 18
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!
Eric Carlson
Comment 19
2014-04-18 12:59:42 PDT
Created
attachment 229667
[details]
Another attempt
Dean Jackson
Comment 20
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.
Eric Carlson
Comment 21
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!
Eric Carlson
Comment 22
2014-04-18 19:40:51 PDT
Created
attachment 229704
[details]
Patch for landing
WebKit Commit Bot
Comment 23
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
>
Alexey Proskuryakov
Comment 24
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.
Eric Carlson
Comment 25
2014-04-21 08:31:58 PDT
Created
attachment 229802
[details]
Patch for landing, rev 2
Eric Carlson
Comment 26
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
Eric Carlson
Comment 27
2014-04-21 16:00:40 PDT
<
rdar://problem/7694831
>
Eric Carlson
Comment 28
2014-04-21 16:19:38 PDT
Committed
r167632
:
https://trac.webkit.org/r167632
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug