Closed Bug 1434399 Opened 2 years ago Closed 2 years ago

Do some cleanup on nsIDOMXULDocument

Categories

(Core :: XUL, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(19 files)

19.92 KB, patch
Nika
: review+
Details | Diff | Splinter Review
3.45 KB, patch
Nika
: review+
Details | Diff | Splinter Review
9.10 KB, patch
Nika
: review+
Details | Diff | Splinter Review
2.56 KB, patch
Nika
: review+
Details | Diff | Splinter Review
11.47 KB, patch
Nika
: review+
Details | Diff | Splinter Review
2.51 KB, patch
Nika
: review+
Details | Diff | Splinter Review
7.22 KB, patch
Nika
: review+
Details | Diff | Splinter Review
4.85 KB, patch
Nika
: review+
Details | Diff | Splinter Review
10.43 KB, patch
Nika
: review+
Details | Diff | Splinter Review
1.96 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.09 KB, patch
Nika
: review+
Details | Diff | Splinter Review
2.95 KB, patch
Nika
: review+
Details | Diff | Splinter Review
14.81 KB, patch
Nika
: review+
Details | Diff | Splinter Review
5.55 KB, patch
Nika
: review+
Details | Diff | Splinter Review
3.32 KB, patch
Nika
: review+
Details | Diff | Splinter Review
3.80 KB, patch
Nika
: review+
Details | Diff | Splinter Review
4.47 KB, patch
Nika
: review+
Details | Diff | Splinter Review
4.63 KB, patch
Nika
: review+
Details | Diff | Splinter Review
16.99 KB, patch
Nika
: review+
Details | Diff | Splinter Review
No description provided.
MozReview-Commit-ID: 5x0Fa12jBvg
Attachment #8946988 - Flags: review?(nika)
MozReview-Commit-ID: 8LKgjwt5Yi6
Attachment #8946995 - Flags: review?(nika)
MozReview-Commit-ID: AvUdAtCMRRq
Attachment #8946996 - Flags: review?(nika)
MozReview-Commit-ID: 4ad9AbkMcl0
Attachment #8946997 - Flags: review?(nika)
MozReview-Commit-ID: HUK8ahBwG0e
Attachment #8946998 - Flags: review?(nika)
MozReview-Commit-ID: KSsXLra5DQk
Attachment #8946999 - Flags: review?(nika)
MozReview-Commit-ID: 9LDh2QByUNV
Attachment #8947001 - Flags: review?(nika)
MozReview-Commit-ID: 7LbPTqKzkO4
Attachment #8947002 - Flags: review?(nika)
MozReview-Commit-ID: 9z4HAVJAl7R
Attachment #8947004 - Flags: review?(nika)
MozReview-Commit-ID: 9jQu4sjOhb2
Attachment #8947005 - Flags: review?(nika)
Comment on attachment 8946987 [details] [diff] [review]
part 1.  Switch to nsINode for the popup node on windowroot

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

::: dom/base/nsWindowRoot.cpp
@@ +370,5 @@
>      focusedWindow = win->GetPrivateParent();
>    }
>  }
>  
> +already_AddRefed<nsINode>

Oof - the fact that the old version of this method returned a non-owning pointer from a weak reference is kinda scary...

::: dom/xul/XULDocument.cpp
@@ +1360,5 @@
>  
>      if (node && nsContentUtils::CanCallerAccess(node)
>          && GetScopeObjectOfNode(node)) {
> +        *aNode = node->AsDOMNode();
> +        NS_ADDREF(*aNode);

Not a super big fan of this here and below. Perhaps use the more common

NS_{IF_}ADDREF(*aNode = node->AsDOMNode());
-or-
nsCOMPtr<nsIDOMNode> domNode = node->AsDOMNode();
domNode.forget(aNode);
-or-
*aNode = do_AddRef(node->AsDOMNode()).take();
Attachment #8946987 - Flags: review?(nika) → review+
Attachment #8946988 - Flags: review?(nika) → review+
Comment on attachment 8946989 [details] [diff] [review]
part 3.  Remove nsIDOMXULDocument's popupRangeParent/popupRangeOffset attributes

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

::: layout/xul/nsXULPopupManager.cpp
@@ +631,5 @@
>    mCachedModifiers = 0;
>  
>    nsCOMPtr<nsIDOMUIEvent> uiEvent = do_QueryInterface(aEvent);
>    if (uiEvent) {
> +    mRangeParent = static_cast<UIEvent*>(uiEvent.get())->GetRangeParent();

nit: Cast once and store the UIEvent* in a local perhaps?
Attachment #8946989 - Flags: review?(nika) → review+
Attachment #8946990 - Flags: review?(nika) → review+
> Oof - the fact that the old version of this method returned a non-owning pointer
> from a weak reference is kinda scary...

Yeah.. it kinda works if callers take a ref, but....

> Not a super big fan of this here and below. Perhaps use the more common

I would fix it, but that code dies in the later patches anyway.

> nit: Cast once and store the UIEvent* in a local perhaps?

Can do.
Attachment #8946991 - Flags: review?(nika) → review+
Comment on attachment 8946992 [details] [diff] [review]
part 6.  Remove nsIDOMXULDocument's width/height attributes

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

::: dom/xul/XULDocument.cpp
@@ +1285,4 @@
>  int32_t
>  XULDocument::GetWidth(ErrorResult& aRv)
>  {
> +    int32_t width = 0, height;

nit: please put on multiple lines and initialize both to 0 explicitly.

@@ +1293,4 @@
>  int32_t
>  XULDocument::GetHeight(ErrorResult& aRv)
>  {
> +    int32_t width, height = 0;

nit: please put on multiple lines and initialize both to 0 explicitly.
Attachment #8946992 - Flags: review?(nika) → review+
> nit: please put on multiple lines and initialize both to 0 explicitly.

Done.  Callee is an annoying API; I don't care about the one that's not initialized.... ;)
Comment on attachment 8946993 [details] [diff] [review]
part 7.  Remove nsIDOMXULDocument's getElementsByAttribute(NS) methods

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

::: accessible/generic/moz.build
@@ +30,5 @@
>      '/accessible/html',
>      '/accessible/xpcom',
>      '/accessible/xul',
>      '/dom/base',
> +    '/dom/xul',

:'-( - ok.

Perhaps we should stop pretending that XULDocument.h is a private header.
Attachment #8946993 - Flags: review?(nika) → review+
Comment on attachment 8946994 [details] [diff] [review]
part 8.  Remove nsIDOMXULDocument's broadcast listener bits

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

::: dom/interfaces/xul/nsIDOMXULDocument.idl
@@ +18,5 @@
>    nsIBoxObject getBoxObjectFor(in nsIDOMElement elt);
>  
>    /**
>     * Loads a XUL overlay and merges it with the current document, notifying an
>     * observer when the merge is complete. 

any chance you wanna clean up this trailing whitespace while you're here :-)?
Attachment #8946994 - Flags: review?(nika) → review+
Attachment #8946995 - Flags: review?(nika) → review+
Comment on attachment 8946996 [details] [diff] [review]
part 10.  Remove nsIDOMXULDocument::GetBoxObjectFor

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

I really like the commits like these, they're so easy to review.
Attachment #8946996 - Flags: review?(nika) → review+
Comment on attachment 8946997 [details] [diff] [review]
part 11.  Remove nsIDOMXULDocument::LoadOverlay

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

::: dom/webidl/XULDocument.webidl
@@ +53,5 @@
>    BoxObject? getBoxObjectFor(Element? element);
>  
> +  /**
> +   * Loads a XUL overlay and merges it with the current document, notifying an
> +   * observer when the merge is complete. 

There's a good amount of trailing whitespace in this comment.

::: dom/xul/XULDocument.cpp
@@ +2204,5 @@
>      bool shouldReturn, failureFromContent;
>      rv = LoadOverlayInternal(uri, true, &shouldReturn, &failureFromContent);
> +    if (NS_FAILED(rv)) {
> +        if (mOverlayLoadObservers) {
> +            mOverlayLoadObservers->Remove(uri); // remove the observer if

nit: move this comment into a separate line above the method call, rather than having it visually aligned to the right.
Attachment #8946997 - Flags: review?(nika) → review+
Attachment #8946998 - Flags: review?(nika) → review+
Comment on attachment 8946999 [details] [diff] [review]
part 13.  Remove C++ uses of nsIDOMXULDocument

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

::: dom/interfaces/xul/nsIDOMXULDocument.idl
@@ -10,5 @@
> -interface nsIObserver;
> -interface nsIBoxObject;
> -
> -[uuid(7790d4c3-e8f0-4e29-9887-d683ed2b2a44)]
> -interface nsIDOMXULDocument : nsIDOMDocument

It's Beautiful.
Attachment #8946999 - Flags: review?(nika) → review+
Comment on attachment 8947000 [details] [diff] [review]
part 14.  Remove add/remove subtree bits from nsIXULDocument

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

::: dom/xbl/nsXBLBinding.cpp
@@ +18,5 @@
>  #include "nsIContent.h"
>  #include "nsIDocument.h"
>  #include "nsContentUtils.h"
>  #include "ChildIterator.h"
>  #ifdef MOZ_XUL

passing thought: Is MOZ_XUL actually a ifdef which we can build anything useful with it turned off? Should we be filing a bug for just ripping these guards out of the tree?
Attachment #8947000 - Flags: review?(nika) → review+
Attachment #8947001 - Flags: review?(nika) → review+
Attachment #8947002 - Flags: review?(nika) → review+
Attachment #8947003 - Flags: review?(nika) → review+
Attachment #8947004 - Flags: review?(nika) → review+
(In reply to Nika Layzell [:mystor] from comment #30)
> passing thought: Is MOZ_XUL actually a ifdef which we can build anything
> useful with it turned off? Should we be filing a bug for just ripping these
> guards out of the tree?

IIRC at some point it was disabled in B2G, so it isn't totally hopeless.
Comment on attachment 8947005 [details] [diff] [review]
part 19.  Remove nsIXULDocument

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

::: dom/xul/XULDocument.h
@@ +48,5 @@
>   * The XUL document class
>   */
>  
> +// Factory function.
> +nsresult NS_NewXULDocument(nsIDocument** result);

Why do you need to forward declare this method here? Is the `friend` decl in XULDocument not enough?

::: layout/build/nsLayoutModule.cpp
@@ +430,5 @@
>  MAKE_CTOR(CreatePlainTextSerializer,      nsIContentSerializer,        NS_NewPlainTextSerializer)
>  MAKE_CTOR(CreateContentPolicy,            nsIContentPolicy,            NS_NewContentPolicy)
>  #ifdef MOZ_XUL
>  MAKE_CTOR(CreateXULSortService,           nsIXULSortService,           NS_NewXULSortService)
> +MAKE_CTOR(CreateXULDocument,              nsIDocument,                 NS_NewXULDocument)

With the ContractID being removed as well, do we still need this CTOR? I peeked in Searchfox and couldn't find any consumers of CreateXULDocument other than the definition of kNS_XULDOCUMENT_CID, which also doesn't seem to be used.
Attachment #8947005 - Flags: review?(nika) → review+
> Perhaps we should stop pretending that XULDocument.h is a private header.

Yeah, maybe we should just export it...

> any chance you wanna clean up this trailing whitespace while you're here 

This whole file will be gone by the end of the patch queue, so not worrying about it.

> There's a good amount of trailing whitespace in this comment.

Ah, same comment.  OK, now I better fix it.  Done.

> nit: move this comment into a separate line above the method call

Done.

> Is MOZ_XUL actually a ifdef which we can build anything useful with it turned off? 

Unclear....

> Why do you need to forward declare this method here?

Friend decls without a preceding forward-declare used to declare the function in weirdly different ways on different compilers at some point.  Not sure whether that's still the case...

> and couldn't find any consumers of CreateXULDocument other than the definition of kNS_XULDOCUMENT_CID

I thought so too when I first wrote the patches, but then I found a consumer.  There's kNS_XULDOCUMENT_CID, which is defined in terms of NS_XULDOCUMENT_CID.  But there's another thing defined in terms of it: kXULDocumentCID over in nsContentDLF.cpp.  And that does:

  nsCOMPtr<nsIDocument> doc = do_CreateInstance(kXULDocumentCID, &rv);

in nsContentDLF::CreateXULDocument.  I'd like to switch nsContentDLF off of XPCOM bits like that, but figured that was out of scope for this patch set.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55250a54852a
part 1.  Switch to nsINode for the popup node on windowroot.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/fab1f8b087a2
part 2.  Remove nsIDOMXULDocument's popupNode attribute.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/21341b656b0f
part 3.  Remove nsIDOMXULDocument's popupRangeParent/popupRangeOffset attributes.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fb30e066cbd
part 4.  Remove nsIDOMXULDocument's tooltipNode attribute.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5343ef34d6c
part 5.  Remove nsIDOMXULDocument's commandDispatcher attribute.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/54cc4cb2fca1
part 6.  Remove nsIDOMXULDocument's width/height attributes.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/b578cc8efb92
part 7.  Remove nsIDOMXULDocument's getElementsByAttribute(NS) methods.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/55cdf8b3a959
part 8.  Remove nsIDOMXULDocument's broadcast listener bits.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/faefb2751fdc
part 9.  Remove nsIDOMXULDocument::Persist.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/60fe73be1a26
part 10.  Remove nsIDOMXULDocument::GetBoxObjectFor.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff378a6e96a
part 11.  Remove nsIDOMXULDocument::LoadOverlay.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b18f4a53be
part 12.  Remove JS uses of nsIDOMXULDocument.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eaa395d6200
part 13.  Remove C++ uses of nsIDOMXULDocument.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc98ed8c609a
part 14.  Remove add/remove subtree bits from nsIXULDocument.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2391af01dd
part 15.  Remove nsIXULDocument::OnPrototypeLoadDone.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ce2826b857
part 16.  Remove nsIXULDocument::OnDocumentParserError.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c3179f8e59
part 17.  Remove nsIXULDocument::ResetDocumentDirection.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/018290612415
part 18.  Remove nsIXULDocument::ResetDocumentLWTheme.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/499f6dffd9cb
part 19.  Remove nsIXULDocument.  r=mystor
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/039178c19726
part 1.  Switch to nsINode for the popup node on windowroot.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/61eb041c6efa
part 2.  Remove nsIDOMXULDocument's popupNode attribute.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/738a665fd4ae
part 3.  Remove nsIDOMXULDocument's popupRangeParent/popupRangeOffset attributes.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/1866e72ff964
part 4.  Remove nsIDOMXULDocument's tooltipNode attribute.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/03810b4ca510
part 5.  Remove nsIDOMXULDocument's commandDispatcher attribute.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a826cbcf96
part 6.  Remove nsIDOMXULDocument's width/height attributes.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d111fb9215f9
part 7.  Remove nsIDOMXULDocument's getElementsByAttribute(NS) methods.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6d524b971c
part 8.  Remove nsIDOMXULDocument's broadcast listener bits.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d343aa825256
part 9.  Remove nsIDOMXULDocument::Persist.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/0215e6ae7cc7
part 10.  Remove nsIDOMXULDocument::GetBoxObjectFor.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a013842506
part 11.  Remove nsIDOMXULDocument::LoadOverlay.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/8010a918c9db
part 12.  Remove JS uses of nsIDOMXULDocument.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2ecd4a52f8
part 13.  Remove C++ uses of nsIDOMXULDocument.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e902b2e7802
part 14.  Remove add/remove subtree bits from nsIXULDocument.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e70b20c40f3
part 15.  Remove nsIXULDocument::OnPrototypeLoadDone.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef2ab1b2bdb
part 16.  Remove nsIXULDocument::OnDocumentParserError.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc26668578dc
part 17.  Remove nsIXULDocument::ResetDocumentDirection.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3deee43606c
part 18.  Remove nsIXULDocument::ResetDocumentLWTheme.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e318052aaa
part 19.  Remove nsIXULDocument.  r=mystor
Flags: needinfo?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.