[webkit-reviews] review granted: [Bug 219769] Infrastructure to store and restore AppHighlights : [Attachment 415971] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 219769: Infrastructure to store and restore AppHighlights
https://bugs.webkit.org/show_bug.cgi?id=219769

Attachment 415971: Patch

https://bugs.webkit.org/attachment.cgi?id=415971&action=review




--- Comment #2 from Tim Horton <thorton at apple.com> ---
Comment on attachment 415971
  --> https://bugs.webkit.org/attachment.cgi?id=415971
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorageController.cpp:172
> +    if (m_document) {

Early return instead?

> Source/WebCore/Modules/highlight/AppHighlightStorageController.cpp:190
> +

Weird double newline. Also, the early return thing.
Also, maybe you want to strong-ify document in each of these before checking
it? Is it possible any of the things you do cause the last ref to drop? It
vaguely looks like "probably not" but the set of things happening under these
might be wider than it first appears.

> Source/WebCore/dom/Document.h:97
> +class AppHighlightStorageController;

This name is kind of wordy. Do we need "controller" at all, really?