Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
On 12/01/2023 06.21, Ryosuke Niwa via webkit-dev wrote: I suggest we *stop using raw pointers and references in any local or heap stored variable*. I don't think this is feasible for code using non-WebKit libraries, and would expect it to not apply to such cases. For instance, many GLib and GStreamer objects are refcounted, and we even have smart pointers for them in WebKit: GRefPtr, which refs/unref using RAII, and GUniquePtr, which frees GLib non-refcounted objects. We use both GRefPtr/GUniquePtr and raw pointers, but the meaning of each is different: GRefPtr and GUniquePtr mean the code owns a reference, and will unref/free when the variable goes out of scope. Raw pointer means the code is borrowing a reference. The code will do something with the object, then leave it, without establishing ownership of a reference to it. This is especially the case for const raw pointers, which you are meant to use within scope and are not allowed to free() them, as you are just borrowing them. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] [MSE] Range ends inclusion is deleting wanted MediaSample's
On 14/11/17 22:55, Jer Noble wrote: > > Yeah, the spec is not the best here. I agree. While WebKit replicates the specified steps faithfully, it's not clear they are good steps in the first place. I spent quite a bit of time trying to wrap my head around the three (!) frame removal cases, all of them apparently doing the same but with slightly different ranges and being used in slightly different situations... Even after all the complexity I found too some cases that are unhandled by the spec. > If you have, e.g. a sample with a > PTS of 0 and a duration of 100, and then insert a sample with a PTS of > 50 and a duration of 100, you’d expect that to cause the first sample to > be removed. But a strict reading of the spec says that sample stays. Or maybe cropped (that would be of questionable utility in practice though)... But yup, the spec says that (supposing they are consecutive) no check at all is performed because PTS < highestEndTimestamp. > Now you have two overlapping samples. It can get even weirder if you > insert a sample with a PTS of 25 and a duration of 50. Now, strictly > implementing the spec, you have a sample overlaps on both ends of > another sample. What does that even mean for a decoder? It’s almost > guaranteed to generate a decode error, unless both of the overlapping > samples are I-frames. It would be implementation specific. In the case of GStreamer, for instance, durations are not needed for decoders to work, only DTS. Same can be said for the presentation, that only needs to know the total length of the movie (or at the very least know whether it should continue playing) and the PTS of each frame. So the first frame would be shown for 25 ms, then the second one for 75 ms. No corruption should occur as the frames have been decoded in the correct order. More serious problems can arise from manipulating the array of frames as many assumptions would be broken: frames would be played for less than their declared durations, the relation "time to frame" would no longer have 0..1 multiplicity, the sum of durations would be longer than the last frame end even though they are thought to be consecutive and the first frame has PTS=0 and so on... Definitively undesirable consequences independently of the platform. > > I think the intent of the spec is clear: if any part of a previous > sample overlaps the new one, it has to be removed, and all samples that > depend on the removed samples must be removed as well. And in order to > do that, you have to take the entire duration of the sample into account. > I wish that part of the MSE spec was rewritten in a simpler yet more comprehensive way... because as of today, that's not what the spec says. Some complexity may need to appear nevertheless because of the float conversions caused by JS interaction. -- Alicia. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] [MSE] Range ends inclusion is deleting wanted MediaSample's
Hi, Jer. Sorry for the late reply, I've been busy investigating all of this. I'm replying to your points in reverse order... On 09/11/17 23:23, Jer Noble wrote: >> As I understand it, the point of that part of the algorithm is to delete >> old samples that are -- even partially -- in the presentation time range >> of the newly appended one, but using (beginTime, endTime] fails to >> accomplish that in two cases: >> >> a) If there is an existing MediaSample with PTS=9 and DUR=1 it will not >> be removed because beginTime (=9) is exclusive. > > Eh, not really. The search space is the entire duration of the sample, > so an exclusive beginTime of 9 _should_ match PTS=9, DUR=1 because 9 < > presentationEndTime=10. I’m pretty sure this is correct, but a mock > test case should clear this up pretty quickly. That's totally not the case, though the first time I gave a look at this code I thought that too... presentationRange() is a std::map whose key type is a single MediaTime specifying the PTS of the MediaSample, not the duration or the full presentation time range. Also, neither findSamplesWithinPresentationRange() or its siblings take durations into account, as they search only with the key (PTS) in mind... Well, actually findSamplesWithinPresentationRangeFromEnd() looks at the values, but it only checks .presentationTimestamp() so it's functionally the same. Adding a sample with same PTS as an existing one does not remove the old one with this check. There are two possible behaviors that can mask the issue: a) The previous frame removal step (see step 1.13 in the spec or 1.14 in WebKit) removed the overlapping frame. This is only the case if the frame started a new Coded Frame Group. b) The existing overlapping frame was not removed, but later on when the new MediaSample was passed to std::map::insert() it was ignored because it was a duplicated key (same PTS as the existing overlapping frame). You can confirm that it's the old MediaSample the one persisted by setting different durations for the old and the new one but the same PTS. After the append of the second frame, the old duration persists and there are no new frames in the TrackBuffer. On 09/11/17 23:23, Jer Noble wrote: >> My question is... shouldn't the range ends inclusivity be the other way >> around i.e. [beginTime, endTime)? > > beginTime was intended to be exclusive so that, given a [0, 9] range, > searching for (9, 10] wouldn’t match the last sample in the [0, 9] range. > > But now that you mention it, perhaps the real problem is that both beginTime > _and_ endTime should be exclusive. This is not an issue because as explained before we are considering just the frame start PTS, not its entire presentation interval. beginTime must be inclusive so that old frames with the same PTS are deleted. endTime must be exclusive so that non overlapping consecutive frames are not deleted. I have checked this with the spec: https://www.w3.org/TR/media-source/#sourcebuffer-coded-frame-processing 1.14. Remove existing coded frames in track buffer: -> If highest end timestamp for track buffer is not set: [...] -> If highest end timestamp for track buffer is set and less than or equal to presentation timestamp: Remove all coded frames from track buffer that have a presentation timestamp greater than or equal to highest end timestamp and less than frame end timestamp. On 09/11/17 23:23, Jer Noble wrote: > It should be possible to make a “mock” testcase which doesn’t rely on > any particular media engine and which can demonstrate this bug. I just finished doing that. I have written two new MediaSource tests using video/mock: one that demonstrates how existing frames whose PTS coincide with the start of the range of the range are not deleted and another one that demonstrates how existing frames whose PTS coincide with the end of the range are deleted. I have also written a patch with the correct checks: https://bugs.webkit.org/show_bug.cgi?id=179690 -- Alicia. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] [MSE] Range ends inclusion is deleting wanted MediaSample's
Hi, WebKittens!
In the YouTube Media Source Extensions conformance tests there is one
called 36.AppendOpusAudioOutOfOrder where two audio media segments are
appended out of order to a SourceBuffer: First, a segment with the PTS
ranges [10, 20) is added. Then, another one with [0, 10) is added.
(I have rounded the actual timestamps to near integers for easier
understanding).
Almost at the very end of the process the buffered ranges are like this:
[ 0, 9)
[10, 20)
At this point, SourceBuffer::sourceBufferPrivateDidReceiveSample() is
called with the last audio frame, that has PTS=9 and DUR=1.
The execution reaches this conditional block:
```
if (trackBuffer.highestPresentationTimestamp.isValid() &&
trackBuffer.highestPresentationTimestamp <= presentationTimestamp) {
```
trackBuffer.highestPresentationTimestamp contains the highest PTS so far
within the current segment. The condition is true (9 <= 9) as expected
for sequentially appended frames.
Inside there is this block of code:
```
MediaTime highestBufferedTime = trackBuffer.buffered.maximumBufferedTime();
PresentationOrderSampleMap::iterator_range range;
if (highestBufferedTime - trackBuffer.highestPresentationTimestamp <
trackBuffer.lastFrameDuration)
range =
trackBuffer.samples.presentationOrder().findSamplesWithinPresentationRangeFromEnd(trackBuffer.highestPresentationTimestamp,
frameEndTimestamp);
else
range =
trackBuffer.samples.presentationOrder().findSamplesWithinPresentationRange(trackBuffer.highestPresentationTimestamp,
frameEndTimestamp);
if (range.first != trackBuffer.samples.presentationOrder().end())
erasedSamples.addRange(range.first, range.second);
```
The first if block there is an optimization, it decides whether to do a
binary search in the entire collection of MediaSample's or do a linear
search starting with the MediaSample with the highest PTS (which is
faster when appends always occur at the end), but the result is the same
in both cases.
presentationOrder() is a std::map.
findSamplesWithinPresentationRange(beginTime, endTime) and its *FromEnd
counterpart both return a pair of STL-style iterators which cover a
range of MediaSample objects whose presentation timestamps sit in the
range (beginTime, endTime] (beginTime is exclusive, endTime is inclusive).
Then, it marks those MediaSample objects (frames) for deletion.
My question is... shouldn't the range ends inclusivity be the other way
around i.e. [beginTime, endTime)?
As I understand it, the point of that part of the algorithm is to delete
old samples that are -- even partially -- in the presentation time range
of the newly appended one, but using (beginTime, endTime] fails to
accomplish that in two cases:
a) If there is an existing MediaSample with PTS=9 and DUR=1 it will not
be removed because beginTime (=9) is exclusive.
b) If there is an existing MediaSample with PTS=10 and DUR=1 it WILL be
removed even though there is no overlap with the sample being appended
(PTS=9 DUR=1) because endTime (=10) is inclusive. This is exactly what
is making the YTTV test fail in my case.
Before:
[ 0, 9)
[10, 20)
Expected result after adding [9, 10):
[0, 20)
Actual result in WebKit:
[ 0, 10)
[11, 20)
-- Alicia.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Growing tired of long build times? Check out this awesome new way to speed up your build... soon (HINT: It's not buying a new computer)
On 08/30/2017 12:00 AM, Keith Miller wrote: > The automated system should handle your concern about bundling similar .cpp > files together. The proposed system bundles by directory and also sorts the > files in those directories when dividing up the files. Is there any reason to > think that we would not do roughly the same thing if we were bundling by hand? As long as the sets are generated consistently that approach should be equally fine and indeed it would reduce the burden for contributors. You may need at least a file to write exceptions (files not to be included in any bundle) though. -- Alicia. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Growing tired of long build times? Check out this awesome new way to speed up your build... soon (HINT: It's not buying a new computer)
I'm all for stable bundle .cpp files. Automatic generation can be nice for the first time... and once in while we can try to modify the bundles to try to make the build even faster, but really most of the time bundle contents should not change unless new files are added and everybody should get the same bundles. If we're going to end up with some name collisions, we'd rather do so that every build of the same source tree gets collisions or no build gets them. The description of these .cpp bundles should become a project artifact, shared and versioned in the repository and treated carefully as such. Also, in my earlier message about unified builds I explained how the distribution of the files within the bundles also affects the number of bundles that are rebuilt in response to changes in the source code. I also proposed that grouping together files implementing the same features could be a good start point to minimize the impact of this issue. On other note, I don't think namespacing static functions and variables as originally proposed is the way to go. The refactor would be ridiculously huge. Running `rg -w '^static' -tcpp --no-filename --no-heading |wc -l` in my local repository yields 16797 instances of static functions and variables, only taking into account those where the word static is at the beginning of the line without any spaces before, which is a common case but not a requirement in C++. Note though that this regex excludes static class members as long as code is properly formatted (they would be indented); these are not included as they would not need any changes. static functions and variables in WebKit are used for hidden implementation details, which tend to very very specific and therefore receive long specific names with relatively little risk of collisions. Furthermore, collisions won't go unnoticed as in every case they will trigger a compilation error of the likes of 'redefinition of ‘int a’'. In this case the compiler will also show the inclusion stack mentioning the bundle file and the conflicting definition. The programmer will notice that both are static members in .cpp files, so the only way that error can arise is because they had a name collision due to unified source builds. We'd probably be better off handling the few collisions on a one by one basis, renaming the symbols or excluding the worst offenders from unified builds rather than plain out forbidding static declarations. -- Alicia ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Growing tired of long build times? Check out this awesome new way to speed up your build... soon (HINT: It's not buying a new computer)
On 08/29/2017 06:20 AM, Daniel Bates wrote: > Do we know what is the cause(es) for the slow clean builds? I am assuming > that much of the speed up from the "unified source" comes from clang being > able to use an in-memory #include cache to avoid disk I/O and re-parsing of a > seen header. Have we exhausted all efforts (or have we given up?) removing > extraneous #includes? Do we think this pursuing this effort would be more > time consuming or have results that pale in comparison to the "unified > source" approach? Whilst having an in-process-memory #include cache is not a bad thing, it's not the greatest gain as the operating systems should already cache file reads just fine. The greatest gain comes from reducing the amount of times C++ headers are parsed. If you are building a certain .cpp file and include a .h file, the compiler has to parse it, which can take quite a bit because C++ is really complex monster, especially when templates are used. Doing this more than necessary raises build times really quickly. Header files almost always are include-guarded (either with #pragma once or traditional #ifndef guards), so including the same header twice within the same .cpp file (or any of its included files) has no cost. On the other hand, if you then start building a different .cpp file that also includes the same header, you have to parse it again because so far C++ is concerned, every inclusion could add different symbols to the AST the compiler is building, so their output can't be reused*. In turn we end up parsing most headers many more times than actually needed (i.e. for every .cpp file that includes Node.h the compiler has to parse Node.h and all its dependencies from source, that's a lot of wasted effort!). *Note that including twice the same .h within the same .cpp file is not fast because the output is cached in anyway, but because the entire .h file is skipped, adding no additional nodes to the AST. The goal of C++ modules is to fix this problem from its root cause: instead of literally including text header files, .cpp files declare dependencies on module files that can be compiled, stored, loaded and referenced from .cpp files in any order, so you would only parse the Node module source code once for the entire project, whilst the compiler could load directly the AST every other time from a cached module object file. Note the great advantage of modules comes from the fact that they can be imported in different contexts and their content is still semantically equivalent, whereas with plain C/C++ includes every header file may act differently depending on the preprocessor variables defined by the includer and other previous inclusions. In the worst case, when they are not include-guarded (luckily this is not too common, but it still happens), even including the same file in the same .cpp twice could add different symbols each time! Unfortunately C++ modules are a work in progress... There are two different competing proposals with implementations, one from Clang and another one from Microsoft, and the C++ technical specification is in a very early stage too: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4681.pdf We know for sure modules are very important for the future of C++, but maybe it's still a bit too early to bet a big project like WebKit on them. So how else can we avoid parsing the same header files so many times and speed up our builds? Enter unified builds. A requirement for unified builds to work correctly is that header files are coded in such a way they work as independent units, much like C++ modules, i.e. including headers should work no matter in what order you place them, and in each case they must define the same symbols. On July 31 I wrote about some issues we currently have because of not doing exactly this in WebKit (particularly, our #include "config.h" lines are ambiguous). They can be worked around so they will not become blockers for unified builds, but I still think we should fix them at some point. Once you have a set of .cpp files whose includes all (1) are guarded (e.g. by #pragma once) and (2) are independent units according to the above rule, you can take advantage of unified builds: Instead of invoking once the compiler for each .cpp file, you create a new artificial "unified" or "bundle" .cpp file that concatenates (or #include's) a number of different .cpp files. This way, headers included within the bundle are parsed only once, even if they are used by different individual .cpp files, as long as they are within the same bundle. This can often result in a massive build speed gain. Unfortunately, there are some pitfalls, as there is a dissonance between what the programmer thinks the compilation unit is (the individual .cpp file) and the actual compilation unit used by the compiler (the bundle .cpp file). * `static` variables and functions are intended to be scoped to the individual .cpp file, but the compiler has no way to kno
Re: [webkit-dev] Python
On 08/12/2017 11:40 PM, Ryosuke Niwa wrote: > On Fri, Aug 11, 2017 at 8:06 AM, Michael Catanzaro > wrote: >> On Mon, Aug 7, 2017 at 10:56 AM, Carlos Alberto Lopez Perez >> wrote: >>> >>> That's a good thing. >>> >>> I believe all Linux distros we support have this, right? >>> And all the scripts actually assume python2.7 (AFAIK). >>> >>> Would it work for everyone having as shebang : >>> >>> #!/usr/bin/env python2.7 >>> >>> ? >> >> >> So this is by far the easiest solution of those that have been proposed. I >> think we should try this as a stopgap until macOS is fixed to provide a >> python2 binary. > > What's wrong with using that as the permanent solution until we make > it Python3 compatible? > > I don't think it's wise to assume /usr/bin/python2 will be added on > Mac anytime soon. > > - R. Niwa > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev > +1, python2.7 is well supported among different systems. It would be desirable to move to Python 3 eventually, but that's an endeavor that should not be taken lightly, as many things can break in subtle ways. -- Alicia ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] What's the rationale for not including config.h in any header files?
On 08/01/2017 08:25 AM, Darin Adler wrote: >> On Jul 31, 2017, at 2:04 PM, Michael Catanzaro wrote: >> >> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler wrote: >>> I don’t think we should add lots of includes of “config.h”, though. I think >>> we can come up with something better. >> >> Like what? > > We should consider reducing the size of “config.h” and only use it for things > that it’s really needed for, using more conventional header includes for > other purposes. > > Ideally we would eliminate “config.h” entirely and replace it with normal > headers. > > I definitely don’t think we need to use the same name, “config.h”, for 9 > separate headers that don’t have the same contents in 9 different directories > in the overall WebKit source code. A risk of putting includes of “config.h” > in header files is that it isn’t clear which of the “config.h” files will be > included. If we aren’t obliged to use the name “config.h” because of > autotools, then perhaps we can name these more in line with WebKit coding > style, something like WebCoreConfiguration.h, but also strive to get rid of > them entirely. > > In projects not using autotools and using IDEs to build I often see a header > referred to as the “prefix” that is automatically included by the IDE, with > no #include statements at all in the source files. That’s used as a sort of > substitute for #define statements on the command line and is what I used > before I became familiar with autotools. > > I am concerned to see there is also a header named “cmakeconfig.h” so maybe > we do need “config.h”. > > — Darin > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev > I'm not sure if it's realistic to get rid of configuration headers. WebKit development builds use just too many build-time flags to enable features. On the other hand, I agree completely in the name issue, which is also an scoping issue. The situation I found is something like this: "cmakeconfig.h" is a file generated by CMake with all the flags we enable at configure time. One reason to do that is precisely because there are too many to put comfortably in the command line (I count 177 #define's there). There is a single cmakeconfig.h file for an entire WebKit build. This makes sense: If a feature is implemented in both WebCore and WebKit you want it to be enabled either both or in none. On the other hand, there is currently a config.h file in every of our sub-project directories: WTF, WebCore, JavaScriptCore, WebKit and WebDriver. Every config.h file includes cmakeconfig.h, plus some other stuff. Unfortunately, the kind of "stuff" that is additionally included or defined varies a lot between sub-projects. Having several files called config.h is confusing at the very least, even more when they are included from different sub-projects. The same #include "config.h" line refers to different files in different contexts. This is a quite nasty problem, as the author of a header file in a sub-project may assume it's safe to rely on something being defined because it's included in that sub-project's config.h file... and most of the time it will work, but it may not be the case when that file is included (even transitively) by any other file from other sub-project that uses a different config.h file. As a consequence, quirks often have to be added to config.h files of projects with dependencies. For instance, JavaScriptCore/config.h includes "runtime/JSExportMacros.h" (and therefore many headers in JSC assume it is included) and the same happens in WebCore, whose config.h includes "WebCore/PlatformExportMacros.h". The WebKit sub-project includes headers from both, so its config.h has to include both "runtime/JSExportMacros.h" and "WebCore/PlatformExport.h" too in order to build. Complicating things a bit more there is ThirdParty/gtest/include/gtest/internal/gtest-port.h. In the gtest compilation context there is no config.h in path, but it uses WTF and JSC files nevertheless, and some of these actually check for configuration flags: #include #include #include #include #include There are a few things all config.h files agree: * Include cmakeconfig.h if a CMake build is being used. * Include wtf/FastMalloc.h. * Include wtf/DisallowCType.h. Other than that, the differences are mostly sub-project specific quirks to get around compilation issues of the sub-project. The case of the WebKit sub-project is a bit more worrying as it has many defaults for flags there, but they would not be read from the context of other sub-projects. We should consider why we have several config.h files and if that makes sense or there are better alternatives. In principle, since config.h is about configuration flags, and there is a single global set of flags for the entire project, it would make much more sense to have a single true config.h instead of sub-project-specific ones.
Re: [webkit-dev] What's the rationale for not including config.h in any header files?
On 07/31/2017 11:16 PM, Maciej Stachowiak wrote: > > >> On Jul 31, 2017, at 5:04 PM, Michael Catanzaro wrote: >> >> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler wrote: >>> I don’t think we should add lots of includes of “config.h”, though. I think >>> we can come up with something better. >> >> Like what? The only alternative is to pass defines as preprocessor flags via >> -D. Our command lines are already hard enough to read as it is. Or, well, we >> could include config.h in Platform.h, where the ENABLE/USE series of macros >> are defined. >> >> I'm curious as to why including config.h in the header file would break >> Autotools builds. I don't see why that would be the case. As far as I know, >> it just happens to be common practice to include config.h only in source >> files. It's special: you always include it first in source files to avoid >> extremely confusing bugs that can result from not doing so, and therefore it >> seems redundant to put it in header files. >> >> I wonder how other projects (and other IDEs, e.g. XCode) handle this >> situation. Anyway, I don't have a strong opinion either way > > I think "config.h" as the first body include, and never in headers, is mostly > just a stylistic choice in projects that use autotools. > > However, here are some specific issues to consider: > (1) Public API headers (or installed private interfaces) should not include > config.h, and should not rely on any macros defined there. > (2) It's probably ok to include it in non-API headers, at least any that use > macros defined by config.h. > (3) Implementation files may need to still include config.h. They might not > include any headers that include config.h, or they might currently but don't > want to rely on it. config.h may also define macros that affect the behavior > of system headers and therefore should be included before any of them, so it > needs to go first. > (4) Given combo of (2) and (3), some includes of config.h in headers may be > redundant. I am not sure if this causes any practical problems though. > > - Maciej > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev > Regarding (1) and (2), the patch I'm proposing only modifies files that use either USE() or ENABLED() macros. Since public API headers should not depend on any compilation flags, they should not use either of these and therefore they will not be modified by the patch. I agree with (3). All WebKit .cpp files should still include "config.h". Regarding (4), there should not be any problems as long as the config.h files are guarded (either by #ifndef or #pragma once). Any effect on build times should be minimal as the compilers resolve these guards very quickly (and so far this has been true in my limited tests). — Alicia. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] What's the rationale for not including config.h in any header files?
Hi all.
The other day I noticed this in the Code Style Guidelines
(https://webkit.org/code-style-guidelines/#include-statements):
> Header files should never include config.h.
Is there a rationale for it?
config.h is a header file that pulls the compilation flags set in CMake,
some macro functions to query them plus some defaults for specific
platforms (e.g. if platform is Cocoa and WebRTC is enabled,
USE_LIBWEBRTC is defined automatically in WTF/wtf/Platform.h).
As of now, there are lots of instances of header files where macros such
as USE() or ENABLED() are used, yet config.h is not included (even
transitively) by that header file, thus breaking the "include what you
use" rule.
This is not a big deal for the compiler, as every .cpp file in the
project includes config.h before any other header, so by the time those
usages are really compiled those macros will be already defined.
On the other hand, static analysis tools, such as those in IDEs may not
be able to notice this. Unlike compilers, editors and human programmers
often treat files as independent units. For instance, in an editor you
open "LibWebRTCAudioCaptureSource.h", not "LibWebRTCAudioCaptureSource.h
as included by LibWebRTCAudioCaptureSource.cpp in line 28".
Ultimately this can be blamed to C++ fault because includes are a really
sloppy way to require dependencies, but I think we can agree that we
want headers to work as if they were independent units most of the time.
I've seen this issue manifest in QtCreator, as the IDE grays out all the
code inside #if USE() blocks as these macros are not defined or the
expressions inside evaluate to false in the context of that header file.
As an experiment to improve this situation, I first added #pragma once
in config.h files so that they can be included by any header file. Then,
I wrote a Python script that finds all usages of USE() and ENABLED()
macros in header files and adds a #include "config.h" where required.
Objective C headers are ignored by the script using some heuristics.
I built webkitgtk successfully both with and without the patch in gcc
and did not found any noticeable increase in compilation time: the
#pragma once guard is very fast, as expected, so that cannot be ruled as
a reason against. The result is the same but with better static analysis.
As the guidelines are right now I can't get this patch upstream because
they forbid from including config.h... but at least I would like to know
if there a compelling reason for them doing so.
(I attached the patch for the script and a few small edits needed so
that the includes work in all compilation contexts. More edits may be
needed in order for it to build in other platforms, but I could not test
that.)
Regards,
Alicia.
diff --git a/Source/JavaScriptCore/config.h b/Source/JavaScriptCore/config.h
index 1bd9d09741b..2203f266085 100644
--- a/Source/JavaScriptCore/config.h
+++ b/Source/JavaScriptCore/config.h
@@ -19,11 +19,13 @@
*
*/
+#pragma once
+
#if defined(HAVE_CONFIG_H) && HAVE_CONFIG_H && defined(BUILDING_WITH_CMAKE)
#include "cmakeconfig.h"
#endif
-#include "JSExportMacros.h"
+#include "runtime/JSExportMacros.h"
#ifdef __cplusplus
#undef new
diff --git a/Source/WTF/config.h b/Source/WTF/config.h
index ddc4f102190..58eeb534544 100644
--- a/Source/WTF/config.h
+++ b/Source/WTF/config.h
@@ -19,6 +19,8 @@
*
*/
+#pragma once
+
#if defined(HAVE_CONFIG_H) && HAVE_CONFIG_H && defined(BUILDING_WITH_CMAKE)
#include "cmakeconfig.h"
#endif
diff --git a/Source/WTF/wtf/glib/GTypedefs.h b/Source/WTF/wtf/glib/GTypedefs.h
index 51053c03d86..3f0610056b0 100644
--- a/Source/WTF/wtf/glib/GTypedefs.h
+++ b/Source/WTF/wtf/glib/GTypedefs.h
@@ -20,6 +20,11 @@
#ifndef WTF_GTypedefs_h
#define WTF_GTypedefs_h
+/* config.h cannot be included here from all contexts.
+ * For instance, this file is included transitively from ThirdParty/gtest/src/gtest.cc,
+ * which is compiled without any "config.h" in path. */
+#include
+
/* Vanilla C code does not seem to be able to handle forward-declaration typedefs. */
#ifdef __cplusplus
diff --git a/Tools/Scripts/include-config-where-needed.py b/Tools/Scripts/include-config-where-needed.py
new file mode 100755
index 000..fa7ed651df0
--- /dev/null
+++ b/Tools/Scripts/include-config-where-needed.py
@@ -0,0 +1,140 @@
+#!/usr/bin/env python
+import os
+import re
+import sys
+
+re_is_guard = re.compile(r'#if.*[^\w](ENABLE|USE)\s*\(')
+re_is_include = re.compile(r'^\s*#\s*include\s*[<"]config.h[>"]\s*')
+re_is_pragma_once = re.compile(r'^\s*#\s*pragma\s+once\s*')
+re_is_ifndef = re.compile(r'^s*#\s*ifndef\s.*')
+re_is_define = re.compile(r'^s*#\s*define\s.*')
+re_is_objc_import = re.compile('^s*#\s*import.*')
+re_is_objc_keyword = re.compile('^s*@\s*(class|interface|property|end)\s.*')
+
+
+def find_pragma_once_line(lines):
+return next((
+n_line
+for n_line, line in enumerate(lines)
+if re_is_pragma_once.match(line)
+), None)
+
+
+def find_tradi
