Bug 53556

Summary: Adding Feature Defines for <track>
Product: WebKit Reporter: Anna Cavender <annacc>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, annacc, buildbot, commit-queue, dbates, eric.carlson, eric, gns, gustavo.noronha, hclam, menard, morrita, peter, sjl, vrk, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 43668    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch which fix the issue with Qt and GTK.
eric.carlson: review+
Rename TRACK to VIDEO_TRACK
none
Rename TRACK to VIDEO_TRACK, try again with updated patch none

Description Anna Cavender 2011-02-01 17:05:45 PST
A first step in implementing <track> and HTMLTrackElement is adding appropriate #ifdefs.
Comment 1 Anna Cavender 2011-03-31 22:10:50 PDT
Created attachment 87825 [details]
Patch
Comment 2 Early Warning System Bot 2011-03-31 22:22:38 PDT
Attachment 87825 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8185082
Comment 3 Collabora GTK+ EWS bot 2011-03-31 22:47:37 PDT
Attachment 87825 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8185088
Comment 4 Anna Cavender 2011-03-31 23:04:11 PDT
Hmm, looks like the qt and qtk bots are deriving code in JSHTMLElementWrapperFactory that expects to see a JSHTMLTrackElement.h.  I would have expected the #ifdefs and ENABLE(TRACK)=0 to prevent that (seems to be ok on other platforms).  Did I miss something?
Comment 5 Eric Carlson 2011-03-31 23:28:43 PDT
Comment on attachment 87825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87825&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:739
> +#if ENABLE(TRACK)
> +void HTMLMediaElement::loadMediaTracks()
> +{
> +    for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +        if (node->hasTagName(trackTag)) {
> +            HTMLTrackElement* track = static_cast<HTMLTrackElement*>(node);
> +            // TODO(annacc): store in an ExclusiveTrackList (4.8.10.10.1)
> +            track->load();
> +        }
> +    }
> +}
> +#endif
> +

I would leave the HTMLMediaElement changes out of this patch. loadMediaTracks doesn't do anything yet, and it isn't clear who will call it or when, so it isn't possible to evaluate the design or code.


> Source/WebCore/html/HTMLTrackElement.cpp:79
> +void HTMLTrackElement::setSrc(const String& url)
> +{
> +    setAttribute(srcAttr, url);
> +}
> +

Isn't a src accessor method also necessary?


> Source/WebCore/html/HTMLTrackElement.cpp:128
> +void HTMLTrackElement::scheduleErrorEvent()
> +{
> +    LOG(Media, "HTMLTrackElement::scheduleErrorEvent - %p", this);
> +    if (m_errorEventTimer.isActive())
> +        return;
> +
> +    m_errorEventTimer.startOneShot(0);
> +}
> +

Who will call scheduleErrorEvent and why? It probably makes sense to only include the track methods that deal with IDL attributes in this patch because without more context it isn't really possible to evaluate this part.


> Source/WebCore/html/HTMLTrackElement.cpp:148
> +    // TODO(annacc): create a LoadableTextTrack and kick of loading

Type "of" -> "off"
Comment 6 Anna Cavender 2011-04-01 08:15:33 PDT
Created attachment 87857 [details]
Patch
Comment 7 Anna Cavender 2011-04-01 08:19:33 PDT
Thanks for the quick response!

I removed all changes not relevant to the IDL for HTMLTrackElement.

Any thoughts to why it's not building on qt and gtk?  I'm not familiar with that area, but I don't see gypi files that look like they will help.
Comment 8 Early Warning System Bot 2011-04-01 08:29:34 PDT
Attachment 87857 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8315519
Comment 9 Collabora GTK+ EWS bot 2011-04-01 08:39:28 PDT
Attachment 87857 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8247203
Comment 10 Anna Cavender 2011-04-01 13:24:56 PDT
*** Bug 53557 has been marked as a duplicate of this bug. ***
Comment 11 Anna Cavender 2011-04-01 15:03:39 PDT
Created attachment 87921 [details]
Patch
Comment 12 Build Bot 2011-04-01 16:00:57 PDT
Attachment 87921 [details] did not build on win:
Build output: http://queues.webkit.org/results/8185310
Comment 13 Anna Cavender 2011-04-01 16:49:27 PDT
Created attachment 87944 [details]
Patch
Comment 14 Anna Cavender 2011-04-04 14:46:31 PDT
Windows build errors fixed.  Other errors look like they could be addressed with a manual commit.
Comment 15 Anna Cavender 2011-04-05 21:27:02 PDT
Created attachment 88361 [details]
Patch
Comment 16 Eric Carlson 2011-04-06 09:40:58 PDT
Comment on attachment 88361 [details]
Patch

r+ with the changes suggested below.

View in context: https://bugs.webkit.org/attachment.cgi?id=88361&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Shouldn't leave an OOPS in the ChangeLog. It would be helpful to say why no new tests were added with this patch.


> Source/WebCore/html/HTMLTrackElement.cpp:66
> +        // TODO(annacc):
> +        // static_cast<HTMLMediaElement*>(parentNode())->trackWasAdded(this);

This isn't right, maybe you mean something like "trackWillBeRemoved(this)"?


> Source/WebCore/html/HTMLTrackElement.cpp:114
> +bool HTMLTrackElement::isDefault() const
> +{
> +    return equalIgnoringCase(getAttribute(defaultAttr), "true");
> +}

"default" is a boolean attribute, so I think you should use "return hasAttribute(defaultAttr)"


> Source/WebCore/html/HTMLTrackElement.cpp:119
> +void HTMLTrackElement::setIsDefault(bool isDefault)
> +{
> +    setAttribute(defaultAttr, isDefault ? "true" : "false");
> +}

And here you can use "setBooleanAttribute(defaultAttr, isDefault);"
Comment 17 Anna Cavender 2011-04-06 10:17:54 PDT
Comment on attachment 88361 [details]
Patch

Thanks for the review! New patch up shortly.

View in context: https://bugs.webkit.org/attachment.cgi?id=88361&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> Shouldn't leave an OOPS in the ChangeLog. It would be helpful to say why no new tests were added with this patch.

Done.

>> Source/WebCore/html/HTMLTrackElement.cpp:66
>> +        // static_cast<HTMLMediaElement*>(parentNode())->trackWasAdded(this);
> 
> This isn't right, maybe you mean something like "trackWillBeRemoved(this)"?

oops, good catch.  Done.

>> Source/WebCore/html/HTMLTrackElement.cpp:114
>> +}
> 
> "default" is a boolean attribute, so I think you should use "return hasAttribute(defaultAttr)"

Done.  Not sure what I was thinking :).

>> Source/WebCore/html/HTMLTrackElement.cpp:119
>> +}
> 
> And here you can use "setBooleanAttribute(defaultAttr, isDefault);"

Done. Thanks!
Comment 18 Anna Cavender 2011-04-06 10:20:20 PDT
Created attachment 88465 [details]
Patch
Comment 19 WebKit Commit Bot 2011-04-06 11:35:58 PDT
Comment on attachment 88465 [details]
Patch

Rejecting attachment 88465 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
 of 2 hunks FAILED -- saving rejects to file WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops.rej
patching file WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops
Hunk #1 FAILED at 9.
Hunk #2 FAILED at 202.
2 out of 2 hunks FAILED -- saving rejects to file WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops.rej
patching file configure.ac

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Carlson', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/8344300
Comment 20 Anna Cavender 2011-04-06 12:50:57 PDT
Need to land this patch manually?
Comment 21 WebKit Review Bot 2011-04-07 15:39:46 PDT
http://trac.webkit.org/changeset/83220 might have broken Qt Linux Release minimal
Comment 22 Anna Cavender 2011-04-07 18:47:33 PDT
Created attachment 88752 [details]
Patch
Comment 23 Anna Cavender 2011-04-07 18:50:02 PDT
Last patch commited by hand by scherkus ran into qt errors similar to the first two patches and had to be reverted (see below).  I don't see why this would be happening, so I'm submitting a new patch here to see what the bots do with it.

../../WebCore/generated/JSHTMLElementWrapperFactory.cpp:99:32: error: JSHTMLTrackElement.h: No such file or directory
distcc[629] ERROR: compile ../../WebCore/generated/JSHTMLElementWrapperFactory.cpp on localhost failed
Comment 24 Anna Cavender 2011-04-07 19:05:42 PDT
Created attachment 88753 [details]
Patch
Comment 25 Anna Cavender 2011-04-07 21:06:31 PDT
Arg.  I think I see the problem.  Changing 
default => (isAppleWebKit() || isGtk()) 
to
default => 0
which I think was an accidental carryover from ENABLE_VIDEO. :(

I'm really sorry about all the patches.  Hopefully this will fix the problems.
Comment 26 Anna Cavender 2011-04-07 21:07:15 PDT
Created attachment 88762 [details]
Patch
Comment 27 Hin-Chung Lam 2011-04-08 13:28:39 PDT
Committed r83335: <http://trac.webkit.org/changeset/83335>
Comment 28 WebKit Review Bot 2011-04-08 13:42:47 PDT
http://trac.webkit.org/changeset/83335 might have broken Chromium Win Release
Comment 29 Hin-Chung Lam 2011-04-08 13:58:33 PDT
Reverted r83335 for reason:

GTK and QT bots are broken

Committed r83342: <http://trac.webkit.org/changeset/83342>
Comment 30 Alexis Menard (darktears) 2011-04-11 14:40:15 PDT
Created attachment 89094 [details]
Patch which fix the issue with Qt and GTK.

Hi,

I believe I fixed the issues with Qt and GTK bots. Eric could you please review it again? Thanks.
Comment 31 Alexis Menard (darktears) 2011-04-11 16:12:05 PDT
Committed r83527: <http://trac.webkit.org/changeset/83527>
Comment 32 Anna Cavender 2011-04-11 16:44:18 PDT
Thanks Alexis, I appreciate the helping hand. :)  Was having trouble replicating gtk and qt issues locally.
Comment 33 Alexis Menard (darktears) 2011-04-11 16:49:56 PDT
(In reply to comment #32)
> Thanks Alexis, I appreciate the helping hand. :)  Was having trouble replicating gtk and qt issues locally.


No problem, there was some mac build issue too after I landed the patch, I hope http://trac.webkit.org/changeset/83534 will fix it :D.
Comment 34 Alexis Menard (darktears) 2011-04-11 17:34:02 PDT
http://trac.webkit.org/changeset/83539 fixed the Mac build :D.
Comment 35 Daniel Bates 2011-04-11 18:47:03 PDT
(In reply to comment #31)
> Committed r83527: <http://trac.webkit.org/changeset/83527>

This change broke the GTK 32-bit Release build (why not the 64 bit one?):

GTK 32-bit Linux Release (build 12633; r83548):
<http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/12633/steps/compile-webkit/logs/stdio>
Comment 36 Daniel Bates 2011-04-11 18:49:57 PDT
Committed GTK build fix in changeset 83549 <http://trac.webkit.org/changeset/83549>.
Comment 37 Alexis Menard (darktears) 2011-04-11 19:01:15 PDT
(In reply to comment #36)
> Committed GTK build fix in changeset 83549 <http://trac.webkit.org/changeset/83549>.

Thanks for the fix.
Comment 38 Anna Cavender 2011-04-13 16:06:50 PDT
Created attachment 89486 [details]
Rename TRACK to VIDEO_TRACK
Comment 39 Anna Cavender 2011-04-14 10:49:35 PDT
one last thing: changing the name from TRACK to VIDEO_TRACK.
Comment 40 WebKit Commit Bot 2011-04-14 11:05:16 PDT
Comment on attachment 89486 [details]
Rename TRACK to VIDEO_TRACK

Rejecting attachment 89486 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
ing file Source/WebKit/mac/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/mac/Configurations/FeatureDefines.xcconfig
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/Configurations/FeatureDefines.xcconfig
patch: **** Only garbage was found in the patch input.
patching file configure.ac

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Carlson', u'--for..." exit_code: 2

Full output: http://queues.webkit.org/results/8399696
Comment 41 Anna Cavender 2011-04-14 12:13:50 PDT
Created attachment 89621 [details]
Rename TRACK to VIDEO_TRACK, try again with updated patch
Comment 42 WebKit Commit Bot 2011-04-15 00:11:03 PDT
The commit-queue encountered the following flaky tests while processing attachment 89621 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: alice.liu@apple.com)
http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org)
fast/images/load-img-with-empty-src.html bug 50855 (author: mitz@webkit.org)
The commit-queue is continuing to process your patch.
Comment 43 WebKit Commit Bot 2011-04-15 00:14:07 PDT
Comment on attachment 89621 [details]
Rename TRACK to VIDEO_TRACK, try again with updated patch

Clearing flags on attachment: 89621

Committed r83952: <http://trac.webkit.org/changeset/83952>
Comment 44 WebKit Review Bot 2011-04-15 14:14:22 PDT
Comment on attachment 88762 [details]
Patch

Cleared Eric Carlson's review+ from obsolete attachment 88762 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 45 Hajime Morrita 2011-06-14 23:33:29 PDT
Can we close this?
Comment 46 Anna Cavender 2011-06-15 08:07:18 PDT
Yep.  Go ahead and close.