- Revision
- 220052
- Author
- da...@apple.com
- Date
- 2017-07-30 14:44:01 -0700 (Sun, 30 Jul 2017)
Log Message
Remove code in HTMLObjectElement attribute parsing that forces style resolution and layout
https://bugs.webkit.org/show_bug.cgi?id=130653
Reviewed by Antti Koivisto.
Source/WebCore:
Also fixes a bug where load events are delivered prematurely in some cases
when an object, embed, frame, or iframe element is still loading.
* dom/Document.cpp:
(WebCore::Document::loadEventDelayTimerFired): Added a call to
FrameLoader::checkLoadComplete. Goes along with the change to
FrameLoader::checkLoadCompleteForThisFrame, which now respects the
isDelayingLoadEvent flag.
* html/HTMLAppletElement.cpp:
(WebCore::HTMLAppletElement::HTMLAppletElement): Removed the createdByParser argument,
no longer needed by the base class.
(WebCore::HTMLAppletElement::create): Added call to finishCreating, which is now part of
the process of creating any object in a class derived from HTMLPlugInImageElement.
(WebCore::HTMLAppletElement::updateWidget): Rearranged logic so setNeedsWidgetUpdate
is only called when it's becoming false; avoids a false/true/false round trip that can
cause trouble.
* html/HTMLAppletElement.h: Updated for the above.
* html/HTMLEmbedElement.cpp:
(WebCore::HTMLEmbedElement::HTMLEmbedElement): Removed the createdByParser argument,
no longer needed by the base class.
(WebCore::HTMLEmbedElement::create): Added call to finishCreating, which is now part of
the process of creating any object in a class derived from HTMLPlugInImageElement.
(WebCore::HTMLEmbedElement::parseAttribute): Changed srcAttr to call
updateImageLoaderWithNewURLSoon to do the image loading logic.
(WebCore::HTMLEmbedElement::updateWidget): Rearranged logic so setNeedsWidgetUpdate
is only called when it's becoming false; avoids a false/true/false round trip that can
cause trouble.
* html/HTMLEmbedElement.h: Updated for the above.
* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::setReadyState): Call setShouldDelayLoadEvent(false) when
transitioning to HAVE_CURRENT_DATA (or beyond), even if we have already fired a loadeddata
event in the past. This matches what the HTML specification calls for, but only if you
read it carefully. Without this change, and with the more complete implementation of
load event delay below, one of the regression tests hangs because are permanently stuck
dealying load events. Also added a FIXME about other code that likely has a similar
problem; the symptom is likely to be subtle and minor, though.
* html/HTMLObjectElement.cpp:
(WebCore::HTMLObjectElement::HTMLObjectElement): Removed the createdByParser argument,
no longer needed by the base class.
(WebCore::HTMLObjectElement::create): Added call to finishCreating, which is now part of
the process of creating any object in a class derived from HTMLPlugInImageElement.
(WebCore::HTMLObjectElement::parseAttribute): Changed dataAttr to use
updateImageLoaderWithNewURLSoon. Explicitly call scheduleUpdateForAfterStyleResolution
since just calling invalidateStyleAndRenderersForSubtree alone is no longer sufficient.
(WebCore::HTMLObjectElement::updateWidget): Rearranged logic so setNeedsWidgetUpdate
is only called when it's becoming false; avoids a false/true/false round trip that can
cause trouble.
(WebCore::HTMLObjectElement::childrenChanged): Added calls to the new
scheduleUpdateForAfterStyleResolution since invalidating style is no longer sufficient.
(WebCore::HTMLObjectElement::renderFallbackContent): Remove the call to
updateStyleIfNeeded. This is the main change that the title of this bug refers to.
* html/HTMLObjectElement.h: Updated for the above. Also removed the
clearUseFallbackContent function because it's clearer to set the data member in
line at the single call site in HTMLObjectElement::parseAttribute.
* html/HTMLPlugInImageElement.cpp:
(WebCore::HTMLPlugInImageElement::HTMLPlugInImageElement): Removed the createdByParser
argument; no need to set an m_needsWidgetUpdate flag differently for parser cases now.
(WebCore::HTMLPlugInImageElement::finshCreating): Added. To be called after creating
an element to do work that can't be done in a constructor.
(WebCore::HTMLPlugInImageElement::didRecalcStyle): Added. Calls the new
scheduleUpdateForAfterStyleResolution function.
(WebCore::HTMLPlugInImageElement::didAttachRenderers): Moved all the logic from this
function into scheduleUpdateForAfterStyleResolution. Also added a call through to the base
class; cleans things up, even though it's just an assertion.
(WebCore::HTMLPlugInImageElement::willDetachRenderers): Removed the call to
setNeedsWidgetUpdate(true) here; no longer needed because the new logic already
does the right thing in this case.
(WebCore::HTMLPlugInImageElement::updateWidgetIfNecessary): Deleted. Now handled by
updateAfterStyleResolution instead.
(WebCore::HTMLPlugInImageElement::finishParsingChildren): Deleted. Handling updates
after parsing all the children now comes naturally out of the new implementation.
(WebCore::HTMLPlugInImageElement::scheduleUpdateForAfterStyleResolution): Added.
Schedules a call to updateAfterStyleResolution when needed, and equally importantly,
increments the load event delay count to make sure that loads that are part of that
update can participate in decision about whether it's time for the load event.
(WebCore::HTMLPlugInImageElement::updateAfterStyleResolution): Added.
Combines updateWidgetIfNecessary and startLoadingImage, and also deals with the new
m_needsImageReload boolean in cases where no actual loading is done.
(WebCore::HTMLPlugInImageElement::didMoveToNewDocument): Update load event delay
count when moving an element that is in the middle of loading. This lets the
updateAfterStyleResolution function do the right thing even when the element is
moved without leaving anything stuck in a strange state.
(WebCore::HTMLPlugInImageElement::prepareForDocumentSuspension): Call the new
scheduleUpdateForAfterStyleResolution since invalidating style is no longer sufficient.
(WebCore::HTMLPlugInImageElement::startLoadingImage): Deleted. Now handled by
updateAfterStyleResolution instead.
(WebCore::HTMLPlugInImageElement::updateImageLoaderWithNewURLSoon): Added. Does all
the right things for when an image URL is changed; for use by the concrete derived classes.
* html/HTMLPlugInImageElement.h: Updated for above changes. Also made m_imageLoader
private rather than protected, and added the two new boolean data members.
* html/HTMLTagNames.in: Removed unneeded constructorNeedsCreatedByParser flags for
applet, embed, and object.
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::isLoadingInAPISense): Return true if the document is
delaying a load event.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::checkLoadCompleteForThisFrame): Don't do any work if
isDelayingLoadEvent is true; otherwise this function can have a side effect of
triggering the load event.
(WebCore::FrameLoader::detachFromParent): Schedule a checkLoadComplete here, too, not
just a checkCompleted. This is relevant if the frame we are detaching was delaying
a load event because it no longer will be and so the load might be complete.
Tools:
* WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:
(WTR::InjectedBundlePage::didFinishLoadForFrame): Omit now-unneeded "shouldDump" argument
when calling frameDidChangeLocation.
(WTR::InjectedBundlePage::frameDidChangeLocation): Removed "shouldDump" argument. This was
causing WebKitTestRunner to not dump anything in cases where DumpRenderTree will dump, and
thus causing mysterious failures of a couple of tests. There are two remaining issues:
1) WebKitTestRunner won't run its dump code if there is no "page", and there is no such
consideration in DumpRenderTree and 2) Both DumpRenderTree and WebKitTestRunner share the
same logic flaw that causes "top loading frame" to get set to one of the subframes in
tests where the following sequence occurs: test calls waitUntilDone, main frame finishes
loading, subframe starts loading. It would be good to clean that up some day, but for now
this patch makes the two work identically rather than changing both.
* WebKitTestRunner/InjectedBundle/InjectedBundlePage.h: Updated for change above.
LayoutTests:
* fast/text/international/embed-bidi-style-in-isolate-crash.html: Removed onerror attribute
in the audio element in this test. The error event does fire during the test, which causes
the test to fail. Before, the test was prematurely exiting before the load failed, preventing
the test from failing, but also meaning we didn't finish running the test.
* imported/blink/fast/dom/Window/open-window-features-fuzz.html: Use waitUntilDone and
notifyDone to prevent the test from exiting prematurely. Use a URL that won't trigger loading
outside the web browser; the URL is not what mattered to this test. Before, the test was
prematurely exiting before the test ran. Note also, that I don't think this is testing
much effectively; not sure we are getting any benefit from this test since before it was
not really running to completion anyway.
* media/event-queue-crash-expected.txt: Updated expectations to expect syntax error. Before
there was a race and often the test exited before the syntax error could be logged.
* platform/mac/TestExpectations: Removed flakiness expectation from the
media/event-queue-crash.html test. What made it flaky was a race with the load event,
and that race should be fixed by the change to FrameLoader::checkLoadCompleteForThisFrame.
The same race existed on all platforms, not just Mac, so this flakiness expectation should
be in the main TextExpectations file if anywhere. But I believe it is not needed at all.
For media/modern-media-controls/media-documents/background-color-and-centering.html,
added image failure expectation because under modern WebKit on Mac the image now captures
the upper left hand corner of the controls overlay. Still seems to pass on iOS and the bug
this was created for was iOS-specific, so should be OK for now.
* webarchive/loading/video-in-webarchive-expected.txt: Updated. The old result shows evidence
of a premature load event, fixed by the change to FrameLoader::checkLoadCompleteForThisFrame.
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (220051 => 220052)
--- trunk/LayoutTests/ChangeLog 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/LayoutTests/ChangeLog 2017-07-30 21:44:01 UTC (rev 220052)
@@ -1,3 +1,38 @@
+2017-07-30 Darin Adler <da...@apple.com>
+
+ Remove code in HTMLObjectElement attribute parsing that forces style resolution and layout
+ https://bugs.webkit.org/show_bug.cgi?id=130653
+
+ Reviewed by Antti Koivisto.
+
+ * fast/text/international/embed-bidi-style-in-isolate-crash.html: Removed onerror attribute
+ in the audio element in this test. The error event does fire during the test, which causes
+ the test to fail. Before, the test was prematurely exiting before the load failed, preventing
+ the test from failing, but also meaning we didn't finish running the test.
+
+ * imported/blink/fast/dom/Window/open-window-features-fuzz.html: Use waitUntilDone and
+ notifyDone to prevent the test from exiting prematurely. Use a URL that won't trigger loading
+ outside the web browser; the URL is not what mattered to this test. Before, the test was
+ prematurely exiting before the test ran. Note also, that I don't think this is testing
+ much effectively; not sure we are getting any benefit from this test since before it was
+ not really running to completion anyway.
+
+ * media/event-queue-crash-expected.txt: Updated expectations to expect syntax error. Before
+ there was a race and often the test exited before the syntax error could be logged.
+
+ * platform/mac/TestExpectations: Removed flakiness expectation from the
+ media/event-queue-crash.html test. What made it flaky was a race with the load event,
+ and that race should be fixed by the change to FrameLoader::checkLoadCompleteForThisFrame.
+ The same race existed on all platforms, not just Mac, so this flakiness expectation should
+ be in the main TextExpectations file if anywhere. But I believe it is not needed at all.
+ For media/modern-media-controls/media-documents/background-color-and-centering.html,
+ added image failure expectation because under modern WebKit on Mac the image now captures
+ the upper left hand corner of the controls overlay. Still seems to pass on iOS and the bug
+ this was created for was iOS-specific, so should be OK for now.
+
+ * webarchive/loading/video-in-webarchive-expected.txt: Updated. The old result shows evidence
+ of a premature load event, fixed by the change to FrameLoader::checkLoadCompleteForThisFrame.
+
2017-07-30 Sam Weinig <s...@webkit.org>
[WebIDL] Remove JS builtin bindings for FetchRequest, DOMWindowFetch and WorkerGlobalScopeFetch
Modified: trunk/LayoutTests/fast/text/international/embed-bidi-style-in-isolate-crash.html (220051 => 220052)
--- trunk/LayoutTests/fast/text/international/embed-bidi-style-in-isolate-crash.html 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/LayoutTests/fast/text/international/embed-bidi-style-in-isolate-crash.html 2017-07-30 21:44:01 UTC (rev 220052)
@@ -2,7 +2,7 @@
<ruby>PASS, if no exception or crash in debug</ruby>
<em dir="ltr">
<embed></embed>
- <audio _onerror_="open()" src=""
+ <audio src=""
</em>
</bdi>
<script>
Modified: trunk/LayoutTests/imported/blink/fast/dom/Window/open-window-features-fuzz.html (220051 => 220052)
--- trunk/LayoutTests/imported/blink/fast/dom/Window/open-window-features-fuzz.html 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/LayoutTests/imported/blink/fast/dom/Window/open-window-features-fuzz.html 2017-07-30 21:44:01 UTC (rev 220052)
@@ -1,15 +1,18 @@
<script>
if (window.testRunner) {
+ testRunner.waitUntilDone();
testRunner.dumpAsText();
testRunner.setCanOpenWindows();
}
function handler() {
- var url = '';
+ var url = '';
var name = 'listing';
var features = unescape('%udc10%u0130%u07b2%u037c%ufb3b%u3bf0%u3e4d%u16c3%u3c4f%u3fe7%u12b3%ub73c%u6c25%u938d%u645c%u2470%udbc3%ud2d4');
window.open(url, name, features);
+ if (window.testRunner)
+ testRunner.notifyDone();
}
</script>
Modified: trunk/LayoutTests/media/event-queue-crash-expected.txt (220051 => 220052)
--- trunk/LayoutTests/media/event-queue-crash-expected.txt 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/LayoutTests/media/event-queue-crash-expected.txt 2017-07-30 21:44:01 UTC (rev 220052)
@@ -1,3 +1,4 @@
+CONSOLE MESSAGE: line 34: SyntaxError: Unexpected token '}'. Expected '(' to start an 'if' condition.
When an element containing video is removed, WebKit should not crash.
PASS. WebKit didn't crash.
Modified: trunk/LayoutTests/platform/mac/TestExpectations (220051 => 220052)
--- trunk/LayoutTests/platform/mac/TestExpectations 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/LayoutTests/platform/mac/TestExpectations 2017-07-30 21:44:01 UTC (rev 220052)
@@ -867,7 +867,6 @@
webkit.org/b/141294 compositing/reflections/masked-reflection-on-composited.html [ Pass Crash ]
webkit.org/b/142152 media/track/track-in-band-cues-added-once.html [ Failure Timeout ]
webkit.org/b/147944 media/video-seek-to-current-time.html [ Pass Failure ]
-webkit.org/b/114177 media/event-queue-crash.html [ Pass Failure ]
webkit.org/b/150408 http/tests/media/video-load-suspend.html [ Pass Timeout ]
webkit.org/b/155956 media/track/track-remove-track.html [ Pass Failure Timeout ]
webkit.org/b/124222 [ Yosemite ] media/track/track-in-band-duplicate-tracks-when-source-changes.html [ Pass Crash Timeout ]
@@ -1602,6 +1601,11 @@
media/modern-media-controls/media-documents/media-document-video-ios-sizing.html [ Skip ]
media/modern-media-controls/scrubber-support/ipad [ Skip ]
+# This test relies on the control overlay not being visible in the top left. But the test now fails on Mac because
+# the image is dumped after the video is loaded and the control overlay is displayed. It seems the test still works
+# on iOS, and the bug it was created to regression-test was iOS-only, so there is no hurry to make it work on Mac.
+media/modern-media-controls/media-documents/background-color-and-centering.html [ Pass ImageOnlyFailure ]
+
# These tests use Picture-in-Picture which isn't supported on El Capitan.
[ ElCapitan ] media/modern-media-controls/placard-support/placard-support-pip.html [ Skip ]
media/modern-media-controls/pip-support/ipad [ Skip ]
Modified: trunk/LayoutTests/webarchive/loading/video-in-webarchive-expected.txt (220051 => 220052)
--- trunk/LayoutTests/webarchive/loading/video-in-webarchive-expected.txt 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/LayoutTests/webarchive/loading/video-in-webarchive-expected.txt 2017-07-30 21:44:01 UTC (rev 220052)
@@ -10,7 +10,8 @@
main frame - didFinishDocumentLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didFinishDocumentLoadForFrame
+frame "<!--framePath //<!--frame0-->-->" - didHandleOnloadEventsForFrame
+main frame - didHandleOnloadEventsForFrame
frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
main frame - didFinishLoadForFrame
-frame "<!--framePath //<!--frame0-->-->" - didHandleOnloadEventsForFrame
Modified: trunk/Source/WebCore/ChangeLog (220051 => 220052)
--- trunk/Source/WebCore/ChangeLog 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/ChangeLog 2017-07-30 21:44:01 UTC (rev 220052)
@@ -1,3 +1,121 @@
+2017-07-30 Darin Adler <da...@apple.com>
+
+ Remove code in HTMLObjectElement attribute parsing that forces style resolution and layout
+ https://bugs.webkit.org/show_bug.cgi?id=130653
+
+ Reviewed by Antti Koivisto.
+
+ Also fixes a bug where load events are delivered prematurely in some cases
+ when an object, embed, frame, or iframe element is still loading.
+
+ * dom/Document.cpp:
+ (WebCore::Document::loadEventDelayTimerFired): Added a call to
+ FrameLoader::checkLoadComplete. Goes along with the change to
+ FrameLoader::checkLoadCompleteForThisFrame, which now respects the
+ isDelayingLoadEvent flag.
+
+ * html/HTMLAppletElement.cpp:
+ (WebCore::HTMLAppletElement::HTMLAppletElement): Removed the createdByParser argument,
+ no longer needed by the base class.
+ (WebCore::HTMLAppletElement::create): Added call to finishCreating, which is now part of
+ the process of creating any object in a class derived from HTMLPlugInImageElement.
+ (WebCore::HTMLAppletElement::updateWidget): Rearranged logic so setNeedsWidgetUpdate
+ is only called when it's becoming false; avoids a false/true/false round trip that can
+ cause trouble.
+ * html/HTMLAppletElement.h: Updated for the above.
+
+ * html/HTMLEmbedElement.cpp:
+ (WebCore::HTMLEmbedElement::HTMLEmbedElement): Removed the createdByParser argument,
+ no longer needed by the base class.
+ (WebCore::HTMLEmbedElement::create): Added call to finishCreating, which is now part of
+ the process of creating any object in a class derived from HTMLPlugInImageElement.
+ (WebCore::HTMLEmbedElement::parseAttribute): Changed srcAttr to call
+ updateImageLoaderWithNewURLSoon to do the image loading logic.
+ (WebCore::HTMLEmbedElement::updateWidget): Rearranged logic so setNeedsWidgetUpdate
+ is only called when it's becoming false; avoids a false/true/false round trip that can
+ cause trouble.
+ * html/HTMLEmbedElement.h: Updated for the above.
+
+ * html/HTMLMediaElement.cpp:
+ (WebCore::HTMLMediaElement::setReadyState): Call setShouldDelayLoadEvent(false) when
+ transitioning to HAVE_CURRENT_DATA (or beyond), even if we have already fired a loadeddata
+ event in the past. This matches what the HTML specification calls for, but only if you
+ read it carefully. Without this change, and with the more complete implementation of
+ load event delay below, one of the regression tests hangs because are permanently stuck
+ dealying load events. Also added a FIXME about other code that likely has a similar
+ problem; the symptom is likely to be subtle and minor, though.
+
+ * html/HTMLObjectElement.cpp:
+ (WebCore::HTMLObjectElement::HTMLObjectElement): Removed the createdByParser argument,
+ no longer needed by the base class.
+ (WebCore::HTMLObjectElement::create): Added call to finishCreating, which is now part of
+ the process of creating any object in a class derived from HTMLPlugInImageElement.
+ (WebCore::HTMLObjectElement::parseAttribute): Changed dataAttr to use
+ updateImageLoaderWithNewURLSoon. Explicitly call scheduleUpdateForAfterStyleResolution
+ since just calling invalidateStyleAndRenderersForSubtree alone is no longer sufficient.
+ (WebCore::HTMLObjectElement::updateWidget): Rearranged logic so setNeedsWidgetUpdate
+ is only called when it's becoming false; avoids a false/true/false round trip that can
+ cause trouble.
+ (WebCore::HTMLObjectElement::childrenChanged): Added calls to the new
+ scheduleUpdateForAfterStyleResolution since invalidating style is no longer sufficient.
+ (WebCore::HTMLObjectElement::renderFallbackContent): Remove the call to
+ updateStyleIfNeeded. This is the main change that the title of this bug refers to.
+ * html/HTMLObjectElement.h: Updated for the above. Also removed the
+ clearUseFallbackContent function because it's clearer to set the data member in
+ line at the single call site in HTMLObjectElement::parseAttribute.
+
+ * html/HTMLPlugInImageElement.cpp:
+ (WebCore::HTMLPlugInImageElement::HTMLPlugInImageElement): Removed the createdByParser
+ argument; no need to set an m_needsWidgetUpdate flag differently for parser cases now.
+ (WebCore::HTMLPlugInImageElement::finshCreating): Added. To be called after creating
+ an element to do work that can't be done in a constructor.
+ (WebCore::HTMLPlugInImageElement::didRecalcStyle): Added. Calls the new
+ scheduleUpdateForAfterStyleResolution function.
+ (WebCore::HTMLPlugInImageElement::didAttachRenderers): Moved all the logic from this
+ function into scheduleUpdateForAfterStyleResolution. Also added a call through to the base
+ class; cleans things up, even though it's just an assertion.
+ (WebCore::HTMLPlugInImageElement::willDetachRenderers): Removed the call to
+ setNeedsWidgetUpdate(true) here; no longer needed because the new logic already
+ does the right thing in this case.
+ (WebCore::HTMLPlugInImageElement::updateWidgetIfNecessary): Deleted. Now handled by
+ updateAfterStyleResolution instead.
+ (WebCore::HTMLPlugInImageElement::finishParsingChildren): Deleted. Handling updates
+ after parsing all the children now comes naturally out of the new implementation.
+ (WebCore::HTMLPlugInImageElement::scheduleUpdateForAfterStyleResolution): Added.
+ Schedules a call to updateAfterStyleResolution when needed, and equally importantly,
+ increments the load event delay count to make sure that loads that are part of that
+ update can participate in decision about whether it's time for the load event.
+ (WebCore::HTMLPlugInImageElement::updateAfterStyleResolution): Added.
+ Combines updateWidgetIfNecessary and startLoadingImage, and also deals with the new
+ m_needsImageReload boolean in cases where no actual loading is done.
+ (WebCore::HTMLPlugInImageElement::didMoveToNewDocument): Update load event delay
+ count when moving an element that is in the middle of loading. This lets the
+ updateAfterStyleResolution function do the right thing even when the element is
+ moved without leaving anything stuck in a strange state.
+ (WebCore::HTMLPlugInImageElement::prepareForDocumentSuspension): Call the new
+ scheduleUpdateForAfterStyleResolution since invalidating style is no longer sufficient.
+ (WebCore::HTMLPlugInImageElement::startLoadingImage): Deleted. Now handled by
+ updateAfterStyleResolution instead.
+ (WebCore::HTMLPlugInImageElement::updateImageLoaderWithNewURLSoon): Added. Does all
+ the right things for when an image URL is changed; for use by the concrete derived classes.
+ * html/HTMLPlugInImageElement.h: Updated for above changes. Also made m_imageLoader
+ private rather than protected, and added the two new boolean data members.
+
+ * html/HTMLTagNames.in: Removed unneeded constructorNeedsCreatedByParser flags for
+ applet, embed, and object.
+
+ * loader/DocumentLoader.cpp:
+ (WebCore::DocumentLoader::isLoadingInAPISense): Return true if the document is
+ delaying a load event.
+
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::checkLoadCompleteForThisFrame): Don't do any work if
+ isDelayingLoadEvent is true; otherwise this function can have a side effect of
+ triggering the load event.
+ (WebCore::FrameLoader::detachFromParent): Schedule a checkLoadComplete here, too, not
+ just a checkCompleted. This is relevant if the frame we are detaching was delaying
+ a load event because it no longer will be and so the load might be complete.
+
2017-07-30 Sam Weinig <s...@webkit.org>
[WebIDL] Remove JS builtin bindings for FetchRequest, DOMWindowFetch and WorkerGlobalScopeFetch
Modified: trunk/Source/WebCore/dom/Document.cpp (220051 => 220052)
--- trunk/Source/WebCore/dom/Document.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/dom/Document.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -5160,7 +5160,7 @@
if (!m_documentTiming.domContentLoadedEventEnd)
m_documentTiming.domContentLoadedEventEnd = MonotonicTime::now();
- if (RefPtr<Frame> f = frame()) {
+ if (RefPtr<Frame> frame = this->frame()) {
// FrameLoader::finishedParsing() might end up calling Document::implicitClose() if all
// resource loads are complete. HTMLObjectElements can start loading their resources from
// post attach callbacks triggered by resolveStyle(). This means if we parse out an <object>
@@ -5170,9 +5170,8 @@
// See https://bugs.webkit.org/show_bug.cgi?id=36864 starting around comment 35.
updateStyleIfNeeded();
- f->loader().finishedParsing();
-
- InspectorInstrumentation::domContentLoadedEventFired(*f);
+ frame->loader().finishedParsing();
+ InspectorInstrumentation::domContentLoadedEventFired(*frame);
}
// Schedule dropping of the DocumentSharedObjectPool. We keep it alive for a while after parsing finishes
@@ -6223,7 +6222,13 @@
void Document::loadEventDelayTimerFired()
{
+ // FIXME: Should the call to FrameLoader::checkLoadComplete be moved inside Document::checkCompleted?
+ // FIXME: Should this also call DocumentLoader::checkLoadComplete?
+ // FIXME: Not obvious why checkCompleted needs to go first. The order these are called is
+ // visible to WebKit clients, but it's more like a race than a well-defined relationship.
checkCompleted();
+ if (auto* frame = this->frame())
+ frame->loader().checkLoadComplete();
}
void Document::checkCompleted()
@@ -6606,7 +6611,7 @@
if (!frame())
return;
- // FIXME: We should call loader()->checkLoadComplete() as well here,
+ // FIXME: We should call DocumentLoader::checkLoadComplete as well here,
// but it seems to cause http/tests/security/feed-urls-from-remote.html
// to timeout on Mac WK1; see http://webkit.org/b/110554 and http://webkit.org/b/110401.
frame()->loader().checkLoadComplete();
Modified: trunk/Source/WebCore/html/HTMLAppletElement.cpp (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLAppletElement.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLAppletElement.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -40,17 +40,19 @@
using namespace HTMLNames;
-HTMLAppletElement::HTMLAppletElement(const QualifiedName& tagName, Document& document, bool createdByParser)
- : HTMLPlugInImageElement(tagName, document, createdByParser)
+inline HTMLAppletElement::HTMLAppletElement(const QualifiedName& tagName, Document& document)
+ : HTMLPlugInImageElement(tagName, document)
{
ASSERT(hasTagName(appletTag));
- m_serviceType = "application/x-java-applet";
+ m_serviceType = ASCIILiteral { "application/x-java-applet" };
}
-Ref<HTMLAppletElement> HTMLAppletElement::create(const QualifiedName& tagName, Document& document, bool createdByParser)
+Ref<HTMLAppletElement> HTMLAppletElement::create(const QualifiedName& tagName, Document& document)
{
- return adoptRef(*new HTMLAppletElement(tagName, document, createdByParser));
+ auto result = adoptRef(*new HTMLAppletElement(tagName, document));
+ result->finishCreating();
+ return result;
}
void HTMLAppletElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
@@ -104,10 +106,11 @@
void HTMLAppletElement::updateWidget(CreatePlugins createPlugins)
{
- setNeedsWidgetUpdate(false);
// FIXME: This should ASSERT isFinishedParsingChildren() instead.
- if (!isFinishedParsingChildren())
+ if (!isFinishedParsingChildren()) {
+ setNeedsWidgetUpdate(false);
return;
+ }
#if PLATFORM(IOS)
UNUSED_PARAM(createPlugins);
@@ -115,12 +118,11 @@
// FIXME: It's sadness that we have this special case here.
// See http://trac.webkit.org/changeset/25128 and
// plugins/netscape-plugin-setwindow-size.html
- if (createPlugins == CreatePlugins::No) {
- // Ensure updateWidget() is called again during layout to create the plug-in.
- setNeedsWidgetUpdate(true);
+ if (createPlugins == CreatePlugins::No)
return;
- }
+ setNeedsWidgetUpdate(false);
+
RenderEmbeddedObject* renderer = renderEmbeddedObject();
LayoutUnit contentWidth = renderer->style().width().isFixed() ? LayoutUnit(renderer->style().width().value()) :
Modified: trunk/Source/WebCore/html/HTMLAppletElement.h (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLAppletElement.h 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLAppletElement.h 2017-07-30 21:44:01 UTC (rev 220052)
@@ -28,10 +28,10 @@
class HTMLAppletElement final : public HTMLPlugInImageElement {
public:
- static Ref<HTMLAppletElement> create(const QualifiedName&, Document&, bool createdByParser);
+ static Ref<HTMLAppletElement> create(const QualifiedName&, Document&);
private:
- HTMLAppletElement(const QualifiedName&, Document&, bool createdByParser);
+ HTMLAppletElement(const QualifiedName&, Document&);
void parseAttribute(const QualifiedName&, const AtomicString&) final;
bool isURLAttribute(const Attribute&) const final;
Modified: trunk/Source/WebCore/html/HTMLEmbedElement.cpp (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLEmbedElement.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLEmbedElement.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -43,20 +43,22 @@
using namespace HTMLNames;
-inline HTMLEmbedElement::HTMLEmbedElement(const QualifiedName& tagName, Document& document, bool createdByParser)
- : HTMLPlugInImageElement(tagName, document, createdByParser)
+inline HTMLEmbedElement::HTMLEmbedElement(const QualifiedName& tagName, Document& document)
+ : HTMLPlugInImageElement(tagName, document)
{
ASSERT(hasTagName(embedTag));
}
-Ref<HTMLEmbedElement> HTMLEmbedElement::create(const QualifiedName& tagName, Document& document, bool createdByParser)
+Ref<HTMLEmbedElement> HTMLEmbedElement::create(const QualifiedName& tagName, Document& document)
{
- return adoptRef(*new HTMLEmbedElement(tagName, document, createdByParser));
+ auto result = adoptRef(*new HTMLEmbedElement(tagName, document));
+ result->finishCreating();
+ return result;
}
Ref<HTMLEmbedElement> HTMLEmbedElement::create(Document& document)
{
- return adoptRef(*new HTMLEmbedElement(embedTag, document, false));
+ return create(embedTag, document);
}
static inline RenderWidget* findWidgetRenderer(const Node* node)
@@ -112,16 +114,11 @@
// this up to the HTMLPlugInImageElement to be shared.
} else if (name == codeAttr) {
m_url = stripLeadingAndTrailingHTMLSpaces(value);
- // FIXME: Why no call to the image loader?
+ // FIXME: Why no call to updateImageLoaderWithNewURLSoon?
// FIXME: If both code and src attributes are specified, last one parsed/changed wins. That can't be right!
} else if (name == srcAttr) {
m_url = stripLeadingAndTrailingHTMLSpaces(value);
- document().updateStyleIfNeeded();
- if (renderer() && isImageType()) {
- if (!m_imageLoader)
- m_imageLoader = std::make_unique<HTMLImageLoader>(*this);
- m_imageLoader->updateFromElementIgnoringPreviousError();
- }
+ updateImageLoaderWithNewURLSoon();
// FIXME: If both code and src attributes are specified, last one parsed/changed wins. That can't be right!
} else
HTMLPlugInImageElement::parseAttribute(name, value);
@@ -144,25 +141,27 @@
{
ASSERT(!renderEmbeddedObject()->isPluginUnavailable());
ASSERT(needsWidgetUpdate());
- setNeedsWidgetUpdate(false);
- if (m_url.isEmpty() && m_serviceType.isEmpty())
+ if (m_url.isEmpty() && m_serviceType.isEmpty()) {
+ setNeedsWidgetUpdate(false);
return;
+ }
// Note these pass m_url and m_serviceType to allow better code sharing with
// <object> which modifies url and serviceType before calling these.
- if (!allowedToLoadFrameURL(m_url))
+ if (!allowedToLoadFrameURL(m_url)) {
+ setNeedsWidgetUpdate(false);
return;
+ }
// FIXME: It's sadness that we have this special case here.
// See http://trac.webkit.org/changeset/25128 and
// plugins/netscape-plugin-setwindow-size.html
- if (createPlugins == CreatePlugins::No && wouldLoadAsPlugIn(m_url, m_serviceType)) {
- // Ensure updateWidget() is called again during layout to create the Netscape plug-in.
- setNeedsWidgetUpdate(true);
+ if (createPlugins == CreatePlugins::No && wouldLoadAsPlugIn(m_url, m_serviceType))
return;
- }
+ setNeedsWidgetUpdate(false);
+
// FIXME: These should be joined into a PluginParameters class.
Vector<String> paramNames;
Vector<String> paramValues;
Modified: trunk/Source/WebCore/html/HTMLEmbedElement.h (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLEmbedElement.h 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLEmbedElement.h 2017-07-30 21:44:01 UTC (rev 220052)
@@ -29,10 +29,10 @@
class HTMLEmbedElement final : public HTMLPlugInImageElement {
public:
static Ref<HTMLEmbedElement> create(Document&);
- static Ref<HTMLEmbedElement> create(const QualifiedName&, Document&, bool createdByParser);
+ static Ref<HTMLEmbedElement> create(const QualifiedName&, Document&);
private:
- HTMLEmbedElement(const QualifiedName&, Document&, bool createdByParser);
+ HTMLEmbedElement(const QualifiedName&, Document&);
void parseAttribute(const QualifiedName&, const AtomicString&) final;
bool isPresentationAttribute(const QualifiedName&) const final;
Modified: trunk/Source/WebCore/html/HTMLMediaElement.cpp (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLMediaElement.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLMediaElement.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -2415,12 +2415,17 @@
bool shouldUpdateDisplayState = false;
- if (m_readyState >= HAVE_CURRENT_DATA && oldState < HAVE_CURRENT_DATA && !m_haveFiredLoadedData) {
- m_haveFiredLoadedData = true;
- shouldUpdateDisplayState = true;
- scheduleEvent(eventNames().loadeddataEvent);
+ if (m_readyState >= HAVE_CURRENT_DATA && oldState < HAVE_CURRENT_DATA) {
+ if (!m_haveFiredLoadedData) {
+ m_haveFiredLoadedData = true;
+ scheduleEvent(eventNames().loadeddataEvent);
+ // FIXME: It's not clear that it's correct to skip these two operations just
+ // because m_haveFiredLoadedData is already true. At one time we were skipping
+ // the call to setShouldDelayLoadEvent, which was definitely incorrect.
+ shouldUpdateDisplayState = true;
+ applyMediaFragmentURI();
+ }
setShouldDelayLoadEvent(false);
- applyMediaFragmentURI();
}
bool isPotentiallyPlaying = potentiallyPlaying();
Modified: trunk/Source/WebCore/html/HTMLObjectElement.cpp (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLObjectElement.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLObjectElement.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -60,22 +60,20 @@
using namespace HTMLNames;
-inline HTMLObjectElement::HTMLObjectElement(const QualifiedName& tagName, Document& document, HTMLFormElement* form, bool createdByParser)
- : HTMLPlugInImageElement(tagName, document, createdByParser)
+inline HTMLObjectElement::HTMLObjectElement(const QualifiedName& tagName, Document& document, HTMLFormElement* form)
+ : HTMLPlugInImageElement(tagName, document)
, FormAssociatedElement(form)
{
ASSERT(hasTagName(objectTag));
}
-inline HTMLObjectElement::~HTMLObjectElement()
+Ref<HTMLObjectElement> HTMLObjectElement::create(const QualifiedName& tagName, Document& document, HTMLFormElement* form)
{
+ auto result = adoptRef(*new HTMLObjectElement(tagName, document, form));
+ result->finishCreating();
+ return result;
}
-Ref<HTMLObjectElement> HTMLObjectElement::create(const QualifiedName& tagName, Document& document, HTMLFormElement* form, bool createdByParser)
-{
- return adoptRef(*new HTMLObjectElement(tagName, document, form, createdByParser));
-}
-
RenderWidget* HTMLObjectElement::renderWidgetLoadingPlugin() const
{
// Needs to load the plugin immediatedly because this function is called
@@ -112,14 +110,9 @@
setNeedsWidgetUpdate(true);
} else if (name == dataAttr) {
m_url = stripLeadingAndTrailingHTMLSpaces(value);
- document().updateStyleIfNeeded();
- if (isImageType() && renderer()) {
- if (!m_imageLoader)
- m_imageLoader = std::make_unique<HTMLImageLoader>(*this);
- m_imageLoader->updateFromElementIgnoringPreviousError();
- }
invalidateRenderer = !hasAttributeWithoutSynchronization(classidAttr);
setNeedsWidgetUpdate(true);
+ updateImageLoaderWithNewURLSoon();
} else if (name == classidAttr) {
invalidateRenderer = true;
setNeedsWidgetUpdate(true);
@@ -129,7 +122,8 @@
if (!invalidateRenderer || !isConnected() || !renderer())
return;
- clearUseFallbackContent();
+ m_useFallbackContent = false;
+ scheduleUpdateForAfterStyleResolution();
invalidateStyleAndRenderersForSubtree();
}
@@ -215,7 +209,6 @@
}
}
-
bool HTMLObjectElement::hasFallbackContent() const
{
for (Node* child = firstChild(); child; child = child->nextSibling()) {
@@ -271,16 +264,20 @@
{
ASSERT(!renderEmbeddedObject()->isPluginUnavailable());
ASSERT(needsWidgetUpdate());
- setNeedsWidgetUpdate(false);
+
// FIXME: This should ASSERT isFinishedParsingChildren() instead.
- if (!isFinishedParsingChildren())
+ if (!isFinishedParsingChildren()) {
+ setNeedsWidgetUpdate(false);
return;
+ }
// FIXME: I'm not sure it's ever possible to get into updateWidget during a
// removal, but just in case we should avoid loading the frame to prevent
// security bugs.
- if (!SubframeLoadingDisabler::canLoadFrame(*this))
+ if (!SubframeLoadingDisabler::canLoadFrame(*this)) {
+ setNeedsWidgetUpdate(false);
return;
+ }
String url = ""
String serviceType = this->serviceType();
@@ -291,18 +288,19 @@
parametersForPlugin(paramNames, paramValues, url, serviceType);
// Note: url is modified above by parametersForPlugin.
- if (!allowedToLoadFrameURL(url))
+ if (!allowedToLoadFrameURL(url)) {
+ setNeedsWidgetUpdate(false);
return;
+ }
// FIXME: It's sadness that we have this special case here.
// See http://trac.webkit.org/changeset/25128 and
// plugins/netscape-plugin-setwindow-size.html
- if (createPlugins == CreatePlugins::No && wouldLoadAsPlugIn(url, serviceType)) {
- // Ensure updateWidget() is called again during layout to create the Netscape plug-in.
- setNeedsWidgetUpdate(true);
+ if (createPlugins == CreatePlugins::No && wouldLoadAsPlugIn(url, serviceType))
return;
- }
+ setNeedsWidgetUpdate(false);
+
Ref<HTMLObjectElement> protectedThis(*this); // beforeload and plugin loading can make arbitrary DOM mutations.
bool beforeLoadAllowedLoad = guardedDispatchBeforeLoadEvent(url);
if (!renderer()) // Do not load the plugin if beforeload removed this element or its renderer.
@@ -338,6 +336,7 @@
updateExposedState();
if (isConnected() && !m_useFallbackContent) {
setNeedsWidgetUpdate(true);
+ scheduleUpdateForAfterStyleResolution();
invalidateStyleForSubtree();
}
HTMLPlugInImageElement::childrenChanged(change);
@@ -361,6 +360,7 @@
if (!isConnected())
return;
+ scheduleUpdateForAfterStyleResolution();
invalidateStyleAndRenderersForSubtree();
// Before we give up and use fallback content, check to see if this is a MIME type issue.
@@ -375,12 +375,6 @@
}
m_useFallbackContent = true;
-
- // This was added to keep Acid 2 non-flaky. A style recalc is required to make fallback resources load.
- // Without forcing, this may happen after all the other resources have been loaded and the document is already
- // considered complete. FIXME: Would be better to address this with incrementLoadEventDelayCount instead
- // or disentangle loading from style entirely.
- document().updateStyleIfNeeded();
}
static inline bool preventsParentObjectFromExposure(const Element& child)
@@ -516,10 +510,9 @@
bool HTMLObjectElement::canContainRangeEndPoint() const
{
- // Call through to HTMLElement because we need to skip HTMLPlugInElement
- // when calling through to the derived class since returns false unconditionally.
- // An object element with fallback content should basically be treated like
- // a generic HTML element.
+ // Call through to HTMLElement because HTMLPlugInElement::canContainRangeEndPoint
+ // returns false unconditionally. An object element using fallback content is
+ // treated like a generic HTML element.
return m_useFallbackContent && HTMLElement::canContainRangeEndPoint();
}
Modified: trunk/Source/WebCore/html/HTMLObjectElement.h (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLObjectElement.h 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLObjectElement.h 2017-07-30 21:44:01 UTC (rev 220052)
@@ -31,8 +31,7 @@
class HTMLObjectElement final : public HTMLPlugInImageElement, public FormAssociatedElement {
public:
- static Ref<HTMLObjectElement> create(const QualifiedName&, Document&, HTMLFormElement*, bool createdByParser);
- virtual ~HTMLObjectElement();
+ static Ref<HTMLObjectElement> create(const QualifiedName&, Document&, HTMLFormElement*);
bool isExposed() const { return m_isExposed; }
bool containsJavaApplet() const;
@@ -57,7 +56,7 @@
HTMLFormElement* form() const final { return FormAssociatedElement::form(); }
private:
- HTMLObjectElement(const QualifiedName&, Document&, HTMLFormElement*, bool createdByParser);
+ HTMLObjectElement(const QualifiedName&, Document&, HTMLFormElement*);
void parseAttribute(const QualifiedName&, const AtomicString&) final;
bool isPresentationAttribute(const QualifiedName&) const final;
@@ -87,7 +86,6 @@
bool shouldAllowQuickTimeClassIdQuirk();
bool hasValidClassId();
- void clearUseFallbackContent() { m_useFallbackContent = false; }
void refFormAssociatedElement() final { ref(); }
void derefFormAssociatedElement() final { deref(); }
Modified: trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLPlugInImageElement.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -92,9 +92,8 @@
}).iterator->value;
};
-HTMLPlugInImageElement::HTMLPlugInImageElement(const QualifiedName& tagName, Document& document, bool createdByParser)
+HTMLPlugInImageElement::HTMLPlugInImageElement(const QualifiedName& tagName, Document& document)
: HTMLPlugInElement(tagName, document)
- , m_needsWidgetUpdate(!createdByParser) // Set true in finishParsingChildren.
, m_simulatedMouseClickTimer(*this, &HTMLPlugInImageElement::simulatedMouseClickTimerFired, simulatedMouseClickTimerDelay)
, m_removeSnapshotTimer(*this, &HTMLPlugInImageElement::removeSnapshotTimerFired)
, m_createdDuringUserGesture(ScriptController::processingUserGesture())
@@ -102,6 +101,11 @@
setHasCustomStyleResolveCallbacks();
}
+void HTMLPlugInImageElement::finishCreating()
+{
+ scheduleUpdateForAfterStyleResolution();
+}
+
HTMLPlugInImageElement::~HTMLPlugInImageElement()
{
if (m_needsDocumentActivationCallbacks)
@@ -205,39 +209,27 @@
// FIXME: There shoudn't be need to force render tree reconstruction here.
// It is only done because loading and load event dispatching is tied to render tree construction.
- if (!useFallbackContent() && needsWidgetUpdate() && renderer() && !isImageType() && (displayState() != DisplayingSnapshot))
+ if (!useFallbackContent() && needsWidgetUpdate() && renderer() && !isImageType() && displayState() != DisplayingSnapshot)
invalidateStyleAndRenderersForSubtree();
}
+void HTMLPlugInImageElement::didRecalcStyle(Style::Change styleChange)
+{
+ scheduleUpdateForAfterStyleResolution();
+
+ HTMLPlugInElement::didRecalcStyle(styleChange);
+}
+
void HTMLPlugInImageElement::didAttachRenderers()
{
- if (!isImageType()) {
- RefPtr<HTMLPlugInImageElement> element = this;
- Style::queuePostResolutionCallback([element]{
- element->updateWidgetIfNecessary();
- });
- return;
- }
- if (!renderer() || useFallbackContent())
- return;
+ m_needsWidgetUpdate = true;
+ scheduleUpdateForAfterStyleResolution();
- // Image load might complete synchronously and cause us to re-enter.
- RefPtr<HTMLPlugInImageElement> element = this;
- Style::queuePostResolutionCallback([element]{
- element->startLoadingImage();
- });
+ HTMLPlugInElement::didAttachRenderers();
}
void HTMLPlugInImageElement::willDetachRenderers()
{
- // FIXME: Because of the insanity that is HTMLPlugInImageElement::willRecalcStyle,
- // we can end up detaching during an attach() call, before we even have a
- // renderer. In that case, don't mark the widget for update.
- if (renderer() && !useFallbackContent()) {
- // Update the widget the next time we attach (detaching destroys the plugin).
- setNeedsWidgetUpdate(true);
- }
-
auto* widget = pluginWidget(PluginLoadingPolicy::DoNotLoad);
if (is<PluginViewBase>(widget))
downcast<PluginViewBase>(*widget).willDetatchRenderer();
@@ -245,30 +237,47 @@
HTMLPlugInElement::willDetachRenderers();
}
-void HTMLPlugInImageElement::updateWidgetIfNecessary()
+void HTMLPlugInImageElement::scheduleUpdateForAfterStyleResolution()
{
- document().updateStyleIfNeeded();
-
- if (!needsWidgetUpdate() || useFallbackContent() || isImageType())
+ if (m_hasUpdateScheduledForAfterStyleResolution)
return;
- if (!renderEmbeddedObject() || renderEmbeddedObject()->isPluginUnavailable())
- return;
+ document().incrementLoadEventDelayCount();
- updateWidget(CreatePlugins::No);
+ m_hasUpdateScheduledForAfterStyleResolution = true;
+
+ Style::queuePostResolutionCallback([protectedThis = makeRef(*this)] {
+ protectedThis->updateAfterStyleResolution();
+ });
}
-void HTMLPlugInImageElement::finishParsingChildren()
+void HTMLPlugInImageElement::updateAfterStyleResolution()
{
- HTMLPlugInElement::finishParsingChildren();
- if (useFallbackContent())
- return;
+ m_hasUpdateScheduledForAfterStyleResolution = false;
- // HTMLObjectElement needs to delay widget updates until after all children are parsed,
- // For HTMLEmbedElement this delay is unnecessary, but there is no harm in doing the same.
- setNeedsWidgetUpdate(true);
- if (isConnected())
- invalidateStyleForSubtree();
+ // Do this after style resolution, since the image or widget load might complete synchronously
+ // and cause us to re-enter otherwise. Also, we can't really answer the question "do I have a renderer"
+ // accurately until after style resolution.
+
+ if (renderer() && !useFallbackContent()) {
+ if (isImageType()) {
+ if (!m_imageLoader)
+ m_imageLoader = std::make_unique<HTMLImageLoader>(*this);
+ if (m_needsImageReload)
+ m_imageLoader->updateFromElementIgnoringPreviousError();
+ else
+ m_imageLoader->updateFromElement();
+ } else {
+ if (needsWidgetUpdate() && renderEmbeddedObject() && !renderEmbeddedObject()->isPluginUnavailable())
+ updateWidget(CreatePlugins::No);
+ }
+ }
+
+ // Either we reloaded the image just now, or we had some reason not to.
+ // Either way, clear the flag now, since we don't need to remember to try again.
+ m_needsImageReload = false;
+
+ document().decrementLoadEventDelayCount();
}
void HTMLPlugInImageElement::didMoveToNewDocument(Document& oldDocument, Document& newDocument)
@@ -282,6 +291,11 @@
if (m_imageLoader)
m_imageLoader->elementDidMoveToNewDocument();
+ if (m_hasUpdateScheduledForAfterStyleResolution) {
+ oldDocument.decrementLoadEventDelayCount();
+ newDocument.incrementLoadEventDelayCount();
+ }
+
HTMLPlugInElement::didMoveToNewDocument(oldDocument, newDocument);
}
@@ -295,18 +309,12 @@
void HTMLPlugInImageElement::resumeFromDocumentSuspension()
{
+ scheduleUpdateForAfterStyleResolution();
invalidateStyleAndRenderersForSubtree();
HTMLPlugInElement::resumeFromDocumentSuspension();
}
-void HTMLPlugInImageElement::startLoadingImage()
-{
- if (!m_imageLoader)
- m_imageLoader = std::make_unique<HTMLImageLoader>(*this);
- m_imageLoader->updateFromElement();
-}
-
void HTMLPlugInImageElement::updateSnapshot(Image* image)
{
if (displayState() > DisplayingSnapshot)
@@ -777,4 +785,14 @@
return document().frame()->loader().subframeLoader().requestObject(*this, url, getNameAttribute(), mimeType, paramNames, paramValues);
}
+void HTMLPlugInImageElement::updateImageLoaderWithNewURLSoon()
+{
+ if (m_needsImageReload)
+ return;
+
+ m_needsImageReload = true;
+ scheduleUpdateForAfterStyleResolution();
+ invalidateStyle();
+}
+
} // namespace WebCore
Modified: trunk/Source/WebCore/html/HTMLPlugInImageElement.h (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLPlugInImageElement.h 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLPlugInImageElement.h 2017-07-30 21:44:01 UTC (rev 220052)
@@ -81,22 +81,25 @@
SnapshotDecision snapshotDecision() const { return m_snapshotDecision; }
protected:
- HTMLPlugInImageElement(const QualifiedName& tagName, Document&, bool createdByParser);
+ HTMLPlugInImageElement(const QualifiedName& tagName, Document&);
+ void finishCreating();
void didMoveToNewDocument(Document& oldDocument, Document& newDocument) override;
+
bool requestObject(const String& url, const String& mimeType, const Vector<String>& paramNames, const Vector<String>& paramValues) final;
bool isImageType();
HTMLImageLoader* imageLoader() { return m_imageLoader.get(); }
+ void updateImageLoaderWithNewURLSoon();
bool allowedToLoadFrameURL(const String& url);
bool wouldLoadAsPlugIn(const String& url, const String& serviceType);
+ void scheduleUpdateForAfterStyleResolution();
+
String m_serviceType;
String m_url;
- std::unique_ptr<HTMLImageLoader> m_imageLoader;
-
private:
bool isPlugInImageElement() const final { return true; }
bool isRestartedPlugin() const final { return m_isRestartedPlugin; }
@@ -103,12 +106,12 @@
bool allowedToLoadPluginContent(const String& url, const String& mimeType) const;
- void finishParsingChildren() final;
void didAddUserAgentShadowRoot(ShadowRoot*) final;
RenderPtr<RenderElement> createElementRenderer(RenderStyle&&, const RenderTreePosition&) override;
bool childShouldCreateRenderer(const Node&) const override;
void willRecalcStyle(Style::Change) final;
+ void didRecalcStyle(Style::Change) final;
void didAttachRenderers() final;
void willDetachRenderers() final;
@@ -120,8 +123,7 @@
void updateSnapshot(Image*) final;
- void startLoadingImage();
- void updateWidgetIfNecessary();
+ void updateAfterStyleResolution();
void simulatedMouseClickTimerFired();
@@ -146,6 +148,9 @@
IntSize m_sizeWhenSnapshotted;
SnapshotDecision m_snapshotDecision { SnapshotNotYetDecided };
bool m_plugInDimensionsSpecified { false };
+ std::unique_ptr<HTMLImageLoader> m_imageLoader;
+ bool m_needsImageReload { false };
+ bool m_hasUpdateScheduledForAfterStyleResolution { false };
};
} // namespace WebCore
Modified: trunk/Source/WebCore/html/HTMLTagNames.in (220051 => 220052)
--- trunk/Source/WebCore/html/HTMLTagNames.in 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/html/HTMLTagNames.in 2017-07-30 21:44:01 UTC (rev 220052)
@@ -8,7 +8,7 @@
abbr interfaceName=HTMLElement
acronym interfaceName=HTMLElement
address interfaceName=HTMLElement
-applet constructorNeedsCreatedByParser
+applet
area
article interfaceName=HTMLElement
aside interfaceName=HTMLElement
@@ -44,7 +44,7 @@
dl interfaceName=HTMLDListElement
dt interfaceName=HTMLElement
em interfaceName=HTMLElement
-embed constructorNeedsCreatedByParser
+embed
fieldset interfaceName=HTMLFieldSetElement, constructorNeedsFormElement
figcaption interfaceName=HTMLElement
figure interfaceName=HTMLElement
@@ -90,7 +90,7 @@
noembed interfaceName=HTMLElement
noframes interfaceName=HTMLElement
nolayer interfaceName=HTMLElement
-object constructorNeedsFormElement, constructorNeedsCreatedByParser
+object constructorNeedsFormElement
ol interfaceName=HTMLOListElement
optgroup interfaceName=HTMLOptGroupElement
option
Modified: trunk/Source/WebCore/loader/DocumentLoader.cpp (220051 => 220052)
--- trunk/Source/WebCore/loader/DocumentLoader.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/loader/DocumentLoader.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -1048,6 +1048,8 @@
return true;
if (m_cachedResourceLoader->requestCount())
return true;
+ if (document.isDelayingLoadEvent())
+ return true;
if (document.processingLoadEvent())
return true;
if (document.hasActiveParser())
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (220051 => 220052)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -2235,6 +2235,11 @@
{
ASSERT(m_client.hasWebView());
+ // FIXME: Should this check be done in checkLoadComplete instead of here?
+ // FIXME: Why does this one check need to be repeated here, and not the many others from checkCompleted?
+ if (m_frame.document()->isDelayingLoadEvent())
+ return;
+
switch (m_state) {
case FrameStateProvisional: {
// FIXME: Prohibiting any provisional load failures from being sent to clients
@@ -2575,6 +2580,7 @@
if (Frame* parent = m_frame.tree().parent()) {
parent->loader().closeAndRemoveChild(m_frame);
parent->loader().scheduleCheckCompleted();
+ parent->loader().scheduleCheckLoadComplete();
} else {
m_frame.setView(nullptr);
m_frame.willDetachPage();
Modified: trunk/Tools/ChangeLog (220051 => 220052)
--- trunk/Tools/ChangeLog 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Tools/ChangeLog 2017-07-30 21:44:01 UTC (rev 220052)
@@ -1,3 +1,24 @@
+2017-07-30 Darin Adler <da...@apple.com>
+
+ Remove code in HTMLObjectElement attribute parsing that forces style resolution and layout
+ https://bugs.webkit.org/show_bug.cgi?id=130653
+
+ Reviewed by Antti Koivisto.
+
+ * WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:
+ (WTR::InjectedBundlePage::didFinishLoadForFrame): Omit now-unneeded "shouldDump" argument
+ when calling frameDidChangeLocation.
+ (WTR::InjectedBundlePage::frameDidChangeLocation): Removed "shouldDump" argument. This was
+ causing WebKitTestRunner to not dump anything in cases where DumpRenderTree will dump, and
+ thus causing mysterious failures of a couple of tests. There are two remaining issues:
+ 1) WebKitTestRunner won't run its dump code if there is no "page", and there is no such
+ consideration in DumpRenderTree and 2) Both DumpRenderTree and WebKitTestRunner share the
+ same logic flaw that causes "top loading frame" to get set to one of the subframes in
+ tests where the following sequence occurs: test calls waitUntilDone, main frame finishes
+ loading, subframe starts loading. It would be good to clean that up some day, but for now
+ this patch makes the two work identically rather than changing both.
+ * WebKitTestRunner/InjectedBundle/InjectedBundlePage.h: Updated for change above.
+
2017-07-30 Wenson Hsieh <wenson_hs...@apple.com>
[WK2] Replace RetainPtr<> with auto when adopting allocated ObjC objects in DataInteractionTests
Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp (220051 => 220052)
--- trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp 2017-07-30 21:44:01 UTC (rev 220052)
@@ -934,7 +934,7 @@
if (injectedBundle.testRunner()->shouldDumpFrameLoadCallbacks())
dumpLoadEvent(frame, "didFinishLoadForFrame");
- frameDidChangeLocation(frame, /*shouldDump*/ true);
+ frameDidChangeLocation(frame);
}
void InjectedBundlePage::didFailLoadWithErrorForFrame(WKBundleFrameRef frame, WKErrorRef)
@@ -2009,7 +2009,7 @@
}
#endif
-void InjectedBundlePage::frameDidChangeLocation(WKBundleFrameRef frame, bool shouldDump)
+void InjectedBundlePage::frameDidChangeLocation(WKBundleFrameRef frame)
{
auto& injectedBundle = InjectedBundle::singleton();
if (frame != injectedBundle.topLoadingFrame())
@@ -2025,7 +2025,7 @@
return;
}
- if (shouldDump)
+ if (injectedBundle.pageCount())
injectedBundle.page()->dump();
else
injectedBundle.done();
Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.h (220051 => 220052)
--- trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.h 2017-07-30 21:35:17 UTC (rev 220051)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.h 2017-07-30 21:44:01 UTC (rev 220052)
@@ -171,7 +171,7 @@
void platformDidStartProvisionalLoadForFrame(WKBundleFrameRef);
String platformResponseMimeType(WKURLResponseRef);
- void frameDidChangeLocation(WKBundleFrameRef, bool shouldDump = false);
+ void frameDidChangeLocation(WKBundleFrameRef);
WKBundlePageRef m_page;
WKRetainPtr<WKBundleScriptWorldRef> m_world;
