Bug 209630 – REGRESSION (r256577): Previous page continues to display after navigating to media document

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

<rdar://problem/60609318>

[zalan]

Created attachment 394680 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 394680 [details]
Patch

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

I think this should have a test.

> Source/WebCore/page/FrameView.cpp:4383
> +        auto& document = *frame().document();

RefPtr?

[Darin Adler]

Comment on attachment 394680 [details]
Patch

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

> Source/WebCore/html/FTPDirectoryDocument.cpp:140
> +    if (document.view())
> +        document.view()->setDocumentHasVisuallyNonEmptyCustomContent();

Never 100% sure if it’s better to put a function like this on Document or require the caller to follow the pointer to the view and check it for null. Presuming we stick with a function on FrameView, I probably would have written the local variable style:

    if (auto view = document.view())
        view->setDocumentHasVisuallyNonEmptyCustomContent();

Gotta admit, though, that if the document doesn’t have a view yet it’s strange that we’ll forget this state.

> Source/WebCore/page/FrameView.cpp:277
>      m_visuallyNonEmptyCharacterCount = 0;
>      m_visuallyNonEmptyPixelCount = 0;
>      m_textRendererCountForVisuallyNonEmptyCharacters = 0;
> +    m_documentHasVisuallyNonEmptyCustomContent = false;

Seems like we are following a poor pattern here. We keep reusing a FrameView and "resetting" it, rather than having an object for the view of one document that we can re-create for each document. That’s the relationship between Frame and Document, FrameLoader and DocumentLoader. Seems like we have a similar problem here but a different solution.

> Source/WebCore/page/FrameView.cpp:4392
>          if (frame().document()->styleScope().hasPendingSheetsBeforeBody())

Still calling frame().document() here.

[zalan]

(In reply to Darin Adler from comment #3)
> Comment on attachment 394680 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394680&action=review
> 
> > Source/WebCore/html/FTPDirectoryDocument.cpp:140
> > +    if (document.view())
> > +        document.view()->setDocumentHasVisuallyNonEmptyCustomContent();
> 
> Never 100% sure if it’s better to put a function like this on Document or
> require the caller to follow the pointer to the view and check it for null.
I try to limit the visually non-empty logic to the FrameView.

> Presuming we stick with a function on FrameView, I probably would have
> written the local variable style:
> 
>     if (auto view = document.view())
>         view->setDocumentHasVisuallyNonEmptyCustomContent();
> 
> Gotta admit, though, that if the document doesn’t have a view yet it’s
> strange that we’ll forget this state.
I am not sure how "late parenting" of the Document works, but I assume by then the document itself is visually non-empty and we would go through the usual code flow.

> 
> > Source/WebCore/page/FrameView.cpp:277
> >      m_visuallyNonEmptyCharacterCount = 0;
> >      m_visuallyNonEmptyPixelCount = 0;
> >      m_textRendererCountForVisuallyNonEmptyCharacters = 0;
> > +    m_documentHasVisuallyNonEmptyCustomContent = false;
> 
> Seems like we are following a poor pattern here. We keep reusing a FrameView
> and "resetting" it, rather than having an object for the view of one
> document that we can re-create for each document. That’s the relationship
> between Frame and Document, FrameLoader and DocumentLoader. Seems like we
> have a similar problem here but a different solution.
100% agree.

> 
> > Source/WebCore/page/FrameView.cpp:4392
> >          if (frame().document()->styleScope().hasPendingSheetsBeforeBody())
> 
> Still calling frame().document() here.
Good catch. Thanks.

[zalan]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 394680 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394680&action=review
> 
> I think this should have a test.
I thought about it but not sure how to come up with one since this patch is really about timing (sooner content display) it's unclear to me how to achieve it.
 
> > Source/WebCore/page/FrameView.cpp:4383
> > +        auto& document = *frame().document();
> 
> RefPtr?
These are supposed to be all const functions, not sure why refing would be needed here.

[zalan]

(In reply to Darin Adler from comment #3)
> Comment on attachment 394680 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394680&action=review
> 
> > Source/WebCore/html/FTPDirectoryDocument.cpp:140
> > +    if (document.view())
> > +        document.view()->setDocumentHasVisuallyNonEmptyCustomContent();
> 
> Never 100% sure if it’s better to put a function like this on Document or
> require the caller to follow the pointer to the view and check it for null.
> Presuming we stick with a function on FrameView, I probably would have
> written the local variable style:
> 
>     if (auto view = document.view())
>         view->setDocumentHasVisuallyNonEmptyCustomContent();
> 
> Gotta admit, though, that if the document doesn’t have a view yet it’s
> strange that we’ll forget this state.
You are right. This should totally be on the Document instead.

[zalan]

Created attachment 394771 [details]
Patch

[zalan]

(In reply to zalan from comment #6)
> (In reply to Darin Adler from comment #3)
> > Comment on attachment 394680 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394680&action=review
> > 
> > > Source/WebCore/html/FTPDirectoryDocument.cpp:140
> > > +    if (document.view())
> > > +        document.view()->setDocumentHasVisuallyNonEmptyCustomContent();
> > 
> > Never 100% sure if it’s better to put a function like this on Document or
> > require the caller to follow the pointer to the view and check it for null.
> > Presuming we stick with a function on FrameView, I probably would have
> > written the local variable style:
> > 
> >     if (auto view = document.view())
> >         view->setDocumentHasVisuallyNonEmptyCustomContent();
> > 
> > Gotta admit, though, that if the document doesn’t have a view yet it’s
> > strange that we’ll forget this state.
> You are right. This should totally be on the Document instead.
It makes the patch simpler also (and more correct)

[zalan]

(In reply to zalan from comment #5)
> (In reply to Simon Fraser (smfr) from comment #2)
> > Comment on attachment 394680 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394680&action=review
> > 
> > I think this should have a test.
> I thought about it but not sure how to come up with one since this patch is
> really about timing (sooner content display) it's unclear to me how to
> achieve it.
As discussed on slack, unfortunately neither ref(lack of internals API (doable), this needs to be mainframe navigating to a media document while capturing milestones(not doable)), nor API test framework(lack of php support) can work with this case.

[Tim Horton]

(In reply to zalan from comment #9)
> (In reply to zalan from comment #5)
> > (In reply to Simon Fraser (smfr) from comment #2)
> > > Comment on attachment 394680 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=394680&action=review
> > > 
> > > I think this should have a test.
> > I thought about it but not sure how to come up with one since this patch is
> > really about timing (sooner content display) it's unclear to me how to
> > achieve it.
> As discussed on slack, unfortunately neither ref(lack of internals API
> (doable), this needs to be mainframe navigating to a media document while
> capturing milestones(not doable)), nor API test framework(lack of php
> support) can work with this case.

Surely an API test that uses WKURLSchemeHandler to load a resource that doesn't complete quickly will do the trick :)

[zalan]

(In reply to Tim Horton from comment #10)
> (In reply to zalan from comment #9)
> > (In reply to zalan from comment #5)
> > > (In reply to Simon Fraser (smfr) from comment #2)
> > > > Comment on attachment 394680 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=394680&action=review
> > > > 
> > > > I think this should have a test.
> > > I thought about it but not sure how to come up with one since this patch is
> > > really about timing (sooner content display) it's unclear to me how to
> > > achieve it.
> > As discussed on slack, unfortunately neither ref(lack of internals API
> > (doable), this needs to be mainframe navigating to a media document while
> > capturing milestones(not doable)), nor API test framework(lack of php
> > support) can work with this case.
> 
> Surely an API test that uses WKURLSchemeHandler to load a resource that
> doesn't complete quickly will do the trick :)
and I asked a couple of people. Surely I should have asked a few more (don't know what WKURLSchemeHandler is but will look into it and land a test case) Thanks

[EWS]

Committed r259148: <https://trac.webkit.org/changeset/259148>

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