Bug 222640 – Hold onto AppHighlights and restore them once the page is loaded.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Hold onto AppHighlights and restore them once the page is loaded.

[Megan Gardner]

Created attachment 422045 [details]
Patch

[Devin Rousso]

Comment on attachment 422045 [details]
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();
```

[Wenson Hsieh]

Comment on attachment 422045 [details]
Patch

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

>> 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.

Is this accurate? I'm curious where the assertion is.

[Megan Gardner]

Created attachment 422123 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 422123 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Megan Gardner]

Created attachment 422659 [details]
Patch for landing

[EWS]

Found 1 new test failure: media/media-fragments/TC0051.html

[Megan Gardner]

Created attachment 422667 [details]
Patch

[Devin Rousso]

Comment on attachment 422045 [details]
Patch

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

>>> 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.
> 
> Is this accurate? I'm curious where the assertion is.

Ah I think I misread the `ASSERT` in `WeakPtr::get`.  I thought it was `ASSERT(m_impl || ...);` 😅

[Megan Gardner]

Created attachment 422718 [details]
Patch for landing

[EWS]

Committed r274155: <https://commits.webkit.org/r274155>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422718 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75222415>