A first step in implementing <track> and HTMLTrackElement is adding appropriate #ifdefs.
Created attachment 87825 [details] Patch
Attachment 87825 [details] did not build on qt: Build output: http://queues.webkit.org/results/8185082
Attachment 87825 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8185088
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 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"
Created attachment 87857 [details] Patch
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.
Attachment 87857 [details] did not build on qt: Build output: http://queues.webkit.org/results/8315519
Attachment 87857 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8247203
*** Bug 53557 has been marked as a duplicate of this bug. ***
Created attachment 87921 [details] Patch
Attachment 87921 [details] did not build on win: Build output: http://queues.webkit.org/results/8185310
Created attachment 87944 [details] Patch
Windows build errors fixed. Other errors look like they could be addressed with a manual commit.
Created attachment 88361 [details] Patch
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 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!
Created attachment 88465 [details] Patch
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
Need to land this patch manually?
http://trac.webkit.org/changeset/83220 might have broken Qt Linux Release minimal
Created attachment 88752 [details] Patch
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
Created attachment 88753 [details] Patch
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.
Created attachment 88762 [details] Patch
Committed r83335: <http://trac.webkit.org/changeset/83335>
http://trac.webkit.org/changeset/83335 might have broken Chromium Win Release
Reverted r83335 for reason: GTK and QT bots are broken Committed r83342: <http://trac.webkit.org/changeset/83342>
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.
Committed r83527: <http://trac.webkit.org/changeset/83527>
Thanks Alexis, I appreciate the helping hand. :) Was having trouble replicating gtk and qt issues locally.
(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.
http://trac.webkit.org/changeset/83539 fixed the Mac build :D.
(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>
Committed GTK build fix in changeset 83549 <http://trac.webkit.org/changeset/83549>.
(In reply to comment #36) > Committed GTK build fix in changeset 83549 <http://trac.webkit.org/changeset/83549>. Thanks for the fix.
Created attachment 89486 [details] Rename TRACK to VIDEO_TRACK
one last thing: changing the name from TRACK to VIDEO_TRACK.
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
Created attachment 89621 [details] Rename TRACK to VIDEO_TRACK, try again with updated patch
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 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 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.
Can we close this?
Yep. Go ahead and close.