Bug 68916 – [fileapi] WebKitFlags should not be constructable per Directories & System spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Klein]

Per http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#the-flags-interface, Flags is marked as [NoInterfaceObject], yet in Chromium WebKitFlags is a constructor hanging off of DOMWindow.  And there's custom V8 code that knows to transform a V8WebKitFlags into a WebKitFlags.

According to ericu, WebKitFlags shouldn't actually be constructable, so this bug just calls for deletion.  We shouldn't have to worry much about compat, since the normal use case is to just use an object literal in the API and the thing is vendor-prefixed anyway.

[Eric U.]

Created attachment 108911 [details]
Patch

[Early Warning System Bot]

Comment on attachment 108911 [details]
Patch

Attachment 108911 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9883386

[Eric U.]

Created attachment 108919 [details]
Added missing build file changes.

[Kinuko Yasuda]

Comment on attachment 108919 [details]
Added missing build file changes.

View in context: https://bugs.webkit.org/attachment.cgi?id=108919&action=review

Thanks for fixing this!

> Source/WebCore/bindings/js/JSDirectoryEntryCustom.cpp:69
> +        return jsUndefined();

I don't think we should return here?  (At least we should handle null and undefined cases)

[Eric U.]

Comment on attachment 108919 [details]
Added missing build file changes.

View in context: https://bugs.webkit.org/attachment.cgi?id=108919&action=review

>> Source/WebCore/bindings/js/JSDirectoryEntryCustom.cpp:69
>> +        return jsUndefined();
> 
> I don't think we should return here?  (At least we should handle null and undefined cases)

Ah, you're right--I changed my mind on that and reverted that code elsewhere, but missed this one.  I'll fix it.

[Eric U.]

Created attachment 113393 [details]
I changed it just to ignore any non-object arguments, just as if you'd passed in null or {}.

[WebKit Review Bot]

Comment on attachment 113393 [details]
I changed it just to ignore any non-object arguments, just as if you'd passed in null or {}.

Rejecting attachment 113393 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
bKitFlags.idl'
patching file Source/WebCore/page/DOMWindow.idl
Hunk #1 succeeded at 195 with fuzz 2.
patching file Source/WebCore/workers/WorkerContext.idl
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/filesystem/flags-passing-expected.txt
patching file LayoutTests/fast/filesystem/script-tests/flags-passing.js

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10996182

[Eric U.]

Created attachment 120242 [details]
Patch

[WebKit Review Bot]

Attachment 120242 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167522 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
CONFLICT (content): Merge conflict in LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/page/ScrollingCoordinator.h
CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h
Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
Auto-merging Source/WebCore/platform/ScrollView.cpp
Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp
CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Auto-merging Tools/Scripts/build-webkit
Auto-merging Tools/Scripts/webkitdirs.pm
CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

I think I may have missed some discussion about this, but I thought the direction we were taking the platform as a whole was to make these sorts of dictionaries constructible, but not required, i.e. you can pass any object, but you can also construct it if you like. The advantage of doing it this way is that  it allows for feature detection.

That all said, whatever constructor/API we expose should have a less generic name than Flags since we're adding these sorts of dictionaries all over the platform.

[Eric U.]

(In reply to comment #10)
> I think I may have missed some discussion about this, but I thought the direction we were taking the platform as a whole was to make these sorts of dictionaries constructible, but not required, i.e. you can pass any object, but you can also construct it if you like. The advantage of doing it this way is that  it allows for feature detection.

We could certainly do that here, but I don't think it's necessary.  You can just check for [prefix]requestFileSystem to do feature detection.  Anyway, that's a spec decision, not an implementation decision.  This patch just brings us in line with the spec.

> That all said, whatever constructor/API we expose should have a less generic name than Flags since we're adding these sorts of dictionaries all over the platform.

Indeed; I wanted to get rid of this first of all.  If we want to add back in a constructable object later, we can take the time to come up with a better name.

[WebKit Review Bot]

Comment on attachment 120242 [details]
Patch

Clearing flags on attachment: 120242

Committed r103624: <http://trac.webkit.org/changeset/103624>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ilya Tikhonovsky]

Reverted r103624 for reason:

Broke Snow Leopard builders

Committed r103625: <http://trac.webkit.org/changeset/103625>

[Eric U.]

Created attachment 121018 [details]
Patch

[Eric U.]

Comment on attachment 121018 [details]
Patch

Build fix: removed the last few references to the file and global object.

[WebKit Review Bot]

Comment on attachment 121018 [details]
Patch

Clearing flags on attachment: 121018

Committed r104126: <http://trac.webkit.org/changeset/104126>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.