Bug 209630 - REGRESSION (r256577): Previous page continues to display after navigating to media document
Summary: REGRESSION (r256577): Previous page continues to display after navigating to ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-26 16:49 PDT by zalan
Modified: 2020-03-27 17:45 PDT (History)
16 users (show)

See Also:


Attachments
Patch (6.99 KB, patch)
2020-03-26 16:59 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (6.03 KB, patch)
2020-03-27 17:13 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2020-03-26 16:49:46 PDT
<rdar://problem/60609318>
Comment 1 zalan 2020-03-26 16:59:50 PDT
Created attachment 394680 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-03-26 17:05:08 PDT
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?
Comment 3 Darin Adler 2020-03-26 18:10:55 PDT
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.
Comment 4 zalan 2020-03-26 18:52:15 PDT
(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.
Comment 5 zalan 2020-03-26 18:56:43 PDT
(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.
Comment 6 zalan 2020-03-27 17:10:40 PDT
(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.
Comment 7 zalan 2020-03-27 17:13:27 PDT
Created attachment 394771 [details]
Patch
Comment 8 zalan 2020-03-27 17:14:07 PDT
(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)
Comment 9 zalan 2020-03-27 17:18:50 PDT
(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.
Comment 10 Tim Horton 2020-03-27 17:28:34 PDT
(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 :)
Comment 11 zalan 2020-03-27 17:31:28 PDT
(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
Comment 12 EWS 2020-03-27 17:45:11 PDT
Committed r259148: <https://trac.webkit.org/changeset/259148>

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