Closed
Bug 1434399
Opened 7 years ago
Closed 7 years ago
Do some cleanup on nsIDOMXULDocument
Categories
(Core :: XUL, enhancement)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(19 files)
No description provided.
![]() |
Assignee | |
Comment 1•7 years ago
|
||
MozReview-Commit-ID: HyiHElDnmSH
Attachment #8946987 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 2•7 years ago
|
||
MozReview-Commit-ID: 5x0Fa12jBvg
Attachment #8946988 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 3•7 years ago
|
||
MozReview-Commit-ID: dU65u8Hx2V
Attachment #8946989 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 4•7 years ago
|
||
MozReview-Commit-ID: 4j1xDBAHHof
Attachment #8946990 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 5•7 years ago
|
||
MozReview-Commit-ID: BjRVr3ScuK5
Attachment #8946991 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 6•7 years ago
|
||
MozReview-Commit-ID: 1pYKNXwqnGq
Attachment #8946992 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 7•7 years ago
|
||
MozReview-Commit-ID: HN1le8EkeGr
Attachment #8946993 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 8•7 years ago
|
||
MozReview-Commit-ID: Auj7Lfu6ex3
Attachment #8946994 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 9•7 years ago
|
||
MozReview-Commit-ID: 8LKgjwt5Yi6
Attachment #8946995 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 10•7 years ago
|
||
MozReview-Commit-ID: AvUdAtCMRRq
Attachment #8946996 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 11•7 years ago
|
||
MozReview-Commit-ID: 4ad9AbkMcl0
Attachment #8946997 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 12•7 years ago
|
||
MozReview-Commit-ID: HUK8ahBwG0e
Attachment #8946998 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 13•7 years ago
|
||
MozReview-Commit-ID: KSsXLra5DQk
Attachment #8946999 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 14•7 years ago
|
||
MozReview-Commit-ID: 987df30BZHq
Attachment #8947000 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 15•7 years ago
|
||
MozReview-Commit-ID: 9LDh2QByUNV
Attachment #8947001 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 16•7 years ago
|
||
MozReview-Commit-ID: 7LbPTqKzkO4
Attachment #8947002 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 17•7 years ago
|
||
MozReview-Commit-ID: 18rQOWg8zvs
Attachment #8947003 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 18•7 years ago
|
||
MozReview-Commit-ID: 9z4HAVJAl7R
Attachment #8947004 -
Flags: review?(nika)
![]() |
Assignee | |
Comment 19•7 years ago
|
||
MozReview-Commit-ID: 9jQu4sjOhb2
Attachment #8947005 -
Flags: review?(nika)
Comment 20•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8946988 -
Flags: review?(nika) → review+
Comment 21•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8946990 -
Flags: review?(nika) → review+
![]() |
Assignee | |
Comment 22•7 years ago
|
||
> 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.
Updated•7 years ago
|
Attachment #8946991 -
Flags: review?(nika) → review+
Comment 23•7 years ago
|
||
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+
![]() |
Assignee | |
Comment 24•7 years ago
|
||
> 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•7 years ago
|
||
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 26•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8946995 -
Flags: review?(nika) → review+
Comment 27•7 years ago
|
||
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 28•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8946998 -
Flags: review?(nika) → review+
Comment 29•7 years ago
|
||
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 30•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8947001 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947002 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947003 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8947004 -
Flags: review?(nika) → review+
Comment 31•7 years ago
|
||
(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•7 years ago
|
||
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+
![]() |
Assignee | |
Comment 33•7 years ago
|
||
> 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•7 years ago
|
||
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
Comment 35•7 years ago
|
||
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
Flags: needinfo?(bzbarsky)
Comment 36•7 years ago
|
||
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
![]() |
Assignee | |
Updated•7 years ago
|
Flags: needinfo?(bzbarsky)
Comment 37•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•