Closed Bug 939049 Opened 6 years ago Closed 6 years ago

staticly type nsIDocument::mDocumentContainer and nsDocumentViewerContainer

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

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
kind of big patch, but I hear smaug likes those ;)
Attachment #832816 - Flags: review?(bugs)
not really subject of the bug, but it is the reason I wanted to fix the bug or at least part of that.
Attachment #832969 - Flags: review?(bugs)
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);
Attachment #832816 - Flags: review?(bugs) → review-
Attachment #832969 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/f2e964f10799
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8333796 - Flags: review?(bugs) → review-
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.
Attachment #8334007 - Flags: review?(bugs) → review+
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
So nsIDocument::GetContainer is used outside libxul fun. smaug any reason I shouldn't make it virtual and MOZ_FINAL?
if that works, fine.
https://hg.mozilla.org/mozilla-central/rev/403bb511d10b
https://hg.mozilla.org/mozilla-central/rev/0a0081b34af7
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Blocks: 942190
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.