Bug 141813 - [WK2][GTK] Fix the build after r180362
Summary: [WK2][GTK] Fix the build after r180362
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Blocker
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 141808 141836
  Show dependency treegraph
 
Reported: 2015-02-19 14:06 PST by Csaba Osztrogonác
Modified: 2015-02-23 23:32 PST (History)
9 users (show)

See Also:


Attachments
Hot Fix (1.89 KB, patch)
2015-02-20 00:22 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (1.84 KB, patch)
2015-02-20 00:24 PST, Hunseop Jeong
gyuyoung.kim: review-
Details | Formatted Diff | Diff
Fix (3.81 KB, patch)
2015-02-20 01:32 PST, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2015-02-19 14:06:04 PST
SSIA. I won't be online until monday morning, feel free to pick it up.
Comment 1 Gyuyoung Kim 2015-02-20 00:22:40 PST
Created attachment 246942 [details]
Hot Fix
Comment 2 Gyuyoung Kim 2015-02-20 00:24:39 PST
I don't find what is correct fix against r180362. However if we don't use API::Object::wrap | API::Object::unwrap, EFL and GTK ports are fine at the moment.
Comment 3 Hunseop Jeong 2015-02-20 00:24:51 PST
Created attachment 246943 [details]
Patch
Comment 4 Gyuyoung Kim 2015-02-20 00:27:32 PST
Comment on attachment 246943 [details]
Patch

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

> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:131
> +#if PLATFORM(COCOA)

I'm not sure whether r180362 was only related with COCOA platform. I think my previous patch looks better.
Comment 5 Carlos Garcia Campos 2015-02-20 00:29:12 PST
I'm not sure this is the correct fix. wrap/unwrapp are defined in cross-platform code and there's an implementation using static casts when !DELEGATE_REF_COUNTING_TO_COCOA
Comment 6 Gyuyoung Kim 2015-02-20 00:30:37 PST
(In reply to comment #5)
> I'm not sure this is the correct fix. wrap/unwrapp are defined in
> cross-platform code and there's an implementation using static casts when
> !DELEGATE_REF_COUNTING_TO_COCOA

Yes, that's why I just uploaded a hot fix. I'm still looking this.
Comment 7 Carlos Garcia Campos 2015-02-20 01:32:17 PST
Created attachment 246945 [details]
Fix

This fixes the build fot GTK+ let's see what happens with EFL
Comment 8 WebKit Commit Bot 2015-02-20 01:34:44 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 9 Carlos Garcia Campos 2015-02-20 01:54:33 PST
Since fixes a different between ports, and I don't know how to fix the EFL build, I think it would be better to use different bugs.
Comment 10 Gyuyoung Kim 2015-02-20 01:55:39 PST
(In reply to comment #9)
> Since fixes a different between ports, and I don't know how to fix the EFL
> build, I think it would be better to use different bugs.

Ok, I'm going to file a new bug for EFL port.
Comment 11 Zan Dobersek 2015-02-20 02:59:50 PST
Comment on attachment 246945 [details]
Fix

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

> Source/WebKit2/UIProcess/API/C/gtk/WKAPICastGtk.h:48
> +inline WKViewRef toAPI(WebKitWebViewBase* view)
> +{
> +    return static_cast<WKViewRef>(static_cast<void*>(view));
> +}
> +
> +inline WebKitWebViewBase* toImpl(WKViewRef view)
> +{
> +    return static_cast<WebKitWebViewBase*>(static_cast<void*>(const_cast<OpaqueWKView*>(view)));
> +}

These should be specializations of the respective templates. They should also mimic the previous behavior of the generic toAPI<>() and toImpl<>() templates.
But ideally you'd have WebKitWebViewBase somehow inherit from API::Object, if that's even possible.

I ended up with this, built with Clang but not tested:

> template<>
> inline WKViewRef toAPI<>(WebKitWebViewBase* view)
> {
>     return reinterpret_cast<WKViewRef>(static_cast<void*>(view));
> }
> 
> template<>
> inline WebKitWebViewBase* toImpl<>(WKViewRef view)
> {
>     return static_cast<WebKitWebViewBase*>(static_cast<void*>(const_cast<typename std::remove_const<typename std::remove_pointer<WKViewRef>::type>::type*>(view)));
> }
Comment 12 Carlos Garcia Campos 2015-02-20 03:07:28 PST
(In reply to comment #11)
> Comment on attachment 246945 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246945&action=review
> 
> > Source/WebKit2/UIProcess/API/C/gtk/WKAPICastGtk.h:48
> > +inline WKViewRef toAPI(WebKitWebViewBase* view)
> > +{
> > +    return static_cast<WKViewRef>(static_cast<void*>(view));
> > +}
> > +
> > +inline WebKitWebViewBase* toImpl(WKViewRef view)
> > +{
> > +    return static_cast<WebKitWebViewBase*>(static_cast<void*>(const_cast<OpaqueWKView*>(view)));
> > +}
> 
> These should be specializations of the respective templates. They should
> also mimic the previous behavior of the generic toAPI<>() and toImpl<>()
> templates.
> But ideally you'd have WebKitWebViewBase somehow inherit from API::Object,
> if that's even possible.
> 
> I ended up with this, built with Clang but not tested:
> 
> > template<>
> > inline WKViewRef toAPI<>(WebKitWebViewBase* view)
> > {
> >     return reinterpret_cast<WKViewRef>(static_cast<void*>(view));
> > }
> > 
> > template<>
> > inline WebKitWebViewBase* toImpl<>(WKViewRef view)
> > {
> >     return static_cast<WebKitWebViewBase*>(static_cast<void*>(const_cast<typename std::remove_const<typename std::remove_pointer<WKViewRef>::type>::type*>(view)));
> > }

That works here.
Comment 13 Carlos Garcia Campos 2015-02-20 03:16:35 PST
Committed r180409: <http://trac.webkit.org/changeset/180409>
Comment 14 Anders Carlsson 2015-02-23 14:51:34 PST
> > I ended up with this, built with Clang but not tested:
> > 
> > > template<>
> > > inline WKViewRef toAPI<>(WebKitWebViewBase* view)
> > > {
> > >     return reinterpret_cast<WKViewRef>(static_cast<void*>(view));
> > > }
> > > 
> > > template<>
> > > inline WebKitWebViewBase* toImpl<>(WKViewRef view)
> > > {
> > >     return static_cast<WebKitWebViewBase*>(static_cast<void*>(const_cast<typename std::remove_const<typename std::remove_pointer<WKViewRef>::type>::type*>(view)));
> > > }
> 
> That works here.

Why didn't you add traits instead?
Comment 15 Carlos Garcia Campos 2015-02-23 23:32:24 PST
(In reply to comment #14)
> > > I ended up with this, built with Clang but not tested:
> > > 
> > > > template<>
> > > > inline WKViewRef toAPI<>(WebKitWebViewBase* view)
> > > > {
> > > >     return reinterpret_cast<WKViewRef>(static_cast<void*>(view));
> > > > }
> > > > 
> > > > template<>
> > > > inline WebKitWebViewBase* toImpl<>(WKViewRef view)
> > > > {
> > > >     return static_cast<WebKitWebViewBase*>(static_cast<void*>(const_cast<typename std::remove_const<typename std::remove_pointer<WKViewRef>::type>::type*>(view)));
> > > > }
> > 
> > That works here.
> 
> Why didn't you add traits instead?

Where? Could you elaborate a bit more? The thing is WebKitWebViewBase is not an API::Object, because it's not even a C++ class, but a GObject, so adding toAPI/toImpl for it was the only way I knew how to fix the problem. If there's a better way, I'll land a follow up patch.