Bug 40872 - experimental directory upload
: experimental directory upload
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-06-18 20:08 PST by
Modified: 2010-07-15 13:37 PST (History)


Attachments
Patch (13.74 KB, patch)
2010-06-18 20:41 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.75 KB, patch)
2010-06-21 11:55 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.56 KB, patch)
2010-06-29 13:14 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.76 KB, patch)
2010-06-29 14:48 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (12.83 KB, patch)
2010-07-01 17:52 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (12.88 KB, patch)
2010-07-01 17:57 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (12.83 KB, patch)
2010-07-02 10:00 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2010-07-07 15:42 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (19.71 KB, patch)
2010-07-07 18:27 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (19.88 KB, patch)
2010-07-12 12:13 PST, John Gregg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.37 KB, patch)
2010-07-13 23:37 PST, John Gregg
jianli: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-06-18 20:08:39 PST
experimental directory upload
------- Comment #1 From 2010-06-18 20:41:32 PST -------
Created an attachment (id=59176) [details]
Patch
------- Comment #2 From 2010-06-21 09:43:24 PST -------
(From update of attachment 59176 [details])
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 61461)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,42 @@
> +2010-06-18  John Gregg  <johnnyg@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Experimental feature of directory attribute on <input type="file"> which
> +        enables the OS to present a folder picker instead of a file picker dialog.
> +
> +        * html/Blob.cpp:
> +        (WebCore::Blob::Blob):
> +        * html/Blob.h:
> +        * html/File.cpp:
> +        (WebCore::File::File):
> +        (WebCore::File::Init):
> +        (WebCore::File::path):
> +        * html/File.h:
> +        (WebCore::File::create):
> +        * html/File.idl:
> +        * html/HTMLAttributeNames.in:
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::setFileListFromRenderer):
> +        (WebCore::HTMLInputElement::directory):
> +        (WebCore::HTMLInputElement::setDirectory):
> +        * html/HTMLInputElement.h:
> +        * html/HTMLInputElement.idl:
> +        * platform/BlobItem.cpp:
> +        (WebCore::RelativePathFileBlobItem::create):
> +        (WebCore::RelativePathFileBlobItem::RelativePathFileBlobItem):
> +        * platform/BlobItem.h:
> +        (WebCore::FileBlobItem::hasRelativePath):
> +        (WebCore::RelativePathFileBlobItem::hasRelativePath):
> +        (WebCore::RelativePathFileBlobItem::relativePath):
> +        * platform/FileChooser.h:
> +        (WebCore::FileChooser::allowsDirectoryUpload):
> +        * platform/network/FormData.cpp:
> +        (WebCore::FormData::appendKeyValuePairItems):
> +        * rendering/RenderFileUploadControl.cpp:
> +        (WebCore::RenderFileUploadControl::allowsDirectoryUpload):
> +        * rendering/RenderFileUploadControl.h:
> +
>  2010-06-18  Anders Carlsson  <andersca@apple.com>
>  
>          Reviewed by Oliver Hunt.
> Index: WebCore/html/Blob.cpp
> ===================================================================
> --- WebCore/html/Blob.cpp	(revision 61458)
> +++ WebCore/html/Blob.cpp	(working copy)
> @@ -43,6 +43,11 @@ Blob::Blob(const String& type, const Blo
>          m_items.append(items[i]);
>  }
>  
> +Blob::Blob(const RefPtr<BlobItem>& item)
> +{
> +    m_items.append(item);
> +}
> +
>  Blob::Blob(const String& path)
>  {
>      // Note: this doesn't initialize the type unlike File(path).
> Index: WebCore/html/Blob.h
> ===================================================================
> --- WebCore/html/Blob.h	(revision 61458)
> +++ WebCore/html/Blob.h	(working copy)
> @@ -71,6 +71,7 @@ public:
>  
>  protected:
>      Blob(const String& type, const BlobItemList&);
> +    Blob(const RefPtr<BlobItem>&);
>  
>      // FIXME: Deprecated constructor.  See also the comment for Blob::create(path).
>      Blob(const String& path);
> Index: WebCore/html/File.cpp
> ===================================================================
> --- WebCore/html/File.cpp	(revision 61458)
> +++ WebCore/html/File.cpp	(working copy)
> @@ -34,6 +34,11 @@ namespace WebCore {
>  File::File(const String& path)
>      : Blob(path)
>  {
> +    Init();
> +}
> +
> +void File::Init()
> +{
>      // We don't use MIMETypeRegistry::getMIMETypeForPath() because it returns "application/octet-stream" upon failure.
>      const String& fileName = name();
>      int index = fileName.reverseFind('.');
> @@ -41,9 +46,27 @@ File::File(const String& path)
>          m_type = MIMETypeRegistry::getMIMETypeForExtension(fileName.substring(index + 1));
>  }
>  
> +#if ENABLE(DIRECTORY_UPLOAD)
> +File::File(const String& relativePath, const String& filePath)
> +    : Blob(RelativePathFileBlobItem::create(filePath, relativePath))
> +{
> +    Init();
> +}
> +#endif
> +
>  const String& File::name() const
>  {
>      return items().at(0)->toFileBlobItem()->name();
>  }
>  
> +#if ENABLE(DIRECTORY_UPLOAD)
> +const String& File::path() const
> +{
> +    const FileBlobItem* item = items().at(0)->toFileBlobItem();
> +    if (item->hasRelativePath())
> +        return static_cast<const RelativePathFileBlobItem*>(item)->relativePath();
> +    return name();
> +}
> +#endif
> +
>  } // namespace WebCore
> Index: WebCore/html/File.h
> ===================================================================
> --- WebCore/html/File.h	(revision 61458)
> +++ WebCore/html/File.h	(working copy)
> @@ -39,9 +39,19 @@ public:
>          return adoptRef(new File(path));
>      }
>  
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    static PassRefPtr<File> create(const String& relativePath, const String& file)
> +    {
> +        return adoptRef(new File(relativePath, file));
> +    }
> +#endif
> +
>      virtual bool isFile() const { return true; }
>  
>      const String& name() const;
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    const String& path() const;
> +#endif
>  
>      // FIXME: obsolete attributes. To be removed.
>      const String& fileName() const { return name(); }
> @@ -49,6 +59,11 @@ public:
>  
>  private:
>      File(const String& path);
> +    void Init();
> +
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    File(const String& relativePath, const String& path);
> +#endif
>  };
>  
>  } // namespace WebCore
> Index: WebCore/html/File.idl
> ===================================================================
> --- WebCore/html/File.idl	(revision 61458)
> +++ WebCore/html/File.idl	(working copy)
> @@ -30,6 +30,9 @@ module html {
>          GenerateToJS
>      ] File : Blob {
>          readonly attribute DOMString name;
> +#if defined(ENABLE_DIRECTORY_UPLOAD) && ENABLE_DIRECTORY_UPLOAD
> +        readonly attribute DOMString path;
> +#endif
>  
>          // FIXME: obsolete attributes. To be removed.
>          readonly attribute DOMString fileName;
> Index: WebCore/html/HTMLAttributeNames.in
> ===================================================================
> --- WebCore/html/HTMLAttributeNames.in	(revision 61458)
> +++ WebCore/html/HTMLAttributeNames.in	(working copy)
> @@ -84,6 +84,7 @@ declare
>  defer
>  dir
>  direction
> +directory
>  disabled
>  draggable
>  enctype
> Index: WebCore/html/HTMLInputElement.cpp
> ===================================================================
> --- WebCore/html/HTMLInputElement.cpp	(revision 61458)
> +++ WebCore/html/HTMLInputElement.cpp	(working copy)
> @@ -1966,8 +1966,28 @@ void HTMLInputElement::setFileListFromRe
>  {
>      m_fileList->clear();
>      int size = paths.size();
> +
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    if (directory() && size > 0) {
> +        String rootPath = directoryName(paths[0]);
> +        // Find the common root path.
> +        for (int i = 0; i < size; i++) {
> +            if (!paths[i].startsWith(rootPath)) {
> +                rootPath = directoryName(rootPath);
> +                i = 0;
> +            }
> +        }
> +        rootPath = directoryName(rootPath);
> +        for (int i = 0; i < size; i++)
> +            m_fileList->append(File::create(paths[i].substring(1 + rootPath.length()), paths[i]));
> +    } else {
> +        for (int i = 0; i < size; i++)
> +            m_fileList->append(File::create(paths[i]));
> +    }
> +#else
>      for (int i = 0; i < size; i++)
>          m_fileList->append(File::create(paths[i]));
> +#endif
>  
>      setFormControlValueMatchesRenderer(true);
>      InputElement::notifyFormStateChanged(this);
> @@ -2486,7 +2506,19 @@ void HTMLInputElement::setMultiple(bool 
>  {
>      setAttribute(multipleAttr, multiple ? "" : 0);
>  }
> -    
> +
> +#if ENABLE(DIRECTORY_UPLOAD)
> +bool HTMLInputElement::directory() const
> +{
> +    return !getAttribute(directoryAttr).isNull();
> +}
> +
> +void HTMLInputElement::setDirectory(bool directory)
> +{
> +    setAttribute(directoryAttr, directory ? "" : 0);
> +}
> +#endif
> +
>  void HTMLInputElement::setSize(unsigned size)
>  {
>      setAttribute(sizeAttr, String::number(size));
> Index: WebCore/html/HTMLInputElement.h
> ===================================================================
> --- WebCore/html/HTMLInputElement.h	(revision 61458)
> +++ WebCore/html/HTMLInputElement.h	(working copy)
> @@ -186,6 +186,11 @@ public:
>      bool multiple() const;
>      void setMultiple(bool);
>  
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    bool directory() const;
> +    void setDirectory(bool);
> +#endif
> +
>      String useMap() const;
>      void setUseMap(const String&);
>  
> Index: WebCore/html/HTMLInputElement.idl
> ===================================================================
> --- WebCore/html/HTMLInputElement.idl	(revision 61458)
> +++ WebCore/html/HTMLInputElement.idl	(working copy)
> @@ -40,6 +40,9 @@ module html {
>          attribute long maxLength setter raises(DOMException);
>          attribute [Reflect] DOMString min;
>          attribute [Reflect] boolean multiple;
> +#if defined(ENABLE_DIRECTORY_UPLOAD) && ENABLE_DIRECTORY_UPLOAD
> +        attribute [Reflect] boolean directory;
> +#endif
>          attribute [Reflect] DOMString name;
>          attribute [Reflect] DOMString pattern;
>          attribute [Reflect] DOMString placeholder;
> Index: WebCore/platform/BlobItem.cpp
> ===================================================================
> --- WebCore/platform/BlobItem.cpp	(revision 61458)
> +++ WebCore/platform/BlobItem.cpp	(working copy)
> @@ -102,6 +102,22 @@ PassRefPtr<BlobItem> FileBlobItem::slice
>  }
>  #endif // ENABLE(BLOB_SLICE)
>  
> +#if ENABLE(DIRECTORY_UPLOAD)
> +// RelativePathFileBlobItem --------------------------------------------------------
> +
> +PassRefPtr<BlobItem> RelativePathFileBlobItem::create(const String& path, const String& relativePath)
> +{
> +    return adoptRef(static_cast<BlobItem*>(new RelativePathFileBlobItem(path, relativePath)));
> +}
> +
> +RelativePathFileBlobItem::RelativePathFileBlobItem(const String& path, const String& relativePath)
> +    : FileBlobItem(path)
> +    , m_relativePath(relativePath)
> +{
> +}
> +
> +#endif
> +
>  // StringBlobItem --------------------------------------------------------------
>  
>  PassRefPtr<BlobItem> StringBlobItem::create(const String& text, LineEnding ending, TextEncoding encoding)
> Index: WebCore/platform/BlobItem.h
> ===================================================================
> --- WebCore/platform/BlobItem.h	(revision 61458)
> +++ WebCore/platform/BlobItem.h	(working copy)
> @@ -111,6 +111,10 @@ public:
>      virtual const String& name() const { return m_fileName; }
>      virtual const String& path() const { return m_path; }
>  
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    virtual bool hasRelativePath() const { return false; }
> +#endif
> +
>      // BlobItem methods.
>      virtual unsigned long long size() const;
>      virtual const FileBlobItem* toFileBlobItem() const { return this; }
> @@ -124,6 +128,20 @@ protected:
>      String m_fileName;
>  };
>  
> +#if ENABLE(DIRECTORY_UPLOAD)
> +class RelativePathFileBlobItem : public FileBlobItem {
> +public:
> +    static PassRefPtr<BlobItem> create(const String& path, const String& relativePath);
> +    virtual bool hasRelativePath() const { return true; }
> +
> +    const String& relativePath() const { return m_relativePath; }
> +
> +protected:
> +    RelativePathFileBlobItem(const String& path, const String& relativePath);
> +    String m_relativePath;
> +};
> +#endif
> +
>  class StringBlobItem : public DataBlobItem {
>  public:
>      static PassRefPtr<BlobItem> create(const String&, LineEnding, TextEncoding);
> Index: WebCore/platform/FileChooser.h
> ===================================================================
> --- WebCore/platform/FileChooser.h	(revision 61458)
> +++ WebCore/platform/FileChooser.h	(working copy)
> @@ -44,6 +44,9 @@ public:
>      virtual void valueChanged() = 0;
>      virtual void repaint() = 0;
>      virtual bool allowsMultipleFiles() = 0;
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    virtual bool allowsDirectoryUpload() = 0;
> +#endif
>      virtual String acceptTypes() = 0;
>      virtual void chooseIconForFiles(FileChooser*, const Vector<String>&) = 0;
>      virtual ~FileChooserClient();
> @@ -70,6 +73,9 @@ public:
>      void iconLoaded(PassRefPtr<Icon>);
>  
>      bool allowsMultipleFiles() const { return m_client ? m_client->allowsMultipleFiles() : false; }
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    bool allowsDirectoryUpload() const { return m_client ? m_client->allowsDirectoryUpload() : false; }
> +#endif
>      // Acceptable MIME types.  It's an 'accept' attribute value of the corresponding INPUT element.
>      String acceptTypes() const { return m_client ? m_client->acceptTypes() : String(); }
>  
> Index: WebCore/platform/network/FormData.cpp
> ===================================================================
> --- WebCore/platform/network/FormData.cpp	(revision 61458)
> +++ WebCore/platform/network/FormData.cpp	(working copy)
> @@ -225,7 +225,14 @@ void FormData::appendKeyValuePairItems(c
>              const FileBlobItem* fileItem = value->toFileBlobItem();
>              if (fileItem) {
>                  const String& path = fileItem->path();
> +
> +#if ENABLE(DIRECTORY_UPLOAD)
> +                String fileName = fileItem->hasRelativePath() ?
> +                    static_cast<const RelativePathFileBlobItem*>(fileItem)->relativePath() :
> +                    fileItem->name();
> +#else
>                  String fileName = fileItem->name();
> +#endif
>  
>                  // Let the application specify a filename if it's going to generate a replacement file for the upload.
>                  if (!path.isEmpty()) {
> Index: WebCore/rendering/RenderFileUploadControl.cpp
> ===================================================================
> --- WebCore/rendering/RenderFileUploadControl.cpp	(revision 61458)
> +++ WebCore/rendering/RenderFileUploadControl.cpp	(working copy)
> @@ -98,6 +98,14 @@ bool RenderFileUploadControl::allowsMult
>      return !input->getAttribute(multipleAttr).isNull();
>  }
>  
> +#if ENABLE(DIRECTORY_UPLOAD)
> +bool RenderFileUploadControl::allowsDirectoryUpload()
> +{
> +    HTMLInputElement* input = static_cast<HTMLInputElement*>(node());
> +    return !input->getAttribute(directoryAttr).isNull();
> +}
> +#endif
> +
>  String RenderFileUploadControl::acceptTypes()
>  {
>      return static_cast<HTMLInputElement*>(node())->accept();
> Index: WebCore/rendering/RenderFileUploadControl.h
> ===================================================================
> --- WebCore/rendering/RenderFileUploadControl.h	(revision 61458)
> +++ WebCore/rendering/RenderFileUploadControl.h	(working copy)
> @@ -62,6 +62,9 @@ private:
>      void valueChanged();
>      void repaint() { RenderBlock::repaint(); }
>      bool allowsMultipleFiles();
> +#if ENABLE(DIRECTORY_UPLOAD)
> +    bool allowsDirectoryUpload();
> +#endif
>      String acceptTypes();
>      void chooseIconForFiles(FileChooser*, const Vector<String>&);
>  

WebCore/html/Blob.cpp:48
 +      m_items.append(item);
Quick drive-by comment: I think it's more efficient to pass in a PassRefPtr instead, as you can avoid an extra ref/deref in the case where the caller doesn't want to hold on to a reference (in fact, I think in your File constructor you are passing in a PassRefPtr anyway, which means the caller has to instantiate an extra temporary RefPtr.
------- Comment #3 From 2010-06-21 11:55:26 PST -------
Created an attachment (id=59269) [details]
Patch
------- Comment #4 From 2010-06-21 12:04:05 PST -------
(In reply to comment #2)

> WebCore/html/Blob.cpp:48
>  +      m_items.append(item);
> Quick drive-by comment: I think it's more efficient to pass in a PassRefPtr
> instead, as you can avoid an extra ref/deref in the case where the caller 
> doesn't want to hold on to a reference (in fact, I think in your File 
> constructor you are passing in a PassRefPtr anyway, which means the caller
> has to instantiate an extra temporary RefPtr.

Fixed in updated patch.
------- Comment #5 From 2010-06-23 00:04:50 PST -------
(From update of attachment 59269 [details])

We need more preparation to introduce new feature flag.
https://bugs.webkit.org/show_bug.cgi?id=40878 is a good example.

WebCore/html/HTMLInputElement.cpp:1975
 +              if (!paths[i].startsWith(rootPath)) {
Do we accept multiple directories?
If so and 'paths' contains the followings, full path names are exposed, right?
  - C:/data/1.jpg
  - C:/data/2.jpg
  - E:/data/1.jpg


WebCore/html/HTMLInputElement.cpp:1977
 +                  i = 0;
Why go back to 0?


WebCore/html/HTMLInputElement.h:190
 +      bool directory() const;
Do you need directory() and setDirectory()?  If you don't use them in C++ code, we can remove them.
------- Comment #6 From 2010-06-23 14:40:17 PST -------
(In reply to comment #5)
> (From update of attachment 59269 [details] [details])
> 
> We need more preparation to introduce new feature flag.
> https://bugs.webkit.org/show_bug.cgi?id=40878 is a good example.
> 

Yes, I am intentionally doing that separately to avoid the noise in this review.  I've now posted it to https://bugs.webkit.org/show_bug.cgi?id=41100

> WebCore/html/HTMLInputElement.cpp:1975
>  +              if (!paths[i].startsWith(rootPath)) {
> Do we accept multiple directories?
> If so and 'paths' contains the followings, full path names are exposed, right?
>   - C:/data/1.jpg
>   - C:/data/2.jpg
>   - E:/data/1.jpg

The UI will only allow one directory to be chosen, so this shouldn't happen.  Do you think I should add more protection for that?

> 
> WebCore/html/HTMLInputElement.cpp:1977
>  +                  i = 0;
> Why go back to 0?

It's necessary for cases like
root/a/b/c.txt
root/d/e/f.txt

If I don't go back to the start, I will miss the fact that the root is 3 levels up.  Alternatively I could change if (!paths[i].startsWith(rootPath)) to be while (...).

> 
> 
> WebCore/html/HTMLInputElement.h:190
>  +      bool directory() const;
> Do you need directory() and setDirectory()?  If you don't use them in C++ code, we can remove them.

I do use directory() in code, but not setDirectory.  I can take that out.
------- Comment #7 From 2010-06-24 20:27:50 PST -------
(In reply to comment #6)
> > Do we accept multiple directories?

> The UI will only allow one directory to be chosen, so this shouldn't happen.  Do you think I should add more protection for that?

I see.  I think we need a comment and ASSERT(rootPath.length())

> > WebCore/html/HTMLInputElement.cpp:1977
> >  +                  i = 0;
> > Why go back to 0?
> 
> It's necessary for cases like
> root/a/b/c.txt
> root/d/e/f.txt
> 
> If I don't go back to the start, I will miss the fact that the root is 3 levels up.  Alternatively I could change if (!paths[i].startsWith(rootPath)) to be while (...).

Understand.  Thanks.
------- Comment #8 From 2010-06-29 13:14:35 PST -------
Created an attachment (id=60044) [details]
Patch
------- Comment #9 From 2010-06-29 13:49:36 PST -------
Attachment 60044 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3383003
------- Comment #10 From 2010-06-29 14:48:49 PST -------
Created an attachment (id=60059) [details]
Patch
------- Comment #11 From 2010-06-29 18:36:38 PST -------
HTMLInputElement and FileChooser parts look ok.
Jian,  would you take a look at Blob part please?
------- Comment #12 From 2010-06-30 18:41:02 PST -------
(From update of attachment 60059 [details])
FYI, I am in refactoring File and Blob in order to better support all kinds of blob scenarios. The BlobItem will probably go away while we will introduce BlobDataItem that is managed by centralized BlobRegistry. Not sure how we can get things resolved and merged.

WebCore/html/File.idl:33
 +  #if defined(ENABLE_DIRECTORY_UPLOAD) && ENABLE_DIRECTORY_UPLOAD
We should use "Conditional" here, like 
  readonly attribute [Conditional=DIRECTORY_UPLOAD] DOMString path; 

WebCore/platform/BlobItem.h:115
 +      virtual bool hasRelativePath() const { return false; }
Why not just having "const String& relativePath()" in FileBlobItem?
------- Comment #13 From 2010-06-30 18:49:32 PST -------
(In reply to comment #12)
> (From update of attachment 60059 [details] [details])
> FYI, I am in refactoring File and Blob in order to better support all kinds of blob scenarios. The BlobItem will probably go away while we will introduce BlobDataItem that is managed by centralized BlobRegistry. Not sure how we can get things resolved and merged.

:(  I've already rewritten this code once to deal with the last File/Blob refactor.  What is your timeframe for that?

> 
> WebCore/html/File.idl:33
>  +  #if defined(ENABLE_DIRECTORY_UPLOAD) && ENABLE_DIRECTORY_UPLOAD
> We should use "Conditional" here, like 
>   readonly attribute [Conditional=DIRECTORY_UPLOAD] DOMString path; 

Seems okay, but the other style is far more prevalent in webkit IDL

> WebCore/platform/BlobItem.h:115
>  +      virtual bool hasRelativePath() const { return false; }
> Why not just having "const String& relativePath()" in FileBlobItem?

I think the relative path should only be included in the form-data if it's a directory upload.
------- Comment #14 From 2010-07-01 17:52:39 PST -------
Created an attachment (id=60322) [details]
Patch
------- Comment #15 From 2010-07-01 17:57:41 PST -------
Created an attachment (id=60323) [details]
Patch
------- Comment #16 From 2010-07-02 03:03:39 PST -------
Attachment 60323 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3391078
------- Comment #17 From 2010-07-02 03:17:13 PST -------
Attachment 60323 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3350339
------- Comment #18 From 2010-07-02 10:00:47 PST -------
Created an attachment (id=60374) [details]
Patch
------- Comment #19 From 2010-07-02 10:29:32 PST -------
(From update of attachment 60374 [details])
WebCore/ChangeLog:5
 +          Experimental feature of directory attribute on <input type="file"> which
Please first have the title, like the bug title and then follow by the bug link. After that, you can give the full description. Like:
  Experimental directory upload.
  https://bugs.webkit.org/show_bug.cgi?id=40872

  Experimental feature of directory attribute on ...

Do you have the test to cover this new feature?

WebCore/html/File.cpp:50
 +  File::File(const String& relativePath, const String& filePath)
Please move this constructor above File::Init().

WebCore/html/File.h:53
 +      const String& path() const;
Do we need to expose this? I am not seeing any reference to it.
If you do want to expose this, please add the comment to document this path is relative path and also change Blob::path() to Blob::fullPath() to avoid the confusion.

WebCore/html/File.idl:34
 +          readonly attribute DOMString path;
Better to use Conditional attribute instead of "#if defined". Please also comment that we only expose relative path.

WebCore/html/HTMLInputElement.idl:44
 +          attribute [Reflect] boolean webkitdirectory;
Better to use Conditional attribute.

WebCore/html/HTMLInputElement.cpp:1973
 +      // and the paths provided here share a single root directory.
The comment "the paths provided here share a single root directory" seems not to reflect what is coded here.

WebCore/html/HTMLInputElement.cpp:1977
 +          for (int i = 0; i < size; i++) {
We can start from 1.
------- Comment #20 From 2010-07-02 10:31:32 PST -------
> WebCore/html/File.h:53
>  +      const String& path() const;
> Do we need to expose this? I am not seeing any reference to it.
> If you do want to expose this, please add the comment to document this path is relative path and also change Blob::path() to Blob::fullPath() to avoid the confusion.
>

Sorry, I just realized that it is exposed in File.idl. Please address my 2nd comment.
------- Comment #21 From 2010-07-02 11:17:10 PST -------
Attachment 60374 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3380148
------- Comment #22 From 2010-07-07 15:42:42 PST -------
Created an attachment (id=60791) [details]
Patch
------- Comment #23 From 2010-07-07 17:55:11 PST -------
(From update of attachment 60791 [details])
Where is WebCore/ChangeLog?
------- Comment #24 From 2010-07-07 17:59:39 PST -------
(In reply to comment #23)
> (From update of attachment 60791 [details] [details])
> Where is WebCore/ChangeLog?

It was in SVN conflict state so it didn't get uploaded.  Uploading again now.
------- Comment #25 From 2010-07-07 18:27:36 PST -------
Created an attachment (id=60820) [details]
Patch
------- Comment #26 From 2010-07-09 15:35:53 PST -------
(From update of attachment 60820 [details])
WebCore/html/File.h:53
 +      const String& path() const;
Please comment that we're returning a relative path here. Please also change Blob::path() to Blob::absolutePath() to avoid the confusion.

LayoutTests/fast/forms/input-file-directory-upload.html:9
 +      {'path': 'resources/dirupload/path1/file1',                   'relpath': 'dirupload/path1/file1'},
Do we really need both 'path' and 'relpath"? Can we just keep relpath in the list and have the map function inserts 'resource/' at the beginning? 

Can we add some test cases to try to cover as many scenarios as possible, like:
  1) dirupload/path1/file1, dirupload/path1/file2
  2) dirupload/path1/file1, dirupload/path1/path2/file1
  ...
------- Comment #27 From 2010-07-12 12:11:37 PST -------
(In reply to comment #26)
> (From update of attachment 60820 [details] [details])
> WebCore/html/File.h:53
>  +      const String& path() const;
> Please comment that we're returning a relative path here. Please also change Blob::path() to Blob::absolutePath() to avoid the confusion.

To avoid the confusion completely, I have renamed File::path() to File::webkitRelativePath(), which is a better interface name for this experimental feature, and left Blob unchanged.  I also added the comment.

> 
> LayoutTests/fast/forms/input-file-directory-upload.html:9
>  +      {'path': 'resources/dirupload/path1/file1',                   'relpath': 'dirupload/path1/file1'},
> Do we really need both 'path' and 'relpath"? Can we just keep relpath in the list and have the map function inserts 'resource/' at the beginning? 

I changed this to be more clear.  'relpath' is storing the expected relative path to result from 'path'.  If we derive one from the other it somewhat defeats the point of the test (and makes other test cases harder), so I renamed 'relpath' to 'expected-relpath'.

> 
> Can we add some test cases to try to cover as many scenarios as possible, like:
>   1) dirupload/path1/file1, dirupload/path1/file2
>   2) dirupload/path1/file1, dirupload/path1/path2/file1
>   ...

I added several more test cases.
------- Comment #28 From 2010-07-12 12:13:34 PST -------
Created an attachment (id=61254) [details]
Patch
------- Comment #29 From 2010-07-13 18:23:12 PST -------
(From update of attachment 61254 [details])
I only see the file "mac/Skipped", not other Skipped files. By the way, have you also tested it on the chromium platform?

LayoutTests/ChangeLog:5
 +          Unit test for experimental directory upload feature.  Only enabled on chromium since that's where the feature is compiled in.
Please remove "Unit" since this is not called unit test in WebKit.

LayoutTests/fast/forms/input-file-directory-upload.html:8
 +  var currentFileList_;
Please do not add the trailing "_" for the variable.

LayoutTests/fast/forms/input-file-directory-upload.html:9
 +  var lastTest_ = false;
ditto.

LayoutTests/fast/forms/input-file-directory-upload.html:60
 +      currentFileList_ = fileList;
Is it possible to avoid using global variables?
Also, is there any possibility we have some sort of indeterminism here? What I mean is that: do we always execute the JS code in the following order:
  doTest(testFileList1)
  onInputFileChange
  doTest(testFileList2)
  onInputFileChange
  ...
------- Comment #30 From 2010-07-13 23:31:41 PST -------
(In reply to comment #29)
> (From update of attachment 61254 [details] [details])
> I only see the file "mac/Skipped", not other Skipped files. By the way, have you also tested it on the chromium platform?
> 

More merge conflicts.   Fixed now.  After this I think it's time to change webkit-patch not to upload when there are conflicts.

> LayoutTests/ChangeLog:5
>  +          Unit test for experimental directory upload feature.  Only enabled on chromium since that's where the feature is compiled in.
> Please remove "Unit" since this is not called unit test in WebKit.
> 

Done.  "Layout test"

> LayoutTests/fast/forms/input-file-directory-upload.html:8
>  +  var currentFileList_;
> Please do not add the trailing "_" for the variable.
> 

Done.

> LayoutTests/fast/forms/input-file-directory-upload.html:9
>  +  var lastTest_ = false;
> ditto.
> 

Done.

> LayoutTests/fast/forms/input-file-directory-upload.html:60
>  +      currentFileList_ = fileList;
> Is it possible to avoid using global variables?

Okay, I rearranged things to not use the globals.

> Also, is there any possibility we have some sort of indeterminism here? What I mean is that: do we always execute the JS code in the following order:
>   doTest(testFileList1)
>   onInputFileChange
>   doTest(testFileList2)
>   onInputFileChange
>   ...

This certainly seems to be the case, that the drag events are queued up in order.
------- Comment #31 From 2010-07-13 23:37:23 PST -------
Created an attachment (id=61474) [details]
Patch
------- Comment #32 From 2010-07-14 18:26:36 PST -------
(From update of attachment 61474 [details])
r=me

Minor comment. Please address it before landing your patch.

WebCore/html/File.h:43
 +      static PassRefPtr<File> create(const String& relativePath, const String& file)
Please change the argument name from 'file' to 'path'.
------- Comment #33 From 2010-07-15 13:37:37 PST -------
Committed r63454: <http://trac.webkit.org/changeset/63454>