860731 - Move LockedFile to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP

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 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Add an 'extern' in .cpp files

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Use FileMode for LockedFile

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Move LockedFile to WebIDL

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 6

[Jan Varga [:janv]]

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 ?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 8

[Jan Varga [:janv]]

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

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 10

[Jan Varga [:janv]]

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 11

[Jan Varga [:janv]]

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 ?

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Consistency with existing files always trumps the style guide.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Jan, ping.

Comment 15

[Jan Varga [:janv]]

working on it

Comment 16

[Jan Varga [:janv]]

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 17

[Jan Varga [:janv]]

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()

Comment 18

[Jan Varga [:janv]]

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);
}

Comment 19

[Jan Varga [:janv]]

Attachment: rebased patch

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c v3

Still need to fix the TypeErrors.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c v4

I think I got most of your comments now.

Comment 22

[Jan Varga [:janv]]

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

Comment 23

[Jan Varga [:janv]]

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

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c v5

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

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Fix packaging

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

Comment 26

[Jan Varga [:janv]]

Attachment: Additional cleanup and fixes

- 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 27

[Jan Varga [:janv]]

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.

Comment 28

[Jan Varga [:janv]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Florian Quèze [:florian]]

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

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 32

[Jan Varga [:janv]]

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.

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 34

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d797ac4d78e

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d797ac4d78e