Bug 105720 – [GTK] invalid use of incomplete type WebCore::ResourceResponse in webkitwebviewprivate.h

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

[ChangSeok Oh]

In this bug, I want to fix some nits and build failure caused by video track feature.

[ChangSeok Oh]

Created attachment 180671 [details]
Patch

[ChangSeok Oh]

Created attachment 180673 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 180673 [details]
Patch

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

[ChangSeok Oh]

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

I think this is a false alarm. Try a clean build!

[Zan Dobersek]

Comment on attachment 180673 [details]
Patch

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

The GTK port is currently in the process of cleaning up the various configuration options located in configure.ac which are not really needed. In short form, if the feature (guarded by any feature define) does not introduce external dependencies, there shouldn't be a corresponding configuration flag available. An exception to this is the SVG feature, the reason for the configuration flag being that the SVG code takes notable time to compile so there should be a way of disabling it.

> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:33
> +#include "ResourceResponse.h"

In what circumstances does build fail without this being added? What feature is enabled/disabled?
If it is of valid cause, please proceed with pushing to get this part of the patch landed.

> configure.ac:662
> +AC_ARG_ENABLE(video_track,

There's no need for this configuration option as it doesn't introduce any additional dependencies.

To prevent compilation failure, changes should be made to CodeGeneratorObject.pm. I've checked the build failures and created a patch for that, I'll upload it up for a review in another bug.

> configure.ac:686
> +AC_ARG_ENABLE(css_transforms_animations_transitions_unprefixed,

This configuration option is unnecessary as well, it was only added due to people adding new features being uninformed about the new configuration flag criteria on the GTK port (which is my fault, really).

[Martin Robinson]

Comment on attachment 180673 [details]
Patch

Yeah, we should just remove the unnecessary configuration options and fix the source list.

[ChangSeok Oh]

(In reply to comment #5)
> (From update of attachment 180673 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180673&action=review
> 
> The GTK port is currently in the process of cleaning up the various configuration options located in configure.ac which are not really needed. In short form, if the feature (guarded by any feature define) does not introduce external dependencies, there shouldn't be a corresponding configuration flag available. An exception to this is the SVG feature, the reason for the configuration flag being that the SVG code takes notable time to compile so there should be a way of disabling it.
O.K I got it. thanks for the kind comment.

> > Source/WebKit/gtk/webkit/webkitwebviewprivate.h:33
> > +#include "ResourceResponse.h"
> 
> In what circumstances does build fail without this being added? What feature is enabled/disabled?
> If it is of valid cause, please proceed with pushing to get this part of the patch landed.
I found adding ResourceResponse.h is required only with the option '--with-acceleration-backend=clutter'. and the main reason of missing header is due to not supporting css_filter with clutter AC. In other words, ResourceResponse.h is not reached when enabling AC clutter since css_filter is turned off.
I think it'll be not bad that adding ResourceResponse.h explicitly here. Any idea?

> 
> > configure.ac:662
> > +AC_ARG_ENABLE(video_track,
> 
> There's no need for this configuration option as it doesn't introduce any additional dependencies.
> 
> To prevent compilation failure, changes should be made to CodeGeneratorObject.pm. I've checked the build failures and created a patch for that, I'll upload it up for a review in another bug.
O.K.. another easy option is to enable video_track feature with video at the same time.

> > configure.ac:686
> > +AC_ARG_ENABLE(css_transforms_animations_transitions_unprefixed,
> 
> This configuration option is unnecessary as well, it was only added due to people adding new features being uninformed about the new configuration flag criteria on the GTK port (which is my fault, really).
Then are you going to remove these unnecessary option on your side?  If that's the case, I think it's better to keep perfect for the meantime.

[Martin Robinson]

Comment on attachment 180673 [details]
Patch

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

>>> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:33
>>> +#include "ResourceResponse.h"
>> 
>> In what circumstances does build fail without this being added? What feature is enabled/disabled?
>> If it is of valid cause, please proceed with pushing to get this part of the patch landed.
> 
> I found adding ResourceResponse.h is required only with the option '--with-acceleration-backend=clutter'. and the main reason of missing header is due to not supporting css_filter with clutter AC. In other words, ResourceResponse.h is not reached when enabling AC clutter since css_filter is turned off.
> I think it'll be not bad that adding ResourceResponse.h explicitly here. Any idea?

Can you paste the header dependency chain?

[ChangSeok Oh]

(In reply to comment #8)
> (From update of attachment 180673 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180673&action=review
> 
> >>> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:33
> >>> +#include "ResourceResponse.h"
> >> 
> >> In what circumstances does build fail without this being added? What feature is enabled/disabled?
> >> If it is of valid cause, please proceed with pushing to get this part of the patch landed.
> > 
> > I found adding ResourceResponse.h is required only with the option '--with-acceleration-backend=clutter'. and the main reason of missing header is due to not supporting css_filter with clutter AC. In other words, ResourceResponse.h is not reached when enabling AC clutter since css_filter is turned off.
> > I think it'll be not bad that adding ResourceResponse.h explicitly here. Any idea?
> 
> Can you paste the header dependency chain?

Sure. here it is.
./Source/WebCore/loader/ResourceResponse.h,
./Source/WebCore/loader/cache/CachedResource.h:33,
./Source/WebCore/loader/cache/CachedResourceHandle.h:29,
./Source/WebCore/loader/cache/CachedSVGDocumentReference.h:30,
./Source/WebCore/platform/graphics/filters/FilterOperation.h:45,
./Source/WebCore/platform/graphics/filters/FilterOperations.h:31,
./Source/WebCore/platform/graphics/GraphicsLayer.h:34,   >> CSS_FILTERS define is here.
./Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:22,
./Source/WebKit/gtk/webkit/webkitwebviewprivate.h:26

[Martin Robinson]

Comment on attachment 180673 [details]
Patch

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

>>>>> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:33
>>>>> +#include "ResourceResponse.h"
>>>> 
>>>> In what circumstances does build fail without this being added? What feature is enabled/disabled?
>>>> If it is of valid cause, please proceed with pushing to get this part of the patch landed.
>>> 
>>> I found adding ResourceResponse.h is required only with the option '--with-acceleration-backend=clutter'. and the main reason of missing header is due to not supporting css_filter with clutter AC. In other words, ResourceResponse.h is not reached when enabling AC clutter since css_filter is turned off.
>>> I think it'll be not bad that adding ResourceResponse.h explicitly here. Any idea?
>> 
>> Can you paste the header dependency chain?
> 
> Sure. here it is.
> ./Source/WebCore/loader/ResourceResponse.h,
> ./Source/WebCore/loader/cache/CachedResource.h:33,
> ./Source/WebCore/loader/cache/CachedResourceHandle.h:29,
> ./Source/WebCore/loader/cache/CachedSVGDocumentReference.h:30,
> ./Source/WebCore/platform/graphics/filters/FilterOperation.h:45,
> ./Source/WebCore/platform/graphics/filters/FilterOperations.h:31,
> ./Source/WebCore/platform/graphics/GraphicsLayer.h:34,   >> CSS_FILTERS define is here.
> ./Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:22,
> ./Source/WebKit/gtk/webkit/webkitwebviewprivate.h:26

Thanks, but whoops! I should have also asked what fails when it isn't included.

[ChangSeok Oh]

(In reply to comment #10)
> (From update of attachment 180673 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180673&action=review
> 
> >>>>> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:33
> >>>>> +#include "ResourceResponse.h"
> >>>> 
> >>>> In what circumstances does build fail without this being added? What feature is enabled/disabled?
> >>>> If it is of valid cause, please proceed with pushing to get this part of the patch landed.
> >>> 
> >>> I found adding ResourceResponse.h is required only with the option '--with-acceleration-backend=clutter'. and the main reason of missing header is due to not supporting css_filter with clutter AC. In other words, ResourceResponse.h is not reached when enabling AC clutter since css_filter is turned off.
> >>> I think it'll be not bad that adding ResourceResponse.h explicitly here. Any idea?
> >> 
> >> Can you paste the header dependency chain?
> > 
> > Sure. here it is.
> > ./Source/WebCore/loader/ResourceResponse.h,
> > ./Source/WebCore/loader/cache/CachedResource.h:33,
> > ./Source/WebCore/loader/cache/CachedResourceHandle.h:29,
> > ./Source/WebCore/loader/cache/CachedSVGDocumentReference.h:30,
> > ./Source/WebCore/platform/graphics/filters/FilterOperation.h:45,
> > ./Source/WebCore/platform/graphics/filters/FilterOperations.h:31,
> > ./Source/WebCore/platform/graphics/GraphicsLayer.h:34,   >> CSS_FILTERS define is here.
> > ./Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:22,
> > ./Source/WebKit/gtk/webkit/webkitwebviewprivate.h:26
> 
> Thanks, but whoops! I should have also asked what fails when it isn't included.

Don't worry. I've kept the log.

In file included from ../../Source/WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp:33:0:
../../Source/WebKit/gtk/webkit/webkitwebviewprivate.h:132:139: error: invalid use of incomplete type ‘struct WebCore::ResourceResponse’
../../Source/WebCore/platform/network/ResourceHandle.h:87:7: error: forward declaration of ‘struct WebCore::ResourceResponse’
make[1]: *** [Source/WebKit/gtk/WebCoreSupport/libwebkitgtk_3_0_la-ContextMenuClientGtk.lo] Error 1
make[1]: *** Waiting for unfinished jobs....

[ChangSeok Oh]

Created attachment 180755 [details]
Patch

[Zan Dobersek]

(In reply to comment #7)
> (In reply to comment #5)
> > 
> > > configure.ac:662
> > > +AC_ARG_ENABLE(video_track,
> > 
> > There's no need for this configuration option as it doesn't introduce any additional dependencies.
> > 
> > To prevent compilation failure, changes should be made to CodeGeneratorObject.pm. I've checked the build failures and created a patch for that, I'll upload it up for a review in another bug.
> O.K.. another easy option is to enable video_track feature with video at the same time.
> 

That should also be done, actually I believe this is what the Mac port currently does. I've posted a patch that addresses the CodeGeneratorObject.pm changes in bug #105743.

> > > configure.ac:686
> > > +AC_ARG_ENABLE(css_transforms_animations_transitions_unprefixed,
> > 
> > This configuration option is unnecessary as well, it was only added due to people adding new features being uninformed about the new configuration flag criteria on the GTK port (which is my fault, really).
> Then are you going to remove these unnecessary option on your side?  If that's the case, I think it's better to keep perfect for the meantime.

Ah, of course, there's already a patch waiting for a review in bug #105522.

[ChangSeok Oh]

Created attachment 180796 [details]
Patch

[ChangSeok Oh]

Thanks for the info. :)
I took out css_transforms_animations_transitions_unprefixed stuff from the patch.

(In reply to comment #13)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > 
> > > > configure.ac:662
> > > > +AC_ARG_ENABLE(video_track,
> > > 
> > > There's no need for this configuration option as it doesn't introduce any additional dependencies.
> > > 
> > > To prevent compilation failure, changes should be made to CodeGeneratorObject.pm. I've checked the build failures and created a patch for that, I'll upload it up for a review in another bug.
> > O.K.. another easy option is to enable video_track feature with video at the same time.
> > 
> 
> That should also be done, actually I believe this is what the Mac port currently does. I've posted a patch that addresses the CodeGeneratorObject.pm changes in bug #105743.
> 
> > > > configure.ac:686
> > > > +AC_ARG_ENABLE(css_transforms_animations_transitions_unprefixed,
> > > 
> > > This configuration option is unnecessary as well, it was only added due to people adding new features being uninformed about the new configuration flag criteria on the GTK port (which is my fault, really).
> > Then are you going to remove these unnecessary option on your side?  If that's the case, I think it's better to keep perfect for the meantime.
> 
> Ah, of course, there's already a patch waiting for a review in bug #105522.

[Martin Robinson]

Comment on attachment 180796 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 180796 [details]
Patch

Clearing flags on attachment: 180796

Committed r138505: <http://trac.webkit.org/changeset/138505>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.