305710 - nsIDocumentEncoder should be moved to idl

Status: RESOLVED FIXED

(Core :: DOM: Serializers, enhancement)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b3) Gecko/20050719 SeaMonkey/1.0a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b3) Gecko/20050719 SeaMonkey/1.0a

nsIDocumentEncoder
(http://lxr.mozilla.org/mozilla/source/content/base/public/nsIDocumentEncoder.h)
contains constants witch used in outputToString() and outputToString() methods
of nsIEditor inteface. Either constants or nsIDocumentEncoder class entirely
should be moved to idl.

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

Agreed.

Comment 2

[jpl24]

Attachment: nsIDocumentEncoder_v1

This is pretty much a direct port of the existing interface to idl. Builds correctly, but probably could use some cleanup and more doxygen love.

Comment 3

[jpl24]

Attachment: build_patch_v1

These are the minor changes required to other files so this builds. I'm not going to request review on these yet to give people a chance to comment.

Comment 4

[jpl24]

Comment on attachment 205773 [details] [diff] [review]
nsIDocumentEncoder_v1

One thing I just noticed, mimetype should probably be a readonly attribute. I can fix that, but I'll wait for review comments first.

Comment 5

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

Comment on attachment 205773 [details] [diff] [review]
nsIDocumentEncoder_v1

> * The Initial Developer of the Original Code is
> * Netscape Communications Corporation.
> * Portions created by the Initial Developer are Copyright (C) 1998
> * the Initial Developer. All Rights Reserved.

This doesn't seem right.

>    /** 
>     * LineBreak processing: we can do either platform line breaks,
>     * CR, LF, or CRLF.  If neither of these flags is set, then we
>     * will use platform line breaks.
>     */
>    const unsigned long OutputCRLineBreak = 512;
>
>    /**
>     *
>     */
>    const unsigned long OutputLFLineBreak = 1024;

Restructure the comments so that you get a proper comment for both values.


>    /**
>     * Encode entities when outputting to a string.
>     * E.g. If set, we'll output   if clear, we'll output 0xa0.
>     * The basic set is just   & < > " for interoperability
>     * with older products that don't support α and friends.
>     * The Latin1 entity set additionally includes 8bit accented letters
>     * between 128 and 255.
>     * The HTML entity set additionally includes accented letters, greek
>     * letters, and other special markup symbols as defined in HTML4.
>     */
>    const unsigned long OutputEncodeBasicEntities = 16384;
>    
>    /** 
>     *
>     */
>    const unsigned long OutputEncodeLatin1Entities = 32768;
>    
>    /** 
>     *
>     */
>    const unsigned long OutputEncodeHTMLEntities = 65536;

Same here. With that, r=me

Comment 6

[jpl24]

Thanks for the review, I'll update the patch as soon as I can. Might be a few days, the laptop the build is on is currently struggling.

Comment 7

[jpl24]

Attachment: patch_v2

Updated to comments, all changes rolled into one patch.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208929 [details] [diff] [review]
patch_v2

>Index: content/base/public/nsIDocumentEncoder.idl
>+ * The Initial Developer of the Original Code is
>+ * the Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2006
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):

Is the original developer really the Mozilla Foundation?  Not you?  At least add yourself to the Contributors list like so:

  First Last <email@whatever> (Original Author)

or something?

>+interface nsIDocumentEncoder;

That line is not needed.

>+interface nsIDocumentEncoderNodeFixup;

Same for this one.

>+interface nsIDocumentEncoderNodeFixup : nsISupports
>+   * may return a new node with fixed up attributes or nsnull.

s/nsnull/null/ in idl

>+   * @param [OUT] The resulting fixed up node.

That's a @return, not a @param [OUT].

So if null is returned does that mean "use the passed-in node as-is"?  Is it ok to just return the passed-in node?

>+  nsIDOMNode fixupNode(in nsIDOMNode node);

aNode, please, for an argument.

>+  // Output methods flag bits. There are a frightening number of these,
>+  // because everyone wants something a little bit different

In fact, there are enough of them that just writing the numbers is starting to not scale well.  Want to write them as (1 << whatever) for various values of whatever?  I think that would be much clearer.  I realize you just left them as-is, but I think this change is worth it.

>+  void init(in nsIDocument document, in AString mimeType, in unsigned long flags);

This interface is scriptable, isn't it?  If so, this method needs to be noscript; nsIDocument is so not a scriptable type.

Also, aDocument, aMimeType, aFlags, please.

>+  void setSelection(in nsISelection selection);

aSelection

>+  void setRange(in nsIDOMRange range);

aRange

>+  void setNode(in nsIDOMNode node);

aNode

>+  void setCharset(in ACString charset);

aCharset

>+  void setWrapColumn(in unsigned long wrapColumn);

aWrapColumn

>+  void encodeToStream(in nsIOutputStream stream);

aStream

+  /**
+   *  The document is encoded, the result is sent to the 
+   *  to aEncodedString.  Parent heirarchy information is encoded
+   *  to aContextString.  Extra context info is encoded in aInfoString.
+   * 
+   */
>+  AString encodeToStringWithContext( out AString contextString,
>+                                     out AString infoString);

This comment doesn't match the signature.  More importantly, the C++ version used to be:

-  NS_IMETHOD EncodeToStringWithContext(nsAString& aEncodedString, 
-                                       nsAString& aContextString, 
-                                       nsAString& aInfoString) = 0;

whereas your IDL will produce the C++ header:

NS_IMETHOD EncodeToStringWithContext(nsAString& aContextString, 
                                     nsAString& aInfoString,
                                     nsAString& _retval) = 0;

I suppose C++ callers are unaffected since the signature is the same, just not the argument names, but now the C++ impl of this method doesn't match the IDL signature, and JS callers will definitely get bad data.  I'd either leave it as three out params, or make the aInfoString the return value and document things.  Or change things like you did, but also fix all existing consumers of this method and the impl to deal with the new argument ordering.

Oh, and use aWhatever for arg names.

>+  void setNodeFixup(in nsIDocumentEncoderNodeFixup fixup);

aFixup.

The rest looks good.  Fix the above issues?

Comment 9

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

You don't need to 'a'-prefix arguments in idl files, the idl compiler will do that for you.

However, the fact that the |init| method takes an nsIDocument sort of makes this interface useless from script.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I think it should be eminently reasonable to do a followup to make this usefully scriptable (eg s/nsIDocument/nsIDOMDocument/ or so) and update existing callers to deal.

Comment 11

[jpl24]

Whoops, forgot I hadn't uploaded the newest version of this patch. Responses to comments follow:

> >+interface nsIDocumentEncoderNodeFixup : nsISupports
> >+   * may return a new node with fixed up attributes or nsnull.
> 
> s/nsnull/null/ in idl
> 
> >+   * @param [OUT] The resulting fixed up node.
> 
> That's a @return, not a @param [OUT].
> 
> So if null is returned does that mean "use the passed-in node as-is"?  Is it ok
> to just return the passed-in node?
Currently the code which uses nsINodeFixup assumes that returning null means to use the node as is. It might be simpler for nsINodeFixup to garuntee the result is non-null, but I left it as is and commented the behavior.

> This comment doesn't match the signature.  More importantly, the C++ version
> used to be:
> 
> -  NS_IMETHOD EncodeToStringWithContext(nsAString& aEncodedString, 
> -                                       nsAString& aContextString, 
> -                                       nsAString& aInfoString) = 0;
> 
> whereas your IDL will produce the C++ header:
> 
> NS_IMETHOD EncodeToStringWithContext(nsAString& aContextString, 
>                                      nsAString& aInfoString,
>                                      nsAString& _retval) = 0;
> 
> I suppose C++ callers are unaffected since the signature is the same, just not
> the argument names, but now the C++ impl of this method doesn't match the IDL
> signature, and JS callers will definitely get bad data.  I'd either leave it as
> three out params, or make the aInfoString the return value and document things.
>  Or change things like you did, but also fix all existing consumers of this
> method and the impl to deal with the new argument ordering.
I changed the callers and implementors to match the signature as I had changed it. I'm not sure why I assumed the return value would be first...

Comment 12

[jpl24]

Attachment: patch_v3

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 214964 [details] [diff] [review]
patch_v3

sr=bzbarsky; please make sure a followup bug is filed to make init() scriptable!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks for the fix!

Comment 15

[Stefan Sitter [:ssitter]]

It seem that this check in has made the Linux galactica tinderbox go red:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird

builds/tinderbox/Sb-Trunk/Linux_2.4.21-32.0.1.ELsmp_Depend/mozilla/extensions/
webdav/src/nsOperationStreamListeners.cpp: In
   member function `virtual nsresult 
   PropfindStreamListener::PropertiesFromPropElt(nsIDOMElement*, 
   nsIProperties**)':
/builds/tinderbox/Sb-Trunk/Linux_2.4.21-32.0.1.ELsmp_Depend/mozilla/extensions/
webdav/src/nsOperationStreamListeners.cpp:371: `
   NS_DOC_ENCODER_CONTRACTID_BASE' undeclared (first use this function)

Comment 16

[jpl24]

Ah, I somehow missed this file when I was making changes. This file of course needs #include "nsContentCID.h". I just crawled through LXR again and I think this file /embedding/browser/activex/src/common/IEHtmlElement.cpp needs the fix as well. Boris if you could fix these I would much appreciate it, I'm not at a computer with a build on it so I can't make a patch right now.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Build bustage fix

I just checked this in.

Comment 18

[jpl24]

Thanks for fixing my bustage. Followed bug 330517 on fixing init to be scriptable.