Reviewed by Anders.

        - fix http://bugs.webkit.org/show_bug.cgi?id=11379
          assertion failure seen in editing tests (in new DeleteButtonController)

        I cleaned up the relationship between the Frame, Editor, EditorClient, and
        DeleteButtonController a bit while also making the simple change to fix the
        assertion failure.

        * bridge/mac/FrameMac.h: Change EditorClient parameter to a PassRefPtr since we take ownership.
        * bridge/mac/FrameMac.mm: (WebCore::FrameMac::FrameMac):

        * page/Frame.h: Change EditorClient parameter to a PassRefPtr since we take ownership.
        * page/Frame.cpp:
        (WebCore::Frame::Frame): Ditto.
        (WebCore::Frame::appliedEditing): Removed unneeded parameter to the respondToChangedContents
        function, and moved it to Editor rather than right on the DeleteButtonController.
        (WebCore::Frame::unappliedEditing): Ditto.
        (WebCore::Frame::reappliedEditing): Ditto.
        * page/FramePrivate.h: (WebCore::FramePrivate::FramePrivate): More of the same.

        * editing/DeleteButtonController.h:
        * editing/DeleteButtonController.cpp:
        (WebCore::DeleteButtonController::DeleteButtonController): Replaced Editor* with a Frame*.
        The general pattern is that we always use the Frame* as the frame identifier. See FrameTree,
        for example.
        (WebCore::enclosingDeletableTable): Added. Helper function used by the respondToChangedSelection
        function. Includes an additional check for whether the container is contentEditable, which
        was missing from the old version. This prevents the assertion failure. Also added a check that
        the table itself is editable.
        (WebCore::DeleteButtonController::respondToChangedSelection): Rewrote to use the helper.
        (WebCore::DeleteButtonController::respondToChangedContents): Removed the unnecessary
        selection parameter. No need to pass in the state of the frame since we can get it if we need it.
        (WebCore::DeleteButtonController::show): Updated to use frame pointer rather than editor pointer.

        * editing/Editor.cpp: Fixed formatting. Even the temporary placeholder functions should be
        formatted on multiple lines as usual.
        (WebCore::Editor::respondToChangedSelection): Added. Forwards to the delete button controller.
        (WebCore::Editor::respondToChangedContents): Ditto.
        (WebCore::Editor::Editor): Changed EditorClient parameter to a PassRefPtr since we take ownership.

        * editing/Editor.h: Changed the DeleteButtonController to use an OwnPtr instead of being
        defined inline to decouple so that we don't have to include DeleteButtonController.h. That way
        changes to DeleteButtonController.h cause very little to recompile.

        * editing/SelectionController.cpp: (WebCore::SelectionController::setSelection):
        Updated to call the editor instead of the delete button controller for the selection change.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@17202 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index bca6d00282d9c08e472966b17aca326c34eb3729..05371c8147deb51ebe3e76a1665046ab42aea7c8 100644 (file)
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,53 @@
+2006-10-21  Darin Adler  <darin@apple.com>
+
+        Reviewed by Anders.
+
+        - fix http://bugs.webkit.org/show_bug.cgi?id=11379
+          assertion failure seen in editing tests (in new DeleteButtonController)
+
+        I cleaned up the relationship between the Frame, Editor, EditorClient, and
+        DeleteButtonController a bit while also making the simple change to fix the
+        assertion failure.
+
+        * bridge/mac/FrameMac.h: Change EditorClient parameter to a PassRefPtr since we take ownership.
+        * bridge/mac/FrameMac.mm: (WebCore::FrameMac::FrameMac):
+
+        * page/Frame.h: Change EditorClient parameter to a PassRefPtr since we take ownership.
+        * page/Frame.cpp:
+        (WebCore::Frame::Frame): Ditto.
+        (WebCore::Frame::appliedEditing): Removed unneeded parameter to the respondToChangedContents
+        function, and moved it to Editor rather than right on the DeleteButtonController.
+        (WebCore::Frame::unappliedEditing): Ditto.
+        (WebCore::Frame::reappliedEditing): Ditto.
+        * page/FramePrivate.h: (WebCore::FramePrivate::FramePrivate): More of the same.
+
+        * editing/DeleteButtonController.h:
+        * editing/DeleteButtonController.cpp:
+        (WebCore::DeleteButtonController::DeleteButtonController): Replaced Editor* with a Frame*.
+        The general pattern is that we always use the Frame* as the frame identifier. See FrameTree,
+        for example.
+        (WebCore::enclosingDeletableTable): Added. Helper function used by the respondToChangedSelection
+        function. Includes an additional check for whether the container is contentEditable, which
+        was missing from the old version. This prevents the assertion failure. Also added a check that
+        the table itself is editable.
+        (WebCore::DeleteButtonController::respondToChangedSelection): Rewrote to use the helper.
+        (WebCore::DeleteButtonController::respondToChangedContents): Removed the unnecessary
+        selection parameter. No need to pass in the state of the frame since we can get it if we need it.
+        (WebCore::DeleteButtonController::show): Updated to use frame pointer rather than editor pointer.
+
+        * editing/Editor.cpp: Fixed formatting. Even the temporary placeholder functions should be
+        formatted on multiple lines as usual.
+        (WebCore::Editor::respondToChangedSelection): Added. Forwards to the delete button controller.
+        (WebCore::Editor::respondToChangedContents): Ditto.
+        (WebCore::Editor::Editor): Changed EditorClient parameter to a PassRefPtr since we take ownership.
+
+        * editing/Editor.h: Changed the DeleteButtonController to use an OwnPtr instead of being
+        defined inline to decouple so that we don't have to include DeleteButtonController.h. That way
+        changes to DeleteButtonController.h cause very little to recompile.
+
+        * editing/SelectionController.cpp: (WebCore::SelectionController::setSelection):
+        Updated to call the editor instead of the delete button controller for the selection change.
+
 2006-10-21  Anders Carlsson  <acarlsson@apple.com>
 
         Reviewed by Darin.

diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj
index 2b2bf1592c473d16909491be1d6f4cfd272cb7f2..3cbbacb2b71aa0c9716ac597510f2a0e5e707468 100644 (file)
--- a/WebCore/WebCore.xcodeproj/project.pbxproj
+++ b/WebCore/WebCore.xcodeproj/project.pbxproj
@@ -10477,7 +10477,6 @@
                0867D690FE84028FC02AAC07 /* Project object */ = {
                        isa = PBXProject;
                        buildConfigurationList = 149C284308902B11008A9EFC /* Build configuration list for PBXProject "WebCore" */;
-                       compatibilityVersion = "Xcode 2.4";
                        hasScannedForEncodings = 1;
                        knownRegions = (
                                English,
@@ -10492,7 +10491,6 @@
                        productRefGroup = 034768DFFF38A50411DB9C8B /* Products */;
                        projectDirPath = "";
                        projectRoot = "";
-                       shouldCheckCompatibility = 1;
                        targets = (
                                93F198A508245E59001E9ABC /* WebCore */,
                                DD041FBE09D9DDBE0010AF2A /* Derived Sources */,

diff --git a/WebCore/bridge/mac/FrameMac.h b/WebCore/bridge/mac/FrameMac.h
index 7deed7040c51d1caa52795ef37f92773ec5d9ea8..37e4fda3d4316d1c0b1b8b75ff3ef9a6ba027e2c 100644 (file)
--- a/WebCore/bridge/mac/FrameMac.h
+++ b/WebCore/bridge/mac/FrameMac.h
@@ -115,7 +115,7 @@ enum SelectionDirection {
 class FrameMac : public Frame
 {
 public:
-    FrameMac(Page*, Element*, EditorClient*);
+    FrameMac(Page*, Element*, PassRefPtr<EditorClient>);
     ~FrameMac();
     
     void clear();

diff --git a/WebCore/bridge/mac/FrameMac.mm b/WebCore/bridge/mac/FrameMac.mm
index e18437dd442559234eafd085a6774c32b9b82ab6..06875c5df4884a7a4d912f70a4685baad84c65e3 100644 (file)
--- a/WebCore/bridge/mac/FrameMac.mm
+++ b/WebCore/bridge/mac/FrameMac.mm
@@ -148,7 +148,7 @@ static SEL selectorForKeyEvent(const PlatformKeyboardEvent* event)
     return selector;
 }
 
-FrameMac::FrameMac(Page* page, Element* ownerElement, EditorClient* client)
+FrameMac::FrameMac(Page* page, Element* ownerElement, PassRefPtr<EditorClient> client)
     : Frame(page, ownerElement, client)
     , _bridge(nil)
     , _mouseDownView(nil)

diff --git a/WebCore/editing/DeleteButtonController.cpp b/WebCore/editing/DeleteButtonController.cpp
index f521141eb7a9ffc869da5946765219b94bc987c9..6028bcec29ab24c28164192d66f3227d9f5f0915 100644 (file)
--- a/WebCore/editing/DeleteButtonController.cpp
+++ b/WebCore/editing/DeleteButtonController.cpp
@@ -53,39 +53,57 @@ const char* const DeleteButtonController::containerElementIdentifier = "WebKit-E
 const char* const DeleteButtonController::buttonElementIdentifier = "WebKit-Editing-Delete-Button";
 const char* const DeleteButtonController::outlineElementIdentifier = "WebKit-Editing-Delete-Outline";
 
-DeleteButtonController::DeleteButtonController(Editor* editor)
-    : m_editor(editor)
+DeleteButtonController::DeleteButtonController(Frame* frame)
+    : m_frame(frame)
     , m_wasStaticPositioned(false)
     , m_wasAutoZIndex(false)
 {
 }
 
-void DeleteButtonController::respondToChangedSelection(const Selection& oldSelection)
+static HTMLElement* enclosingDeletableTable(const Selection& selection)
 {
-    ExceptionCode ec = 0;
-    Node* oldTableNode = 0;
-    Node* newTableNode = 0;
+    if (!selection.isContentEditable())
+        return 0;
 
-    RefPtr<Range> range = oldSelection.toRange();
-    if (oldSelection.isContentEditable() && range.get())
-        oldTableNode = enclosingNodeWithTag(range->commonAncestorContainer(ec), tableTag);
-    ASSERT(ec == 0);
+    RefPtr<Range> range = selection.toRange();
+    if (!range)
+        return 0;
 
-    range = m_editor->frame()->selectionController()->toRange();
-    if (m_editor->frame()->selectionController()->isContentEditable() && range.get())
-        newTableNode = enclosingNodeWithTag(range->commonAncestorContainer(ec), tableTag);
+    ExceptionCode ec = 0;
+    Node* container = range->commonAncestorContainer(ec);
+    ASSERT(container);
     ASSERT(ec == 0);
 
-    if (oldTableNode != newTableNode) {
-        // If the base is inside an editable table, give the table a close widget.
-        if (newTableNode)
-            show(static_cast<HTMLElement*>(newTableNode));
-        else if (oldTableNode)
-            hide();
-    }
+    // The enclosingNodeWithTag function only works on nodes that are editable
+    // (which is strange, given its name).
+    if (!container->isContentEditable())
+        return 0;
+
+    Node* table = enclosingNodeWithTag(container, tableTag);
+    ASSERT(!table || table->isHTMLElement());
+
+    // The table must be editable too.
+    if (!table->isContentEditable())
+        return 0;
+
+    return static_cast<HTMLElement*>(table);
 }
-    
-void DeleteButtonController::respondToChangedContents(const Selection& endingSelection)
+
+void DeleteButtonController::respondToChangedSelection(const Selection& oldSelection)
+{
+    HTMLElement* oldTable = enclosingDeletableTable(oldSelection);
+    HTMLElement* newTable = enclosingDeletableTable(m_frame->selectionController()->selection());
+    if (oldTable == newTable)
+        return;
+
+    // If the base is inside an editable table, give the table a close widget.
+    if (newTable)
+        show(newTable);
+    else
+        hide();
+}
+
+void DeleteButtonController::respondToChangedContents()
 {
     updateOutlineStyle();
 }
@@ -107,7 +125,7 @@ void DeleteButtonController::show(HTMLElement* element)
     if (!(element->renderer() && element->renderer()->isRenderBlock()))
         return;
 
-    if (!m_editor->shouldShowDeleteInterface(static_cast<HTMLElement*>(element)))
+    if (!m_frame->editor()->shouldShowDeleteInterface(static_cast<HTMLElement*>(element)))
         return;
 
     m_element = element;

diff --git a/WebCore/editing/DeleteButtonController.h b/WebCore/editing/DeleteButtonController.h
index 69d7d10fd7bfbfb90569acd795d95505e010cec4..a52fde4347c35d4f505071a4bcc8486417749e59 100644 (file)
--- a/WebCore/editing/DeleteButtonController.h
+++ b/WebCore/editing/DeleteButtonController.h
@@ -31,14 +31,14 @@
 namespace WebCore {
 
 class DeleteButton;
-class Editor;
+class Frame;
 class HTMLElement;
 class RenderObject;
 class Selection;
 
 class DeleteButtonController {
 public:
-    DeleteButtonController(Editor*);
+    DeleteButtonController(Frame*);
 
     static const char* const containerElementIdentifier;
     static const char* const buttonElementIdentifier;
@@ -47,7 +47,7 @@ public:
     HTMLElement* target() const { return m_element.get(); };
 
     void respondToChangedSelection(const Selection& oldSelection);
-    void respondToChangedContents(const Selection& endingSelection);
+    void respondToChangedContents();
 
     void show(HTMLElement*);
     void hide();
@@ -57,7 +57,7 @@ public:
 private:
     void updateOutlineStyle();
 
-    Editor* m_editor;
+    Frame* m_frame;
     RefPtr<HTMLElement> m_element;
     RefPtr<HTMLElement> m_containerElement;
     RefPtr<HTMLElement> m_outlineElement;

diff --git a/WebCore/editing/Editor.cpp b/WebCore/editing/Editor.cpp
index 1d6067918039f82442268125b98bd13da7da6312..ea57b9e1356e273334d8b84c99817d5cc72f35f0 100644 (file)
--- a/WebCore/editing/Editor.cpp
+++ b/WebCore/editing/Editor.cpp
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "Editor.h"
 
+#include "DeleteButtonController.h"
 #include "EditorClient.h"
 #include "Frame.h"
 #include "HTMLElement.h"
@@ -35,25 +36,31 @@
 namespace WebCore {
 
 // implement as platform-specific
-Pasteboard generalPasteboard()
+static Pasteboard generalPasteboard()
 {
     return 0;
 }
 
 bool Editor::canCopy()
-{ return false; }
+{
+    return false;
+}
 
 bool Editor::canCut()
-{ return false; }
+{
+    return false;
+}
 
 bool Editor::canDelete()
-{ return false; }
+{
+    return false;
+}
 
 bool Editor::canDeleteRange(Range* range)
 {
     ExceptionCode ec = 0;
-    Node *startContainer = range->startContainer(ec);
-    Node *endContainer = range->endContainer(ec);
+    Node* startContainer = range->startContainer(ec);
+    Node* endContainer = range->endContainer(ec);
     if (!startContainer || !endContainer)
         return false;
     
@@ -71,28 +78,40 @@ bool Editor::canDeleteRange(Range* range)
 }
 
 bool Editor::canPaste()
-{ return false; }
+{
+    return false;
+}
 
 bool Editor::canSmartCopyOrDelete()
-{ return false; }
+{
+    return false;
+}
 
 void Editor::deleteSelection()
-{}
+{
+}
 
 void Editor::deleteSelectionWithSmartDelete(bool enabled)
-{}
+{
+}
 
 bool Editor::isSelectionRichlyEditable()
-{ return false; }
+{
+    return false;
+}
 
 void Editor::pasteAsPlainTextWithPasteboard(Pasteboard pasteboard)
-{}
+{
+}
 
 void Editor::pasteWithPasteboard(Pasteboard pasteboard, bool allowPlainText)
-{}
+{
+}
 
 Range* Editor::selectedRange()
-{ return NULL; }
+{
+    return 0;
+}
 
 bool Editor::shouldDeleteRange(Range* range)
 {
@@ -125,23 +144,34 @@ bool Editor::tryDHTMLPaste()
 }
 
 void Editor::writeSelectionToPasteboard(Pasteboard pasteboard)
-{}
+{
+}
 
 bool Editor::shouldShowDeleteInterface(HTMLElement* element)
 {
     return m_client->shouldShowDeleteInterface(element);
 }
 
+void Editor::respondToChangedSelection(const Selection& oldSelection)
+{
+    m_deleteButtonController->respondToChangedSelection(oldSelection);
+}
+
+void Editor::respondToChangedContents()
+{
+    m_deleteButtonController->respondToChangedContents();
+}
+
 // =============================================================================
 //
 // public editing commands
 //
 // =============================================================================
 
-Editor::Editor(Frame* frame, EditorClient* client)
+Editor::Editor(Frame* frame, PassRefPtr<EditorClient> client)
     : m_frame(frame)
     , m_client(client)
-    , m_deleteButtonController(this)
+    , m_deleteButtonController(new DeleteButtonController(frame))
 { 
 }
 
@@ -166,7 +196,6 @@ void Editor::cut()
 
 void Editor::copy()
 {
-
     if (tryDHTMLCopy())
         return; // DHTML did the whole operation
     if (!canCopy()) {

diff --git a/WebCore/editing/Editor.h b/WebCore/editing/Editor.h
index 640ecb1680c98391f0c5310a4ee9243209af27cc..c64745d8318e2e5aec6c18989346d734cbcf3d08 100644 (file)
--- a/WebCore/editing/Editor.h
+++ b/WebCore/editing/Editor.h
@@ -26,20 +26,25 @@
 #ifndef EDITOR_H
 #define EDITOR_H
 
-#include "DeleteButtonController.h"
+#include <wtf/Forward.h>
+#include <wtf/OwnPtr.h>
+#include <wtf/RefPtr.h>
 
 namespace WebCore {
 
+class DeleteButtonController;
 class EditorClient;
 class Frame;
+class HTMLElement;
 class Range;
+class Selection;
 
 // make platform-specific and implement - this is temporary placeholder
 typedef int Pasteboard;
 
 class Editor {
 public:
-    Editor(Frame*, EditorClient*);
+    Editor(Frame*, PassRefPtr<EditorClient>);
     ~Editor();
 
     void cut();
@@ -49,13 +54,16 @@ public:
 
     bool shouldShowDeleteInterface(HTMLElement*);
 
+    void respondToChangedSelection(const Selection& oldSelection);
+    void respondToChangedContents();
+
     Frame* frame() const { return m_frame; }
-    DeleteButtonController* deleteButtonController() { return &m_deleteButtonController; }
+    DeleteButtonController* deleteButtonController() const { return m_deleteButtonController.get(); }
 
 private:
     Frame* m_frame;
     RefPtr<EditorClient> m_client;
-    DeleteButtonController m_deleteButtonController;
+    OwnPtr<DeleteButtonController> m_deleteButtonController;
 
     bool canCopy();
     bool canCut();

diff --git a/WebCore/editing/SelectionController.cpp b/WebCore/editing/SelectionController.cpp
index e6c82add50001bc58e963d5f4fdb1e958e5800c5..73f4618cbc281438280f97780febd2ee9f3bb872 100644 (file)
--- a/WebCore/editing/SelectionController.cpp
+++ b/WebCore/editing/SelectionController.cpp
@@ -26,7 +26,6 @@
 #include "config.h"
 #include "SelectionController.h"
 
-#include "DeleteButtonController.h"
 #include "Document.h"
 #include "Editor.h"
 #include "Element.h"
@@ -121,7 +120,7 @@ void SelectionController::setSelection(const Selection& s, bool closeTyping, boo
     m_frame->selectFrameElementInParentIfFullySelected();
     m_frame->notifyRendererOfSelectionChange(userTriggered);
     m_frame->respondToChangedSelection(oldSelection, closeTyping);
-    m_frame->editor()->deleteButtonController()->respondToChangedSelection(oldSelection);
+    m_frame->editor()->respondToChangedSelection(oldSelection);
     if (userTriggered)
         m_frame->revealCaret(RenderLayer::gAlignToEdgeIfNeeded);
 }

diff --git a/WebCore/page/Frame.cpp b/WebCore/page/Frame.cpp
index d43758b740b8bd6dbf1dc34ebcfd7408984a66b2..7335fdcad7890db7b51f460ab68dbb7425140a29 100644 (file)
--- a/WebCore/page/Frame.cpp
+++ b/WebCore/page/Frame.cpp
@@ -158,7 +158,7 @@ static inline Frame* parentFromOwnerElement(Element* ownerElement)
     return ownerElement->document()->frame();
 }
 
-Frame::Frame(Page* page, Element* ownerElement, EditorClient* client) 
+Frame::Frame(Page* page, Element* ownerElement, PassRefPtr<EditorClient> client) 
     : d(new FramePrivate(page, parentFromOwnerElement(ownerElement), this, ownerElement, client))
 {
     AtomicString::init();
@@ -2156,7 +2156,7 @@ void Frame::appliedEditing(PassRefPtr<EditCommand> cmd)
         registerCommandForUndo(cmd);
     }
     respondToChangedContents(newSelection);
-    editor()->deleteButtonController()->respondToChangedContents(newSelection);
+    editor()->respondToChangedContents();
 }
 
 void Frame::unappliedEditing(PassRefPtr<EditCommand> cmd)
@@ -2170,7 +2170,7 @@ void Frame::unappliedEditing(PassRefPtr<EditCommand> cmd)
     d->m_lastEditCommand = 0;
     registerCommandForRedo(cmd);
     respondToChangedContents(newSelection);
-    editor()->deleteButtonController()->respondToChangedContents(newSelection);
+    editor()->respondToChangedContents();
 }
 
 void Frame::reappliedEditing(PassRefPtr<EditCommand> cmd)
@@ -2184,7 +2184,7 @@ void Frame::reappliedEditing(PassRefPtr<EditCommand> cmd)
     d->m_lastEditCommand = 0;
     registerCommandForUndo(cmd);
     respondToChangedContents(newSelection);
-    editor()->deleteButtonController()->respondToChangedContents(newSelection);
+    editor()->respondToChangedContents();
 }
 
 CSSMutableStyleDeclaration *Frame::typingStyle() const

diff --git a/WebCore/page/Frame.h b/WebCore/page/Frame.h
index 09d0ec4ee7061430f64d25b4940d6b883871b2fd..c5f22faf104f10afae469dd0a7cdc617e59285c9 100644 (file)
--- a/WebCore/page/Frame.h
+++ b/WebCore/page/Frame.h
@@ -104,7 +104,7 @@ class Frame : public Shared<Frame> {
 public:
   enum { NoXPosForVerticalArrowNavigation = INT_MIN };
 
-  Frame(Page*, Element*, EditorClient*);
+  Frame(Page*, Element*, PassRefPtr<EditorClient>);
   virtual ~Frame();
 
   virtual bool openURL(const KURL&);

diff --git a/WebCore/page/FramePrivate.h b/WebCore/page/FramePrivate.h
index 7d01268af851b1ddc5efeae30118a844b2346aa5..e1732b9330a84e397c4b79753b333b062ad51d0a 100644 (file)
--- a/WebCore/page/FramePrivate.h
+++ b/WebCore/page/FramePrivate.h
@@ -35,6 +35,7 @@
 #include "Decoder.h"
 #include "EditCommand.h"
 #include "Editor.h"
+#include "EditorClient.h"
 #include "FormData.h"
 #include "Frame.h"
 #include "FrameTree.h"
@@ -61,7 +62,7 @@ namespace WebCore {
     
     class FramePrivate {
     public:
-        FramePrivate(Page* page, Frame* parent, Frame* thisFrame, Element* ownerElement, EditorClient* client)
+        FramePrivate(Page* page, Frame* parent, Frame* thisFrame, Element* ownerElement, PassRefPtr<EditorClient> client)
             : m_page(page)
             , m_treeNode(thisFrame, parent)
             , m_ownerElement(ownerElement)