939049 - staticly type nsIDocument::mDocumentContainer and nsDocumentViewerContainer

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Trevor Saunders (:tbsaunde)]

I'm pretty sure these always point at a docshell, but currently they use nsWeakPtr which means we need to use nsISupports all over :-( So patch comming up to use mozilla::WeakPtr<nsDocShell> saddly this means a few more local includes but not too too many places need to see nsDocShell.h and this should let us remove a whole bunch of QI crap so it seems like a win

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: staticly type nsIDocument::mDocumentContainer and nsDocumentViewerContainer::mContainer

kind of big patch, but I hear smaug likes those ;)

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: get rid of useless QIs

not really subject of the bug, but it is the reason I wanted to fix the bug or at least part of that.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 832816 [details] [diff] [review]
staticly type nsIDocument::mDocumentContainer and nsDocumentViewerContainer::mContainer

The patch uses .get() in many many places. Makes the code a bit ugly.
Could you get rid of that?



>@@ -29,17 +30,17 @@ class nsDOMNavigationTiming;
> 
> [scriptable, builtinclass, uuid(b9d92b8b-5623-4079-ae11-36bb341f322e)]
> interface nsIContentViewer : nsISupports
update uuid

> nsresult nsPrintEngine::Initialize(nsIDocumentViewerPrint* aDocViewerPrint, 
>-                                   nsIWeakReference*       aContainer,
>+                                   nsIDocShell*       aContainer,
Missing spaces before aContainer

>--- a/layout/printing/nsPrintEngine.h
>+++ b/layout/printing/nsPrintEngine.h
>@@ -77,17 +77,17 @@ public:
> 
>   nsPrintEngine();
>   ~nsPrintEngine();
> 
>   void Destroy();
>   void DestroyPrintingData();
> 
>   nsresult Initialize(nsIDocumentViewerPrint* aDocViewerPrint, 
>-                      nsIWeakReference*       aContainer,
>+                      nsIDocShell*       aContainer,
Missing spaces before aContainer


>                       nsIDocument*            aDocument,
>                       float                   aScreenDPI,
>                       FILE*                   aDebugFile);

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f2e964f10799

Comment 5

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #4)
> https://hg.mozilla.org/mozilla-central/rev/f2e964f10799

Wrong bug number in commit message - was intended for bug 935049.

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: staticly type nsIDocument::mDocumentContainer and nsDocumentViewerContainer::mContainer

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8333796 [details] [diff] [review]
staticly type nsIDocument::mDocumentContainer and nsDocumentViewerContainer::mContainer

>-  virtual void SetContainer(nsISupports *aContainer);
>+  virtual void SetContainer(nsDocShell *aContainer);
nsDocShell* aContainer


> void
>-nsIDocument::SetContainer(nsISupports* aContainer)
>+nsIDocument::SetContainer(nsDocShell* aContainer)
> {
>-  mDocumentContainer = do_GetWeakReference(aContainer);
>+  if (!aContainer) {
>+    return;
>+  }
>+
>+  mDocumentContainer = aContainer->asWeakPtr();
>   EnumerateFreezableElements(NotifyActivityChanged, nullptr);

You're changing behavior here.
The old code calls EnumerateFreezableElements even if null is passed, the new code return early.

I'd like to see a new patch where nsIDocument::SetContainer is fixed.

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: staticly type nsIDocument::mDocumentContainer and nsDocumentViewerContainer::mContainer

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8334007 [details] [diff] [review]
staticly type nsIDocument::mDocumentContainer and nsDocumentViewerContainer::mContainer


>-nsIDocument::SetContainer(nsISupports* aContainer)
>+nsIDocument::SetContainer(nsDocShell* aContainer)
> {
>-  mDocumentContainer = do_GetWeakReference(aContainer);
>+  if (aContainer) {
>+    mDocumentContainer = aContainer->asWeakPtr();
>+  } else {
>+    mDocumentContainer = WeakPtr<nsDocShell>();
This is just so horrible, but WeakPtr is.

Comment 10

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ad287f78608a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/babac1cc0741

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a92288689224 - there must be something funky in the Mac shell service (waits for the gasps of shock to die down) setting desktop background, which died building, like https://tbpl.mozilla.org/php/getParsedLog.php?id=30768913&tree=Mozilla-Inbound

Comment 12

[Trevor Saunders (:tbsaunde)]

So nsIDocument::GetContainer is used outside libxul fun. smaug any reason I shouldn't make it virtual and MOZ_FINAL?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

if that works, fine.

Comment 14

[Trevor Saunders (:tbsaunde)]

tried again
https://hg.mozilla.org/integration/mozilla-inbound/rev/403bb511d10b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0a0081b34af7

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/403bb511d10b
https://hg.mozilla.org/mozilla-central/rev/0a0081b34af7