Bug 219769 – Infrastructure to store and restore AppHighlights

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Infrustructure to store and restore AppHighlights

[Megan Gardner]

Created attachment 415971 [details]
Patch

[Tim Horton]

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?

[Wenson Hsieh]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

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)

[Devin Rousso]

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

[Megan Gardner]

Created attachment 416072 [details]
Patch

[Megan Gardner]

Created attachment 416082 [details]
Patch for landing

[EWS]

Committed r270729: <https://trac.webkit.org/changeset/270729>

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

[Radar WebKit Bug Importer]

<rdar://problem/72248110>

[Wenson Hsieh]

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`

[Don Olmstead]

Follow up to fix non-unified builds after this patch is at https://bugs.webkit.org/show_bug.cgi?id=219834