Bug 191957 - Streamline ListHashSet use in floating object code
Summary: Streamline ListHashSet use in floating object code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-25 19:14 PST by Sam Weinig
Modified: 2018-12-02 22:09 PST (History)
5 users (show)

See Also:


Attachments
Patch (12.31 KB, patch)
2018-11-25 19:22 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2018-11-25 19:14:12 PST
Streamline ListHashSet use in floating object code
Comment 1 Sam Weinig 2018-11-25 19:22:46 PST
Created attachment 355614 [details]
Patch
Comment 2 WebKit Commit Bot 2018-11-26 11:29:31 PST
Comment on attachment 355614 [details]
Patch

Clearing flags on attachment: 355614

Committed r238504: <https://trac.webkit.org/changeset/238504>
Comment 3 WebKit Commit Bot 2018-11-26 11:29:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Radar WebKit Bug Importer 2018-11-26 11:31:08 PST
<rdar://problem/46250861>
Comment 5 Darin Adler 2018-12-02 22:09:59 PST
Comment on attachment 355614 [details]
Patch

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

> Source/WebCore/rendering/FloatingObjects.cpp:349
>  void FloatingObjects::remove(FloatingObject* floatingObject)

Since this won’t work with null, probably change this to take a reference at some point.

> Source/WebCore/rendering/FloatingObjects.cpp:351
> +    ASSERT((m_set.contains(floatingObject)));

Would like to remove the extra set of parentheses here, too.

> Source/WebCore/rendering/FloatingObjects.h:122
> +    static unsigned hash(PtrType key) { return PtrHash<RenderBox*>::hash(&key->renderer()); }
> +    static bool equal(PtrType a, PtrType b) { return &a->renderer() == &b->renderer(); }

Could we have used reference types here instead of pointer types? I’m not thrilled with functions that use "pointers that are known to never be null". We’d have to add some * at the various call sites, but I think that’s better than having the dereferencing hidden inside the functions.