Bug 235444 – [GTK][STABLE] More missing headers and preprocessor guards for non-unified 2.34.4 build

• Component: WebKitGTK
• Status: NEW

[Dennis Nezic]

Created attachment 449665 [details]
more missing headers and buggy ENABLE preprocessing

I bumped into 20 more compilation errors after fiddling with more ENABLE_ configuration variables. This patch is against 2.34.3 ... I'll see if any more show up with 2.34.4.

[Michael Catanzaro]

Comment on attachment 449665 [details]
more missing headers and buggy ENABLE preprocessing

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

Hi, to land in WebKit you'd need (a) ChangeLog entries generated with the prepareChangeLog script, and (b) r? Bugzilla flag set.

Fixes should land in trunk first, then a second patch fixing the issues in the stable branch can be accepted second. If we fix the issues in stable first without fixing trunk, that just sets us up to have the same problems again next release.

#include fixes and #if fixes are best split into two separate patches, not all in one patch, to match the title of the bug (which is your commit message title).

Lastly, while #if fixes in cross-platform code are always welcome, in the platform-specific code we instead prefer to minimize the number of preprocessor guards that we use. Changing the value of private build options is not supported at all and that's not expected to build successfully. There's no reason to ever build GTK port with fullscreen API or resource load statistics disabled, for example, because they have no build dependencies. I guess you might save a few bytes on disk, but that's not worth the cost of maintaining the guards in our code. Building without ENABLE_USER_MESSAGE_HANDLERS will disable some pretty core functionality. These flags are private because it doesn't make sense to use them. If you really want them for some reason, please propose a patch to expose them as public, including justification for why it makes sense to build without.

> b/Tools/MiniBrowser/gtk/BrowserTab.c:345
> +    }
> +#if ENABLE(POINTER_LOCK)
> +		else if (WEBKIT_IS_POINTER_LOCK_PERMISSION_REQUEST(request)) {

Careful here too.

> b/Tools/MiniBrowser/gtk/BrowserTab.c:360
> -    } else if (WEBKIT_IS_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST(request)) {
> +    }
> +#endif
> +		else if (WEBKIT_IS_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST(request)) {

Careful with indentation

[Dennis Nezic]

Created attachment 449763 [details]
missing headers and if preprocessor guards against 2.34.4

Attached are the updated patches against stable 2.34.4. It includes the previous one that I submitted bug 235273 (I'll close it), plus one more missing header (Frame.h in Source/WebCore/inspector/agents/InspectorCanvasAgent.h).

What's the point of having those ENABLE preprocessor guards if they don't work? It won't compile if they're used -- might as well get rid of them then? And many of them sound very useful to be configurable, like ENABLE_GEOLOCATION ENABLE_ACCESSIBILITY ENABLE_VIDEO, etc.

I don't know what "EWS" means, and I assumed that having this patch reviewed was implied.

I fixed the indentation (the tab instead of the spaces), woops.

Guys, I spent many hours hunting down these bugs. And sooo many more waiting for it to compile -- *it takes well over 24 hours of non-stop 100% cpu time here to compile this!*. Can you guys do the final commit or whatever is needed?

[Dennis Nezic]

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

[Dennis Nezic]

Created attachment 449764 [details]
woops, the previous one dragged in some changes that others made in those files, between versions .3 and .4

[Michael Catanzaro]

(In reply to Dennis from comment #2)
> What's the point of having those ENABLE preprocessor guards if they don't
> work? 

They're for other WebKit ports, and in cross-platform code they do need to be maintained.

In platform-specific code, they are just clutter.
 
> I don't know what "EWS" means, and I assumed that having this patch reviewed
> was implied.

Sorry, EWS is the CI system that builds your patch (scroll up a bit). Your previous patch was red on every single bot, indicating that the patch doesn't apply to trunk. They need to be at least mostly green (it's OK if bots are failing as long as the failure looks unrelated to your work).

You still need to set the r? Bugzilla flag to request a review. And again, if you don't follow my previous instructions to include changelog entries, it will be rejected.

> Guys, I spent many hours hunting down these bugs. And sooo many more waiting
> for it to compile -- *it takes well over 24 hours of non-stop 100% cpu time
> here to compile this!*.

Wow, that's... pretty intense.

> Can you guys do the final commit or whatever is needed?

No, sorry.

[Dennis Nezic]

So webkit-gtk users shouldn't be able to disable "accessibility"? And why would I need a "fullscreen api" - maybe I'm not understanding what it means, but I never intend to use my browser in fullscreen mode. And why would I ever need "resource load statistics" - I'm pretty sure I've never used them in the past several years?

[Dennis Nezic]

Also, are you saying that those preprocessor guards work properly in other (non-unified) WebKit ports? That I'm the only one bumping into those bugs with my gtk port?

[Michael Catanzaro]

(In reply to Dennis from comment #6)
> So webkit-gtk users shouldn't be able to disable "accessibility"? And why
> would I need a "fullscreen api" - maybe I'm not understanding what it means,
> but I never intend to use my browser in fullscreen mode. And why would I
> ever need "resource load statistics" - I'm pretty sure I've never used them
> in the past several years?

Normally we only expose build options when there is a new build dependency that might not be available on older systems. You can simply not use the feature at runtime if you don't want it. WebKitGTK always has accessibility support enabled. I don't know much about fullscreen, but I imagine disabling that option would likely break the fullscreen button on every video player, for instance. Not sure why other ports would ever use it.

Then resource load statistics is intelligent tracking prevention. That flag has been renamed in trunk, which is why your patch failed EWS again. You failed to read my first comment in this issue. I don't think your patch is going to be accepted if you're unable to respond to my feedback. We don't accept patches onto the stable branch unless a corresponding patch has landed in master first. Fixing 2.34 only to see you come back with similar patches for 2.36 a few months later, because you didn't land your changes into trunk first, is a waste of everybody's time, which is why it's not allowed here. (This best practice applies to all software projects, not just WebKit.)

(In reply to Dennis from comment #7)
> Also, are you saying that those preprocessor guards work properly in other
> (non-unified) WebKit ports?

They would be expected to work in at least one other port. (Sometimes this is not always true. :)

> That I'm the only one bumping into those bugs with my gtk port?

You're trying to use options that are not meant to be used with WebKitGTK, and discovering that they do not build, as expected. How did you even find these options? We have them marked as ADVANCED to ensure CMake doesn't expose them unless you purposefully go looking for the hidden options.

The non-ADVANCED options are public and expected to work. If you change them and then wind up with build failures, that's a bug to be fixed.

I don't mind adding #includes where useful. Nobody is going to complain about that. But in the GTK-specific files, you should only use guards if the GTK port actually allows changing the value of those guards (i.e. it is not marked ADVANCED).

[Dennis Nezic]

> You can simply not use the feature at runtime if you don't want it.

That's not a good policy. It increases the attack surface and increases the stress on system resources. All of the few bloated "modern browsers" that still exist are constantly being hacked because the code is so bloated and unmaintainable - I don't want hackers to even have the possibility of accessing the geolocation api, for example - I don't want it anywhere in my code.

Again, I don't want fullscreen capability, or "intelligent tracking prevention", or countless other features. I recently randomly saw some SpeechRecognition module being compiled. Things have gotten way out of control.

> You're trying to use options that are not meant to be used with WebKitGTK

Says you. You shouldn't be telling users what kind of software they should be running, especially when you're pushing them into a more insecure and bloated direction.

Also, at least one of my fixes (the ones involving the *TransientZoom functions, is broken because I had USE_OPENGL_OR_ES=OFF ... is that not public/"allowed" either?

And I think all the other preprocessor guards that I added are just adding to what already exists ... if my ones aren't worth it, than the existing ones shouldn't be either. They literally do the same thing - someone just forgot to maintain them over time.

[Michael Catanzaro]

(In reply to Dennis Nezic from comment #9)
> Also, at least one of my fixes (the ones involving the *TransientZoom
> functions, is broken because I had USE_OPENGL_OR_ES=OFF ... is that not
> public/"allowed" either?

That one is public: if the build fails, it's a bug. The rest I've said above still applies.
 
> And I think all the other preprocessor guards that I added are just adding
> to what already exists ... if my ones aren't worth it, than the existing
> ones shouldn't be either. They literally do the same thing - someone just
> forgot to maintain them over time.

Again, these guards are needed in *cross-platform files* but not our platform-specific files.