Bug 120610

Summary: Need a way to initialize Ref<T> efficiently with ::create() methods.
Product: WebKit Reporter: Andreas Kling <kling>
Component: Web Template FrameworkAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, commit-queue, kling
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
EWS experiment
none
EWS experiment 2
none
Patch sans ChangeLog
none
Patch idea.
none
Patch
none
Patch
none
Patch none

Andreas Kling
Reported 2013-09-02 15:05:27 PDT
Let's take this to the next level.
Attachments
EWS experiment (11.27 KB, patch)
2013-09-02 15:06 PDT, Andreas Kling
no flags
EWS experiment 2 (11.87 KB, patch)
2013-09-02 15:41 PDT, Andreas Kling
no flags
Patch sans ChangeLog (13.92 KB, patch)
2013-09-02 17:03 PDT, Andreas Kling
no flags
Patch idea. (15.68 KB, patch)
2013-09-02 17:42 PDT, Andreas Kling
no flags
Patch (14.79 KB, patch)
2013-09-02 18:47 PDT, Andreas Kling
no flags
Patch (15.67 KB, patch)
2013-09-02 21:45 PDT, Andreas Kling
no flags
Patch (8.22 KB, patch)
2013-09-06 20:11 PDT, Andreas Kling
no flags
Andreas Kling
Comment 1 2013-09-02 15:06:10 PDT
Created attachment 210311 [details] EWS experiment
Kenneth Rohde Christiansen
Comment 2 2013-09-02 15:09:51 PDT
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
Comment 3 2013-09-02 15:22:12 PDT
(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
Comment 4 2013-09-02 15:24:51 PDT
(This patch is broken as-is, I'm just here checking compiler support so far.)
Andreas Kling
Comment 5 2013-09-02 15:41:32 PDT
Created attachment 210313 [details] EWS experiment 2 Alternate version without move constructors.
Andreas Kling
Comment 6 2013-09-02 17:03:24 PDT
Created attachment 210315 [details] Patch sans ChangeLog
WebKit Commit Bot
Comment 7 2013-09-02 17:04:51 PDT
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 8 2013-09-02 17:05:16 PDT
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 9 2013-09-02 17:07:02 PDT
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
Comment 10 2013-09-02 17:42:27 PDT
Created attachment 210316 [details] Patch idea.
Andreas Kling
Comment 11 2013-09-02 18:47:20 PDT
Andreas Kling
Comment 12 2013-09-02 21:45:41 PDT
Andreas Kling
Comment 13 2013-09-06 20:11:50 PDT
Andreas Kling
Comment 14 2013-09-07 20:48:40 PDT
Päging Darin Adler for comment.
Andreas Kling
Comment 15 2013-09-08 21:02:28 PDT
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 16 2013-09-08 21:03:16 PDT
Comment on attachment 210830 [details] Patch r- from Darin Darinson.
Andreas Kling
Comment 17 2013-12-05 02:07:33 PST
Darin fixed this with PassRef.
Note You need to log in before you can comment on or make changes to this bug.