Bug 149643 - Compute document marker rects at use time instead of paint time
Summary: Compute document marker rects at use time instead of paint time
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: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-29 13:41 PDT by Tim Horton
Modified: 2018-08-28 14:38 PDT (History)
9 users (show)

See Also:


Attachments
Patch (63.23 KB, patch)
2015-09-29 13:41 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (62.63 KB, patch)
2015-09-29 14:01 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (64.57 KB, patch)
2015-09-29 22:52 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (64.37 KB, patch)
2015-09-29 23:21 PDT, Tim Horton
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (666.37 KB, application/zip)
2015-09-30 00:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (599.58 KB, application/zip)
2015-09-30 00:12 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-09-29 13:41:17 PDT
Compute document marker rects at use time instead of paint time
Comment 1 Tim Horton 2015-09-29 13:41:53 PDT
Created attachment 262102 [details]
Patch
Comment 2 Tim Horton 2015-09-29 13:42:53 PDT
Need to more heavily test non-find-in-page document markers.
Comment 3 WebKit Commit Bot 2015-09-29 13:43:48 PDT
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.
Comment 4 Tim Horton 2015-09-29 14:01:43 PDT
Created attachment 262104 [details]
Patch
Comment 5 WebKit Commit Bot 2015-09-29 14:05:00 PDT
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.
Comment 6 Darin Adler 2015-09-29 22:51:12 PDT
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.
Comment 7 Tim Horton 2015-09-29 22:52:23 PDT
Created attachment 262140 [details]
Patch
Comment 8 WebKit Commit Bot 2015-09-29 22:53:49 PDT
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.
Comment 9 Tim Horton 2015-09-29 22:54:33 PDT
Woah, new patch does not have Darin's review comments applied (I hadn't noticed that yet). Thanks for all the comments, Darin!
Comment 10 Tim Horton 2015-09-29 23:13:00 PDT
(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.
Comment 11 Tim Horton 2015-09-29 23:21:28 PDT
Created attachment 262141 [details]
Patch
Comment 12 WebKit Commit Bot 2015-09-29 23:23:43 PDT
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.
Comment 13 Build Bot 2015-09-30 00:02:21 PDT
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
Comment 14 Build Bot 2015-09-30 00:02:27 PDT
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
Comment 15 Build Bot 2015-09-30 00:12:23 PDT
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
Comment 16 Build Bot 2015-09-30 00:12:28 PDT
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
Comment 17 Tim Horton 2015-09-30 12:37:32 PDT
http://trac.webkit.org/changeset/190363
Comment 18 Simon Fraser (smfr) 2018-08-28 14:30:17 PDT
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??
Comment 19 Tim Horton 2018-08-28 14:37:53 PDT
Statute of limitations is long past!
Comment 20 Tim Horton 2018-08-28 14:38:58 PDT
That said, it does look wrong.