[webkit-reviews] review granted: [Bug 222640] Hold onto AppHighlights and restore them once the page is loaded. : [Attachment 422045] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 222640: Hold onto AppHighlights and restore them once the page is loaded.
https://bugs.webkit.org/show_bug.cgi?id=222640

Attachment 422045: Patch

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




--- Comment #2 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 422045
  --> https://bugs.webkit.org/attachment.cgi?id=422045
Patch

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

r=me

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:254
> +    auto strongDocument = makeRefPtr(m_document.get());

I think this will `ASSERT` if `m_document` is not set or has been destroyed. 
You might have to move this below the `if (!m_document)` check.

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:50
> +    ~AppHighlightStorage();

NIT: Can you also make this class `final`?

> Source/WebCore/dom/Document.cpp:6024
> +    if (appHighlightStorageIfExists())

NIT: You could save the return value and use that instead of
`appHighlightStorage()`
```
    if (auto* appHighlightStorage = appHighlightStorageIfExists())
	appHighlightStorage->restoreUnrestoredAppHighlights();
```