Bug 53556 – Adding Feature Defines for <track>

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

A first step in implementing <track> and HTMLTrackElement is adding appropriate #ifdefs.

[Anna Cavender]

Created attachment 87825 [details]
Patch

[Early Warning System Bot]

Attachment 87825 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8185082

[Collabora GTK+ EWS bot]

Attachment 87825 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8185088

[Anna Cavender]

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?

[Eric Carlson]

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"

[Anna Cavender]

Created attachment 87857 [details]
Patch

[Anna Cavender]

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.

[Early Warning System Bot]

Attachment 87857 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8315519

[Collabora GTK+ EWS bot]

Attachment 87857 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8247203

[Anna Cavender]

*** Bug 53557 has been marked as a duplicate of this bug. ***

[Anna Cavender]

Created attachment 87921 [details]
Patch

[Build Bot]

Attachment 87921 [details] did not build on win:
Build output: http://queues.webkit.org/results/8185310

[Anna Cavender]

Created attachment 87944 [details]
Patch

[Anna Cavender]

Windows build errors fixed.  Other errors look like they could be addressed with a manual commit.

[Anna Cavender]

Created attachment 88361 [details]
Patch

[Eric Carlson]

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);"

[Anna Cavender]

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!

[Anna Cavender]

Created attachment 88465 [details]
Patch

[WebKit Commit Bot]

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

[Anna Cavender]

Need to land this patch manually?

[WebKit Review Bot]

http://trac.webkit.org/changeset/83220 might have broken Qt Linux Release minimal

[Anna Cavender]

Created attachment 88752 [details]
Patch

[Anna Cavender]

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

[Anna Cavender]

Created attachment 88753 [details]
Patch

[Anna Cavender]

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.

[Anna Cavender]

Created attachment 88762 [details]
Patch

[Hin-Chung Lam]

Committed r83335: <http://trac.webkit.org/changeset/83335>

[WebKit Review Bot]

http://trac.webkit.org/changeset/83335 might have broken Chromium Win Release

[Hin-Chung Lam]

Reverted r83335 for reason:

GTK and QT bots are broken

Committed r83342: <http://trac.webkit.org/changeset/83342>

[Alexis Menard (darktears)]

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.

[Alexis Menard (darktears)]

Committed r83527: <http://trac.webkit.org/changeset/83527>

[Anna Cavender]

Thanks Alexis, I appreciate the helping hand. :)  Was having trouble replicating gtk and qt issues locally.

[Alexis Menard (darktears)]

(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.

[Alexis Menard (darktears)]

http://trac.webkit.org/changeset/83539 fixed the Mac build :D.

[Daniel Bates]

(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>

[Daniel Bates]

Committed GTK build fix in changeset 83549 <http://trac.webkit.org/changeset/83549>.

[Alexis Menard (darktears)]

(In reply to comment #36)
> Committed GTK build fix in changeset 83549 <http://trac.webkit.org/changeset/83549>.

Thanks for the fix.

[Anna Cavender]

Created attachment 89486 [details]
Rename TRACK to VIDEO_TRACK

[Anna Cavender]

one last thing: changing the name from TRACK to VIDEO_TRACK.

[WebKit Commit Bot]

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

[Anna Cavender]

Created attachment 89621 [details]
Rename TRACK to VIDEO_TRACK, try again with updated patch

[WebKit Commit Bot]

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: [e-mail])
http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: [e-mail])
fast/images/load-img-with-empty-src.html bug 50855 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Review Bot]

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.

[Hajime Morrita]

Can we close this?

[Anna Cavender]

Yep.  Go ahead and close.