Bug 87664 – [Gtk] Remove configuration options for features that are not supported by the Gtk port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Zan Dobersek]

Remove configuration options for futures that are not supported by the Gtk port - there's no need for them currently. They should be readded when the support for a specific feature is added and introduces a new dependency.

[Zan Dobersek]

Created attachment 144361 [details]
Patch

[Zan Dobersek]

(In reply to comment #1)
> Created an attachment (id=144361) [details]
> Patch

The only options I've left out due to some recent activity towards adding support are --enable-gamepad, --enable-indexed-database and --enable-filesystem.

[Gustavo Noronha (kov)]

Comment on attachment 144361 [details]
Patch

\o/

[Zan Dobersek]

(In reply to comment #3)
> (From update of attachment 144361 [details])
> \o/

Thanks for the quick review. I'll land this once I'm confirmed there's not an unstable release planned in the near future (two weeks or there about).

[Gustavo Noronha (kov)]

Comment on attachment 144361 [details]
Patch

Attachment 144361 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12844244

[Zan Dobersek]

(In reply to comment #5)
> (From update of attachment 144361 [details])
> Attachment 144361 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/12844244

Eek! Looks like files added to the build for the unsupported features will have to be removed.

[Gustavo Noronha (kov)]

Or we could leave them in and have the appropriate defines without a configure option, perhaps?

[Martin Robinson]

(In reply to comment #7)
> Or we could leave them in and have the appropriate defines without a configure option, perhaps?

Yeah, that seems like a sensible approach. We just want to remove the interface to the user for these features, not the actual option. The autogeneration of the build options should still be able to turn them on and off.

[Zan Dobersek]

(In reply to comment #8)
> (In reply to comment #7)
> > Or we could leave them in and have the appropriate defines without a configure option, perhaps?
> 
> Yeah, that seems like a sensible approach. We just want to remove the interface to the user for these features, not the actual option. The autogeneration of the build options should still be able to turn them on and off.

It seems the solution to this would be to include all the files in the build, regardless of the feature that file partly implements is enabled or not. I'm not entirely sure this is even possible, but it would make sense that the build should work as expected that way, with all the source files included and the feature being actually built only if the macro is present in the cppflags.

As your comments suggest, the option for feature-enabling should stay, but in my opinion not in the shape of AM conditionals. Perhaps it would be better then to first sort out the feature defines into a GNUmakefile.features.am (bug #87127)? If that, together with all the files being included in the build, is achieved, then they (AM conditionals used for including extra files into the build) are not really needed anymore.

[Martin Robinson]

(In reply to comment #9)

> It seems the solution to this would be to include all the files in the build, regardless of the feature that file partly implements is enabled or not. I'm not entirely sure this is even possible, but it would make sense that the build should work as expected that way, with all the source files included and the feature being actually built only if the macro is present in the cppflags.
> 
> As your comments suggest, the option for feature-enabling should stay, but in my opinion not in the shape of AM conditionals. Perhaps it would be better then to first sort out the feature defines into a GNUmakefile.features.am (bug #87127)? If that, together with all the files being included in the build, is achieved, then they (AM conditionals used for including extra files into the build) are not really needed anymore.

I'm not exactly sure how it would look other than AM conditionals, but I guess I can just wait for the patch at #87127. One thing that concerns me though is if we do not remove the files from the build, this removes the benefit of disabling SVG for creating faster builds.

[Gustavo Noronha (kov)]

Handling SVG specially sounds like the practical solution.

[Martin Robinson]

(In reply to comment #11)
> Handling SVG specially sounds like the practical solution.

I'm not sure how removing the files from allows enabling and disabling features via the feature  list autogeneration.

[Zan Dobersek]

(In reply to comment #10)
> One thing that concerns me though is if we do not remove the files from the build, this removes the benefit of disabling SVG for creating faster builds.

I've done three builds with clean cache:
1. SVG disabled, files not included in the build - 27m:43s
2. SVG disabled, files included in the build - 28m:04s
3. SVG enabled - 35m:04s

Seems the preprocessor does its job formidably and I think the time difference can be lived with.

[Zan Dobersek]

(In reply to comment #1)
> Created an attachment (id=144361) [details]
> Patch

As for this patch, I recommend splitting it up:
- create new bug entries for the data-transfer-items and mhtml options, uploading a patch for each that removes the configuration option, AM conditonal and moves the files into the general build (after proper testing)
- reupload a patch for other options here

This seems somewhat contrary to what I was recommending in comment #9 (getting a GNUmakefile.features.am done), but I think these configuration options are safe to remove before that. They're unsupported and there's probably no one trying to enable them, so there's not much harm done in leaving them disabled with no option to enable for a short amount of time. But that said, I'd like some confirmation that a release is not just around the corner.

[Martin Robinson]

(In reply to comment #13)
> (In reply to comment #10)
> > One thing that concerns me though is if we do not remove the files from the build, this removes the benefit of disabling SVG for creating faster builds.
> 
> I've done three builds with clean cache:
> 1. SVG disabled, files not included in the build - 27m:43s
> 2. SVG disabled, files included in the build - 28m:04s
> 3. SVG enabled - 35m:04s

Okay. At some point I moved all the files into the main list and then others moved them back out. Now you are moving them back in again. I don't really care where they are, as long as they stop moving. :)

[Martin Robinson]

(In reply to comment #14)

> This seems somewhat contrary to what I was recommending in comment #9 (getting a GNUmakefile.features.am done), but I think these configuration options are safe to remove before that. They're unsupported and there's probably no one trying to enable them, so there's not much harm done in leaving them disabled with no option to enable for a short amount of time. But that said, I'd like some confirmation that a release is not just around the corner.

I think it's safe to do these things as long as you are careful.

[Martin Robinson]

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

[Martin Robinson]

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

[Martin Robinson]

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

[Zan Dobersek]

The GNUmakefile.features.am was created and is now working OK, but this bug kind of fell off my radar.

I'll upload an updated patch, IMO it should get another review as it's been over 3 months since the previous patch.

[Zan Dobersek]

Created attachment 162448 [details]
Patch

[Zan Dobersek]

Comment on attachment 162448 [details]
Patch

Clearing flags on attachment: 162448

Committed r127760: <http://trac.webkit.org/changeset/127760>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.