Applies on top of <https://hg.mozilla.org/try/rev/ca650e49228b>. Unfortunately, the build dies while linking libxul with 16:47.60 /usr/bin/ld.gold.real: error: obj/toolkit/library/../../dom/bindings/FileHandleBinding.o: requires dynamic R_X86_64_PC32 reloc against 'mozilla::dom::FileModeValues::strings' which may overflow at runtime; recompile with -fPIC 16:47.60 /usr/bin/ld.gold.real: error: obj/toolkit/library/../../dom/bindings/LockedFileBinding.o: requires dynamic R_X86_64_PC32 reloc against 'mozilla::dom::FileModeValues::strings' which may overflow at runtime; recompile with -fPIC 16:47.60 obj/dom/bindings/FileHandleBinding.cpp:85: error: undefined reference to 'mozilla::dom::FileModeValues::strings' 16:47.60 obj/dom/bindings/LockedFileBinding.cpp:169: error: undefined reference to 'mozilla::dom::FileModeValues::strings' 16:47.60 obj/dom/bindings/LockedFileBinding.cpp:170: error: undefined reference to 'mozilla::dom::FileModeValues::strings' 16:47.60 obj/dom/bindings/LockedFileBinding.cpp:170: error: undefined reference to 'mozilla::dom::FileModeValues::strings'

Comment on attachment 757497 [details] [diff] [review] Part a: Add an 'extern' in .cpp files r=me

Comment on attachment 757498 [details] [diff] [review] Part b: Use FileMode for LockedFile Review of attachment 757498 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/FileService.cpp @@ +460,4 @@ > if (!IsFileLockedForWriting(fileName)) { > LockFileForWriting(fileName); > } > + } else { Why is this needed ?

(In reply to Jan Varga [:janv] from comment #6) > Comment on attachment 757498 [details] [diff] [review] > Part b: Use FileMode for LockedFile > > Review of attachment 757498 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/file/FileService.cpp > @@ +460,4 @@ > > if (!IsFileLockedForWriting(fileName)) { > > LockFileForWriting(fileName); > > } > > + } else { > > Why is this needed ? For consistency with the style guide.

What about consistency with the style in the file or entire module ?

I can deal with the other code too, if you like.

I'll let bent to judge. Ben, IIRC we put "else" on a new line in dom/indexedDB, dom/file, dom/quota, dom/workers, etc. The decision will probably apply to the second patch too.

Comment on attachment 757499 [details] [diff] [review] Part c: Move LockedFile to WebIDL Review of attachment 757499 [details] [diff] [review]: ----------------------------------------------------------------- Ok, here are some initial comments, I haven't looked at LockedFile.cpp and LockedFile.h ::: dom/file/LockedFile.cpp @@ -19,5 @@ > -#include "nsJSUtils.h" > -#include "nsStringStream.h" > -#include "nsWidgetsCID.h" > -#include "xpcpublic.h" > - Please try to keep the existing ordering scheme. The ordering scheme we've tried to maintain in indexeddb/file/quota code (.cpp files) is: 1. The class header, LockedFile.h in this case 2. Interfaces 3. Other moz headers 4. Other class headers from the file module. The scheme for .h files is similar: 1. FileCommon.h 2. Interfaces 3. Other moz headers 4. Other class headers from the file module 5. Forward declarations of other classes. please reorder elsewhere too (where you're touching it) ::: dom/webidl/LockedFile.webidl @@ +37,5 @@ > + DOMRequest? append(DOMString value); > + [Throws] > + DOMRequest? truncate(optional unsigned long long size); > + [Throws] > + DOMRequest? flush(); Shouldn't all these methods return the FileRequest instead ?

Consistency with existing files always trumps the style guide.

https://hg.mozilla.org/mozilla-central/rev/e20539356853 https://hg.mozilla.org/mozilla-central/rev/7880cbf6200f

Jan, ping.

working on it

Comment on attachment 757499 [details] [diff] [review] Part c: Move LockedFile to WebIDL Review of attachment 757499 [details] [diff] [review]: ----------------------------------------------------------------- almost done, will send comments for LockedFile.cpp soon ::: dom/file/LockedFile.h @@ +88,5 @@ > > nsresult > OpenInputStream(bool aWholeFile, uint64_t aStart, uint64_t aLength, > nsIInputStream** aResult); > Nit: I would add |// WrapperCache| here @@ +92,5 @@ > > + nsPIDOMWindow* GetParentObject() const > + { > + return GetOwner(); > + } Nit: |GetParentObject() const| on a new line and an empty line between methods I would apply the same pattern elsewhere too @@ +187,5 @@ > + GetInputStream(nsIDOMBlob* aValue, uint64_t* aInputLength, ErrorResult& aRv); > + static already_AddRefed<nsIInputStream> > + GetInputStream(const nsAString& aValue, uint64_t* aInputLength, > + ErrorResult& aRv); > + Nit: I would move these static methods after Finish() @@ +207,5 @@ > Finish(); > > + bool CheckArgumentsForRead(uint64_t aSize, ErrorResult& aRv); > + bool CheckArgumentsForWrite(ErrorResult& aRv); > + What about CheckStateAndArgumentsForRead() and CheckStateForWrite() ? Nit: I would also move these before GenerateFileRequest()

Comment on attachment 757499 [details] [diff] [review] Part c: Move LockedFile to WebIDL Review of attachment 757499 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/LockedFile.cpp @@ +33,4 @@ > > #define STREAM_COPY_BLOCK_SIZE 32768 > > +BEGIN_FILE_NAMESPACE Why is this needed ? @@ +207,5 @@ > return event.forget(); > } > > +} // anonymous namespace > + Nit: I would move these static methods after Finish() Nit: /* static */ -> // static on a separate line here and elsewhere @@ +441,3 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > Hm, what about other implementation methods in the .h file? Why do they miss this assertion ? @@ +453,5 @@ > > + nsRefPtr<MetadataParameters> params = > + new MetadataParameters(aParameters.mSize, aParameters.mLastModified); > + if (!params->IsConfigured()) { > + // XXX aRv.ThrowTypeError(); do you plan to fix this ? @@ +485,4 @@ > } > > if (!aSize) { > + // XXX aRv.ThrowTypeError(); Do you plan to fix this ? @@ +572,5 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + if (!CheckArgumentsForWrite(aRv)) { > + return nullptr; This should go after the following if/else block. Otherwise the method won't throw on bad state when there's no owner. @@ +760,5 @@ > > return NS_OK; > } > > +/* virtual */ JSObject* Nit: /* virtual */ -> // virtual on a new line @@ +765,5 @@ > +LockedFile::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) > +{ > + return LockedFileBinding::Wrap(aCx, aScope, this); > +} > + Nit: I would move this method before GetMetadata()

Maybe you could do this: bool LockedFile::CheckState(ErrorResult& aRv) { if (!IsOpen()) { aRv.Throw(NS_ERROR_DOM_FILEHANDLE_LOCKEDFILE_INACTIVE_ERR); return false; } // Do nothing if the window is closed if (!GetOwner()) { return false; } return true; } LockedFile::GetMetadata() { ... if (!CheckState(aRv)) { return nullptr; } ... } bool LockedFile::CheckStateAndArgumentsForRead(uint64_t aSize, ErrorResult& aRv) { if (mLocation == UINT64_MAX) { aRv.Throw(NS_ERROR_DOM_FILEHANDLE_NOT_ALLOWED_ERR); return false; } if (!aSize) { // XXX aRv.ThrowTypeError(); return false; } return CheckState(aRv); } bool LockedFile::CheckStateForWrite(ErrorResult& aRv) { if (mMode != FileMode::Readwrite) { aRv.Throw(NS_ERROR_DOM_FILEHANDLE_READ_ONLY_ERR); return false; } return CheckState(aRv); }

Still need to fix the TypeErrors.

I think I got most of your comments now.

Comment on attachment 8355084 [details] [diff] [review] Part c v4 Review of attachment 8355084 [details] [diff] [review]: ----------------------------------------------------------------- Almost there. Btw, what about comment 18, don't you like the idea ? ::: dom/file/LockedFile.cpp @@ +33,1 @@ > /me cries @@ +241,5 @@ > > return lockedFile.forget(); > } > > +/* static */ already_AddRefed<nsIInputStream> is this in any style guide ? I would prefer: // static already_AddRefed<nsIInputStream> which is used all over the file module @@ +259,5 @@ > + *aInputLength = length; > + return stream.forget(); > +} > + > +/* static */ already_AddRefed<nsIInputStream> dtto @@ +270,5 @@ > + } > + > + nsCOMPtr<nsIInputStream> stream; > + aRv = aValue->GetInternalStream(getter_AddRefs(stream)); > + return stream.forget(); this is not your fault, but technically we don't need to initialize aInputLength if something fails @@ +273,5 @@ > + aRv = aValue->GetInternalStream(getter_AddRefs(stream)); > + return stream.forget(); > +} > + > +/* static */ already_AddRefed<nsIInputStream> dtto @@ +760,5 @@ > > return NS_OK; > } > > +/* virtual */ JSObject* I know that archive impl in the file module isn't consistent with this, but again, if there's no general rule for this kind of comments, then I would rather prefer: // virtual JSObject* @@ +761,5 @@ > return NS_OK; > } > > +/* virtual */ JSObject* > +LockedFile::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) Nit: It would be nice to follow the order in the .h file, so move it before GetMetadata() implementation, please. ::: dom/file/LockedFile.h @@ +16,2 @@ > #include "nsIRunnable.h" > This isn't structured as I suggested, but it's in alphabetical order at least. @@ +94,5 @@ > + // WrapperCache > + nsPIDOMWindow* GetParentObject() const > + { > + return GetOwner(); > + } Nit: |GetParentObject() const| on a new line and an empty line between methods I would apply the same pattern elsewhere too @@ +101,5 @@ > + WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > + > + // WebIDL > + FileHandle* GetFileHandle() const > + { All these inline WebIDL methods should assert the main thread. @@ +128,5 @@ > + { > + // Null means the end-of-file. > + if (aLocation.IsNull()) { > + mLocation = UINT64_MAX; > + } else { this doesn't follow the style in the file module, but ok @@ +188,5 @@ > > void > OnRequestFinished(); > > + bool CheckStateAndArgumentsForRead(uint64_t aSize, ErrorResult& aRv); Nit: return value on its own line and an empty line between methods @@ +215,5 @@ > Finish(); > > + static already_AddRefed<nsIInputStream> > + GetInputStream(const ArrayBuffer& aValue, uint64_t* aInputLength, > + ErrorResult& aRv); Nit: an empty line between methods @@ +217,5 @@ > + static already_AddRefed<nsIInputStream> > + GetInputStream(const ArrayBuffer& aValue, uint64_t* aInputLength, > + ErrorResult& aRv); > + static already_AddRefed<nsIInputStream> > + GetInputStream(nsIDOMBlob* aValue, uint64_t* aInputLength, ErrorResult& aRv); Nit: an empty line between methods

Ms2ger, would you mind if I took your patch, rebase, fix the nits and land it ?

I think your nits are wrong, but I don't have time to fight over them. Feel free to take over.

Since I removed the last XPIDL for dom/file, the xpt needs to go.

- addressed remaining review comments - fixed some NS_ENSURE_SUCCESS -> NS_FAILED to NS_ENSURE_SUCCESS -> NS_WARN_IF(NS_FAILED) - consistent state and argument checking

Comment on attachment 8387531 [details] [diff] [review] Part c v5 I'm going to land this separately, but please don't close this bug until the "Additional cleanup and fixes" patch lands.

https://hg.mozilla.org/integration/mozilla-inbound/rev/94c92db0b503 https://hg.mozilla.org/integration/mozilla-inbound/rev/85555bfd4844

https://hg.mozilla.org/mozilla-central/rev/94c92db0b503 https://hg.mozilla.org/mozilla-central/rev/85555bfd4844

Fixed comm-central bustage: https://hg.mozilla.org/comm-central/rev/5eccf54ec4c0

Comment on attachment 8391945 [details] [diff] [review] Additional cleanup and fixes Review of attachment 8391945 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok, but: ::: dom/file/LockedFile.cpp @@ +443,5 @@ > } > > + // Do nothing if the window is closed > + if (!GetOwner()) { > + return nullptr; I think this should throw, right? Below too. ::: dom/file/LockedFile.h @@ +235,5 @@ > + return nullptr; > + } > + > + if (!length) { > + return nullptr; Shouldn't this return a request? @@ +240,5 @@ > + } > + > + // Do nothing if the window is closed > + if (!GetOwner()) { > + return nullptr; Shouldn't this set aRv?

Can we fix it in a followup ? This bug is about moving to WebIDL, the patch doesn't change the current behavior (the behavior when the window is closed or when passed data has zero length). I'll file a new bug if you agree.

Ok, that's fine by me. We just need to make sure it happens! :)