Convert XULDocument to WebIDL

RESOLVED FIXED in mozilla22

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla22
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Created attachment 722379 [details] [diff] [review]
part 1.  Rename nsXULDocument to mozilla::dom::XULDocument.
Attachment #722379 - Flags: review?(Ms2ger)
Comment on attachment 722379 [details] [diff] [review]
part 1.  Rename nsXULDocument to mozilla::dom::XULDocument.

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

r=me

::: .gdbinit
@@ +87,5 @@
>    end
>  end
>  
>  # define a "pxul" command to display the type of a XUL element from
> +# an XULDocument* pointer.

If I pronounce XUL correctly, "a XULDocument", not "an".

On another note, I'm surprised this isn't for nsXULElement rather than -Document.

::: content/xul/document/src/nsXULDocument.cpp
@@ +100,5 @@
>  static NS_DEFINE_CID(kParserCID,                 NS_PARSER_CID);
>  
>  static bool IsChromeURI(nsIURI* aURI)
>  {
> +    // why is this check a member function of XULDocument? -gagan

It isn't, gagan.

@@ +1791,1 @@
>                                       nsIXULTemplateBuilder* aBuilder)

Indentation

@@ +1813,1 @@
>                                       nsIXULTemplateBuilder** aResult)

Again

@@ +3878,5 @@
>  
>  
>  nsresult
> +XULDocument::OverlayForwardReference::Merge(nsIContent* aTargetNode,
> +                                            nsIContent* aOverlayNode, 

Please fix the trailing whitespace on this line

@@ +4437,1 @@
>                                                            nsISupports* acontext)

Indentation

@@ +4445,2 @@
>                                                           nsISupports* aContext,
>                                                           nsresult aStatus)

More so

@@ +4457,4 @@
>                                                             nsISupports* aContext,
>                                                             nsIInputStream* aInStr,
>                                                             uint64_t aSourceOffset,
>                                                             uint32_t aCount)

And more

::: content/xul/document/src/nsXULDocument.h
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef XULDocument_h

I would use mozilla_dom_XULDocument_h

@@ +634,1 @@
>          bool           mProtoLoaded;

It looks like these were aligned before.

::: content/xul/document/src/nsXULPrototypeDocument.cpp
@@ +33,5 @@
>  #include "mozilla/dom/BindingUtils.h"
>  
>  using mozilla::dom::DestroyProtoAndIfaceCache;
>  using mozilla::AutoPushJSContext;
> +using mozilla::dom::XULDocument;

Might as well do

using namespace mozilla;
using namespace mozilla::dom;
Attachment #722379 - Flags: review?(Ms2ger) → review+
> On another note, I'm surprised this isn't for nsXULElement rather than -Document.

Comment was a lie.  Fixed.

> It isn't, gagan.

Heh.  Fixed.

Fixed the whitespace nits.

> I would use mozilla_dom_XULDocument_h

OK, done.

> It looks like these were aligned before.

Done.

> Might as well do

Yeah, but it wasn't really needed.
Created attachment 724567 [details] [diff] [review]
part 2.  Add WebIDL API for XULDocument.
Attachment #724567 - Flags: review?(peterv)
Created attachment 724568 [details] [diff] [review]
part 3.  Convert XULDocument to WebIDL.
Attachment #724568 - Flags: review?(peterv)
Whiteboard: [need review]
Comment on attachment 724567 [details] [diff] [review]
part 2.  Add WebIDL API for XULDocument.

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

::: content/base/public/nsIDocument.h
@@ +1306,5 @@
>     * scriptable interface except for XUL documents.
>     */
> +  virtual already_AddRefed<nsIBoxObject>
> +    GetBoxObjectFor(mozilla::dom::Element* aElement,
> +                    mozilla::ErrorResult& aRv) = 0;

Need to rev the IID?

::: content/base/src/nsDocument.cpp
@@ +5936,5 @@
>    } else {
> +    nsIBoxObject* boxObject = mBoxObjectTable->GetWeak(aElement);
> +    if (boxObject) {
> +      NS_ADDREF(boxObject);
> +      return boxObject;

Nit, I think you can do:

    nsCOMPtr<nsIBoxObject> boxObject = mBoxObjectTable->Get(aElement);
    if (boxObject) {
      return boxObject.forget();
    }

::: content/xul/document/src/XULDocument.cpp
@@ +842,5 @@
>          bl = static_cast<BroadcastListener*>(entry->mListeners[i]);
>  
> +        nsCOMPtr<Element> blListener = do_QueryReferent(bl->mListener);
> +
> +        if ((blListener == &aListener) && (bl->mAttribute == attr))

While you're here, remove the inner brackets?

@@ +891,5 @@
>                  static_cast<BroadcastListener*>(entry->mListeners[i]);
>  
> +            nsCOMPtr<Element> blListener = do_QueryReferent(bl->mListener);
> +
> +            if ((blListener == &aListener) && (bl->mAttribute == attr)) {

While you're here, remove the inner brackets?

::: content/xul/document/src/XULDocument.h
@@ +221,5 @@
> +                     ErrorResult& aRv)
> +    {
> +        aRv = LoadOverlay(aURL, aObserver);
> +    }
> +    

Trailing whitespace.

::: dom/webidl/XULDocument.webidl
@@ +16,5 @@
> +
> +  /**
> +   * These attributes correspond to trustedGetPopupNode().rangeOffset and
> +   * rangeParent. They will help you find where in the DOM the popup is
> +   * happening. Can be accessed from chrome only, and only during a popup

We don't want to make these ChromeOnly?
Attachment #724567 - Flags: review?(peterv) → review+
Attachment #724568 - Flags: review?(peterv) → review+
> Need to rev the IID?

Done.

> Nit, I think you can do:

Done, with nsCOMPtr<nsPIBoxObject>.

> While you're here, remove the inner brackets?

Done, twice.

> Trailing whitespace.

Done.

> We don't want to make these ChromeOnly?

Hmm.  I think we do.  Done, and comment adjusted.
You need to log in before you can comment on or make changes to this bug.