Bug 40872 - experimental directory upload
: experimental directory upload
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: John Gregg
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-18 20:08 PDT by John Gregg
Modified: 2010-07-15 13:37 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description John Gregg 2010-06-18 20:08:39 PDT
experimental directory upload
Comment 1 John Gregg 2010-06-18 20:41:32 PDT
Created attachment 59176 [details]
Patch
Comment 2 Andrew Wilson 2010-06-21 09:43:24 PDT
Comment on attachment 59176 [details]
Patch

> 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 John Gregg 2010-06-21 11:55:26 PDT
Created attachment 59269 [details]
Patch
Comment 4 John Gregg 2010-06-21 12:04:05 PDT
(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 Kent Tamura 2010-06-23 00:04:50 PDT
Comment on attachment 59269 [details]
Patch


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 John Gregg 2010-06-23 14:40:17 PDT
(In reply to comment #5)
> (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.
> 

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 Kent Tamura 2010-06-24 20:27:50 PDT
(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 John Gregg 2010-06-29 13:14:35 PDT
Created attachment 60044 [details]
Patch
Comment 9 WebKit Review Bot 2010-06-29 13:49:36 PDT
Attachment 60044 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3383003
Comment 10 John Gregg 2010-06-29 14:48:49 PDT
Created attachment 60059 [details]
Patch
Comment 11 Kent Tamura 2010-06-29 18:36:38 PDT
HTMLInputElement and FileChooser parts look ok.
Jian,  would you take a look at Blob part please?
Comment 12 Jian Li 2010-06-30 18:41:02 PDT
Comment on attachment 60059 [details]
Patch

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 John Gregg 2010-06-30 18:49:32 PDT
(In reply to comment #12)
> (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.

:(  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 John Gregg 2010-07-01 17:52:39 PDT
Created attachment 60322 [details]
Patch
Comment 15 John Gregg 2010-07-01 17:57:41 PDT
Created attachment 60323 [details]
Patch
Comment 16 WebKit Review Bot 2010-07-02 03:03:39 PDT
Attachment 60323 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3391078
Comment 17 WebKit Review Bot 2010-07-02 03:17:13 PDT
Attachment 60323 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3350339
Comment 18 John Gregg 2010-07-02 10:00:47 PDT
Created attachment 60374 [details]
Patch
Comment 19 Jian Li 2010-07-02 10:29:32 PDT
Comment on attachment 60374 [details]
Patch

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 Jian Li 2010-07-02 10:31:32 PDT
> 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 WebKit Review Bot 2010-07-02 11:17:10 PDT
Attachment 60374 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3380148
Comment 22 John Gregg 2010-07-07 15:42:42 PDT
Created attachment 60791 [details]
Patch
Comment 23 Jian Li 2010-07-07 17:55:11 PDT
Comment on attachment 60791 [details]
Patch

Where is WebCore/ChangeLog?
Comment 24 John Gregg 2010-07-07 17:59:39 PDT
(In reply to comment #23)
> (From update of attachment 60791 [details])
> Where is WebCore/ChangeLog?

It was in SVN conflict state so it didn't get uploaded.  Uploading again now.
Comment 25 John Gregg 2010-07-07 18:27:36 PDT
Created attachment 60820 [details]
Patch
Comment 26 Jian Li 2010-07-09 15:35:53 PDT
Comment on attachment 60820 [details]
Patch

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 John Gregg 2010-07-12 12:11:37 PDT
(In reply to comment #26)
> (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.

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 John Gregg 2010-07-12 12:13:34 PDT
Created attachment 61254 [details]
Patch
Comment 29 Jian Li 2010-07-13 18:23:12 PDT
Comment on attachment 61254 [details]
Patch

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 John Gregg 2010-07-13 23:31:41 PDT
(In reply to comment #29)
> (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?
> 

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 John Gregg 2010-07-13 23:37:23 PDT
Created attachment 61474 [details]
Patch
Comment 32 Jian Li 2010-07-14 18:26:36 PDT
Comment on attachment 61474 [details]
Patch

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 John Gregg 2010-07-15 13:37:37 PDT
Committed r63454: <http://trac.webkit.org/changeset/63454>