Fix FileChooserClient design

https://bugs.webkit.org/show_bug.cgi?id=116195

Reviewed by Andreas Kling.

FileChooserClient doesn't match the standard WebCore client idiom of only having virtual member functions.
Instead it holds on to its FileChooser and it's even a factory for creating new file choosers(!).

Fix this by making it an abstract class, and moving FileChooser into FileInputType.

* html/FileInputType.cpp:
(WebCore::FileInputType::~FileInputType):
Invalidate the file chooser.

(WebCore::FileInputType::handleDOMActivateEvent):
Apply the file chooser settings.

(WebCore::FileInputType::applyFileChooserSettings):
Recreate the file chooser with new settings.

(WebCore::FileInputType::receiveDropForDirectoryUpload):
Apply the settings.

* platform/FileChooser.cpp:
(WebCore::FileChooser::invalidate):
Set m_client to null.

(WebCore::FileChooser::chooseFiles):
Early return.

* platform/FileChooser.h:

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 95afcbb..9b24b34 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,37 @@
+2013-05-15  Anders Carlsson  <andersca@apple.com>
+
+        Fix FileChooserClient design
+        https://bugs.webkit.org/show_bug.cgi?id=116195
+
+        Reviewed by Andreas Kling.
+
+        FileChooserClient doesn't match the standard WebCore client idiom of only having virtual member functions.
+        Instead it holds on to its FileChooser and it's even a factory for creating new file choosers(!).
+
+        Fix this by making it an abstract class, and moving FileChooser into FileInputType.
+
+        * html/FileInputType.cpp:
+        (WebCore::FileInputType::~FileInputType):
+        Invalidate the file chooser.
+
+        (WebCore::FileInputType::handleDOMActivateEvent):
+        Apply the file chooser settings.
+
+        (WebCore::FileInputType::applyFileChooserSettings):
+        Recreate the file chooser with new settings.
+
+        (WebCore::FileInputType::receiveDropForDirectoryUpload):
+        Apply the settings.
+
+        * platform/FileChooser.cpp:
+        (WebCore::FileChooser::invalidate):
+        Set m_client to null.
+
+        (WebCore::FileChooser::chooseFiles):
+        Early return.
+
+        * platform/FileChooser.h:
+
 2013-05-15  Gavin Barraclough  <barraclough@apple.com>
 
         ScriptedAnimationController::setThrottled should extend MinimumAnimationInterval

diff --git a/Source/WebCore/html/FileInputType.cpp b/Source/WebCore/html/FileInputType.cpp
index b0cff83..e8594d3 100644 (file)
--- a/Source/WebCore/html/FileInputType.cpp
+++ b/Source/WebCore/html/FileInputType.cpp
@@ -86,15 +86,21 @@ const AtomicString& UploadButtonElement::shadowPseudoId() const
     return pseudoId;
 }
 
-inline FileInputType::FileInputType(HTMLInputElement* element)
+PassOwnPtr<InputType> FileInputType::create(HTMLInputElement* element)
+{
+    return adoptPtr(new FileInputType(element));
+}
+
+FileInputType::FileInputType(HTMLInputElement* element)
     : BaseClickableWithKeyInputType(element)
     , m_fileList(FileList::create())
 {
 }
 
-PassOwnPtr<InputType> FileInputType::create(HTMLInputElement* element)
+FileInputType::~FileInputType()
 {
-    return adoptPtr(new FileInputType(element));
+    if (m_fileChooser)
+        m_fileChooser->invalidate();
 }
 
 Vector<FileChooserFileInfo> FileInputType::filesFromFormControlState(const FormControlState& state)
@@ -196,8 +202,11 @@ void FileInputType::handleDOMActivateEvent(Event* event)
 #if ENABLE(MEDIA_CAPTURE)
         settings.capture = input->capture();
 #endif
-        chrome->runOpenPanel(input->document()->frame(), newFileChooser(settings));
+
+        applyFileChooserSettings(settings);
+        chrome->runOpenPanel(input->document()->frame(), m_fileChooser);
     }
+
     event->setDefaultHandled();
 }
 
@@ -330,6 +339,14 @@ void FileInputType::requestIcon(const Vector<String>& paths)
         chrome->loadIconForFiles(paths, newFileIconLoader());
 }
 
+void FileInputType::applyFileChooserSettings(const FileChooserSettings& settings)
+{
+    if (m_fileChooser)
+        m_fileChooser->invalidate();
+
+    m_fileChooser = FileChooser::create(this, settings);
+}
+
 void FileInputType::setFiles(PassRefPtr<FileList> files)
 {
     if (!files)
@@ -379,16 +396,20 @@ void FileInputType::filesChosen(const Vector<FileChooserFileInfo>& files)
 #if ENABLE(DIRECTORY_UPLOAD)
 void FileInputType::receiveDropForDirectoryUpload(const Vector<String>& paths)
 {
-    if (Chrome* chrome = this->chrome()) {
-        FileChooserSettings settings;
-        HTMLInputElement* input = element();
-        settings.allowsDirectoryUpload = true;
-        settings.allowsMultipleFiles = true;
-        settings.selectedFiles.append(paths[0]);
-        settings.acceptMIMETypes = input->acceptMIMETypes();
-        settings.acceptFileExtensions = input->acceptFileExtensions();
-        chrome->enumerateChosenDirectory(newFileChooser(settings));
-    }
+    Chrome* chrome = this->chrome();
+    if (!chrome)
+        return;
+
+    FileChooserSettings settings;
+    HTMLInputElement* input = element();
+    settings.allowsDirectoryUpload = true;
+    settings.allowsMultipleFiles = true;
+    settings.selectedFiles.append(paths[0]);
+    settings.acceptMIMETypes = input->acceptMIMETypes();
+    settings.acceptFileExtensions = input->acceptFileExtensions();
+
+    applyFileChooserSettings(settings);
+    chrome->enumerateChosenDirectory(m_fileChooser);
 }
 #endif
 
@@ -466,4 +487,5 @@ String FileInputType::defaultToolTip() const
     return names.toString();
 }
 
+
 } // namespace WebCore

diff --git a/Source/WebCore/html/FileInputType.h b/Source/WebCore/html/FileInputType.h
index a0f4292..edaa1b4 100644 (file)
--- a/Source/WebCore/html/FileInputType.h
+++ b/Source/WebCore/html/FileInputType.h
@@ -45,10 +45,13 @@ class FileList;
 class FileInputType : public BaseClickableWithKeyInputType, private FileChooserClient, private FileIconLoaderClient {
 public:
     static PassOwnPtr<InputType> create(HTMLInputElement*);
+    virtual ~FileInputType();
+
     static Vector<FileChooserFileInfo> filesFromFormControlState(const FormControlState&);
 
 private:
     FileInputType(HTMLInputElement*);
+
     virtual const AtomicString& formControlType() const OVERRIDE;
     virtual FormControlState saveFormControlState() const OVERRIDE;
     virtual void restoreFormControlState(const FormControlState&) OVERRIDE;
@@ -87,6 +90,10 @@ private:
 #endif
     void requestIcon(const Vector<String>&);
 
+    void applyFileChooserSettings(const FileChooserSettings&);
+
+    RefPtr<FileChooser> m_fileChooser;
+
     RefPtr<FileList> m_fileList;
     RefPtr<Icon> m_icon;
 

diff --git a/Source/WebCore/platform/FileChooser.cpp b/Source/WebCore/platform/FileChooser.cpp
index cd50ee2..dc971b3 100644 (file)
--- a/Source/WebCore/platform/FileChooser.cpp
+++ b/Source/WebCore/platform/FileChooser.cpp
@@ -31,26 +31,7 @@
 
 namespace WebCore {
 
-FileChooserClient::~FileChooserClient()
-{
-    discardChooser();
-}
-
-FileChooser* FileChooserClient::newFileChooser(const FileChooserSettings& settings)
-{
-    discardChooser();
-
-    m_chooser = FileChooser::create(this, settings);
-    return m_chooser.get();
-}
-
-void FileChooserClient::discardChooser()
-{
-    if (m_chooser)
-        m_chooser->disconnectClient();
-}
-
-inline FileChooser::FileChooser(FileChooserClient* client, const FileChooserSettings& settings)
+FileChooser::FileChooser(FileChooserClient* client, const FileChooserSettings& settings)
     : m_client(client)
     , m_settings(settings)
 {
@@ -65,6 +46,13 @@ FileChooser::~FileChooser()
 {
 }
 
+void FileChooser::invalidate()
+{
+    ASSERT(m_client);
+
+    m_client = 0;
+}
+
 void FileChooser::chooseFile(const String& filename)
 {
     Vector<String> filenames;
@@ -78,12 +66,13 @@ void FileChooser::chooseFiles(const Vector<String>& filenames)
     if (m_settings.selectedFiles == filenames)
         return;
 
-    if (m_client) {
-        Vector<FileChooserFileInfo> files;
-        for (unsigned i = 0; i < filenames.size(); ++i)
-            files.append(FileChooserFileInfo(filenames[i]));
-        m_client->filesChosen(files);
-    }
+    if (!m_client)
+        return;
+
+    Vector<FileChooserFileInfo> files;
+    for (unsigned i = 0; i < filenames.size(); ++i)
+        files.append(FileChooserFileInfo(filenames[i]));
+    m_client->filesChosen(files);
 }
 
 void FileChooser::chooseFiles(const Vector<FileChooserFileInfo>& files)
@@ -92,6 +81,7 @@ void FileChooser::chooseFiles(const Vector<FileChooserFileInfo>& files)
     Vector<String> paths;
     for (unsigned i = 0; i < files.size(); ++i)
         paths.append(files[i].path);
+
     if (m_settings.selectedFiles == paths)
         return;
 

diff --git a/Source/WebCore/platform/FileChooser.h b/Source/WebCore/platform/FileChooser.h
index e6f6146..6c023e3 100644 (file)
--- a/Source/WebCore/platform/FileChooser.h
+++ b/Source/WebCore/platform/FileChooser.h
@@ -67,16 +67,9 @@ struct FileChooserSettings {
 
 class FileChooserClient {
 public:
-    virtual void filesChosen(const Vector<FileChooserFileInfo>&) = 0;
-    virtual ~FileChooserClient();
-
-protected:
-    FileChooser* newFileChooser(const FileChooserSettings&);
+    virtual ~FileChooserClient() { }
 
-private:
-    void discardChooser();
-
-    RefPtr<FileChooser> m_chooser;
+    virtual void filesChosen(const Vector<FileChooserFileInfo>&) = 0;
 };
 
 class FileChooser : public RefCounted<FileChooser> {
@@ -84,7 +77,7 @@ public:
     static PassRefPtr<FileChooser> create(FileChooserClient*, const FileChooserSettings&);
     ~FileChooser();
 
-    void disconnectClient() { m_client = 0; }
+    void invalidate();
 
     void chooseFile(const String& path);
     void chooseFiles(const Vector<String>& paths);