Bug 205905

Created attachment 387068 [details]
Patch

Created attachment 387070 [details]
Patch

<rdar://problem/58398934>

Created attachment 387071 [details]
Patch

Comment on attachment 387071 [details]
Patch

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

> Source/WTF/ChangeLog:19
> +        This patch introduces NativeRef<T>. This is similar to Ref while it does not have any ownership. So it

Native => Naked.

Comment on attachment 387071 [details]
Patch

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

Nice!!

r=me

> Source/WTF/ChangeLog:8
> +        Objective-C has reflection mechanism. This means that fields, methods, and their types

"has reflection" => "has a reflection"

> Source/WTF/ChangeLog:24
> +            2. T* / T& is in Objective-C fields or paramter types (including a return type).

paramter => parameter

Comment on attachment 387071 [details]
Patch

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

>> Source/WTF/ChangeLog:8
>> +        Objective-C has reflection mechanism. This means that fields, methods, and their types
> 
> "has reflection" => "has a reflection"

actually, probably this should just be:
"mechanism" => "mechanisms"

since ObjC has more than one kind of reflection

Created attachment 387073 [details]
Patch

iOS build fix

Comment on attachment 387073 [details]
Patch

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

> Source/WTF/ChangeLog:36
> +           becomes available in our platforms, we can consider it. But using NakedRef / NakedPtr is not harmless.

Do you mean "is harmless" instead of "is not harmless", as in there is effectively no cost to using NakedRef / NakedPtr?

I'm now looking WebKitLegacy crash.

(In reply to Yusuke Suzuki from comment #10)
> I'm now looking WebKitLegacy crash.

OK, found the thing. Signature in WebViewInternal.h and WebView.mm were different.
I really think this must be compile error even in Objective-C!

(In reply to Yusuke Suzuki from comment #11)
> (In reply to Yusuke Suzuki from comment #10)
> > I'm now looking WebKitLegacy crash.
> 
> OK, found the thing. Signature in WebViewInternal.h and WebView.mm were
> different.
> I really think this must be compile error even in Objective-C!

I'll double-check and upload the patch for EWS before landing.

Created attachment 387081 [details]
Patch

WebKitLegacy fix

How do we ensure that people correctly use Naked<> in future? It's going to be very easy for new code to regress this again, right?

(In reply to Simon Fraser (smfr) from comment #14)
> How do we ensure that people correctly use Naked<> in future? It's going to
> be very easy for new code to regress this again, right?

I think we should track binary size as we are doing in performance testing.

Created attachment 387147 [details]
Patch

Fix

I've ensured that iOS simulator API tests pass locally.

Committed r254241: <https://trac.webkit.org/changeset/254241>

(In reply to Yusuke Suzuki from comment #15)
> (In reply to Simon Fraser (smfr) from comment #14)
> > How do we ensure that people correctly use Naked<> in future? It's going to
> > be very easy for new code to regress this again, right?
> 
> I think we should track binary size as we are doing in performance testing.

Is this something we could automate at build time? e.g. could we add a script phase (similar to check-for-weak-vtables-and-externals) that checks for this specific issue,  C++ type information in Objective-C fields and parameters?

(In reply to Sam Weinig from comment #19)
> (In reply to Yusuke Suzuki from comment #15)
> > (In reply to Simon Fraser (smfr) from comment #14)
> > > How do we ensure that people correctly use Naked<> in future? It's going to
> > > be very easy for new code to regress this again, right?
> > 
> > I think we should track binary size as we are doing in performance testing.
> 
> Is this something we could automate at build time? e.g. could we add a
> script phase (similar to check-for-weak-vtables-and-externals) that checks
> for this specific issue,  C++ type information in Objective-C fields and
> parameters?

PoC of this exists here :) https://bugs.webkit.org/show_bug.cgi?id=205968
Discussed with Simon and we can put this type of script as the same way to Scripts/check-for-exit-time-destructors etc.

(In reply to Yusuke Suzuki from comment #20)
> (In reply to Sam Weinig from comment #19)
> > (In reply to Yusuke Suzuki from comment #15)
> > > (In reply to Simon Fraser (smfr) from comment #14)
> > > > How do we ensure that people correctly use Naked<> in future? It's going to
> > > > be very easy for new code to regress this again, right?
> > > 
> > > I think we should track binary size as we are doing in performance testing.
> > 
> > Is this something we could automate at build time? e.g. could we add a
> > script phase (similar to check-for-weak-vtables-and-externals) that checks
> > for this specific issue,  C++ type information in Objective-C fields and
> > parameters?
> 
> PoC of this exists here :) https://bugs.webkit.org/show_bug.cgi?id=205968
> Discussed with Simon and we can put this type of script as the same way to
> Scripts/check-for-exit-time-destructors etc.

Very nice.