- Revision
- 154573
- Author
- [email protected]
- Date
- 2013-08-25 02:54:09 -0700 (Sun, 25 Aug 2013)
Log Message
No need for notifyChromeClientWheelEventHandlerCountChanged in Frame
https://bugs.webkit.org/show_bug.cgi?id=120264
Reviewed by Andreas Kling.
* dom/Document.cpp:
(WebCore::Document::createRenderTree): Renamed attach to this.
This made it practical to remove a comment that says the same thing and
also helps make the purpose of the function considerably more clear,
although the relationship to the attached and detach functions is now
less clear; should fix that soon.
(WebCore::pageWheelEventHandlerCountChanged): Added. Contains the code
from Frame::notifyChromeClientWheelEventHandlerCountChanged, minus some
assertions that were only needed because the function was passed a frame
rather than a page.
(WebCore::Document::didBecomeCurrentDocumentInFrame): Added. Contains
most of the code from Frame::setDocument. Looking at before and after,
we can see that most of the work is within the document class and matches
up with other code already in this class. Added FIXMEs about many problems
spotted in the code.
(WebCore::Document::topDocument): Added FIXME and tweaked formatting.
(WebCore::wheelEventHandlerCountChanged): Moved the call to the
pageWheelEventHandlerCountChanged in here from the two call sites.
Also added a FIXME.
(WebCore::Document::didAddWheelEventHandler): Removed the call to
notifyChromeClientWheelEventHandlerCountChanged, since that's now handled
inside wheelEventHandlerCountChanged.
(WebCore::Document::didRemoveWheelEventHandler): Ditto.
* dom/Document.h: Renamed attach to createRenderTree, made it private,
and added a new didBecomeCurrentDocumentInFrame function.
* loader/PlaceholderDocument.cpp:
(WebCore::PlaceholderDocument::createRenderTree): Renamed from attach.
* loader/PlaceholderDocument.h: Did the rename and made the function a
private override.
* page/Frame.cpp:
(WebCore::Frame::setDocument): Moved most of this function out of here
into the new Document::didBecomeCurrentDocumentInFrame function.
Also deleted notifyChromeClientWheelEventHandlerCountChanged.
* page/Frame.h: Deleted notifyChromeClientWheelEventHandlerCountChanged.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (154572 => 154573)
--- trunk/Source/WebCore/ChangeLog 2013-08-25 09:51:42 UTC (rev 154572)
+++ trunk/Source/WebCore/ChangeLog 2013-08-25 09:54:09 UTC (rev 154573)
@@ -1,5 +1,51 @@
2013-08-25 Darin Adler <[email protected]>
+ No need for notifyChromeClientWheelEventHandlerCountChanged in Frame
+ https://bugs.webkit.org/show_bug.cgi?id=120264
+
+ Reviewed by Andreas Kling.
+
+ * dom/Document.cpp:
+ (WebCore::Document::createRenderTree): Renamed attach to this.
+ This made it practical to remove a comment that says the same thing and
+ also helps make the purpose of the function considerably more clear,
+ although the relationship to the attached and detach functions is now
+ less clear; should fix that soon.
+ (WebCore::pageWheelEventHandlerCountChanged): Added. Contains the code
+ from Frame::notifyChromeClientWheelEventHandlerCountChanged, minus some
+ assertions that were only needed because the function was passed a frame
+ rather than a page.
+ (WebCore::Document::didBecomeCurrentDocumentInFrame): Added. Contains
+ most of the code from Frame::setDocument. Looking at before and after,
+ we can see that most of the work is within the document class and matches
+ up with other code already in this class. Added FIXMEs about many problems
+ spotted in the code.
+ (WebCore::Document::topDocument): Added FIXME and tweaked formatting.
+ (WebCore::wheelEventHandlerCountChanged): Moved the call to the
+ pageWheelEventHandlerCountChanged in here from the two call sites.
+ Also added a FIXME.
+ (WebCore::Document::didAddWheelEventHandler): Removed the call to
+ notifyChromeClientWheelEventHandlerCountChanged, since that's now handled
+ inside wheelEventHandlerCountChanged.
+ (WebCore::Document::didRemoveWheelEventHandler): Ditto.
+
+ * dom/Document.h: Renamed attach to createRenderTree, made it private,
+ and added a new didBecomeCurrentDocumentInFrame function.
+
+ * loader/PlaceholderDocument.cpp:
+ (WebCore::PlaceholderDocument::createRenderTree): Renamed from attach.
+ * loader/PlaceholderDocument.h: Did the rename and made the function a
+ private override.
+
+ * page/Frame.cpp:
+ (WebCore::Frame::setDocument): Moved most of this function out of here
+ into the new Document::didBecomeCurrentDocumentInFrame function.
+ Also deleted notifyChromeClientWheelEventHandlerCountChanged.
+
+ * page/Frame.h: Deleted notifyChromeClientWheelEventHandlerCountChanged.
+
+2013-08-25 Darin Adler <[email protected]>
+
No need for dispatchVisibilityStateChangeEvent function
https://bugs.webkit.org/show_bug.cgi?id=120261
Modified: trunk/Source/WebCore/dom/Document.cpp (154572 => 154573)
--- trunk/Source/WebCore/dom/Document.cpp 2013-08-25 09:51:42 UTC (rev 154572)
+++ trunk/Source/WebCore/dom/Document.cpp 2013-08-25 09:54:09 UTC (rev 154573)
@@ -1960,7 +1960,7 @@
m_styleResolver.clear();
}
-void Document::attach()
+void Document::createRenderTree()
{
ASSERT(!attached());
ASSERT(!m_inPageCache);
@@ -1969,7 +1969,6 @@
if (!m_renderArena)
m_renderArena = RenderArena::create();
- // Create the rendering tree
setRenderer(new (m_renderArena.get()) RenderView(this));
#if USE(ACCELERATED_COMPOSITING)
renderView()->setIsInWindow(true);
@@ -1983,6 +1982,52 @@
setAttached(true);
}
+static void pageWheelEventHandlerCountChanged(Page& page)
+{
+ unsigned count = 0;
+ for (const Frame* frame = page.mainFrame(); frame; frame = frame->tree().traverseNext()) {
+ if (Document* document = frame->document())
+ count += document->wheelEventHandlerCount();
+ }
+ page.chrome().client().numWheelEventHandlersChanged(count);
+}
+
+void Document::didBecomeCurrentDocumentInFrame()
+{
+ // FIXME: Are there cases where the document can be dislodged from the frame during the event handling below?
+ // If so, then m_frame could become 0, and we need to do something about that.
+
+ m_frame->script().updateDocument();
+
+ if (!attached())
+ createRenderTree();
+
+ updateViewportArguments();
+
+ // FIXME: Doing this only for the main frame is insufficient.
+ // Changing a subframe can also change the wheel event handler count.
+ // FIXME: Doing this only when a document goes into the frame is insufficient.
+ // Removing a document can also change the wheel event handler count.
+ // FIXME: Doing this every time is a waste. If the current document and its
+ // subframes' documents have no wheel event handlers, then the count did not change,
+ // unless the documents they are replacing had wheel event handlers.
+ if (page() && page()->mainFrame() == m_frame)
+ pageWheelEventHandlerCountChanged(*page());
+
+#if ENABLE(TOUCH_EVENTS)
+ // FIXME: Doing this only for the main frame is insufficient.
+ // A subframe could have touch event handlers.
+ if (hasTouchEventHandlers() && page() && page()->mainFrame() == m_frame)
+ page()->chrome().client().needTouchEvents(true);
+#endif
+
+ if (m_frame->activeDOMObjectsAndAnimationsSuspended()) {
+ suspendScriptedAnimationControllerCallbacks();
+ m_frame->animation().suspendAnimationsForDocument(this);
+ suspendActiveDOMObjects(ActiveDOMObject::PageWillBeSuspended);
+ }
+}
+
void Document::detach()
{
ASSERT(attached());
@@ -4236,12 +4281,12 @@
Document* Document::topDocument() const
{
- Document* doc = const_cast<Document *>(this);
- Element* element;
- while ((element = doc->ownerElement()))
- doc = element->document();
-
- return doc;
+ // FIXME: Why does this walk up owner elements instead using the frame tree as parentDocument does?
+ // The frame tree even has a top() function.
+ Document* document = const_cast<Document*>(this);
+ while (Element* element = document->ownerElement())
+ document = element->document();
+ return document;
}
PassRefPtr<Attr> Document::createAttribute(const String& name, ExceptionCode& ec)
@@ -5487,6 +5532,8 @@
if (!page)
return;
+ pageWheelEventHandlerCountChanged(*page);
+
ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator();
if (!scrollingCoordinator)
return;
@@ -5495,16 +5542,13 @@
if (!frameView)
return;
+ // FIXME: Why doesn't this need to be called in didBecomeCurrentDocumentInFrame?
scrollingCoordinator->frameViewWheelEventHandlerCountChanged(frameView);
}
void Document::didAddWheelEventHandler()
{
++m_wheelEventHandlerCount;
- Frame* mainFrame = page() ? page()->mainFrame() : 0;
- if (mainFrame)
- mainFrame->notifyChromeClientWheelEventHandlerCountChanged();
-
wheelEventHandlerCountChanged(this);
}
@@ -5512,10 +5556,6 @@
{
ASSERT(m_wheelEventHandlerCount > 0);
--m_wheelEventHandlerCount;
- Frame* mainFrame = page() ? page()->mainFrame() : 0;
- if (mainFrame)
- mainFrame->notifyChromeClientWheelEventHandlerCountChanged();
-
wheelEventHandlerCountChanged(this);
}
Modified: trunk/Source/WebCore/dom/Document.h (154572 => 154573)
--- trunk/Source/WebCore/dom/Document.h 2013-08-25 09:51:42 UTC (rev 154572)
+++ trunk/Source/WebCore/dom/Document.h 2013-08-25 09:54:09 UTC (rev 154573)
@@ -524,7 +524,7 @@
CachedResourceLoader* cachedResourceLoader() { return m_cachedResourceLoader.get(); }
- virtual void attach();
+ void didBecomeCurrentDocumentInFrame();
virtual void detach();
void prepareForDestruction();
@@ -1196,6 +1196,7 @@
friend class Node;
friend class IgnoreDestructiveWriteCountIncrementer;
+ virtual void createRenderTree();
virtual void dispose() OVERRIDE;
void detachParser();
Modified: trunk/Source/WebCore/loader/PlaceholderDocument.cpp (154572 => 154573)
--- trunk/Source/WebCore/loader/PlaceholderDocument.cpp 2013-08-25 09:51:42 UTC (rev 154572)
+++ trunk/Source/WebCore/loader/PlaceholderDocument.cpp 2013-08-25 09:54:09 UTC (rev 154573)
@@ -30,7 +30,7 @@
namespace WebCore {
-void PlaceholderDocument::attach()
+void PlaceholderDocument::createRenderTree()
{
ASSERT(!attached());
Modified: trunk/Source/WebCore/loader/PlaceholderDocument.h (154572 => 154573)
--- trunk/Source/WebCore/loader/PlaceholderDocument.h 2013-08-25 09:51:42 UTC (rev 154572)
+++ trunk/Source/WebCore/loader/PlaceholderDocument.h 2013-08-25 09:54:09 UTC (rev 154573)
@@ -30,19 +30,20 @@
namespace WebCore {
-class PlaceholderDocument : public Document {
+class PlaceholderDocument FINAL : public Document {
public:
static PassRefPtr<PlaceholderDocument> create(Frame* frame, const KURL& url)
{
return adoptRef(new PlaceholderDocument(frame, url));
}
- virtual void attach();
+private:
+ virtual void createRenderTree() OVERRIDE;
-private:
PlaceholderDocument(Frame* frame, const KURL& url)
: Document(frame, url)
- { }
+ {
+ }
};
} // namespace WebCore
Modified: trunk/Source/WebCore/page/Frame.cpp (154572 => 154573)
--- trunk/Source/WebCore/page/Frame.cpp 2013-08-25 09:51:42 UTC (rev 154572)
+++ trunk/Source/WebCore/page/Frame.cpp 2013-08-25 09:54:09 UTC (rev 154573)
@@ -272,40 +272,23 @@
#endif
}
-void Frame::setDocument(PassRefPtr<Document> newDoc)
+void Frame::setDocument(PassRefPtr<Document> newDocument)
{
- ASSERT(!newDoc || newDoc->frame() == this);
+ ASSERT(!newDocument || newDocument->frame() == this);
+
if (m_doc && m_doc->attached() && !m_doc->inPageCache()) {
// FIXME: We don't call willRemove here. Why is that OK?
m_doc->detach();
}
- m_doc = newDoc;
+ m_doc = newDocument.get();
ASSERT(!m_doc || m_doc->domWindow());
ASSERT(!m_doc || m_doc->domWindow()->frame() == this);
- if (m_doc && !m_doc->attached())
- m_doc->attach();
-
- if (m_doc) {
- m_script->updateDocument();
- m_doc->updateViewportArguments();
- }
-
- if (m_page && m_page->mainFrame() == this) {
- notifyChromeClientWheelEventHandlerCountChanged();
-#if ENABLE(TOUCH_EVENTS)
- if (m_doc && m_doc->hasTouchEventHandlers())
- m_page->chrome().client().needTouchEvents(true);
-#endif
- }
-
- // Suspend document if this frame was created in suspended state.
- if (m_doc && activeDOMObjectsAndAnimationsSuspended()) {
- m_doc->suspendScriptedAnimationControllerCallbacks();
- m_animationController->suspendAnimationsForDocument(m_doc.get());
- m_doc->suspendActiveDOMObjects(ActiveDOMObject::PageWillBeSuspended);
- }
+ // Don't use m_doc because it can be overwritten and we want to guarantee
+ // that the document is not destroyed during this function call.
+ if (newDocument)
+ newDocument->didBecomeCurrentDocumentInFrame();
}
#if ENABLE(ORIENTATION_EVENTS)
@@ -954,20 +937,7 @@
root->compositor().deviceOrPageScaleFactorChanged();
}
#endif
-void Frame::notifyChromeClientWheelEventHandlerCountChanged() const
-{
- // Ensure that this method is being called on the main frame of the page.
- ASSERT(m_page && m_page->mainFrame() == this);
- unsigned count = 0;
- for (const Frame* frame = this; frame; frame = frame->tree().traverseNext()) {
- if (frame->document())
- count += frame->document()->wheelEventHandlerCount();
- }
-
- m_page->chrome().client().numWheelEventHandlersChanged(count);
-}
-
bool Frame::isURLAllowed(const KURL& url) const
{
// We allow one level of self-reference because some sites depend on that,
Modified: trunk/Source/WebCore/page/Frame.h (154572 => 154573)
--- trunk/Source/WebCore/page/Frame.h 2013-08-25 09:51:42 UTC (rev 154572)
+++ trunk/Source/WebCore/page/Frame.h 2013-08-25 09:54:09 UTC (rev 154573)
@@ -184,9 +184,6 @@
void resumeActiveDOMObjectsAndAnimations();
bool activeDOMObjectsAndAnimationsSuspended() const { return m_activeDOMObjectsAndAnimationsSuspendedCount > 0; }
- // Should only be called on the main frame of a page.
- void notifyChromeClientWheelEventHandlerCountChanged() const;
-
bool isURLAllowed(const KURL&) const;
// ========
