Make it clearer when serveAsynchronousMockedRequests can be used

Currently serveAsynchronousMockedRequests is used
in 17 places in 6 different files including one outside blink (under content)
and pumpPendingRequestsDoNotUse is used
in 12 places in 4 files, both excluding ones in FrameTestHelpers.

Probably it'd be clearer/simpler to just make these method
names and documentation clearer to suggest when they can
be used or not.

BUG=571332
R=dcheng@chromium.org

Review URL: https://codereview.chromium.org/1771353002

Cr-Commit-Position: refs/heads/master@{#380043}

third_party/WebKit/PRESUBMIT.py

@@ -170,24 +170,6 @@ def _CheckForPrintfDebugging(input_api, output_api):
     return []
 
 
-def _CheckForDangerousTestFunctions(input_api, output_api):
-    """Tests should not be using serveAsynchronousMockedRequests, since it does
-    not guarantee that the threaded HTML parser will have completed."""
-    serve_async_requests_re = input_api.re.compile(
-        r'serveAsynchronousMockedRequests')
-    errors = input_api.canned_checks._FindNewViolationsOfRule(
-        lambda _, x: not serve_async_requests_re.search(x),
-        input_api, None)
-    errors = ['  * %s' % violation for violation in errors]
-    if errors:
-        return [output_api.PresubmitPromptOrNotify(
-            'You should probably be using one of the FrameTestHelpers::'
-            '(re)load* functions instead of '
-            'serveAsynchronousMockedRequests() in the following '
-            'locations:\n%s' % '\n'.join(errors))]
-    return []
-
-
 def _CheckForFailInFile(input_api, f):
     pattern = input_api.re.compile('^FAIL')
     errors = []
@@ -266,7 +248,6 @@ def CheckChangeOnUpload(input_api, output_api):
     results.extend(_CommonChecks(input_api, output_api))
     results.extend(_CheckStyle(input_api, output_api))
     results.extend(_CheckForPrintfDebugging(input_api, output_api))
-    results.extend(_CheckForDangerousTestFunctions(input_api, output_api))
     results.extend(_CheckForInvalidPreferenceError(input_api, output_api))
     results.extend(_CheckForForbiddenNamespace(input_api, output_api))
     return results

third_party/WebKit/Source/web/tests/ActivityLoggerTest.cpp

@@ -17,7 +17,7 @@
 namespace blink {
 
 using blink::FrameTestHelpers::WebViewHelper;
-using blink::FrameTestHelpers::pumpPendingRequestsDoNotUse;
+using blink::FrameTestHelpers::pumpPendingRequestsForFrameToLoad;
 
 class TestActivityLogger : public V8DOMActivityLogger {
 public:
@@ -77,7 +77,7 @@ class ActivityLoggerTest : public testing::Test {
     {
         v8::HandleScope scope(v8::Isolate::GetCurrent());
         m_scriptController->executeScriptInMainWorld(script);
-        pumpPendingRequestsDoNotUse(m_webViewHelper.webViewImpl()->mainFrame());
+        pumpPendingRequestsForFrameToLoad(m_webViewHelper.webViewImpl()->mainFrame());
     }
 
     void executeScriptInIsolatedWorld(const String& script) const
@@ -87,7 +87,7 @@ class ActivityLoggerTest : public testing::Test {
         sources.append(ScriptSourceCode(script));
         Vector<v8::Local<v8::Value>> results;
         m_scriptController->executeScriptInIsolatedWorld(isolatedWorldId, sources, extensionGroup, 0);
-        pumpPendingRequestsDoNotUse(m_webViewHelper.webViewImpl()->mainFrame());
+        pumpPendingRequestsForFrameToLoad(m_webViewHelper.webViewImpl()->mainFrame());
     }
 
     bool verifyActivities(const String& activities)

third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp

@@ -84,12 +84,6 @@ void runServeAsyncRequestsTask(TestWebFrameClient* client)
         testing::exitRunLoop();
 }
 
-void pumpPendingRequests(WebFrame* frame)
-{
-    Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, bind(&runServeAsyncRequestsTask, testClientForFrame(frame)));
-    testing::enterRunLoop();
-}
-
 TestWebFrameClient* defaultWebFrameClient()
 {
     DEFINE_STATIC_LOCAL(TestWebFrameClient, client, ());
@@ -123,36 +117,37 @@ void loadFrame(WebFrame* frame, const std::string& url)
     urlRequest.initialize();
     urlRequest.setURL(URLTestHelpers::toKURL(url));
     frame->loadRequest(urlRequest);
-    pumpPendingRequests(frame);
+    pumpPendingRequestsForFrameToLoad(frame);
 }
 
 void loadHTMLString(WebFrame* frame, const std::string& html, const WebURL& baseURL)
 {
     frame->loadHTMLString(WebData(html.data(), html.size()), baseURL);
-    pumpPendingRequests(frame);
+    pumpPendingRequestsForFrameToLoad(frame);
 }
 
 void loadHistoryItem(WebFrame* frame, const WebHistoryItem& item, WebHistoryLoadType loadType, WebURLRequest::CachePolicy cachePolicy)
 {
     frame->loadHistoryItem(item, loadType, cachePolicy);
-    pumpPendingRequests(frame);
+    pumpPendingRequestsForFrameToLoad(frame);
 }
 
 void reloadFrame(WebFrame* frame)
 {
     frame->reload(false);
-    pumpPendingRequests(frame);
+    pumpPendingRequestsForFrameToLoad(frame);
 }
 
 void reloadFrameIgnoringCache(WebFrame* frame)
 {
     frame->reload(true);
-    pumpPendingRequests(frame);
+    pumpPendingRequestsForFrameToLoad(frame);
 }
 
-void pumpPendingRequestsDoNotUse(WebFrame* frame)
+void pumpPendingRequestsForFrameToLoad(WebFrame* frame)
 {
-    pumpPendingRequests(frame);
+    Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, bind(&runServeAsyncRequestsTask, testClientForFrame(frame)));
+    testing::enterRunLoop();
 }
 
 WebLocalFrame* createLocalChild(WebRemoteFrame* parent, const WebString& name, WebFrameClient* client, WebFrame* previousSibling, const WebFrameOwnerProperties& properties)
@@ -252,7 +247,7 @@ void WebViewHelper::resize(WebSize size)
     m_testWebViewClient->clearAnimationScheduled();
 }
 
-TestWebFrameClient::TestWebFrameClient() : m_loadsInProgress(0)
+TestWebFrameClient::TestWebFrameClient()
 {
 }
 
@@ -281,21 +276,6 @@ void TestWebFrameClient::didStopLoading()
     --m_loadsInProgress;
 }
 
-void TestWebFrameClient::waitForLoadToComplete()
-{
-    for (;;) {
-        // We call runPendingTasks multiple times as single call of
-        // runPendingTasks may not be enough.
-        // runPendingTasks only ensures that main thread task queue is empty,
-        // and asynchronous parsing make use of off main thread HTML parser.
-        testing::runPendingTasks();
-        if (!isLoading())
-            break;
-
-        testing::yieldCurrentThread();
-    }
-}
-
 TestWebRemoteFrameClient::TestWebRemoteFrameClient()
     : m_frame(WebRemoteFrameImpl::create(WebTreeScopeType::Document, this, nullptr))
 {

third_party/WebKit/Source/web/tests/FrameTestHelpers.h

@@ -71,9 +71,9 @@ void loadHistoryItem(WebFrame*, const WebHistoryItem&, WebHistoryLoadType, WebUR
 void reloadFrame(WebFrame*);
 void reloadFrameIgnoringCache(WebFrame*);
 
-// Pumps pending resource requests while waiting for a frame to load. Don't use
-// this. Use one of the above helpers.
-void pumpPendingRequestsDoNotUse(WebFrame*);
+// Pumps pending resource requests while waiting for a frame to load. Consider
+// using one of the above helper methods whenever possible.
+void pumpPendingRequestsForFrameToLoad(WebFrame*);
 
 // Calls WebRemoteFrame::createLocalChild, but with some arguments prefilled
 // with default test values (i.e. with a default |client| or |properties| and/or
@@ -175,10 +175,9 @@ class TestWebFrameClient : public WebFrameClient {
     void didStopLoading() override;
 
     bool isLoading() { return m_loadsInProgress > 0; }
-    void waitForLoadToComplete();
 
 private:
-    int m_loadsInProgress;
+    int m_loadsInProgress = 0;
 };
 
 // Minimal implementation of WebRemoteFrameClient needed for unit tests that load remote frames. Tests that load

third_party/WebKit/Source/web/tests/WebFrameTest.cpp

@@ -3273,21 +3273,21 @@ TEST_F(WebFrameTest, ReloadWithOverrideURLPreservesState)
 
     // Reload the page and end up at the same url. State should be propagated.
     webViewHelper.webViewImpl()->mainFrame()->reloadWithOverrideURL(toKURL(m_baseURL + firstURL), false);
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(webViewHelper.webViewImpl()->mainFrame());
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(webViewHelper.webViewImpl()->mainFrame());
     EXPECT_EQ(previousOffset.width, webViewHelper.webViewImpl()->mainFrame()->scrollOffset().width);
     EXPECT_EQ(previousOffset.height, webViewHelper.webViewImpl()->mainFrame()->scrollOffset().height);
     EXPECT_EQ(previousScale, webViewHelper.webViewImpl()->pageScaleFactor());
 
     // Reload the page using the cache. State should not be propagated.
     webViewHelper.webViewImpl()->mainFrame()->reloadWithOverrideURL(toKURL(m_baseURL + secondURL), false);
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(webViewHelper.webViewImpl()->mainFrame());
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(webViewHelper.webViewImpl()->mainFrame());
     EXPECT_EQ(0, webViewHelper.webViewImpl()->mainFrame()->scrollOffset().width);
     EXPECT_EQ(0, webViewHelper.webViewImpl()->mainFrame()->scrollOffset().height);
     EXPECT_EQ(1.0f, webViewHelper.webViewImpl()->pageScaleFactor());
 
     // Reload the page while ignoring the cache. State should not be propagated.
     webViewHelper.webViewImpl()->mainFrame()->reloadWithOverrideURL(toKURL(m_baseURL + thirdURL), true);
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(webViewHelper.webViewImpl()->mainFrame());
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(webViewHelper.webViewImpl()->mainFrame());
     EXPECT_EQ(0, webViewHelper.webViewImpl()->mainFrame()->scrollOffset().width);
     EXPECT_EQ(0, webViewHelper.webViewImpl()->mainFrame()->scrollOffset().height);
     EXPECT_EQ(1.0f, webViewHelper.webViewImpl()->pageScaleFactor());
@@ -3340,7 +3340,7 @@ TEST_P(ParameterizedWebFrameTest, IframeRedirect)
     FrameTestHelpers::WebViewHelper webViewHelper(this);
     webViewHelper.initializeAndLoad(m_baseURL + "iframe_redirect.html", true);
     // Pump pending requests one more time. The test page loads script that navigates.
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(webViewHelper.webView()->mainFrame());
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(webViewHelper.webView()->mainFrame());
 
     WebFrame* iframe = webViewHelper.webView()->findFrameByName(WebString::fromUTF8("ifr"));
     ASSERT_TRUE(iframe);
@@ -5868,7 +5868,7 @@ TEST_P(ParameterizedWebFrameTest, ModifiedClickNewWindow)
     frameRequest.setTriggeringEvent(event);
     UserGestureIndicator gesture(DefinitelyProcessingUserGesture);
     toLocalFrame(webViewHelper.webViewImpl()->page()->mainFrame())->loader().load(frameRequest);
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(webViewHelper.webView()->mainFrame());
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(webViewHelper.webView()->mainFrame());
 
     // decidePolicyForNavigation should be called both for the original request and the ctrl+click.
     EXPECT_EQ(2, webFrameClient.decidePolicyCallCount());
@@ -5931,7 +5931,7 @@ TEST_P(ParameterizedWebFrameTest, ReloadPost)
     FrameTestHelpers::loadFrame(webViewHelper.webView()->mainFrame(), "javascript:document.forms[0].submit()");
     // Pump requests one more time after the javascript URL has executed to
     // trigger the actual POST load request.
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(webViewHelper.webView()->mainFrame());
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(webViewHelper.webView()->mainFrame());
     EXPECT_EQ(WebString::fromUTF8("POST"), frame->dataSource()->request().httpMethod());
 
     FrameTestHelpers::reloadFrame(frame);
@@ -6076,7 +6076,7 @@ TEST_P(ParameterizedWebFrameTest, NavigateToSame)
 
     FrameLoadRequest frameRequest(0, ResourceRequest(toLocalFrame(webViewHelper.webViewImpl()->page()->mainFrame())->document()->url()));
     toLocalFrame(webViewHelper.webViewImpl()->page()->mainFrame())->loader().load(frameRequest);
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(webViewHelper.webView()->mainFrame());
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(webViewHelper.webView()->mainFrame());
 
     EXPECT_TRUE(client.frameLoadTypeSameSeen());
 }
@@ -6353,7 +6353,7 @@ TEST_P(ParameterizedWebFrameTest, CurrentHistoryItem)
     // Before commit, there is no history item.
     EXPECT_FALSE(mainFrameLoader.currentItem());
 
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(frame);
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(frame);
 
     // After commit, there is.
     HistoryItem* item = mainFrameLoader.currentItem();

third_party/WebKit/Source/web/tests/WebViewTest.cpp

@@ -1360,7 +1360,7 @@ static void DragAndDropURL(WebViewImpl* webView, const std::string& url)
     const WebPoint screenPoint(0, 0);
     webView->dragTargetDragEnter(dragData, clientPoint, screenPoint, WebDragOperationCopy, 0);
     webView->dragTargetDrop(clientPoint, screenPoint, 0);
-    FrameTestHelpers::pumpPendingRequestsDoNotUse(webView->mainFrame());
+    FrameTestHelpers::pumpPendingRequestsForFrameToLoad(webView->mainFrame());
 }
 
 TEST_F(WebViewTest, DragDropURL)

third_party/WebKit/public/platform/WebUnitTestSupport.h

@@ -51,6 +51,11 @@ class WebUnitTestSupport {
 
     // Causes all pending asynchronous requests to be served. When this method
     // returns all the pending requests have been processed.
+    // Note: this may not work as expected if more requests could be made
+    // asynchronously from different threads (e.g. when HTML parser thread
+    // is being involved).
+    // DO NOT USE THIS for Frame loading; always use methods defined in
+    // FrameTestHelpers instead.
     virtual void serveAsynchronousMockedRequests() { }
 
     // Set a delegate that allows callbacks for all WebURLLoaderClients to be intercepted.