Bug 120610 – Need a way to initialize Ref<T> efficiently with ::create() methods.

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Andreas Kling]

Let's take this to the next level.

[Andreas Kling]

Created attachment 210311 [details]
EWS experiment

[Kenneth Rohde Christiansen]

Comment on attachment 210311 [details]
EWS experiment

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

> Source/WebCore/page/Frame.h:203
> -        const RefPtr<Settings> m_settings;
> +        Ref<Settings> m_settings;

Maybe I am totally missing something here (probably am) but isn't the naming a bit confusing?

RefPtr - reference counted pointer
Ref    - reference counted reference?

I don't have any better suggestions though :-) RefRef would be terrible.

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 210311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210311&action=review
> 
> > Source/WebCore/page/Frame.h:203
> > -        const RefPtr<Settings> m_settings;
> > +        Ref<Settings> m_settings;
> 
> Maybe I am totally missing something here (probably am) but isn't the naming a bit confusing?
> 
> RefPtr - reference counted pointer
> Ref    - reference counted reference?
> 
> I don't have any better suggestions though :-) RefRef would be terrible.

Yeah RefRef is way too ugly to even consider. :)

I went with "Ref" because it's less wordy than RefPtr, providing a kind of minimalist incentive for using it (while attempting to offset the not-so-minimalist get() syntax.)

[Andreas Kling]

(This patch is broken as-is, I'm just here checking compiler support so far.)

[Andreas Kling]

Created attachment 210313 [details]
EWS experiment 2

Alternate version without move constructors.

[Andreas Kling]

Created attachment 210315 [details]
Patch sans ChangeLog

[WebKit Commit Bot]

Attachment 210315 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ref.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/Frame.h', u'Source/WebCore/page/Page.cpp', u'Source/WebCore/page/Page.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h']" exit_code: 1
Source/WebCore/ChangeLog:3:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 210315 [details]
Patch sans ChangeLog

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

> Source/WTF/wtf/Ref.h:82
> +    const T& get() const { return *m_ptr; }

I don’t think this is right. We don’t need to support const Ref<X> since we have Ref<const X> already.

[Darin Adler]

Comment on attachment 210315 [details]
Patch sans ChangeLog

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

> Source/WTF/wtf/Ref.h:36
> +template<typename T> class PassRef {

I say we call this RefInitializer instead (as we discussed).

> Source/WTF/wtf/Ref.h:83
> +    T& get() { return *m_ptr; }

This should be a const member function.

[Andreas Kling]

Created attachment 210316 [details]
Patch idea.

[Andreas Kling]

Created attachment 210317 [details]
Patch

[Andreas Kling]

Created attachment 210320 [details]
Patch

[Andreas Kling]

Created attachment 210830 [details]
Patch

[Andreas Kling]

Päging Darin Adler for comment.

[Andreas Kling]

05:45:28  * darin reads a patch
05:45:30 < darin> not saying which one
05:46:23 < darin> kling: that patch is all wrong
05:46:28 < darin> it uses #ifdef ASSERT_DISABLED
05:46:35 < kling> oh LOL
05:46:36 < darin> it's idea is supercool
05:46:39 < kling> it's supposed to be #if :(
05:46:48 < darin> I think m_wasAdopted needs to be mutable too
05:46:55 < darin> I could say this in review comments (was starting to)
05:47:13 < darin> it also has something needed only before Vector::append(U&&) so obsolete I think
05:47:27 < kling> we don't have Vector::append(U&&) yet
05:47:37 < kling> only uncheckedAppend()
05:47:49 < darin> oh, OK
05:47:53 < kling> anders was blocked on some GCC segfault :(
05:48:04 < darin> also RefInitializer should use a ref, not a pointer
05:48:15 < darin> otherwise it will be copyable
05:48:16 < kling> !
05:48:24 < darin> hmm, maybe even with a ref it is copyable
05:48:31 < darin> I guess you want it copyable
05:48:35 < darin> you made it copyable on purose
05:48:40 < darin> purpose
05:48:44 < darin> it's assignable too
05:48:55 < darin> you should delete operator=
05:49:04 < darin> kling: this is not good form -- should be in review comments
05:49:07 < darin> but those are my comments
05:49:24  * darin pats self on back even though he was naughty
05:49:34 < kling> that's okay, a little public humiliation is good for patch quality
05:49:45 < darin> also we normally use magic enums instead of bools for special second constructors
05:49:54 < darin> cause true is not pretty
05:49:57 < kling> but but but i was just copying PassRefPtr
05:50:00 < kling> but yeah you're right

[Andreas Kling]

Comment on attachment 210830 [details]
Patch

r- from Darin Darinson.

[Andreas Kling]

Darin fixed this with PassRef.