WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
115516
Remove WTF::ListRefPtr class
https://bugs.webkit.org/show_bug.cgi?id=115516
Summary
Remove WTF::ListRefPtr class
Mikhail Pozdnyakov
Reported
2013-05-02 07:22:08 PDT
ListRefPtr is used only by FontFamily class, besides it strongly depends on FontFamily class semantics which makes it non-generic and inappropriate for being present inside WTF.
Attachments
WIP
(6.94 KB, patch)
2013-05-02 07:29 PDT
,
Mikhail Pozdnyakov
no flags
Details
Formatted Diff
Diff
patch
(15.73 KB, patch)
2013-05-03 01:08 PDT
,
Mikhail Pozdnyakov
andersca
: review-
Details
Formatted Diff
Diff
patch v2
(13.57 KB, patch)
2013-05-03 07:19 PDT
,
Mikhail Pozdnyakov
andersca
: review+
Details
Formatted Diff
Diff
to be landed
(13.22 KB, patch)
2013-05-03 09:42 PDT
,
Mikhail Pozdnyakov
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Mikhail Pozdnyakov
Comment 1
2013-05-02 07:29:13 PDT
Created
attachment 200315
[details]
WIP project files are not updated yet.
Anders Carlsson
Comment 2
2013-05-02 07:39:06 PDT
Nice! I seem to remember that ListRefPtr was added to avoid overflowing the stack when destroying AST Nodes in JSC when you had very deep syntax trees. I'm not sure how many nested FontFamily objects you could have linked together, so it might be OK to just have the RefPtr and not do any special processing in the destructor.
Mikhail Pozdnyakov
Comment 3
2013-05-03 00:42:28 PDT
(In reply to
comment #2
)
> Nice! > > I seem to remember that ListRefPtr was added to avoid overflowing the stack when destroying AST Nodes in JSC when you had very deep syntax trees. > > I'm not sure how many nested FontFamily objects you could have linked together, so it might be OK to just have the RefPtr and not do any special processing in the destructor.
I would keep the existing behaviour so far, as I am not sure that having just RefPtr won't bring the regression, because at least in principal it is still possible to face the same issue(overflowing the stack) again.
Mikhail Pozdnyakov
Comment 4
2013-05-03 01:08:41 PDT
Created
attachment 200392
[details]
patch
Anders Carlsson
Comment 5
2013-05-03 07:07:50 PDT
Comment on
attachment 200392
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=200392&action=review
No need to update the order files, they're re-generated by someone in the WebKit performance team every once in a while.
> Source/WebCore/platform/graphics/FontFamily.h:74 > + RefPtr<SharedFontFamily> reaper = m_next.release(); > + while (reaper && reaper->hasOneRef()) > + reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper
I'd add a comment stating that this initially came from ListRefPtr and that it can probably be a regular RefPtr.
Mikhail Pozdnyakov
Comment 6
2013-05-03 07:19:11 PDT
Created
attachment 200413
[details]
patch v2 Took comments from Anders into consideration.
Mikhail Pozdnyakov
Comment 7
2013-05-03 09:42:34 PDT
Created
attachment 200422
[details]
to be landed
WebKit Commit Bot
Comment 8
2013-05-03 10:11:51 PDT
Comment on
attachment 200422
[details]
to be landed Clearing flags on attachment: 200422 Committed
r149523
: <
http://trac.webkit.org/changeset/149523
>
WebKit Commit Bot
Comment 9
2013-05-03 10:11:54 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 10
2013-05-03 15:57:22 PDT
(In reply to
comment #0
)
> it strongly depends on FontFamily class semantics
How does it depend on that?
Mikhail Pozdnyakov
Comment 11
2013-05-05 21:18:37 PDT
(In reply to
comment #10
)
> (In reply to
comment #0
) > > it strongly depends on FontFamily class semantics > > How does it depend on that?
~ListRefPtr() { .... reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper }
Darin Adler
Comment 12
2013-05-06 09:14:36 PDT
(In reply to
comment #11
)
> (In reply to
comment #10
) > > (In reply to
comment #0
) > > > it strongly depends on FontFamily class semantics > > > > How does it depend on that? > > ~ListRefPtr() > { > .... > reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper > }
Yes, that’s the requirement to be a list in the ListRefPtr sense--a releaseNext function. What was missing was documentation of that in the ListRefPtr header. The reason I know this is not dependent on “FontFamily class semantics” is that it wasn’t even first created for FontFamily! It was created for JavaScriptCore. So I guess I am OK with removing this class, but I don’t think the remarks “strongly dependent on class semantics” and “non-generic” are fair.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug