Bug 219769 - Infrastructure to store and restore AppHighlights
Summary: Infrastructure to store and restore AppHighlights
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: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-10 23:07 PST by Megan Gardner
Modified: 2020-12-13 08:56 PST (History)
16 users (show)

See Also:


Attachments
Patch (37.15 KB, patch)
2020-12-10 23:17 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (36.81 KB, patch)
2020-12-11 17:00 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (36.81 KB, patch)
2020-12-11 18:22 PST, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2020-12-10 23:07:44 PST
Infrustructure to store and restore AppHighlights
Comment 1 Megan Gardner 2020-12-10 23:17:27 PST
Created attachment 415971 [details]
Patch
Comment 2 Tim Horton 2020-12-11 15:34:32 PST
Comment on attachment 415971 [details]
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?
Comment 3 Wenson Hsieh 2020-12-11 15:45:12 PST
Comment on attachment 415971 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorageController.h:49
> +

Nit - extra newline here.
Comment 4 Wenson Hsieh 2020-12-11 15:50:33 PST
Comment on attachment 415971 [details]
Patch

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

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

Good catch — findRangeBySearchingText() is going to do TextIterator-y (and VisiblePosition-y) things which can trigger layout, so we need to be careful here.
Comment 5 Wenson Hsieh 2020-12-11 15:51:20 PST
Comment on attachment 415971 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightListData.h:107
> +

(Nit - extra newline here as well)
Comment 6 Devin Rousso 2020-12-11 16:14:42 PST
Comment on attachment 415971 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightListData.cpp:56
> +        is<Element>(node) ? downcast<Element>(node).getIdAttribute().string() : String { },

`nullString()`?

> Source/WebCore/Modules/highlight/AppHighlightStorageController.cpp:134
> +    for (auto iterator = range.endContainer().rbegin(), end = range.endContainer().rend(); iterator != end; ++iterator) {

Should one of these be `startContainer()`?  If not, maybe worth a comment as to why?

> Source/WebCore/Modules/highlight/AppHighlightStorageController.cpp:188
> +    Vector<AppHighlightRangeData> restoredRanges;

Is this variable needed?  All you ever seem to do is append to it.

> Source/WebCore/Modules/highlight/AppHighlightStorageController.h:40
> +    WTF_MAKE_FAST_ALLOCATED;

NIT: `WTF_MAKE_NONCOPYABLE` too?  In my experience if one is added usually the other is too (especially for `RefCounted`).

> Source/WebCore/dom/Document.h:1585
> +    AppHighlightStorageController* appHighlightStorageControllerIfExists() const { return m_appHighlightStorageController.get(); };

NIT: This is never used.  I realize the style guide suggests to create an `*IfExists` method, but I don't think it meant that as "you must create an `*IfExists` if there is another function without it".
Comment 7 Megan Gardner 2020-12-11 17:00:03 PST
Created attachment 416072 [details]
Patch
Comment 8 Megan Gardner 2020-12-11 18:22:18 PST
Created attachment 416082 [details]
Patch for landing
Comment 9 EWS 2020-12-11 19:20:07 PST
Committed r270729: <https://trac.webkit.org/changeset/270729>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416082 [details].
Comment 10 Radar WebKit Bug Importer 2020-12-11 19:21:19 PST
<rdar://problem/72248110>
Comment 11 Wenson Hsieh 2020-12-12 08:19:54 PST
Comment on attachment 415971 [details]
Patch

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

>> Source/WebCore/Modules/highlight/AppHighlightStorageController.cpp:134
>> +    for (auto iterator = range.endContainer().rbegin(), end = range.endContainer().rend(); iterator != end; ++iterator) {
> 
> Should one of these be `startContainer()`?  If not, maybe worth a comment as to why?

Both of these should be endContainer(). This code is trying to find the deepest common ancestor in the XPath that has an ID attribute, so we iterate backwards through the end container's XPath here and bail when we find the first match.

I think a comment might be good, or perhaps `foundElement` should be named something more descriptive, like `deepestCommonIdentifiableAncestor`
Comment 12 Don Olmstead 2020-12-13 08:56:23 PST
Follow up to fix non-unified builds after this patch is at https://bugs.webkit.org/show_bug.cgi?id=219834