Andrea Marchesini [:baku] Assignee
	Description • 10 years ago

      No description provided.

	Andrea Marchesini [:baku] Assignee
	Comment 1 • 10 years ago

This patch is about the porting of DOMFile/DOMBlob to WebIDL bindings. It's a big patch but I don't know how to split it in independent sub-patches.

I would like to receive a review from

. hbolley for any js/xpconnect/* changes
. smaug for dom/browser-element/* and for nsFrameMessageManager

bz, I marked you for a review, but feel free to assign it to somebody else. khuey maybe? Thanks.

This patch is the first of 3 and it's only about the porting. In a separated patch I renamed nsDOMFile.h to mozilla/dom/DOMFile.h. Then in another patch I'll get rid of nsDOMBlobBuilder, integrating its methods into DOMFile.

	Ben Kelly [:bkelly, not reviewing]
	Comment 2 • 10 years ago

Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

I spoke to Boris on IRC and offered to help with his review queue.  He suggested I could do the majority of the review here and pass off to Ollie or Kyle for the webidl parts requiring a DOM peer.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 10 years ago

Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

I'm going to punt most of the review to bkelly, but here are some comments on the IDL:

1) .type doesn't need GetterThrows.  Just return an empty string from DOMFileImplFile::GetType if there is no MIME service, just like you would if there is a MIME service that doesn't know about the file type.

2)  Why does the "contentType" argument to slice not have "" as the default value?  Seems like it should; please raise a spec issue if you agree that this gives the same black-box behavior as the spec doing the empty string thing in prose.

3)  In the File constructor, "fileName" should be a ScalarValueString, right?

4) 
>+ // These constructors is just for chrome callee:

s/is/are/ and s/callee/callers/?

5) The FilePropertyBag dictionary in this patch looks nothing like the one in the spec.  Why not?  If the spec is just wrong, raise issues as needed, I guess...

6) Need a spec issue about the prose about FilePropertyBag being confused.  The dictionary is always present, and it always has a type member.

7) Is there a bug on deprecating/removing lastModifiedDate?

8) Should mozFullPath really be exposed to the web?  That seems moderately fishy.

r=me on the idl with those issues addressed.

	Ben Kelly [:bkelly, not reviewing]
	Comment 4 • 10 years ago

Maybe I am looking at the wrong spec, but shouldn't there also be a constructor like this on File:

  [Constructor(Blob fileBits, [EnsureUTF16] DOMString fileName)]

Looking at this:

  http://www.w3.org/TR/FileAPI/#file

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 10 years ago

Welcome to the world of the W3C.  You're looking at the last "officially published" draft, dated 2013-09-12.

The editor's draft is linked from that and is at http://dev.w3.org/2006/webapi/FileAPI/ and was last updated 2014-07-15.  Use that as your spec reference.  ;)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 10 years ago

Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

Could you ask review after bkelly has gone through this once, so that
we don't both need to comment on the same issues.
(This is a *huge* patch after all.)

	Ben Kelly [:bkelly, not reviewing]
	Comment 7 • 10 years ago

This patch also needs to be rebased as it does not apply cleanly to mozilla-central.

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 10 years ago

Here's my attempt at rebasing:

https://github.com/wanderview/gecko-patches/blob/review-bug1047483/webidl.patch

	Andrea Marchesini [:baku] Assignee
	Comment 9 • 10 years ago

Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

I'll ask a review to smaug and bholley when bkelly finishes its review process.

	Andrea Marchesini [:baku] Assignee
	Comment 10 • 10 years ago

> 3)  In the File constructor, "fileName" should be a ScalarValueString, right?

Do we have such type?

	Ben Kelly [:bkelly, not reviewing]
	Comment 11 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #10)
> > 3)  In the File constructor, "fileName" should be a ScalarValueString, right?
> 
> Do we have such type?

It just landed in inbound on Friday via bug 1025183.

	Ben Kelly [:bkelly, not reviewing]
	Comment 12 • 10 years ago

I'm about half done.  I'll have to finish up tomorrow morning.

	Andrea Marchesini [:baku] Assignee
	Comment 13 • 10 years ago

I'm uploading the other patches but I don't ask for a review yet.

	Ben Kelly [:bkelly, not reviewing]
	Comment 16 • 10 years ago

Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

Review of attachment 8466273 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good.  It would definitely be cleaner if we could use DOMFile in more places instead of converting back and forth with nsIDOMBlob.  I understand this is coming in a follow-up patch, though.

I think the only major question/concern I have is about DOMFile::GetParentObject().  Right now it returns nullptr in all cases which does not seem correct.

I was previously concerned about using static_cast to convert between nsIDOMBlob and DOMFile, but it was explained to me in IRC this is safe as long as the XPCOM interface is builtintype and there is only one implementation.

I did not review the xpconnect portions.

r=me with these items addressed.

::: content/base/public/nsDOMFile.h
@@ +8,3 @@
>  
>  #include "mozilla/Attributes.h"
> +

Nit: Remove extra line.

@@ +22,2 @@
>  #include "mozilla/dom/DOMError.h"
> +#include "mozilla/dom/FileBinding.h"

Please move the includes for BlobBinding.h and FileBinding.h to nsDOMFile.cpp.  All you need in the header here are forward includes for BlobPropertyBag and FilePropertyBag.

You'll also then need to add the binding headers to cpp files that use UNWRAP_OBJECT.

@@ +61,5 @@
> +/* FOLLOWUP TODO:
> +1. remove nsDOMBlobBuilder.h
> +2. rename nsDOMFile.h/cpp to DOMFile.h/cpp
> +3. rename nsDOMFileList to DOMFileList
> +*/

Can you reference bug numbers for these?

@@ +77,5 @@
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(DOMFile, nsIDOMFile)
>  
> +  static already_AddRefed<DOMFile> CreateDOMBlob();
> +

I don't see this method referenced.  It seems you've removed its only use in nsLayoutModule.cpp.  Can it just be removed?

@@ +152,5 @@
> +  // WebIDL methods
> +  nsISupports* GetParentObject() const
> +  {
> +    return nullptr;
> +  }

If I understand correctly, this should return a valid object when the File is constructed within a Window context.  If thats not the case, can you document why?

It seems most bindings just return the value of GlobalObject::GetAsSupports() from aGlobal passed into the constructor.

@@ +196,5 @@
> +              ErrorResult& aRv);
> +
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;
> +
> +  uint64_t GetSize(ErrorResult& aRv);

Can this be made const?  The webidl binding allows it and might prevent some const_casts further up the call stack.  Please double-check the const-ness of the other accessors as well.

@@ +200,5 @@
> +  uint64_t GetSize(ErrorResult& aRv);
> +
> +  void GetType(nsAString& aType, ErrorResult& aRv);
> +
> +  int64_t GetLastModified(ErrorResult& aRv);

Maybe note in a comment that GetName() for the WebIDL name attribute is declared as part of NS_DECL_NSIDOMFILE above.

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +157,5 @@
> +
> +    else if (data.IsArrayBuffer()) {
> +      const ArrayBuffer& buffer = data.GetAsArrayBuffer();
> +      buffer.ComputeLengthAndData();
> +      aRv = blobSet.AppendVoidPtr(buffer.Data(), buffer.Length());

Can you expand the tests to verify that this works as expected with zero length ArrayBuffers.

@@ +173,5 @@
> +      }
> +    }
> +
> +    else {
> +      MOZ_ASSUME_UNREACHABLE("This should not happen.");

Better message please.  Maybe something like "Impossible blob data type"?

@@ +365,2 @@
>  {
> +  nsCString utf8Str = NS_ConvertUTF16toUTF8(aString);

I think this can be simplified to:

  NS_ConvertUTF16toUTF8 utf8Str(aString);

::: content/base/src/nsDOMBlobBuilder.h
@@ +76,2 @@
>  
> +  void InitializeChromeFile(DOMFile& aData,

Can this be |const DOMFile& aData|?

@@ +113,5 @@
> +
> +  void SetFromNsIFile(bool aValue)
> +  {
> +    mIsFromNsiFile = aValue;
> +  }

Nit: Can you fix these to be spelled the same?  "NsI" vs "Nsi".  I don't have a preference for which is chosen.

::: content/base/src/nsDOMFile.cpp
@@ +342,5 @@
> +  if (rv.Failed()) {
> +    return rv.ErrorCode();
> +  }
> +
> +  JS::Rooted<JSObject*> date(aCx, JS_NewDateObjectMsec(aCx, value));

I think this can be simplified to avoid duplicating some code by doing something like this:

  ErrorResult rv;
  Date value = GetLastModifiedDate(rv);
  if (rv.Failed()) {
    return rv.ErrorCode();
  }

  if (!value.ToDateObject(aCx, aDate)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

  return NS_OK;

@@ +395,5 @@
>  DOMFile::GetSize(uint64_t* aSize)
>  {
> +  ErrorResult rv;
> +  *aSize = GetSize(rv);
> +  return rv.ErrorCode();

MOZ_ASSERT(aSize) above here.

@@ +423,5 @@
>  DOMFile::GetMozLastModifiedDate(uint64_t* aDate)
>  {
> +  ErrorResult rv;
> +  *aDate = GetLastModified(rv);
> +  return rv.ErrorCode();

MOZ_ASSERT(aDate) above here.

@@ +480,5 @@
> +  }
> +
> +  Optional<nsAString> contentType;
> +  if (aArgc > 1) {
> +    contentType = &aContentType;

I believe this should be |if (aArgc > 2)|.

On a related note, can you expand test_fileapi_slice.html to test the content type argument?

@@ +577,5 @@
> +/* static */ already_AddRefed<DOMFile>
> +DOMFile::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
> +{
> +  nsRefPtr<DOMMultipartFileImpl> impl = new DOMMultipartFileImpl();
> +  MOZ_ASSERT(!impl->IsFile());

The other constructors place this assert after InitializeBlob().  Why the different ordering here?

@@ +616,5 @@
> +        ErrorResult& aRv)
> +{
> +  nsRefPtr<DOMMultipartFileImpl> impl = new DOMMultipartFileImpl(aName);
> +
> +// TODO: read the spec about the FilePropertyBag

Is this stale at this point?  If not, can we get a bug number in the comment to follow-up?

@@ +715,5 @@
>  
> +  int64_t start = aStart.WasPassed() ? aStart.Value() : 0;
> +  int64_t end = aEnd.WasPassed() ? aEnd.Value() : (int64_t)thisLength;
> +
> +  nsString contentType;

I believe we favor nsAutoString for stack variables.

@@ +907,5 @@
> +  ErrorResult error;
> +  *aContentLength = GetSize(error);
> +  if (NS_WARN_IF(error.Failed())) {
> +    return error.ErrorCode();
> +  }

MOZ_ASSERT(aContentLength) above here.

@@ +912,3 @@
>  
>    nsString contentType;
> +  this->GetType(contentType, error);

Nit: Lets drop the |this->| as this file seems to mostly not use it and you removed it from GetSize() above.

@@ +912,4 @@
>  
>    nsString contentType;
> +  this->GetType(contentType, error);
> +  if (error.Failed()) {

NS_WARN_IF() here.

@@ +944,1 @@
>      nsString dummyString;

nsAutoString here.

@@ +944,3 @@
>      nsString dummyString;
> +    GetType(dummyString, error);
> +    if (error.Failed()) {

NS_WARN_IF() here.

::: content/base/src/nsDOMFileReader.h
@@ +18,5 @@
>  #include "prtime.h"
>  #include "nsITimer.h"
>  #include "nsIAsyncInputStream.h"
>  
> +#include "nsDOMFile.h"

This can be replaced with a forward declaration of DOMFile.

::: content/base/src/nsFormData.h
@@ +5,5 @@
>  #ifndef nsFormData_h__
>  #define nsFormData_h__
>  
>  #include "mozilla/Attributes.h"
> +#include "nsDOMFile.h"

This can be replaced with a forward declaration.

::: content/base/src/nsFrameMessageManager.cpp
@@ +210,5 @@
>      uint32_t length = blobs.Length();
>      blobList.SetCapacity(length);
>      for (uint32_t i = 0; i < length; ++i) {
>        typename BlobTraits<Flavor>::BlobType* protocolActor =
> +        aManager->GetOrCreateActorForBlob(static_cast<DOMFile*>(blobs[i].get()));

Why do you need a cast here?  Shouldn't nsRefPtr<DOMFile>::get() return DOMFile* already?

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +16,5 @@
>  #include "nsIMemoryReporter.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/LoadInfo.h"
>  
> +using mozilla;

This doesn't compile.  It should be |using namespace mozilla;|.

That being said, I think it would be nicer to be explicit about the class you need.  So I would prefer if it was |using mozilla::ErrorResult;|.  This also helps minimize unified build bleed over.

@@ +518,4 @@
>  
>    if (blob->IsFile()) {
>      nsString filename;
> +    blob->GetName(filename);

Why drop the NS_ENSURE_SUCCESS?  Even if it always returns NS_OK today, it may not always do so.  It would seem the safe thing to do would be to leave the return code checking in.

::: content/base/src/nsXMLHttpRequest.h
@@ +355,3 @@
>      {
>        mValue.mBlob = aBlob;
>      }

Nit: Might be slightly nicer to pass a DOMFile& reference into the constructor since you already have one where this is called.  Then convert to a pointer when assigning to mValue.mBlob.

::: dom/archivereader/ArchiveReader.cpp
@@ +50,2 @@
>                               const nsACString& aEncoding)
> +  : mBlob(&aBlob)

Rather than cast back to an XPCOM interface here (as mBlob is nsCOMPtr<nsIDOMBlob>), would it be better to just change mBlob to be nsRefPtr<DOMFile>?

::: dom/archivereader/ArchiveZipEvent.cpp
@@ +85,5 @@
>    }
>  
> +  nsCOMPtr<nsPIDOMWindow> window =
> +    do_QueryInterface(aArchiveReader->GetParentObject());
> +

This variable doesn't seem to be used.  Can you explain what this statement is here for?

::: dom/base/MessagePort.cpp
@@ +10,5 @@
>  #include "mozilla/dom/MessagePortBinding.h"
>  #include "mozilla/dom/MessagePortList.h"
>  #include "mozilla/dom/StructuredCloneTags.h"
>  #include "nsContentUtils.h"
> +#include "nsDOMFile.h"

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

::: dom/browser-element/BrowserElementParent.jsm
@@ +542,5 @@
>      }
>      else {
>        debug("Got error in gotDOMRequestResult.");
> +      Services.DOMRequest.fireErrorAsync(req,
> +        Cu.cloneInto(data.json.errorMsg, this._window));

Can you explain why these clones are now necessary?

::: dom/camera/DOMCameraControl.cpp
@@ +1182,2 @@
>    ErrorResult ignored;
> +  cb->Call(*blob, ignored);

Please add a MOZ_ASSERT(blob) or MOZ_ASSERT(aPicture) above this.

::: dom/canvas/ImageEncoder.cpp
@@ +48,5 @@
>          jsapi.Init(mGlobal);
>          JS_updateMallocCounter(jsapi.cx(), mImgSize);
>        }
>  
> +      mCallback->Call(*blob, rv);

Please add a MOZ_ASSERT(blob) above this.

::: dom/contacts/ContactManager.js
@@ +91,5 @@
>  
>    _convertContact: function(aContact) {
> +    var contact = Cu.cloneInto(aContact, this._window);
> +    let newContact = new this._window.mozContact(contact.properties);
> +    newContact.setMetadata(contact.id, contact.published, contact.updated);

Again, can you explain why these clones are now needed?  They seem unrelated.

::: dom/filehandle/FileHandle.cpp
@@ +626,3 @@
>                                 ErrorResult& aRv)
>  {
> +  DOMFile& file = const_cast<DOMFile&>(aValue);

If we could make DOMFile::GetSize() const, then perhaps this cast could go away.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1623,5 @@
>      return true;
>    }
>  
> +  DOMFile* domBlob = nullptr;
> +  if (NS_SUCCEEDED(UNWRAP_OBJECT(Blob, aObj, domBlob))) {

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

::: dom/ipc/StructuredCloneUtils.cpp
@@ +9,5 @@
>  #include "nsIDOMDOMException.h"
>  #include "nsIMutable.h"
>  #include "nsIXPConnect.h"
>  
> +#include "nsDOMFile.h"

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

::: dom/webidl/Blob.webidl
@@ +21,3 @@
>    readonly attribute DOMString type;
>  
> +  // TODO readonly attribute boolean isClosed;

Can we reference a bug so we don't lose track of this TODO?

@@ +31,2 @@
>  
> +  // TODO void close();

Reference follow-up bug.

::: dom/workers/URL.cpp
@@ +849,5 @@
> +
> +  NS_NAMED_LITERAL_STRING(argStr, "Argument 1 of URL.createObjectURL");
> +  NS_NAMED_LITERAL_STRING(blobStr, "Blob");
> +  aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &argStr, &blobStr);
> +}

Do we have to maintain this JSObject* implementation?  Won't the webidl bindings throw an error for us before it calls into the implementation class?

::: dom/workers/WorkerPrivate.cpp
@@ +364,3 @@
>      {
> +      DOMFile* blob = nullptr;
> +      if (NS_SUCCEEDED(UNWRAP_OBJECT(Blob, aObj, blob))) {

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

::: dom/workers/XMLHttpRequest.cpp
@@ +2150,5 @@
>  void
> +XMLHttpRequest::Send(DOMFile& aBody, ErrorResult& aRv)
> +{
> +  JSContext* cx = mWorkerPrivate->GetJSContext();
> +  mWorkerPrivate->AssertIsOnWorkerThread();

Move the assertion to the top of the method please.

::: js/xpconnect/src/ExportHelpers.cpp
@@ +13,5 @@
>  #include "js/StructuredClone.h"
>  #include "mozilla/dom/BindingUtils.h"
>  #include "nsGlobalWindow.h"
>  #include "nsJSUtils.h"
> +#include "nsDOMFile.h"

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 10 years ago

> Can you reference bug numbers for these?

These are the other patches.

> Can this be made const?  The webidl binding allows it and might prevent some
> const_casts further up the call stack.  Please double-check the const-ness
> of the other accessors as well.

It cannot be const because DOMFileImplFile uses nsIFile->GetFileSize() that is not const.

> ::: content/base/src/nsDOMBlobBuilder.cpp
> @@ +157,5 @@
> > +
> > +    else if (data.IsArrayBuffer()) {
> > +      const ArrayBuffer& buffer = data.GetAsArrayBuffer();
> > +      buffer.ComputeLengthAndData();
> > +      aRv = blobSet.AppendVoidPtr(buffer.Data(), buffer.Length());
> 
> Can you expand the tests to verify that this works as expected with zero
> length ArrayBuffers.


> ::: content/base/src/nsDOMBlobBuilder.h
> @@ +76,2 @@
> >  
> > +  void InitializeChromeFile(DOMFile& aData,
> 
> Can this be |const DOMFile& aData|?

DOMFileImplFile::GetType cannot be const :/

> On a related note, can you expand test_fileapi_slice.html to test the
> content type argument?

with the changes suggested by bz, contentType cannot be null.
And we already have this check:
testSlice(fileFile, size, "", fileData, "fileFile");

is this what you meant?

> @@ +518,4 @@
> >  
> >    if (blob->IsFile()) {
> >      nsString filename;
> > +    blob->GetName(filename);

blob is DOMFileImpl and GetName returns void.

> ::: dom/browser-element/BrowserElementParent.jsm
> @@ +542,5 @@
> >      }
> >      else {
> >        debug("Got error in gotDOMRequestResult.");
> > +      Services.DOMRequest.fireErrorAsync(req,
> > +        Cu.cloneInto(data.json.errorMsg, this._window));
> 
> Can you explain why these clones are now necessary?

DOMFile has a chrome wrapper and cloning it we create a new object for content.
This probably will disappear when DOMFile::GetParentObject() returns something and not null.

> ::: dom/workers/URL.cpp
> @@ +849,5 @@
> > +
> > +  NS_NAMED_LITERAL_STRING(argStr, "Argument 1 of URL.createObjectURL");
> > +  NS_NAMED_LITERAL_STRING(blobStr, "Blob");
> > +  aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &argStr, &blobStr);
> > +}
> 
> Do we have to maintain this JSObject* implementation?  Won't the webidl
> bindings throw an error for us before it calls into the implementation class?

Actually it's not for Blob but for MediaStream. Code updated.

I'm going to proceed to add something in GetParentObject(). Then interdiff.
Thanks.

	Andrea Marchesini [:baku] Assignee
	Comment 18 • 10 years ago

Bent, can you take a look of this? The issue is that DOMFile must have a parent before returning it to content. In order to do that I had to change many components. I'm asking you a review because many of those are related to IPC.

In particular BlobChild/BlobParent::GetBlob() now takes a parent nsISupport object. This is used for creating a new DOMFile if needed. Sometimes I set it to null when I'm 100% sure that that blob is not returned to content.

If you have better ideas, tell me. Thanks

	Andrea Marchesini [:baku] Assignee
	Comment 19 • 10 years ago

Comment on attachment 8467755 [details] [diff] [review]
patch 1b

I asked bz to give me a review for the DOM part.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 10 years ago

Comment on attachment 8467755 [details] [diff] [review]
patch 1b

>+++ b/content/base/public/nsDOMFile.h

I'm sorry this took so long.  :(

>+  // threads. This method has to be used in order to set the set the correct

s/set the set/set/

>+++ b/content/html/content/src/HTMLCanvasElement.cpp
>+                                       OwnerDoc()->GetWindow(),

That will pass null in cases when OwnerDoc() is not in a window... but wouldn't "global" make more sense here?  That should be non-null even when GetWindow() is null.

Furthermore, GetWindow() returns the outer, but the owner should be an inner.  We should add an assert somewhere in the blob/file code that the owner it gets passed, if it's a window, is an inner window.

>+  return DOMFile::CreateMemoryFile(OwnerDoc()->GetWindow(), imgData,

Similar issue here.

>+++ b/content/html/content/src/HTMLInputElement.cpp
>+      mFileList[i]->SetParentObject(mInput->OwnerDoc()->GetWindow());

And here.

>+        DOMFile::CreateFromFile(OwnerDoc()->GetWindow(), file);

And here.

>+++ b/dom/devicestorage/DeviceStorageRequestParent.cpp

Please document where, if anywhere, the parents of these blobs and file are set?

>+++ b/dom/filesystem/*

I'm assuming that you've checked that these GetWindow return values are inner windows, not outer ones?

>+++ b/dom/ipc/Blob.cpp
>+      nsCOMPtr<nsIDOMBlob> blob = newActor->GetBlob(nullptr);

Document why nullptr is ok?  Same for other places in this file.

>+  // If aParent is null, this blob will not exposed to content.

"will not be"

>+      nsCOMPtr<nsIDOMBlob> blob =newActor->GetBlob(nullptr);

Add the space after '=' while you're here, please.

>+++ b/dom/ipc/Blob.h
>+  // Note: If this blob is exposed to content, aParent must be a proper value.

This needs some more explanation of what "proper" is, in both places.

>+++ b/dom/ipc/StructuredCloneUtils.cpp
>+    // Let's create a new blob in the with the correct parent.

s/in the //

The rest of this looks good.  r=me with the above addressed.

	Andrea Marchesini [:baku] Assignee
	Comment 21 • 10 years ago

bholley, this patch is big, and it implements the porting of DOMFile/DOMBlob to WebIDL. bz and bkelly did the first reviews but I would like to have a review from you too.

In particular I would like to have a review on how I use JSCompartments, global objects, parents and and all those things. Of course, any other comment is welcome! Thanks.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 10 years ago

I really think you should remove all the WrapObject() calls in this patch in favor of WrapNewBindingObject, which will ensure the result is in your compartment....

	Jean-Yves Perrier [:teoli]
	Comment 23 • 10 years ago

:baku, will these patches change the behavior for the user (a quick look seems to confirm this, I see more attribute to the FilePropertyBag, but I'm unsure if this lead to more user-visible change).

(Meanwhile I'm adding dev-doc-needed)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #22)
> I really think you should remove all the WrapObject() calls in this patch in
> favor of WrapNewBindingObject, which will ensure the result is in your
> compartment....

Sounds like we need some documentation somewhere...
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.h#840 doesn't exactly
hint that it deals with compartments, nor does the method name itself.

	Andrea Marchesini [:baku] Assignee
	Comment 26 • 10 years ago

(In reply to Jean-Yves Perrier [:teoli] from comment #23)
> :baku, will these patches change the behavior for the user (a quick look
> seems to confirm this, I see more attribute to the FilePropertyBag, but I'm
> unsure if this lead to more user-visible change).

You are right. The constructor of the File object is changed and I guess we need to update our documentation pages. No other changes are made.

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 10 years ago

> Sounds like we need some documentation somewhere...

OK, https://hg.mozilla.org/integration/mozilla-inbound/rev/9de64636f186

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 10 years ago

Comment on attachment 8488979 [details] [diff] [review]
patch 1b - WrapObject

>+      if (WrapNewBindingObject(cx, blob, &val)) {
>+        return val.toObjectOrNull();

So if WrapNewBindingObject returns false... where will our control flow end up?

Better would be:

      if (!WrapNewBindingObject(cx, blob, &val)) {
        return nullptr;
      }
      return &val.toObject();

You have this pattern in a few places.

>+++ b/dom/workers/WorkerPrivate.cpp
>-        return result;

That seems wrong.  Why was this change made?

r- because of that result thing...

	Bobby Holley (:bholley)
	Comment 29 • 10 years ago

Comment on attachment 8488767 [details] [diff] [review]
patch 1 - WebIDL

Review of attachment 8488767 [details] [diff] [review]:
-----------------------------------------------------------------

I just skimmed large parts of this patch looking for things that I'd be more likely to have input on. This would have been much, much easier to review as a long series of incremental patches (and I probably would have had to sift through a lot less code that I had no opinion on). Please try to avoid megapatches in the future.

Handwavy r=bholley with comments addressed.

::: content/base/public/nsDOMFile.h
@@ +155,5 @@
> +  }
> +
> +  // Some DOMFile objects are created without a parent maybe on different
> +  // threads. This method has to be used in order to set the correct
> +  // parent before sending these objects to content.

Given this, do we want to assert mParent in GetParentObject?

Looking over the patch, there are lots of places where we set this to null under the assumption that it won't ever be reflected into JS. Given that, I think we need some sort of strong assertion that this holds.

::: content/base/src/nsContentUtils.cpp
@@ +5998,5 @@
> +
> +  JS::Rooted<JSObject*> obj(aCx, blob->WrapObject(aCx));
> +  if (!obj || !JS_WrapObject(aCx, &obj)) {
> +    return NS_ERROR_FAILURE;
> +  }

As Boris notes, this (and things like it) should convert to WrapNewBindingObject.

::: content/html/content/src/HTMLInputElement.cpp
@@ +552,5 @@
>      if (mCanceled) { // The last progress event may have canceled us
>        return NS_OK;
>      }
>  
> +    // Setting the parent to the DOMFile

"to"?

::: dom/base/nsGlobalWindow.cpp
@@ +7913,5 @@
>  
> +  // See if this is a File/Blob object.
> +  {
> +    DOMFile* blob = nullptr;
> +    if (NS_SUCCEEDED(UNWRAP_OBJECT(Blob, obj, blob)) && scInfo->subsumes) {

It makes more sense to me to invert the order here and check subsumes first.

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +1500,5 @@
>  
>      for (uint32_t index = 0; index < length; index++) {
>        BlobParent* actor = static_cast<BlobParent*>(aActors[index]);
> +
> +      // We can use null because this blob is not exposed to content.

I think "exposed to content" in all of these comments isn't quite right. "content" generally means "web content", generally defined in opposition to "chrome" (System JS).

So I think it would make more sense to say "...because this blob is not reflected into JS". Comments like this appear in quite a few places in this patch, and I think they should all be updated.

::: dom/workers/XMLHttpRequest.cpp
@@ +1072,5 @@
>      AutoSafeJSContext cx;
>      JSAutoRequest ar(cx);
> +
> +    JS::Rooted<JSObject*> scope(cx, xpc::UnprivilegedJunkScope());
> +    JSAutoCompartment ac(cx, scope);

Hm, why do we need the junk scope, exactly? Just to find an initial scope to enter? If so, I think that's not a good reason. We should either explicitly enter the scope of the XHR's owner, or add some machinery to BindingUtils to handle to case where the caller is in a null compartment.

I really want to avoid a proliferation of junk scope usage, which is why I'm going to be stubborn about uses like this one. ;-)

@@ +2160,5 @@
>    SendInternal(EmptyString(), Move(buffer), clonedObjects, aRv);
>  }
>  
>  void
> +XMLHttpRequest::Send(DOMFile& aBody, ErrorResult& aRv)

This looks an awful lot like the JSObject overload of Send(). Why can't we just invoke that overload with aBody.WrapObject(cx)? We should find some way to share this code.

::: js/xpconnect/tests/chrome/test_cloneInto.xul
@@ -147,5 @@
>      true,
>      'hello world',
>      [1, 2, 3],
>      { a: 1, b: 2 },
> -    { blob: new Blob([]), file: new File(new Blob([])) },

Er, why do the blob tests go away here? Aren't they explicitly supported with the changes in ExportHelpers.cpp?

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 10 years ago

> We should either explicitly enter the scope of the XHR's owner

The problem is that it's a pain to get there.  The XHR's owner is just nsISupports, iirc,, so we have to wrap it, which means we need to be in a scope, etc.

> or add some machinery to BindingUtils to handle to case where the caller is in a null
> compartment.

What would that machinery do?  Enter the junk scope?

The only sane and principled thing here is to quickly get to an nsIGlobalObject and get its JS object or something, but that's a major hassle in the general case.  In this case it _could_ be done, but I for one would be OK with that happening in a followup.

	Bobby Holley (:bholley)
	Comment 31 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #30)
> What would that machinery do?  Enter the junk scope?

What code do we have that requires us to be in a compartment before we're dealing with a JSObject at all?

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 10 years ago

WrapNewBindingObject promises to wrap into the caller's compartment.  That means it assumes there is a caller's compartment, no?

	Bobby Holley (:bholley)
	Comment 33 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #32)
> WrapNewBindingObject promises to wrap into the caller's compartment.  That
> means it assumes there is a caller's compartment, no?

It seems like that could easily be changed, right? Or we could move most of the existing code there into WrapNewBindingObjectDontWrapValue, and WrapNewBindingObject could delegate to that and add a tail call to JS_WrapValue?

In general, if we don't have any JSObjects yet, it seems wrong to require us to be in "some compartment". We've had requirements like that in the past that required us to bootstrap a dummy global every time we wanted to create a Window global, and I had to work very hard to fix it.

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 10 years ago

> It seems like that could easily be changed, right?

To do what?

We need a function, to be used from codegen, that will do exactly what WrapNewBindingObject does right now.

Furthermore, this function is very hot, and needs to be fast in the common (same-compartment) case.

I'm open to refactoring this somehow while preserving those properties, and agree it would be a good idea.  I don't think we should block this bug on it, but rather should do a followup.

	Bobby Holley (:bholley)
	Comment 35 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #34)
> > It seems like that could easily be changed, right?
> 
> To do what?
> 
> We need a function, to be used from codegen, that will do exactly what
> WrapNewBindingObject does right now.
> 
> Furthermore, this function is very hot, and needs to be fast in the common
> (same-compartment) case.

Right, so we do what I suggest in comment 33. Is there anything about that which doesn't satisfy the above?

> I'm open to refactoring this somehow while preserving those properties, and
> agree it would be a good idea.  I don't think we should block this bug on
> it, but rather should do a followup.

It depends - what kind of SLA would the followup have? The current patch uses the JunkScope, which is governed by special rules to avoid it becoming another hiddenDOMWindow.

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 10 years ago

> Is there anything about that which doesn't satisfy the above?

I'd need to write the code to make sure.

> It depends - what kind of SLA would the followup have?

I'll make sure I fix it before 35 branches.  I just can't do it this week, and I'd much rather get this landed to unblock other things than have it blocked on me fixing up this part.

	Bobby Holley (:bholley)
	Comment 37 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #36)
> I'll make sure I fix it before 35 branches.  I just can't do it this week,
> and I'd much rather get this landed to unblock other things than have it
> blocked on me fixing up this part.

Sounds good to me. Thanks a lot for doing that, and let me know if you need help.

	Andrea Marchesini [:baku] Assignee
	Comment 38 • 10 years ago

I'm going to merge this patch in patch 1 if you give me a +

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 10 years ago

Comment on attachment 8494607 [details] [diff] [review]
wrapObject.patch

>+++ b/dom/workers/WorkerPrivate.cpp

The change here is still wrong.  The "return result;" needs to be after the closing curly of that block.  There's even a comment that explains why!

Please fix that.

r=me with that fixed, but please do fix it.

	Andrea Marchesini [:baku] Assignee
	Comment 40 • 10 years ago

> Sounds good to me. Thanks a lot for doing that, and let me know if you need
> help.

In the meantime, can I keep the code as it is and file a followup?

	Andrea Marchesini [:baku] Assignee
	Comment 41 • 10 years ago

1. rebased
2. fixed the return issue
3. applied (almost) all the bholley comments (read the comments I wrote to know why)

Smaug, there are not so many changes about SystemMessage API, but I changed a couple of things and I appreciate if you can spend a couple of minutes (?) reading this patch.

I guess a couple of tests are broken after the merging.
I'm going to fix them and in case there are big changes, I'll ask a review in a interdiff.

	Andrea Marchesini [:baku] Assignee
	Comment 42 • 10 years ago

> >  void
> > +XMLHttpRequest::Send(DOMFile& aBody, ErrorResult& aRv)
> 
> This looks an awful lot like the JSObject overload of Send(). Why can't we
> just invoke that overload with aBody.WrapObject(cx)? We should find some way
> to share this code.

In that case  I have to wrap it here and then unwrap it on the other Send() just to reuse < 10 lines.
I would like to avoid this.

> > -    { blob: new Blob([]), file: new File(new Blob([])) },
> 
> Er, why do the blob tests go away here? Aren't they explicitly supported
> with the changes in ExportHelpers.cpp?

Because comparing cloned objects fails. The previous blob is actually a different object and we don't have any way to compare the real data from JS yet. Correct?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 43 • 10 years ago

It is not clear to me what I should review here.
I know nothing about SystemMessage API, but I also don't see SystemMessage API changes in the patch.

	Andrea Marchesini [:baku] Assignee
	Comment 44 • 10 years ago

smaug, yes, sorry, I meant FrameMessageManager. No big changes, bug an additional glance can help.

	SUN Haitao
	Comment 45 • 10 years ago

> Because comparing cloned objects fails. The previous blob is actually a
> different object and we don't have any way to compare the real data from JS
> yet. Correct?

As far as I know, we can compare blobs by read them into array buffers via FileReader. For Example:

  function readAsArrayBuffer(blob) {
    return new Promise(function (resolve) {
      var reader = new FileReader();
      reader.onload = function () {
        resolve(reader.result);
      }
      reader.readAsArrayBuffer(blob);
    });
  }

  function compareArrayBuffer(bufferA, bufferB) {
    if (bufferA.byteLength != bufferB.byteLength) return false;
    var va = new Uint8Array(bufferA);
    var vb = new Uint8Array(bufferB);
    for (var i = 0; i < va.length; i++) {
      if (va[i] != vb[i]) return false;
    }
    return true;
  }

  var a = new Blob(['ABCDE'], {contentType:'text/plain;carset=utf-8'});
  var b = new Blob(['ABCDE'], {contentType:'text/plain;carset=utf-8'});
  var c = new Blob(['01234'], {contentType:'text/plain;carset=utf-8'});

  readAsArrayBuffer(a).then(function (arrayA) {
    readAsArrayBuffer(a).then(function (arrayB) {
      console.log(a === b, compareArrayBuffer(arrayA, arrayB)); // logs `false true`
    });
  });

  readAsArrayBuffer(a).then(function (arrayA) {
    readAsArrayBuffer(c).then(function (arrayC) {
      console.log(a === c, compareArrayBuffer(arrayA, arrayC)); // logs `false false`
    });
  });

	Boris Zbarsky [:bzbarsky]
	Comment 46 • 10 years ago

> In the meantime, can I keep the code as it is and file a followup?

Yes, but please get a move on with it.  I committed to fixing that followup before Oct 13, but that was two weeks ago!  At this point timing on that is going to be really tight for me.  :(

	Bobby Holley (:bholley)
	Comment 47 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #40)
> > Sounds good to me. Thanks a lot for doing that, and let me know if you need
> > help.
> 
> In the meantime, can I keep the code as it is and file a followup?

Yes.

	Bobby Holley (:bholley)
	Comment 48 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #42)
> In that case  I have to wrap it here and then unwrap it on the other Send()
> just to reuse < 10 lines.
> I would like to avoid this.

I don't follow - XMLHttpRequest::Send(JS::Handle<JSObject*> aBody, ErrorResult& aRv) doesn't do any unwrapping that I see.

	Bobby Holley (:bholley)
	Comment 49 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #46)
> > In the meantime, can I keep the code as it is and file a followup?
> 
> Yes, but please get a move on with it.  I committed to fixing that followup
> before Oct 13, but that was two weeks ago!  At this point timing on that is
> going to be really tight for me.  :(

As long as it gets done in a near-ish timeframe I don't really care about exact dates.

	Andrea Marchesini [:baku] Assignee
	Comment 50 • 10 years ago

Interdiff:

1. MOZ_ASSERT(mParent) cannot be there because for chrome code, and workers, sometimes we don't have mParent.

2. Instead SetParentObject() I think it's cleaner to create a new DOMFile.

	Boris Zbarsky [:bzbarsky]
	Comment 52 • 10 years ago

Comment on attachment 8498286 [details] [diff] [review]
patch 1 - interdiff

r=me

	Andrea Marchesini [:baku] Assignee
	Comment 53 • 10 years ago

Comment on attachment 8467051 [details] [diff] [review]
patch 2 - nsDOMFileList to FileList

A simple renaming for nsDOMFileList.

	Andrea Marchesini [:baku] Assignee
	Comment 54 • 10 years ago

Comment on attachment 8467052 [details] [diff] [review]
patch 3 - nsDOMFile.h to mozilla/dom/File.h

renaming nsDOMFile to File.

	Andrea Marchesini [:baku] Assignee
	Comment 55 • 10 years ago

Comment on attachment 8467053 [details] [diff] [review]
patch 4 - Remove nsDOMBlobBuilder.h

Moving code in order to remove nsDOMBlobBuilder.*

	(no longer active)
	Comment 56 • 10 years ago

Comment on attachment 8467052 [details] [diff] [review]
patch 3 - nsDOMFile.h to mozilla/dom/File.h

Review of attachment 8467052 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Bindings.conf
@@ +150,5 @@
>  },
>  
>  'Blob': {
> +    'nativeType': 'mozilla::dom::File',
> +    'headerFile': 'mozilla/dom/File.h',

I think you should be able to drop headerFile...

	Andrea Marchesini [:baku] Assignee
	Comment 57 • 10 years ago

bz, sorry for this new interdiff, but making all the tests pass took a while.
Something about this interdiff:

1. hazard rooting fixed

2. lastModified and lastModifiedData return the same value.
I spoke with smaug about the reason this issue appears just now. Without my patch lastModified returns JS_Now() all the time if the lastModified date is unknown.
Wtih my patches we return the creation time if lastModified is unknown.

This is actually better because now lastModifiedDate and lastModified return the same value.

3. JS_Now() vs PR_Now() on windows they can return different values. So I had to use JS_Now() in the constructor. There is a comment about why.

After that try seems to be fully green and we can land these patches.

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 10 years ago

Comment on attachment 8500573 [details] [diff] [review]
patch 1 - interdiff

>+    //   x.getTime() < f.dateModified.getTime()

This might be false with JS_Now() too, because JS_Now() times can go backwards (e.g. due to NTP adjustments and the like).

If you actually need a monotonic clock, you should be using TimeStamp in C++ and peformance.now() in JS.  If you're not doing that, do NOT assume a monotonic clock (e.g. in tests).

>+// In our implementation of File object, lastModifiedDate is unknown only for in new objects.

s/for in/for/ ?

>+++ b/content/base/test/test_bug403852.html

Yeah, this test is bogus because it assumes precisely that thing I said not to assume.  ;)

>+      // called because otherwise the static analysis thinks it can gc the
>+      // JSObject via the stack.

No, because the static analysis thinks dereferencing XPCOM objects can GC (because in some cases it can!), and a return statement with a JSObject* type means that JSObject* is on the stack as a raw pointer while destructors are running.

Same thing in all the other copy-pasted places.

r=me but please file a followup about that test or something?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 61 • 10 years ago

Comment on attachment 8500931 [details] [diff] [review]
patch 5 - FilePropertyBag doesn't have 'name'

Why doesn't ChromeFilePropertyBag just extend FilePropertyBag?

	Andrea Marchesini [:baku] Assignee
	Comment 63 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2577b5ab46b1

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6238ff5d3ed0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e087289ccfbb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/18830d07884c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c199f1057d7e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c09d7f95554a

	Ed Morley [:emorley]
	Comment 64 • 10 years ago

Test failures:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2822880&repo=mozilla-inbound

	Ryan VanderMeulen [:RyanVM]
	Comment 65 • 10 years ago

Backed out for the webplatform test failures noted in comment 64.
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a1219c6324

	Ryan VanderMeulen [:RyanVM]
	Comment 66 • 10 years ago

I believe these failures are also related?
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2822888&repo=mozilla-inbound

	Andrea Marchesini [:baku] Assignee
	Comment 67 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6b65fb9ba64a

Ready to land again if this patch is ok.

	Boris Zbarsky [:bzbarsky]
	Comment 68 • 10 years ago

Comment on attachment 8501720 [details] [diff] [review]
patch 6 - webplatform tests fix

>+  [no-argument Blob constructor without 'new']

This should throw (though Web IDL hasn't been updated to say that yet, while we figure out ES6 subclassing stuff).  Can you please file a bug on this test?

>+  [Changes to the blobParts array should be reflected in the returned Blob (pop).]
>+  [Changes to the blobParts array should be reflected in the returned Blob (unshift).]

This is bogus per spec.  The argument is a sequence, which is passed by value, not by reference.  Again, file a bug on this test.

r=me

	Andrea Marchesini [:baku] Assignee
	Comment 69 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/53a14a912960
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/13c71c435af3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ce357af3db
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/664690fed9c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1d38e11a4dc4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b72ab356368f

I asked jgraham to file the 2 bugs for the webplatform test suite.

	James Graham [:jgraham]
	Comment 70 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #68)

> >+  [Changes to the blobParts array should be reflected in the returned Blob (pop).]
> >+  [Changes to the blobParts array should be reflected in the returned Blob (unshift).]
> 
> This is bogus per spec.  The argument is a sequence, which is passed by
> value, not by reference.  Again, file a bug on this test.

It's not clear to me why this is bogus, but I might be missing something.

It looks like the test causes mutation of the array to happen as it's being converted to a WebIDL sequence. As far as I can tell from the spec, this conversion happens using an iterator over the original array, which appears to just store an index into the array and so the output sequence will reflect changes to the array made during iteration.

	Boris Zbarsky [:bzbarsky]
	Comment 71 • 10 years ago

My apologies.  I should have looked at the test more carefully; I had assumed (bad me) that it was calling the constructor and then later mutating the array.

The pop() test is still incorrect: the problem is that the test is depending on the old duck-typing behavior of sequences, not the iterator behavior.

For example, stepping through the pop() test:

  1)  We create the array iterator, which has [[ArrayIteratorNextIndex]] set to 0 to start.
  2)  We call next() on it, this sets [[ArrayIteratorNextIndex]] to 1.
  3)  We take the returned object and ToString() it.  This calls pop() on the array.
  4)  We call next() on the iterator again, landing in 
    http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%arrayiteratorprototype%.next
  5)  In step 6, index is 1.
  6)  In step 8 we do .length on the array, which returns 1 (because we popped the value).
  7)  In step 11 we detect index >= len and return an iterator result that indicates
      iteration is done.

The output of this is a Web IDL sequence of length 1, containing the single string "PASS".  But the test expects a sequence of length 2, containing the strings "PASS" and "undefined".

The unshift() test is incorrect for the same reason: it assumes that length is grabbed up front, while ArrayIterator doesn't do that.

	Andrew McCreight [:mccr8]
	Comment 72 • 10 years ago

Thanks to RyanVM for pointing me at some non-unified build bustage.  I pushed what is hopefully a fix:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c6bd8c963e94

	Andrew McCreight [:mccr8]
	Comment 73 • 10 years ago

Adding an include didn't work.  Maybe I need to explicitly prefix ErrorResult with mozilla::
  https://hg.mozilla.org/integration/mozilla-inbound/rev/bb693abe8de0

	Boris Zbarsky [:bzbarsky]
	Comment 74 • 10 years ago

Or throw in "using namespace mozilla".

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 75 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/53a14a912960
https://hg.mozilla.org/mozilla-central/rev/13c71c435af3
https://hg.mozilla.org/mozilla-central/rev/c0ce357af3db
https://hg.mozilla.org/mozilla-central/rev/664690fed9c8
https://hg.mozilla.org/mozilla-central/rev/1d38e11a4dc4
https://hg.mozilla.org/mozilla-central/rev/b72ab356368f
https://hg.mozilla.org/mozilla-central/rev/c6bd8c963e94
https://hg.mozilla.org/mozilla-central/rev/bb693abe8de0

	Boris Zbarsky [:bzbarsky]
	Comment 76 • 10 years ago

I filed bug 1087851 on comment 34.

	Kohei Yoshino [:kohei]
	Comment 77 • 10 years ago

Hi, are there any effects on site compatibility?

	Andrea Marchesini [:baku] Assignee
	Comment 78 • 10 years ago

(In reply to Kohei Yoshino [:kohei] from comment #77)
> Hi, are there any effects on site compatibility?

No. We should not have any effects on site compatibility.

	Kohei Yoshino [:kohei]
	Comment 79 • 10 years ago

Okay, thanks.