Skip to content

Commit

Permalink
Use event loop to set title
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=218496

Patch by Rob Buis <rbuis@igalia.com> on 2021-02-10
Reviewed by Darin Adler.

Source/WebCore:

Use event loop to set title to avoid calling WebFrameLoaderClient
within HTMLTitleElement::insertedIntoAncestor.

* dom/Document.cpp:
(WebCore::Document::updateTitle):
* dom/Document.h:
(WebCore::Document::titleWithDirection const):
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::setTitle):
* loader/EmptyClients.h:
* page/Chrome.cpp:
(WebCore::Chrome::print):
* page/ChromeClient.h:

Source/WebKit:

Add title parameter to PrintFrame message.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::printFrame):
* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in:
* WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::print):
* WebProcess/WebCoreSupport/WebChromeClient.h:

Source/WebKitLegacy/mac:

Adjust to API change.

* WebCoreSupport/WebChromeClient.h:
* WebCoreSupport/WebChromeClient.mm:
(WebChromeClient::print):

Source/WebKitLegacy/win:

Adjust to API change.

* WebCoreSupport/WebChromeClient.cpp:
(WebChromeClient::print):
* WebCoreSupport/WebChromeClient.h:

Tools:

Adapt unit tests to wait for title change tasks
to be processed.

* TestWebKitAPI/Tests/WebKit/PageLoadState.cpp:
(TestWebKitAPI::didChangeTitle):
(TestWebKitAPI::TEST):
* TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm:
(TEST):
* TestWebKitAPI/Tests/WebKitGLib/TestAuthentication.cpp:
(testWebViewAuthenticationFailure):
(testWebViewAuthenticationNoCredential):
(testWebViewAuthenticationSuccess):
(testWebViewAuthenticationEmptyRealm):
* TestWebKitAPI/Tests/WebKitGLib/TestBackForwardList.cpp:
(testBackForwardListNavigation):
* TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp:
(testWebViewTitle):
* TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:
(testLoadFailedWithTLSErrors):
* TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:
(testWebKitSettingsJavaScriptMarkup):
* TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:
(testWebViewTitleChange):

LayoutTests:

Adapt tests to make sure pending title change tasks
are processed before the test is done.

* TestExpectations:
* fast/dom/title-text-property-2.html:
* fast/dom/title-text-property-assigning-empty-string.html:
* fast/dom/title-text-property.html:
* http/tests/globalhistory/history-delegate-basic-title-expected.txt:
* http/tests/globalhistory/history-delegate-basic-title.html:
* http/tests/loading/basic-auth-load-URL-with-consecutive-slashes-expected.txt:
* http/tests/loading/basic-auth-load-URL-with-consecutive-slashes.html:
* http/tests/loading/redirect-with-no-location-crash-expected.txt:
* http/tests/loading/redirect-with-no-location-crash.html:
* platform/mac-wk2/TestExpectations:
* platform/win/http/tests/loading/basic-auth-load-URL-with-consecutive-slashes-expected.txt: Copied from LayoutTests/http/tests/loading/basic-auth-load-URL-with-consecutive-slashes-expected.txt.
* platform/wk2/http/tests/loading/basic-auth-load-URL-with-consecutive-slashes-expected.txt:
* platform/wk2/http/tests/loading/redirect-with-no-location-crash-expected.txt:

Canonical link: https://commits.webkit.org/233952@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@272707 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
rwlbuis authored and webkit-commit-queue committed Feb 11, 2021
1 parent 9e03d9d commit 0ee3e32
Show file tree
Hide file tree
Showing 43 changed files with 246 additions and 63 deletions.
25 changes: 25 additions & 0 deletions LayoutTests/ChangeLog
@@ -1,3 +1,28 @@
2021-02-10 Rob Buis <rbuis@igalia.com>

Use event loop to set title
https://bugs.webkit.org/show_bug.cgi?id=218496

Reviewed by Darin Adler.

Adapt tests to make sure pending title change tasks
are processed before the test is done.

* TestExpectations:
* fast/dom/title-text-property-2.html:
* fast/dom/title-text-property-assigning-empty-string.html:
* fast/dom/title-text-property.html:
* http/tests/globalhistory/history-delegate-basic-title-expected.txt:
* http/tests/globalhistory/history-delegate-basic-title.html:
* http/tests/loading/basic-auth-load-URL-with-consecutive-slashes-expected.txt:
* http/tests/loading/basic-auth-load-URL-with-consecutive-slashes.html:
* http/tests/loading/redirect-with-no-location-crash-expected.txt:
* http/tests/loading/redirect-with-no-location-crash.html:
* platform/mac-wk2/TestExpectations:
* platform/win/http/tests/loading/basic-auth-load-URL-with-consecutive-slashes-expected.txt: Copied from LayoutTests/http/tests/loading/basic-auth-load-URL-with-consecutive-slashes-expected.txt.
* platform/wk2/http/tests/loading/basic-auth-load-URL-with-consecutive-slashes-expected.txt:
* platform/wk2/http/tests/loading/redirect-with-no-location-crash-expected.txt:

2021-02-10 Ryan Haddad <ryanhaddad@apple.com>

[ Big Sur ] imported/w3c/web-platform-tests/css/css-flexbox/contain-size-layout-abspos-flex-container-crash.html is failing
Expand Down
3 changes: 0 additions & 3 deletions LayoutTests/TestExpectations
Expand Up @@ -4743,6 +4743,3 @@ webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-004
webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-014.html [ ImageOnlyFailure ] # Requires fallback (at parse time) support
webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-015.html [ ImageOnlyFailure ] # Requires fallback (at parse time) support (unclear if this makes sense)
webkit.org/b/220928 imported/w3c/web-platform-tests/css/css-color/predefined-017.html [ ImageOnlyFailure ] # Invalid test, percentages are not allowed for color(xyz).

webkit.org/b/218496 http/tests/loading/redirect-with-no-location-crash.html [ Skip ]

15 changes: 13 additions & 2 deletions LayoutTests/fast/dom/title-text-property-2.html
@@ -1,20 +1,29 @@
<html>
<head>
<script>
function runTests() {
function startTest() {
if (window.testRunner) {
testRunner.waitUntilDone();
testRunner.dumpAsText();
testRunner.dumpTitleChanges();
}

console.log("Setting document.title to TITLE1");
document.title = 'TITLE1';

internals.queueTask("DOMManipulation", () => continueTest());
}

function continueTest() {
title = document.getElementsByTagName('title').item(0);

console.log("Setting title element's text to TITLE2");
title.text = 'TITLE2';

internals.queueTask("DOMManipulation", () => endTest());
}

function endTest() {
newTitle = document.createElement('title');
console.log("Should not set title");
newTitle.appendChild(document.createTextNode('FAIL'));
Expand All @@ -28,10 +37,12 @@
titleElement = titleElements[titleElements.length - 1];
titleElement.parentNode.removeChild(titleElement);
}

internals.queueTask("DOMManipulation", () => testRunner.notifyDone());
}
</script>
<title>Initial title</title>
</head>
<body onload="runTests();" >
<body onload="startTest();" >
</body>
</html>
@@ -1,17 +1,23 @@
<html>
<head>
<script>
function runTests() {
function startTest() {
if (window.testRunner) {
testRunner.waitUntilDone();
testRunner.dumpAsText();
testRunner.dumpTitleChanges();
}

document.title = 'New non-empty title';
internals.queueTask("DOMManipulation", () => endTest())
}

function endTest() {
document.title = '';
internals.queueTask("DOMManipulation", () => testRunner.notifyDone())
}
</script>
</head>
<body onload='runTests();'>
<body onload='startTest();'>
</body>
</html>
3 changes: 3 additions & 0 deletions LayoutTests/fast/dom/title-text-property.html
Expand Up @@ -12,6 +12,7 @@

function runTests() {
if (window.testRunner) {
testRunner.waitUntilDone();
testRunner.dumpAsText();
testRunner.dumpTitleChanges();
}
Expand All @@ -24,6 +25,8 @@
titleElem.text = newTitle;

debugOutput('New title is: \'' + titleElem.text + '\'');

internals.queueTask("DOMManipulation", () => testRunner.notifyDone());
}
</script>
</head>
Expand Down
@@ -1,4 +1,3 @@
WebView navigated to url "http://127.0.0.1:8000/globalhistory/history-delegate-basic-title.html" with title "" with HTTP equivalent method "GET". The navigation was successful and was not a client redirect.
WebView updated the title for history URL "http://127.0.0.1:8000/globalhistory/history-delegate-basic-title.html" to "Test Title 1".
WebView updated the title for history URL "http://127.0.0.1:8000/globalhistory/history-delegate-basic-title.html" to "Test Title 2".
This test sees if the history delegate is notified of title changes.
@@ -1,15 +1,20 @@
<html>
<head>
<script>
if (window.testRunner)
if (window.testRunner) {
testRunner.waitUntilDone();
testRunner.dumpAsText();
}
</script>
<title>Test Title 1</title>
</head>
<body>
<body onload="runTest()">
This test sees if the history delegate is notified of title changes.
</body>
<script>
function runTest() {
document.title = "Test Title 2";
internals.queueTask("DOMManipulation", () => testRunner.notifyDone());
}
</script>
</body>
</html>
Expand Up @@ -5,8 +5,8 @@ main frame - didFinishDocumentLoadForFrame
http://127.0.0.1:8000/loading/resources/basic-auth-testing.php?username=webkit&password=rocks - didReceiveAuthenticationChallenge - Responding with webkit:rocks
frame "<!--frame1-->" - didCommitLoadForFrame
frame "<!--frame1-->" - didFinishDocumentLoadForFrame
frame "<!--frame1-->" - willPerformClientRedirectToURL: http://127.0.0.1:8000/a//b/non-existent-file.html
frame "<!--frame1-->" - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - willPerformClientRedirectToURL: http://127.0.0.1:8000/a//b/non-existent-file.html
main frame - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - didFinishLoadForFrame
main frame - didFinishLoadForFrame
Expand All @@ -15,5 +15,6 @@ frame "<!--frame1-->" - didCancelClientRedirectForFrame
frame "<!--frame1-->" - didCommitLoadForFrame
frame "<!--frame1-->" - didReceiveTitle: 404 Not Found
frame "<!--frame1-->" - didFinishDocumentLoadForFrame
frame "<!--frame1-->" - didFailLoadWithError
frame "<!--frame1-->" - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - didFinishLoadForFrame
PASS did not cause assertion failure.
Expand Up @@ -12,9 +12,11 @@

function done()
{
document.body.removeChild(document.getElementById("frame"));
if (window.testRunner)
testRunner.notifyDone();
setTimeout(function() {
document.body.removeChild(document.getElementById("frame"));
if (window.testRunner)
testRunner.notifyDone();
}, 500);
}

function notifyFrameDidLoad(frame)
Expand All @@ -24,8 +26,8 @@
}
</script>
</head>
<body>
<iframe id="frame" src="resources/basic-auth-testing.php?username=webkit&password=rocks" onload="notifyFrameDidLoad(this)"></iframe>
<body onload="notifyFrameDidLoad(frame)">
<iframe id="frame" src="resources/basic-auth-testing.php?username=webkit&password=rocks"></iframe>
<p>PASS did not cause assertion failure.</p>
</body>
</html>
@@ -1,6 +1,5 @@
main frame - didStartProvisionalLoadForFrame
main frame - didCommitLoadForFrame
main frame - didReceiveTitle: Test for https://bugs.webkit.org/show_bug.cgi?id=29293
frame "<!--frame1-->" - didStartProvisionalLoadForFrame
main frame - didFinishDocumentLoadForFrame
frame "<!--frame1-->" - didCommitLoadForFrame
Expand Down
Expand Up @@ -2,11 +2,13 @@
<head>
<!-- Test for https://bugs.webkit.org/show_bug.cgi?id=29293 -->
<script>
if (window.testRunner)
if (window.testRunner) {
testRunner.dumpAsText();
testRunner.waitUntilDone();
}
</script>
</head>
<body>
<body onload="setTimeout(function() { if (window.testRunner) testRunner.notifyDone(); })">
<iframe src="resources/redirect-with-no-location-crash.php"></iframe>
</body>
<html>
2 changes: 0 additions & 2 deletions LayoutTests/platform/mac-wk2/TestExpectations
Expand Up @@ -583,8 +583,6 @@ webkit.org/b/161649 [ Debug ] http/tests/cache/disk-cache/resource-becomes-uncac

webkit.org/b/163136 http/tests/xmlhttprequest/auth-reject-protection-space.html [ Pass Failure ]

webkit.org/b/163139 http/tests/loading/basic-auth-load-URL-with-consecutive-slashes.html [ Pass Failure ]

webkit.org/b/162975 http/tests/cache/disk-cache/memory-cache-revalidation-updates-disk-cache.html [ Pass Failure ]

webkit.org/b/161653 [ Debug ] storage/indexeddb/key-generator.html [ Pass Timeout ]
Expand Down
@@ -0,0 +1,20 @@
main frame - didStartProvisionalLoadForFrame
main frame - didCommitLoadForFrame
frame "<!--frame1-->" - didStartProvisionalLoadForFrame
main frame - didFinishDocumentLoadForFrame
http://127.0.0.1:8000/loading/resources/basic-auth-testing.php?username=webkit&password=rocks - didReceiveAuthenticationChallenge - Responding with webkit:rocks
frame "<!--frame1-->" - didCommitLoadForFrame
frame "<!--frame1-->" - didFinishDocumentLoadForFrame
frame "<!--frame1-->" - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - willPerformClientRedirectToURL: http://127.0.0.1:8000/a//b/non-existent-file.html
main frame - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - didFinishLoadForFrame
main frame - didFinishLoadForFrame
frame "<!--frame1-->" - didStartProvisionalLoadForFrame
frame "<!--frame1-->" - didCancelClientRedirectForFrame
frame "<!--frame1-->" - didCommitLoadForFrame
frame "<!--frame1-->" - didFinishDocumentLoadForFrame
frame "<!--frame1-->" - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - didFinishLoadForFrame
frame "<!--frame1-->" - didReceiveTitle: 404 Not Found
PASS did not cause assertion failure.
Expand Up @@ -5,15 +5,16 @@ frame "<!--frame1-->" - didStartProvisionalLoadForFrame
127.0.0.1:8000 - didReceiveAuthenticationChallenge - ProtectionSpaceAuthenticationSchemeHTTPBasic - Responding with webkit:rocks
frame "<!--frame1-->" - didCommitLoadForFrame
frame "<!--frame1-->" - didFinishDocumentLoadForFrame
frame "<!--frame1-->" - willPerformClientRedirectToURL: http://127.0.0.1:8000/a//b/non-existent-file.html
frame "<!--frame1-->" - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - willPerformClientRedirectToURL: http://127.0.0.1:8000/a//b/non-existent-file.html
main frame - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - didFinishLoadForFrame
main frame - didFinishLoadForFrame
frame "<!--frame1-->" - didStartProvisionalLoadForFrame
frame "<!--frame1-->" - didCancelClientRedirectForFrame
frame "<!--frame1-->" - didCommitLoadForFrame
frame "<!--frame1-->" - didReceiveTitle: 404 Not Found
frame "<!--frame1-->" - didFinishDocumentLoadForFrame
frame "<!--frame1-->" - didFailLoadWithError
frame "<!--frame1-->" - didHandleOnloadEventsForFrame
frame "<!--frame1-->" - didFinishLoadForFrame
frame "<!--frame1-->" - didReceiveTitle: 404 Not Found
PASS did not cause assertion failure.
@@ -1,6 +1,5 @@
main frame - didStartProvisionalLoadForFrame
main frame - didCommitLoadForFrame
main frame - didReceiveTitle: Test for https://bugs.webkit.org/show_bug.cgi?id=29293
main frame - didFinishDocumentLoadForFrame
frame "<!--frame1-->" - didStartProvisionalLoadForFrame
frame "<!--frame1-->" - didCommitLoadForFrame
Expand Down
21 changes: 21 additions & 0 deletions Source/WebCore/ChangeLog
@@ -1,3 +1,24 @@
2021-02-10 Rob Buis <rbuis@igalia.com>

Use event loop to set title
https://bugs.webkit.org/show_bug.cgi?id=218496

Reviewed by Darin Adler.

Use event loop to set title to avoid calling WebFrameLoaderClient
within HTMLTitleElement::insertedIntoAncestor.

* dom/Document.cpp:
(WebCore::Document::updateTitle):
* dom/Document.h:
(WebCore::Document::titleWithDirection const):
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::setTitle):
* loader/EmptyClients.h:
* page/Chrome.cpp:
(WebCore::Chrome::print):
* page/ChromeClient.h:

2021-02-10 Myles C. Maxfield <mmaxfield@apple.com>

Move pal/spi/cocoa/CoreTextSPI.h to pal/spi/cf/CoreTextSPI.h
Expand Down
10 changes: 8 additions & 2 deletions Source/WebCore/dom/Document.cpp
Expand Up @@ -1687,8 +1687,14 @@ void Document::updateTitle(const StringWithDirection& title)
m_title.string = canonicalizedTitle(*this, title.string);
m_title.direction = title.direction;

if (auto* loader = this->loader())
loader->setTitle(m_title);
if (!m_updateTitleTaskScheduled) {
eventLoop().queueTask(TaskSource::DOMManipulation, [protectedThis = makeRef(*this), this]() mutable {
m_updateTitleTaskScheduled = false;
if (auto documentLoader = makeRefPtr(loader()))
documentLoader->setTitle(m_title);
});
m_updateTitleTaskScheduled = true;
}
}

void Document::updateTitleFromTitleElement()
Expand Down
3 changes: 3 additions & 0 deletions Source/WebCore/dom/Document.h
Expand Up @@ -924,6 +924,7 @@ class Document
// Used by DOM bindings; no direction known.
const String& title() const { return m_title.string; }
WEBCORE_EXPORT void setTitle(const String&);
const StringWithDirection& titleWithDirection() const { return m_title; }

WEBCORE_EXPORT const AtomString& dir() const;
WEBCORE_EXPORT void setDir(const AtomString&);
Expand Down Expand Up @@ -2122,6 +2123,8 @@ class Document
bool m_didDispatchViewportPropertiesChanged { false };
#endif

bool m_updateTitleTaskScheduled { false };

OrientationNotifier m_orientationNotifier;
mutable RefPtr<Logger> m_logger;
RefPtr<StringCallback> m_consoleMessageListener;
Expand Down
3 changes: 2 additions & 1 deletion Source/WebCore/loader/DocumentLoader.cpp
Expand Up @@ -1721,7 +1721,8 @@ void DocumentLoader::setTitle(const StringWithDirection& title)

frameLoader()->willChangeTitle(this);
m_pageTitle = title;
frameLoader()->didChangeTitle(this);
if (frameLoader())
frameLoader()->didChangeTitle(this);
}

URL DocumentLoader::urlForHistory() const
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/loader/EmptyClients.h
Expand Up @@ -125,7 +125,7 @@ class EmptyChromeClient : public ChromeClient {

void mouseDidMoveOverElement(const HitTestResult&, unsigned, const String&, TextDirection) final { }

void print(Frame&) final { }
void print(Frame&, const StringWithDirection&) final { }

void exceededDatabaseQuota(Frame&, const String&, DatabaseDetails) final { }

Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/page/Chrome.cpp
Expand Up @@ -406,7 +406,7 @@ bool Chrome::print(Frame& frame)
return false;
}

m_client.print(frame);
m_client.print(frame, frame.document()->titleWithDirection());
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/page/ChromeClient.h
Expand Up @@ -222,7 +222,7 @@ class ChromeClient {
virtual void unavailablePluginButtonClicked(Element&, RenderEmbeddedObject::PluginUnavailabilityReason) const { }
virtual void mouseDidMoveOverElement(const HitTestResult&, unsigned modifierFlags, const String& toolTip, TextDirection) = 0;

virtual void print(Frame&) = 0;
virtual void print(Frame&, const StringWithDirection&) = 0;

virtual Color underlayColor() const { return Color(); }

Expand Down
17 changes: 17 additions & 0 deletions Source/WebKit/ChangeLog
@@ -1,3 +1,20 @@
2021-02-10 Rob Buis <rbuis@igalia.com>

Use event loop to set title
https://bugs.webkit.org/show_bug.cgi?id=218496

Reviewed by Darin Adler.

Add title parameter to PrintFrame message.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::printFrame):
* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in:
* WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::print):
* WebProcess/WebCoreSupport/WebChromeClient.h:

2021-02-10 Myles C. Maxfield <mmaxfield@apple.com>

Move pal/spi/cocoa/CoreTextSPI.h to pal/spi/cf/CoreTextSPI.h
Expand Down

0 comments on commit 0ee3e32

Please sign in to comment.