Bug 28883 - [V8] FileList cannot be accessed via index in Chromium.
Summary: [V8] FileList cannot be accessed via index in Chromium.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-01 11:57 PDT by Jian Li
Modified: 2009-09-01 13:17 PDT (History)
2 users (show)

See Also:


Attachments
Proposed Patch (4.98 KB, patch)
2009-09-01 12:11 PDT, Jian Li
dglazkov: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-09-01 11:57:10 PDT
Need to fix V8 binding code to access the indexed property of FileList.
Comment 1 Jian Li 2009-09-01 12:11:11 PDT
Created attachment 38877 [details]
Proposed Patch
Comment 2 Dimitri Glazkov (Google) 2009-09-01 12:14:25 PDT
Comment on attachment 38877 [details]
Proposed Patch

r=me.
Comment 3 David Levin 2009-09-01 12:18:57 PDT
Hmm.. twice reviewed :)

Consider fixing these on landing.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-09-01  Jian Li  <jianli@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [V8] FileList cannot be accessed via index in Chromium.
> +        https://bugs.webkit.org/show_bug.cgi?id=28883
> +
> +        The test is covered by clipboard-file-access.html.

Tested by clipboard-file-access.html.

> diff --git a/WebCore/bindings/v8/custom/V8FileListCustom.cpp b/WebCore/bindings/v8/custom/V8FileListCustom.cpp

> +#include "config.h"
> +#include "FileList.h"
> +
> +#include "File.h"

I don't understand why there is a blank line here.

> +
> +#include "V8Binding.h"
> +#include "V8CustomBinding.h"

> +INDEXED_PROPERTY_GETTER(FileList)
> +{
> +    INC_STATS("DOM.FileList.IndexedPropertyGetter");
> +    FileList* imp = V8DOMWrapper::convertToNativeObject<FileList>(V8ClassIndex::FILELIST, info.Holder());

Use full words for variables "imp".  Perhaps "fileList" instead?

> +    RefPtr<File> result = imp->item(index);

"result" doesn't sound like a very descriptive variable name.
How about "file"?
Comment 4 Jian Li 2009-09-01 13:17:50 PDT
All fixed and committed as http://trac.webkit.org/changeset/47947.