1434399 - Do some cleanup on nsIDOMXULDocument

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch to nsINode for the popup node on windowroot

MozReview-Commit-ID: HyiHElDnmSH

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove nsIDOMXULDocument's popupNode attribute

MozReview-Commit-ID: 5x0Fa12jBvg

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove nsIDOMXULDocument's popupRangeParent/popupRangeOffset attributes

MozReview-Commit-ID: dU65u8Hx2V

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove nsIDOMXULDocument's tooltipNode attribute

MozReview-Commit-ID: 4j1xDBAHHof

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove nsIDOMXULDocument's commandDispatcher attribute

MozReview-Commit-ID: BjRVr3ScuK5

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Remove nsIDOMXULDocument's width/height attributes

MozReview-Commit-ID: 1pYKNXwqnGq

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove nsIDOMXULDocument's getElementsByAttribute(NS) methods

MozReview-Commit-ID: HN1le8EkeGr

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove nsIDOMXULDocument's broadcast listener bits

MozReview-Commit-ID: Auj7Lfu6ex3

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Remove nsIDOMXULDocument::Persist

MozReview-Commit-ID: 8LKgjwt5Yi6

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Remove nsIDOMXULDocument::GetBoxObjectFor

MozReview-Commit-ID: AvUdAtCMRRq

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Remove nsIDOMXULDocument::LoadOverlay

MozReview-Commit-ID: 4ad9AbkMcl0

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Remove JS uses of nsIDOMXULDocument

MozReview-Commit-ID: HUK8ahBwG0e

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Remove C++ uses of nsIDOMXULDocument

MozReview-Commit-ID: KSsXLra5DQk

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Remove add/remove subtree bits from nsIXULDocument

MozReview-Commit-ID: 987df30BZHq

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Remove nsIXULDocument::OnPrototypeLoadDone

MozReview-Commit-ID: 9LDh2QByUNV

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Remove nsIXULDocument::OnDocumentParserError

MozReview-Commit-ID: 7LbPTqKzkO4

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 17. Remove nsIXULDocument::ResetDocumentDirection

MozReview-Commit-ID: 18rQOWg8zvs

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 18. Remove nsIXULDocument::ResetDocumentLWTheme

MozReview-Commit-ID: 9z4HAVJAl7R

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 19. Remove nsIXULDocument

MozReview-Commit-ID: 9jQu4sjOhb2

Comment 20

[Nika Layzell [:nika] (ni? for response)]

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();

Comment 21

[Nika Layzell [:nika] (ni? for response)]

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?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 26

[Nika Layzell [:nika] (ni? for response)]

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 :-)?

Comment 27

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 28

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 29

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 30

[Nika Layzell [:nika] (ni? for response)]

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?

Comment 31

[Andrew McCreight [:mccr8]]

(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 32

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

[Cosmin Sabou [:CosminS]]

Backed out for build bustages on nsXULPopupManager.cpp

Treehder push with the fails: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=499f6dffd9cb27f7c5101e200b53396acbdc8289&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=159661128

Failure Log: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=499f6dffd9cb27f7c5101e200b53396acbdc8289&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=159661128

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/76bf3def2b51dae62623a6815eb4e663abb53797

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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

Comment 37

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/039178c19726
https://hg.mozilla.org/mozilla-central/rev/61eb041c6efa
https://hg.mozilla.org/mozilla-central/rev/738a665fd4ae
https://hg.mozilla.org/mozilla-central/rev/1866e72ff964
https://hg.mozilla.org/mozilla-central/rev/03810b4ca510
https://hg.mozilla.org/mozilla-central/rev/d7a826cbcf96
https://hg.mozilla.org/mozilla-central/rev/d111fb9215f9
https://hg.mozilla.org/mozilla-central/rev/ea6d524b971c
https://hg.mozilla.org/mozilla-central/rev/d343aa825256
https://hg.mozilla.org/mozilla-central/rev/0215e6ae7cc7
https://hg.mozilla.org/mozilla-central/rev/76a013842506
https://hg.mozilla.org/mozilla-central/rev/8010a918c9db
https://hg.mozilla.org/mozilla-central/rev/ac2ecd4a52f8
https://hg.mozilla.org/mozilla-central/rev/5e902b2e7802
https://hg.mozilla.org/mozilla-central/rev/9e70b20c40f3
https://hg.mozilla.org/mozilla-central/rev/4ef2ab1b2bdb
https://hg.mozilla.org/mozilla-central/rev/bc26668578dc
https://hg.mozilla.org/mozilla-central/rev/c3deee43606c
https://hg.mozilla.org/mozilla-central/rev/48e318052aaa