868629 - [webvtt] remove WEBVTT_NO_CONFIG_H

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

Right now the webvtt library headers must be used with WEBVTT_NO_CONFIG_H unless the build includes a generated config.h in the public header set. This is confusing and brittle and quite a few developers have complained about it.

This bug is to make it possible to build and/or #include the headers in Firefox without setting a magic flag.

Comment 1

[Caitlin Potter (:caitp)]

As discussed on irc:

The configuration header doesn't really contain much -- it only tells us whether or not stdint.h is available, and whether strerror() is available (only used by the demo application).

I've suggested that we can get around requiring a check for stdint.h by assuming it is present, except when it is known to not be present. eg, defined(_MSC_VER) && _MSC_VER < 1600).

The demo application doesn't necessarily need to use strerror(), and so this can be removed as well.

The result is that there is no real need to depend on a configuration header in the first place.

If there are any other suggestions I'm happy to implement them on monday

Comment 2

[Caitlin Potter (:caitp)]

It would also simplify things further to just not build webvtt as a shared library, which would eliminate the requirement for -DWEBVTT_STATIC as well.

Comment 3

[Ralph Giles (:rillian)]

It's fine to #ifdef HAVE_CONFIG_H / #include "config.h" in the demo application for things like strerror().

Comment 4

[Caitlin Potter (:caitp)]

https://github.com/mozilla/webvtt/pull/389 -- The current strategy checks if HAVE_CONFIG_H is defined (if true, attempts to include "config.h") -- but otherwise, some assumptions are made regarding the availability of C99 features like stdint.h

I've removed the conditional strerror() in the demo application, because I think it's a pretty widely supported feature that mostly varies in terms of thread safety. Should be okay for the synchronous demo app, and if it fails to build it's not really going to hurt anybody too badly. But this can be updated if need be.

What are your thoughts on the removal of the shared library build? It would simplify things a bit.

Comment 5

[Caitlin Potter (:caitp)]

Attachment: Update media/webvtt (revision no longer requires -DWEBVTT_NO_CONFIG_H)

Requirement for passing -DWEBVTT_NO_CONFIG_H on the command line has been removed. -DWEBVTT_STATIC is still required, unfortunately.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 745985 [details] [diff] [review]
Update media/webvtt (revision no longer requires -DWEBVTT_NO_CONFIG_H)

Is this an update to the current revision? I'd rather you backported the specific fix for this bug (removing webvtt-config.h) as a patch until we can tag a new upstream release.

Comment 7

[Caitlin Potter (:caitp)]

Attachment: webvtt no longer needs -DWEBVTT_NO_CONFIG_H.

Backported fixes added in mozilla/webvtt bb41b6128f. (config.h is no longer included at all in webvtt/util.h).

Readme updated to reflect this.

Comment 8

[Caitlin Potter (:caitp)]

Attachment: webvtt no longer needs -DWEBVTT_NO_CONFIG_H. r=rillian

Quick fixup to attachment 746543 [details] [diff] [review] -- Make sure WEBVTT_OS_WIN32/64 get defined (they are used further down)

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 746565 [details] [diff] [review]
webvtt no longer needs -DWEBVTT_NO_CONFIG_H. r=rillian

Review of attachment 746565 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for backporting just the relevant changes. Those look good.

Please also include a patch file and apply it at the bottom of media/webvtt/update.sh so running the script produces the correct code. This makes it obvious how we've diverged from the upstream release.

Comment 10

[Caitlin Potter (:caitp)]

Attachment: webvtt no longer needs -DWEBVTT_NO_CONFIG_H. r=rillian

Changes to update.sh:
- Given execute permissions (filemode 755 now)
- Applies patch for bug 868629 (This will probably need to be removed after the next tagged release is imported)

Now includes the original patch (media/webvtt/868629.patch)

Comment 11

[Caitlin Potter (:caitp)]

Forgot to mention above, I've also changed the shebang line for the update script, because it's not valid Bourne shell script (which is the default on this system).

Maybe some effort into making it more portable can be done later on.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 747189 [details] [diff] [review]
webvtt no longer needs -DWEBVTT_NO_CONFIG_H. r=rillian

Review of attachment 747189 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with fixes.

::: media/webvtt/868629.patch
@@ +4,5 @@
> +# Node ID 2becb21900559e271175e08d7ec33ed35b034967
> +# Parent  41ff3b67b69232297191c8f8ef78e5facc1c1d19
> +Bug 868629 - webvtt no longer needs -DWEBVTT_NO_CONFIG_H. r=rillian
> +
> +diff --git a/media/webvtt/README_MOZILLA b/media/webvtt/README_MOZILLA

Don't include README_MOZILLA in the patch. This file is only in the mozilla repo, so changes are already tracked. We just want the diff to the upstream code.

Also lets you re-run update.sh without getting 'already applied' errors.

::: media/webvtt/update.sh
@@ +77,5 @@
>  if [ ${have_hg} -eq 0 -a -d ${start_dir}/.hg ]; then
>    hg addremove ${webvtt_dir}/
>  fi
> +
> +# apply patches (from root of tree)

This doesn't work when invoked as './update.sh' because webvtt_dir is '.' Just assume you're running in the local directory like all the other media/*/update.sh.

You can use patch -p3 instead to strip the prefix.

@@ +79,5 @@
>  fi
> +
> +# apply patches (from root of tree)
> +cd ${webvtt_dir}/../..
> +patch -p1 -i ${webvtt_dir}/868629.patch

'patch -i' isn't portable. Please use 'patch -p3 < 868629.patch' instead.

Comment 13

[Ralph Giles (:rillian)]

Updated code builds on try. https://tbpl.mozilla.org/?tree=Try&rev=1e6fb308ec75

Comment 14

[Caitlin Potter (:caitp)]

Attachment: Bug 868629 - webvtt no longer needs -DWEBVTT_NO_CONFIG_H. r=rillian

Addressed Ralph's comments from https://bugzilla.mozilla.org/show_bug.cgi?id=868629#c12

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 749384 [details] [diff] [review]
Bug 868629 - webvtt no longer needs -DWEBVTT_NO_CONFIG_H. r=rillian

https://hg.mozilla.org/integration/mozilla-inbound/rev/e635e544621e

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e635e544621e