Bug 40872 – experimental directory upload

• Component: New Bugs
• Status: RESOLVED FIXED

[John Gregg]

experimental directory upload

[John Gregg]

Created attachment 59176 [details]
Patch

[Andrew Wilson]

Comment on attachment 59176 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 61461)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,42 @@
> [phone]  John Gregg  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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.

[John Gregg]

Created attachment 59269 [details]
Patch

[John Gregg]

(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.

[Kent Tamura]

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.

[John Gregg]

(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.

[Kent Tamura]

(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.

[John Gregg]

Created attachment 60044 [details]
Patch

[WebKit Review Bot]

Attachment 60044 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3383003

[John Gregg]

Created attachment 60059 [details]
Patch

[Kent Tamura]

HTMLInputElement and FileChooser parts look ok.
Jian,  would you take a look at Blob part please?

[Jian Li]

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?

[John Gregg]

(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.

[John Gregg]

Created attachment 60322 [details]
Patch

[John Gregg]

Created attachment 60323 [details]
Patch

[WebKit Review Bot]

Attachment 60323 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3391078

[WebKit Review Bot]

Attachment 60323 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3350339

[John Gregg]

Created attachment 60374 [details]
Patch

[Jian Li]

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.

[Jian Li]

> 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.

[WebKit Review Bot]

Attachment 60374 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3380148

[John Gregg]

Created attachment 60791 [details]
Patch

[Jian Li]

Comment on attachment 60791 [details]
Patch

Where is WebCore/ChangeLog?

[John Gregg]

(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.

[John Gregg]

Created attachment 60820 [details]
Patch

[Jian Li]

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
  ...

[John Gregg]

(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.

[John Gregg]

Created attachment 61254 [details]
Patch

[Jian Li]

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
  ...

[John Gregg]

(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.

[John Gregg]

Created attachment 61474 [details]
Patch

[Jian Li]

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'.

[John Gregg]

Committed r63454: <http://trac.webkit.org/changeset/63454>