See summary.

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;

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

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?

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/698edae2049f https://hg.mozilla.org/integration/mozilla-inbound/rev/117757b468b0 https://hg.mozilla.org/integration/mozilla-inbound/rev/952ba47dcfb7

https://hg.mozilla.org/mozilla-central/rev/698edae2049f https://hg.mozilla.org/mozilla-central/rev/117757b468b0 https://hg.mozilla.org/mozilla-central/rev/952ba47dcfb7