Closed Bug 860731 Opened 7 years ago Closed 6 years ago

Move LockedFile to WebIDL

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 5 obsolete files)

No description provided.
Attached patch WIP (obsolete) — Splinter Review
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'
Attachment #757497 - Flags: review?(bzbarsky)
Attachment #757498 - Flags: review?(Jan.Varga)
Attachment #736401 - Attachment is obsolete: true
Attachment #757499 - Flags: review?(Jan.Varga)
Comment on attachment 757497 [details] [diff] [review]
Part a: Add an 'extern' in .cpp files

r=me
Attachment #757497 - Flags: review?(bzbarsky) → review+
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 ?
Attachment #757498 - Flags: review?(Jan.Varga) → review+
(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.
Flags: needinfo?(bent.mozilla)
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.
Flags: needinfo?(bent.mozilla)
Blocks: 888591
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()
Attachment #757499 - Flags: review?(Jan.Varga)
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);
}
Attached patch rebased patch (obsolete) — Splinter Review
Depends on: 953425
Attachment #757497 - Flags: checkin+
Attachment #757498 - Flags: checkin+
Attached patch Part c v3 (obsolete) — Splinter Review
Still need to fix the TypeErrors.
Attachment #757499 - Attachment is obsolete: true
Attachment #771596 - Attachment is obsolete: true
Attached patch Part c v4 (obsolete) — Splinter Review
I think I got most of your comments now.
Attachment #8351722 - Attachment is obsolete: true
Attachment #8355084 - Flags: review?(Jan.Varga)
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
Attachment #8355084 - Flags: review?(Jan.Varga)
Blocks: 942542
Ms2ger, would you mind if I took your patch, rebase, fix the nits and land it ?
Attached patch Part c v5Splinter Review
I think your nits are wrong, but I don't have time to fight over them. Feel free to take over.
Attachment #8355084 - Attachment is obsolete: true
Attached patch Fix packagingSplinter Review
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
Attachment #8391945 - Flags: review?(bent.mozilla)
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.
Attachment #8387531 - Flags: review+
Keywords: leave-open
Attachment #8387790 - Flags: review+
Blocks: 987509
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?
Attachment #8391945 - Flags: review?(bent.mozilla) → review+
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.
Flags: needinfo?(bent.mozilla)
Ok, that's fine by me. We just need to make sure it happens! :)
Flags: needinfo?(bent.mozilla)
Blocks: 992888
No longer blocks: 992888
https://hg.mozilla.org/mozilla-central/rev/6d797ac4d78e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.