Bug 149643 – Compute document marker rects at use time instead of paint time

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

[Tim Horton]

Compute document marker rects at use time instead of paint time

[Tim Horton]

Created attachment 262102 [details]
Patch

[Tim Horton]

Need to more heavily test non-find-in-page document markers.

[WebKit Commit Bot]

Attachment 262102 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarkerController.cpp:657:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 262104 [details]
Patch

[WebKit Commit Bot]

Attachment 262104 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarkerController.cpp:660:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 262104 [details]
Patch

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

> Source/WebCore/dom/DocumentMarkerController.cpp:169
> +    for (const auto& markers : m_markers.values()) {

Not sure the const buys us anything here.

> Source/WebCore/dom/DocumentMarkerController.cpp:174
> +    m_document->page()->chrome().client().didInvalidateDocumentMarkerRects();

What guarantees this long chain of pointers has no null pointers in it?

> Source/WebCore/dom/DocumentMarkerController.cpp:188
> +    m_document->page()->chrome().client().didInvalidateDocumentMarkerRects();

What guarantees this long chain of pointers has no null pointers in it?

> Source/WebCore/dom/DocumentMarkerController.cpp:199
> +    for (const auto& nodeAndMarkers : m_markers) {

Again, not sure the const is helpful.

> Source/WebCore/dom/DocumentMarkerController.cpp:210
> +                m_document->frame()->mainFrame().view()->updateLayoutAndStyleIfNeededRecursive();

What guarantees this long chain of pointers has no null pointers in it?

> Source/WebCore/dom/DocumentMarkerController.cpp:229
> +    bool isMainFrameDocument = m_document->frame()->isMainFrame();

What guarantees that m_document and the frame are both non-null?

> Source/WebCore/dom/DocumentMarkerController.cpp:230
> +    FrameView* frameView = m_document->view();

What guarantees that frameView is non-null? If frameView is guaranteed to be non-null, then why not use a reference instead of a pointer for it?

> Source/WebCore/dom/DocumentMarkerController.cpp:231
> +    IntRect frameClipRect = frameView->windowToContents(frameView->windowClipRect());

Looks like this is unused for the main frame document; should we avoid even computing it in that case?

> Source/WebCore/dom/DocumentMarkerController.cpp:464
> +    for (const auto& nodeAndMarker : m_markers) {

Not sure the const adds value here.

> Source/WebCore/dom/DocumentMarkerController.cpp:601
> +        for (const auto& marker : *list) {

Not sure the const adds value here.

> Source/WebCore/dom/DocumentMarkerController.cpp:605
> +            if (!markerTypes.contains(marker.type()))
> +                continue;
> +            nodeNeedsRepaint = true;
> +            break;

I know we often do early return, but I think this read better the old way, with a plain old if (x) break.

> Source/WebCore/dom/DocumentMarkerController.cpp:630
> +        // FIXME: How can this possibly be iOS-specific code?

How indeed?

> Source/WebCore/dom/DocumentMarkerController.cpp:721
> +        for (const auto* marker : markersFor(node)) {

Not sure the const really helps here, but you added it so maybe you have a good reason.

> Source/WebCore/testing/Internals.cpp:1074
>          rectString.append("(");

Should be append('(').

> Source/WebCore/testing/Internals.cpp:1076
>          rectString.append(", ");

Should use appendLiteral.

> Source/WebCore/testing/Internals.cpp:1078
>          rectString.append(", ");

Should use appendLiteral.

> Source/WebCore/testing/Internals.cpp:1080
>          rectString.append(", ");

Should use appendLiteral.

> Source/WebCore/testing/Internals.cpp:1082
>          rectString.append(") ");

Should use appendLiteral.

[Tim Horton]

Created attachment 262140 [details]
Patch

[WebKit Commit Bot]

Attachment 262140 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarkerController.cpp:666:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Woah, new patch does not have Darin's review comments applied (I hadn't noticed that yet). Thanks for all the comments, Darin!

[Tim Horton]

(In reply to comment #6)
> Comment on attachment 262104 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262104&action=review
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:169
> > +    for (const auto& markers : m_markers.values()) {
> 
> Not sure the const buys us anything here.
> ...
> Not sure the const really helps here, but you added it so maybe you have a
> good reason.

OK! I may have gone crazy with the const.

> > Source/WebCore/dom/DocumentMarkerController.cpp:174
> > +    m_document->page()->chrome().client().didInvalidateDocumentMarkerRects();
> 
> What guarantees this long chain of pointers has no null pointers in it?
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:188
> > +    m_document->page()->chrome().client().didInvalidateDocumentMarkerRects();
> 
> What guarantees this long chain of pointers has no null pointers in it?
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:210
> > +                m_document->frame()->mainFrame().view()->updateLayoutAndStyleIfNeededRecursive();
> 
> What guarantees this long chain of pointers has no null pointers in it?
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:229
> > +    bool isMainFrameDocument = m_document->frame()->isMainFrame();
> 
> What guarantees that m_document and the frame are both non-null?
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:230
> > +    FrameView* frameView = m_document->view();
> 
> What guarantees that frameView is non-null? If frameView is guaranteed to be
> non-null, then why not use a reference instead of a pointer for it?

I think m_document can be a reference, so that removes some of the question here (but not all of it! I'll add some null checks since I'm not sure there's a guarantee in some of these cases).

I initially didn't make it a reference because of the crazy thing where Document can sort-of stay alive after the last reference goes away, and because I thought we had to null out our Document backpointer in detach(), but it looks like that pointer won't actually be dangling (and it's pretty clear that nobody's going to call any of the DocumentMarkerController methods at this point anyway), so it should be OK to make it a reference (DocumentMarkerController is owned by Document).

> > Source/WebCore/dom/DocumentMarkerController.cpp:231
> > +    IntRect frameClipRect = frameView->windowToContents(frameView->windowClipRect());
> 
> Looks like this is unused for the main frame document; should we avoid even
> computing it in that case?

Good point.

> > Source/WebCore/dom/DocumentMarkerController.cpp:605
> > +            if (!markerTypes.contains(marker.type()))
> > +                continue;
> > +            nodeNeedsRepaint = true;
> > +            break;
> 
> I know we often do early return, but I think this read better the old way,
> with a plain old if (x) break.

OK!

> > Source/WebCore/dom/DocumentMarkerController.cpp:630
> > +        // FIXME: How can this possibly be iOS-specific code?
> 
> How indeed?

I'm not going to try to figure this puzzle out in this patch. But it is mysterious.

> > Source/WebCore/testing/Internals.cpp:1074
> >          rectString.append("(");
> 
> Should be append('(').
> ...
> Should use appendLiteral.

Excellent.

[Tim Horton]

Created attachment 262141 [details]
Patch

[WebKit Commit Bot]

Attachment 262141 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DocumentMarkerController.cpp:688:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 262141 [details]
Patch

Attachment 262141 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/226171

New failing tests:
fast/text/mark-matches-overflow-clip.html

[Build Bot]

Created attachment 262142 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 262141 [details]
Patch

Attachment 262141 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/226195

New failing tests:
fast/text/mark-matches-overflow-clip.html

[Build Bot]

Created attachment 262144 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Tim Horton]

http://trac.webkit.org/changeset/190363

[Simon Fraser (smfr)]

Comment on attachment 262141 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:882
> +    renderer().document().markers().invalidateRectsForAllMarkers();

Soooo, for every RenderLayer, you go and invalidate some global state??

[Tim Horton]

Statute of limitations is long past!

[Tim Horton]

That said, it does look wrong.