657994 - Factor out common bits of FileReader

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

FileReader and FileSaver have a lot in common.  They're essentially the same object, just going in different ways.  The attached patch splits FileReader into FileReader and FileIOObject.  The latter contains the common bits.

This also adds some macros and stuff to make it easier to implement EventTargets and to contain some common code that XHR has.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 533338 [details] [diff] [review]
Patch

Actually, I ended up pushing a bit more code out of FileReader into the base class.  Final patch later today.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 533474 [details] [diff] [review]
Patch

This updated patch pushes some streamlistener stuff down into the base class since FileReader and FileSaver both end up using it.

Asking smaug for review of the events stuff and sicking for the rest.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 533474 [details] [diff] [review]
Patch


>+++ b/content/base/public/EventTargetImpl.h
What is the reason for this file? Couldn't you just
move all this to nsDOMEventTargetHelper?


>+interface nsIDOMFileIOObjectEventTarget : nsIDOMEventTarget {
>+  // event handler attributes
>+  attribute nsIDOMEventListener onabort;
>+  attribute nsIDOMEventListener onerror;
>+  attribute nsIDOMEventListener onprogress;
>+};
>+
>+[scriptable, uuid(b6c49cba-4f41-4e1e-a660-e486152ebc2c)]
>+interface nsIDOMFileIOObject : nsISupports {
>+  void abort();
>+  readonly attribute unsigned short readyState;
>+  readonly attribute nsIDOMFileError error;
>+};

>+[scriptable, uuid(e504d521-048a-45a0-9270-a4c7194d25b5)]
>+interface nsIDOMFileReaderEventTarget : nsIDOMEventTarget
>+{
>+  // event handler attributes
>+  attribute nsIDOMEventListener onload;
>+  attribute nsIDOMEventListener onloadstart;
>+  attribute nsIDOMEventListener onloadend;
>+};
What are these interfaces? I mean, why do they have nsIDOM prefix?
Couldn't we just put the properties to the same interfaces they are in
the spec? I believe the patch exposes all the new interfaces to
web, though I didn't test. What does alert("nsIDOMFileIOObject" in window); say?



>+void nsDOMEventTargetWrapperCache::Init()
>+{
>+  // Set the original mScriptContext and mPrincipal, if available.
>+  // Get JSContext from stack.
>+  nsCOMPtr<nsIJSContextStack> stack =
>+    do_GetService("@mozilla.org/js/xpc/ContextStack;1");
nsContentUtils::ThreadJSContextStack()


>+  // Just to get a warning in a debug build.
>+  NS_ENSURE_SUCCESS(rv, NS_OK);
Well, use then perhaps some debug only code.
NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Some warning...");


>+NS_EVENT_TARGET_IMPL6(XHR, nsIXMLHttpRequestEventTarget,
>+                      load, error, abort, loadstart, progress, loadend)
Nice macro :)
EventSource and WebSockets could use something similar, but better to change them
in a different bug.


> nsXMLHttpRequest::Init()
> {
...
>+  nsDOMEventTargetWrapperCache::Init();

So this checks jscontextstack...

> 
>   nsIScriptSecurityManager *secMan = nsContentUtils::GetSecurityManager();
>   nsCOMPtr<nsIPrincipal> subjectPrincipal;
>   if (secMan) {
>     secMan->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
>   }
>-  NS_ENSURE_STATE(subjectPrincipal);
>+  if (!subjectPrincipal) {
>+    // If we have no principal, see if there's JS on the stack
>+    nsCOMPtr<nsIJSContextStack> stack =
>+      do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+
>+    if (!stack) {
>+      return NS_OK;
>+    }
>+ 
>+    JSContext *cx;
>+ 
>+    if (NS_FAILED(stack->Peek(&cx)) || !cx) {
>+      return NS_OK;
>+    }
>+
>+    // There's JS on the stack and no principal?!?
>+    NS_NOTREACHED("Why is there no principal!?");
>+    return NS_ERROR_UNEXPECTED;
And then we do the same here...
That is kind of ugly.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #4)
> Comment on attachment 533474 [details] [diff] [review] [review]
> Patch
> 
> 
> >+++ b/content/base/public/EventTargetImpl.h
> What is the reason for this file? Couldn't you just
> move all this to nsDOMEventTargetHelper?

Yes.
 
> >+interface nsIDOMFileIOObjectEventTarget : nsIDOMEventTarget {
> >+  // event handler attributes
> >+  attribute nsIDOMEventListener onabort;
> >+  attribute nsIDOMEventListener onerror;
> >+  attribute nsIDOMEventListener onprogress;
> >+};
> >+
> >+[scriptable, uuid(b6c49cba-4f41-4e1e-a660-e486152ebc2c)]
> >+interface nsIDOMFileIOObject : nsISupports {
> >+  void abort();
> >+  readonly attribute unsigned short readyState;
> >+  readonly attribute nsIDOMFileError error;
> >+};
> 
> >+[scriptable, uuid(e504d521-048a-45a0-9270-a4c7194d25b5)]
> >+interface nsIDOMFileReaderEventTarget : nsIDOMEventTarget
> >+{
> >+  // event handler attributes
> >+  attribute nsIDOMEventListener onload;
> >+  attribute nsIDOMEventListener onloadstart;
> >+  attribute nsIDOMEventListener onloadend;
> >+};
> What are these interfaces? I mean, why do they have nsIDOM prefix?
> Couldn't we just put the properties to the same interfaces they are in
> the spec? I believe the patch exposes all the new interfaces to
> web, though I didn't test. What does alert("nsIDOMFileIOObject" in window);
> say?

I guess they don't need the nsIDOM prefix ...

What is alert("nsIDOMFileIOObject" in window); supposed to do?  It alerts false for every interface I tested (these and even nsIDOMWindow).
 
> > nsXMLHttpRequest::Init()
> > {
> ...
> >+  nsDOMEventTargetWrapperCache::Init();
> 
> So this checks jscontextstack...
> 
> > 
> >   nsIScriptSecurityManager *secMan = nsContentUtils::GetSecurityManager();
> >   nsCOMPtr<nsIPrincipal> subjectPrincipal;
> >   if (secMan) {
> >     secMan->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
> >   }
> >-  NS_ENSURE_STATE(subjectPrincipal);
> >+  if (!subjectPrincipal) {
> >+    // If we have no principal, see if there's JS on the stack
> >+    nsCOMPtr<nsIJSContextStack> stack =
> >+      do_GetService("@mozilla.org/js/xpc/ContextStack;1");
> >+
> >+    if (!stack) {
> >+      return NS_OK;
> >+    }
> >+ 
> >+    JSContext *cx;
> >+ 
> >+    if (NS_FAILED(stack->Peek(&cx)) || !cx) {
> >+      return NS_OK;
> >+    }
> >+
> >+    // There's JS on the stack and no principal?!?
> >+    NS_NOTREACHED("Why is there no principal!?");
> >+    return NS_ERROR_UNEXPECTED;
> And then we do the same here...
> That is kind of ugly.

Yeah we could pass in a jscontextstack or something ...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

 
> What is alert("nsIDOMFileIOObject" in window); supposed to do?  It alerts
> false for every interface I tested (these and even nsIDOMWindow).

Sorry, wrong JS.
alert("FileIOObject" in window);

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ah, yes, that alerts true.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

What do you think of this?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 539584 [details] [diff] [review]
Patch


> EXPORTS_mozilla/dom = \
>-		Element.h \
>-		FromParser.h \
>-		$(NULL)
>+    Element.h \
>+    FromParser.h \
>+    $(NULL)
Nothing to do with this bug.


>+++ b/content/base/public/nsIDOMFileIOObject.idl
>@@ -0,0 +1,55 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is the Mozilla Foundation
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Kyle Huey <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "nsIDOMEventTarget.idl"
>+
>+interface nsIDOMFileError;
>+
>+[scriptable, uuid(7fc294d5-cedd-4aea-8329-d1bf141eb5e8)]
>+interface nsIFileIOObjectEventTarget : nsIDOMEventTarget {
>+  // event handler attributes
>+  attribute nsIDOMEventListener onabort;
>+  attribute nsIDOMEventListener onerror;
>+  attribute nsIDOMEventListener onprogress;
>+};
>+
>+[scriptable, uuid(b6c49cba-4f41-4e1e-a660-e486152ebc2c)]
>+interface nsIFileIOObject : nsISupports {
>+  void abort();
>+  readonly attribute unsigned short readyState;
>+  readonly attribute nsIDOMFileError error;
>+};

>+[scriptable, uuid(e504d521-048a-45a0-9270-a4c7194d25b5)]
>+interface nsIDOMFileReaderEventTarget : nsIDOMEventTarget
The spec doesn't have nsIDOMFileReaderEventTarget.
Why should we have it?


> [scriptable, uuid(f186170f-f07c-4f0b-9e3c-08f7dd496e74)]
> interface nsIDOMFileReader : nsISupports 
Per spec nsIDOMFileReader should extend EventTarget

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Factor out common bits of FileReader.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 545232 [details] [diff] [review]
Factor out common bits of FileReader.

OK, I think I cleaned up the interface situation here.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 545232 [details] [diff] [review]
Factor out common bits of FileReader.

>+++ b/content/base/public/nsIDOMFileReader.idl	Mon Jul 11 12:55:04 2011 -0700
>@@ -41,25 +41,26 @@
> interface nsIDOMBlob;
> interface nsIDOMFileError;
> 
>-[scriptable, uuid(3d77e784-1459-4206-b8a2-0855d826f569)]
>-interface nsIDOMFileReader : nsISupports 
>+[scriptable, builtinclass, uuid(3d77e784-1459-4206-b8a2-0855d826f569)]
>+interface nsIDOMFileReader : nsIDOMEventTarget
> {
>+  // event handler attributes
>+  attribute nsIDOMEventListener onload;
>+  attribute nsIDOMEventListener onloadstart;
>+  attribute nsIDOMEventListener onloadend;
>+
>   [implicit_jscontext]
>   void readAsArrayBuffer(in nsIDOMBlob filedata);
>   void readAsBinaryString(in nsIDOMBlob filedata);
>   void readAsText(in nsIDOMBlob filedata, [optional] in DOMString encoding);
>   void readAsDataURL(in nsIDOMBlob file);
> 
>-  void abort();
>-
Why are you removing abort()?
The spec does have it


>   const unsigned short EMPTY = 0;
>   const unsigned short LOADING = 1;
>   const unsigned short DONE = 2;
>-  readonly attribute unsigned short readyState;
ditto.


> 
>   [implicit_jscontext]
>   readonly attribute jsval result;
>-  readonly attribute nsIDOMFileError error;
And here.


>+[scriptable, uuid(7fc294d5-cedd-4aea-8329-d1bf141eb5e8)]
>+interface nsIFileIOObject : nsISupports {
{ should be in the next line.

>+  // event handler attributes
>+  attribute nsIDOMEventListener onabort;
>+  attribute nsIDOMEventListener onerror;
>+  attribute nsIDOMEventListener onprogress;
>+
>+  void abort();
>+  readonly attribute unsigned short readyState;
>+  readonly attribute nsIDOMFileError error;
>+};
Aha, you're moving them here.


does this change anything in the prototype chain?
Which prototype has 'abort' etc?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

[14:10:36.661] FileReader
[14:10:36.667] [object FileReader]
[14:10:39.503] FileReader.prototype
[14:10:39.508] [xpconnect wrapped native prototype]
--
[14:10:54.650] var props = []; for (var prop in FileReader) { if(FileReader.hasOwnProperty(prop)) { props.push(prop); } }; props
[14:10:54.656] ["EMPTY", "LOADING", "DONE"]
--
[14:11:06.139] var props = []; for (var prop in FileReader.prototype) { if(FileReader.prototype.hasOwnProperty(prop)) { props.push(prop); } }; props
[14:11:06.145] ["addEventListener", "removeEventListener", "dispatchEvent", "onload", "onloadstart", "onloadend", "result", "readAsArrayBuffer", "readAsText", "readAsBinaryString", "readAsDataURL", "EMPTY", "LOADING", "DONE", "onabort", "onerror", "onprogress", "abort", "readyState", "error", "getInterface"]

It looks like XPConnect is just dumping everything on one prototype?  This is what it seems to be doing for everything.  e.g.

--
[14:12:22.702] HTMLDivElement.prototype
[14:12:22.710] [xpconnect wrapped native prototype]
--
[14:12:38.547] var props = []; for (var prop in HTMLDivElement.prototype) { if(HTMLDivElement.prototype.hasOwnProperty(prop)) { props.push(prop); } }; props
[14:12:38.560] ["querySelector", "querySelectorAll", "scrollTop", "scrollLeft", "scrollHeight", "scrollWidth", "clientTop", "clientLeft", "clientHeight", "clientWidth", "firstElementChild", "lastElementChild", "previousElementSibling", "nextElementSibling", "childElementCount", "children", "classList", "setCapture", "getElementsByClassName", "getClientRects", "getBoundingClientRect", "releaseCapture", "mozMatchesSelector", "addEventListener", "removeEventListener", "dispatchEvent", "style", "contentEditable", "isContentEditable", "offsetParent", "innerHTML", "offsetLeft", "offsetTop", "offsetHeight", "offsetWidth", "scrollIntoView", "id", "title", "lang", "dir", "className", "accessKey", "blur", "focus", "click", "tagName", "removeAttributeNS", "removeAttribute", "getAttribute", "getElementsByTagName", "setAttribute", "getElementsByTagNameNS", "hasAttributeNS", "setAttributeNS", "hasAttribute", "getAttributeNS", "nodeName", "nodeValue", "nodeType", "parentNode", "childNodes", "firstChild", "lastChild", "previousSibling", "nextSibling", "attributes", "ownerDocument", "namespaceURI", "prefix", "localName", "baseURI", "textContent", "setUserData", "getUserData", "insertBefore", "replaceChild", "removeChild", "appendChild", "hasChildNodes", "cloneNode", "normalize", "isSupported", "hasAttributes", "compareDocumentPosition", "isSameNode", "lookupPrefix", "isDefaultNamespace", "lookupNamespaceURI", "isEqualNode", "getAttributeNode", "setAttributeNode", "removeAttributeNode", "getAttributeNodeNS", "setAttributeNodeNS", "align", "ELEMENT_NODE", "ATTRIBUTE_NODE", "TEXT_NODE", "CDATA_SECTION_NODE", "ENTITY_REFERENCE_NODE", "ENTITY_NODE", "PROCESSING_INSTRUCTION_NODE", "COMMENT_NODE", "DOCUMENT_NODE", "DOCUMENT_TYPE_NODE", "DOCUMENT_FRAGMENT_NODE", "NOTATION_NODE", "DOCUMENT_POSITION_DISCONNECTED", "DOCUMENT_POSITION_PRECEDING", "DOCUMENT_POSITION_FOLLOWING", "DOCUMENT_POSITION_CONTAINS", "DOCUMENT_POSITION_CONTAINED_BY", "DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC", "hidden", "tabIndex", "draggable", "spellcheck", "dataset"]

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And XHR is similar.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

But what does WebIDL say? I think we want to implement eventually what
that spec says.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #15)
> But what does WebIDL say? I think we want to implement eventually what
> that spec says.

I don't see what that has to do with this bug ... especially if we're going to rewrite our DOM bindings anyways ...

Comment 17

[Cameron McCormack (:heycam)]

I'm assuming that the DOM bindings rewrite will put things on the right prototypes, assuming our IDL matches the IDL from the relevant specs.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So, I'm not really a fan of storing the onfoo listeners as members in the object. First of all it creates a lot of duplicated code in the getter/setter/tracer/unlinker. While we can hide this behind macros, the code is still there. Second, it makes objects bigger and won't really scale once we do onfoo listeners correctly on things like Nodes.

What we should really do is to store the listener in the nsEventListenerManager. It has the ability to keep a bit indicating that a given listener is a onfoo listener. That way it can be in charge of converting from Function to EventListener and back. And it will automatically take care of the tracing and unlinking.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #18)
> What we should really do is to store the listener in the
> nsEventListenerManager. It has the ability to keep a bit indicating that a
> given listener is a onfoo listener. That way it can be in charge of
> converting from Function to EventListener and back. And it will
> automatically take care of the tracing and unlinking.
That doesn't quite work atm because
obj.onfoo = somefun;
obj.removeEventListener("foo", somefun, false); // This must not do anything.

I assume bz' onfoo handling rewrite will fix that all.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yeah, that would make sense.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

By idea is something like this:

1. Add a nsresult SetOnFooListener(jsval aListener, nsIAtom* aType); function
   to nsEventListenerManager
2. SetOnFooListener checks that the aListener argument is a function, returns
   an error otherwise, and creates a new object implementing
   nsIDOMEventListener otherwise. This nsIDOMEventListener is initialized with
   the jsval as a member.
3. The nsIDOMEventListener is added to the normal list of listeners but with a
   flag set indicating that it's a onfoo listener
4. Add a jsval GetOnFooListener(nsIAtom* aType); function to
   nsEventListenerManager
5. GetOnFooListener checks for a listener of the appropriate time and with the
   onfoo-flag set. If found, it casts the nsIDOMEventListener and grabs the
   contained jsval.
6. Implement the onfoo getters/setters on our various nsIDOMEventTarget in
   terms of nsEventListenerManager::G/SetOnFooListener.

Since the nsIDOMEventListener never leaves the nsEventListenerManager there is no way to call RemoveEventListener to unregister it.
There's possibly also more details to deal with regarding onfoo attributes in addition to onfoo functions.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Factor out common bits of FileReader.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 546555 [details] [diff] [review]
Factor out common bits of FileReader.

This avoids mucking with much non-FileReader code, and implements the interfaces in the way we'll want for the WebIDL-promised-land.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 546555 [details] [diff] [review]
Factor out common bits of FileReader.

If (expr) {
  stmt;
}

You're inconsistent with comments, I'd prefer the latter style:
//foobar
vs.
// foobar

(And nsDOMFileReader::DoOnStopRequest has the 2 latter parameters so that FileSaver can set them differently. )

r=me for event handling changes

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Review ping.  This has been sitting for over a month ...

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 546555 [details] [diff] [review]
Factor out common bits of FileReader.

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

::: content/base/public/nsDOMEventTargetWrapperCache.h
@@ +101,5 @@
> +#define NS_DECL_AND_IMPL_EVENT_HANDLER(_event)                                \
> +  protected:                                                                  \
> +    nsRefPtr<nsDOMEventListenerWrapper> mOn##_event##Listener;                \
> +  public:                                                                     \
> +    NS_IMETHOD GetOn##_event(nsIDOMEventListener** a##_event)                 \

Doesn't seem like there's an advantage to use a##_event here. Just use aListener to make it more readable.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 546555 [details] [diff] [review]
Factor out common bits of FileReader.

I'm totally fine with just Olli's review here. So unless there's something in particular you want me to look at here, just go as-is.

Just sent the one comment I had after very briefly starting to look at this.

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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