Created attachment 65794 [details] Patch

Comment on attachment 65794 [details] Patch WebCore/page/DOMWindow.cpp:740 + errorCallback->handleEvent(FileError::create(INVALID_STATE_ERR).get()); i asked this question on the other code review, but shouldn't these dom events be dispatched asynchronously? otherwise, we end up nesting JS -> C++ -> JS calls, which is never a great thing to do. does the spec say anything about this? it seems like it would be nice to always dispatch callbacks asynchronously for consistency. WebCore/page/DOMWindow.cpp:759 + #define COMPILE_ASSERT_MATCHING_ENUM(domwindowname, asyncfsname) \ since there are only two enum values being compared here, it might not be worth defining the macro: COMPILE_ASSERT(int(DOMWindow::TEMPORARY) == int(AsyncFileSystem::Temporary), enum_mismatch); COMPILE_ASSERT(int(DOMWindow::PERSISTENT) == int(AsyncFileSystem::Persistent), enum_mismatch); WebCore/page/Settings.h:243 + #if ENABLE(FILE_SYSTEM) i don't know that this setting needs to be protected by the ifdef. afterall, localStorageDatabasePath is not protected by ENABLE(DOM_STORAGE).

Created attachment 65846 [details] Patch This change depends on the the other change for bug 44732

(In reply to comment #2) > (From update of attachment 65794 [details]) > WebCore/page/DOMWindow.cpp:740 > + errorCallback->handleEvent(FileError::create(INVALID_STATE_ERR).get()); > i asked this question on the other code review, but shouldn't these dom events be > dispatched asynchronously? otherwise, we end up nesting JS -> C++ -> JS calls, > which is never a great thing to do. does the spec say anything about this? it > seems like it would be nice to always dispatch callbacks asynchronously for > consistency. Fixed. (The fix depends on the other change for bug 44732) > WebCore/page/DOMWindow.cpp:759 > + #define COMPILE_ASSERT_MATCHING_ENUM(domwindowname, asyncfsname) \ > since there are only two enum values being compared here, it might not be worth defining the macro: Fixed. > WebCore/page/Settings.h:243 > + #if ENABLE(FILE_SYSTEM) > i don't know that this setting needs to be protected by the ifdef. > afterall, localStorageDatabasePath is not protected by ENABLE(DOM_STORAGE). Removed the ifdefs.

Could someone review this one? Thanks,

Comment on attachment 65846 [details] Patch > WebCore/bindings/generic/RuntimeEnabledFeatures.h:141 > + static bool requestFileSystemEnabled() { return isFileSystemEnabled; } Why do we need requestFileSystemEnabled? What's the difference between fileSystemEnabled and requestFileSystemEnabled? > WebCore/page/DOMWindow.h:83 > +#if ENABLE(FILE_SYSTEM) Normally we do not need to guard this. > WebCore/page/DOMWindow.h:86 > + class ErrorCallback; Please sort these. > WebCore/page/Settings.h:244 > + const String& fileSystemRootPath() const { return m_fileSystemRootPath; } Do you need to guard these? > WebKit/chromium/ChangeLog:15 > +2010-08-27 Kinuko Yasuda <kinuko@chromium.org> Why do you produce 2 ChangeLog entries?

Created attachment 66117 [details] Patch

Thanks! (In reply to comment #6) > (From update of attachment 65846 [details]) > > WebCore/bindings/generic/RuntimeEnabledFeatures.h:141 > > + static bool requestFileSystemEnabled() { return isFileSystemEnabled; } > Why do we need requestFileSystemEnabled? What's the difference between fileSystemEnabled and requestFileSystemEnabled? We need someIdlNameEnabled() method to dynamically enable someIdlName at the binding time. I basically followed what the other code does here (have one boolean flag for the feature and associate the flag with multiple methods - though for now we have only one method associated with the flag). > > WebCore/page/DOMWindow.h:83 > > +#if ENABLE(FILE_SYSTEM) > Normally we do not need to guard this. Removed. > > WebCore/page/DOMWindow.h:86 > > + class ErrorCallback; > Please sort these. Fixed. > > WebCore/page/Settings.h:244 > > + const String& fileSystemRootPath() const { return m_fileSystemRootPath; } > Do you need to guard these? Darin suggested that we may not need the ifdefs here. > > WebKit/chromium/ChangeLog:15 > > +2010-08-27 Kinuko Yasuda <kinuko@chromium.org> > Why do you produce 2 ChangeLog entries? Mistake... fixed.

Created attachment 66121 [details] Patch Fixed ChangeLog date

Comment on attachment 66121 [details] Patch > WebCore/bindings/generic/RuntimeEnabledFeatures.h:141 > + static bool requestFileSystemEnabled() { return isFileSystemEnabled; } I still don't understand your comment. requestFileSystemEnabled() seems to be identical to fileSystemEnabled(). By looking into the name, I do not know what requestFileSystemEnabled is used for. > WebCore/page/DOMWindow.cpp:742 > + if (!document) { Can this check be moved to the beginning of this method? That is, can we still call m_localFileSystem->requestFileSystem if document is gone? > WebCore/page/DOMWindow.cpp:759 > + m_localFileSystem->requestFileSystem(document, static_cast<AsyncFileSystem::Type>(type), size, successCallback, errorCallback); This could be merged with the first call of "m_localFileSystem->requestFileSystem", like: if (!m_localFileSystem) { // Create m_localFileSystem ... } m_localFileSystem->requestFileSystem(document, static_cast<AsyncFileSystem::Type>(type), size, successCallback, errorCallback); > WebCore/page/DOMWindow.h:86 > + class LocalFileSystem; Please move all these forward declarations to the right place.

(In reply to comment #10) > (From update of attachment 66121 [details]) > > WebCore/bindings/generic/RuntimeEnabledFeatures.h:141 > > + static bool requestFileSystemEnabled() { return isFileSystemEnabled; } > I still don't understand your comment. requestFileSystemEnabled() seems to be identical to fileSystemEnabled(). By looking into the name, I do not know what requestFileSystemEnabled is used for. This is a function of how runtime enablement works in the v8 bindings layer. The IDL compiler generates c++ code with a particular signature based on the name of the attribute/method that is tagged as being [EnabledAtRuntime].

Created attachment 66139 [details] Patch

(In reply to comment #10) > (From update of attachment 66121 [details]) > > WebCore/bindings/generic/RuntimeEnabledFeatures.h:141 > > + static bool requestFileSystemEnabled() { return isFileSystemEnabled; } > I still don't understand your comment. requestFileSystemEnabled() seems to be identical to fileSystemEnabled(). By looking into the name, I do not know what requestFileSystemEnabled is used for. (I think Michael's comment is explaining things better...) requestFileSystemEnabled() is there to dynamically enable requestFileSystem() interface defined in DOMWindow.idl annotated with [EnabledAtRuntime]. fileSystemEnabled() and isFileSystemEnabled() is supposed to be a flag for the entire feature and requestFileSystem is one of the feature's methods that needs to be exposed if the feature is enabled. If we have more methods to be enabled at runtime for the feature (actually the FileSystem API defines one more DOMWindow method that hasn't been added yet) we'll have similar xxxEnabled() methods that will look identical to fileSystemEnabled(). (And again, I don't have strong opinion here and basically am trying to follow other code.) > > WebCore/page/DOMWindow.cpp:742 > > + if (!document) { > Can this check be moved to the beginning of this method? That is, can we still call m_localFileSystem->requestFileSystem if document is gone? Do you mean we shouldn't call requestFileSystem if the document is gone? That's true I moved the check to the beginning. > > WebCore/page/DOMWindow.cpp:759 > > + m_localFileSystem->requestFileSystem(document, static_cast<AsyncFileSystem::Type>(type), size, successCallback, errorCallback); > This could be merged with the first call of "m_localFileSystem->requestFileSystem", like: Fixed. > > WebCore/page/DOMWindow.h:86 > > + class LocalFileSystem; > Please move all these forward declarations to the right place. Fixed.

Comment on attachment 66139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66139&action=prettypatch > WebCore/page/DOMWindow.cpp:738 > + // No context? Not firing the errorCallback. I do not think we need to add any comment here, like what we do in other DOMWindow method.

Committed r66570: <http://trac.webkit.org/changeset/66570>