825419 - outparamdel nsIDocShell::GetPresShell()

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Trevor Saunders (:tbsaunde)]

basically nobody checks the return value anyway, and even when they do it only ever returns NS_OK.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: outparamdel nsIDocShell::GetPresShell()

Comment 2

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

Comment on attachment 696499 [details] [diff] [review]
outparamdel nsIDocShell::GetPresShell()


>   static DocAccessible* GetDocAccessibleFor(nsIDocShellTreeItem* aContainer)
>   {
>     nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(aContainer));
>-    nsCOMPtr<nsIPresShell> presShell;
>-    docShell->GetPresShell(getter_AddRefs(presShell));
>-    return GetAccService()->GetDocAccessible(presShell);
>+    return GetAccService()->GetDocAccessible(docShell->GetPresShell());
I assume GetDocAccessible doesn't expect caller to keep presshell alive.


>@@ -189,18 +189,17 @@ nsAccessibilityService::GetRootDocumentAccessible(nsIPresShell* aPresShell,
>   if (documentNode) {
>     nsCOMPtr<nsISupports> container = documentNode->GetContainer();
>     nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(container));
>     if (treeItem) {
>       nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
>       treeItem->GetRootTreeItem(getter_AddRefs(rootTreeItem));
>       if (treeItem != rootTreeItem) {
>         nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(rootTreeItem));
>-        nsCOMPtr<nsIPresShell> presShell;
>-        docShell->GetPresShell(getter_AddRefs(presShell));
>+        nsCOMPtr<nsIPresShell> presShell = docShell->GetPresShell();
>         documentNode = presShell->GetDocument();
I don't see reason for nsCOMPtr here.

> nsDocShell::ScrollToAnchor(nsACString & aCurHash, nsACString & aNewHash,
>                            uint32_t aLoadType)
> {
>     if (!mCurrentURI) {
>         return NS_OK;
>     }
> 
>-    nsCOMPtr<nsIPresShell> shell;
>-    nsresult rv = GetPresShell(getter_AddRefs(shell));
>-    if (NS_FAILED(rv) || !shell) {
>+    nsCOMPtr<nsIPresShell> shell = GetPresShell();
>+    if (!shell) {
>         // If we failed to get the shell, or if there is no shell,
>         // nothing left to do here.
>-        return rv;
>+        return NS_ERROR_FAILURE;
>     }
You change behavior here. Why? I'd just return NS_OK;

+++ b/docshell/base/nsIDocShell.idl
update uuid

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 696499 [details] [diff] [review]
> outparamdel nsIDocShell::GetPresShell()
> 
> 
> >   static DocAccessible* GetDocAccessibleFor(nsIDocShellTreeItem* aContainer)
> >   {
> >     nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(aContainer));
> >-    nsCOMPtr<nsIPresShell> presShell;
> >-    docShell->GetPresShell(getter_AddRefs(presShell));
> >-    return GetAccService()->GetDocAccessible(presShell);
> >+    return GetAccService()->GetDocAccessible(docShell->GetPresShell());
> I assume GetDocAccessible doesn't expect caller to keep presshell alive.

correct, there's even code slightly higher in the file that assumes that.

> >@@ -189,18 +189,17 @@ nsAccessibilityService::GetRootDocumentAccessible(nsIPresShell* aPresShell,
> >   if (documentNode) {
> >     nsCOMPtr<nsISupports> container = documentNode->GetContainer();
> >     nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(container));
> >     if (treeItem) {
> >       nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
> >       treeItem->GetRootTreeItem(getter_AddRefs(rootTreeItem));
> >       if (treeItem != rootTreeItem) {
> >         nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(rootTreeItem));
> >-        nsCOMPtr<nsIPresShell> presShell;
> >-        docShell->GetPresShell(getter_AddRefs(presShell));
> >+        nsCOMPtr<nsIPresShell> presShell = docShell->GetPresShell();
> >         documentNode = presShell->GetDocument();
> I don't see reason for nsCOMPtr here.

yeah, it can go away, it will anyway when I rebase it on top of bug 822664.

> > nsDocShell::ScrollToAnchor(nsACString & aCurHash, nsACString & aNewHash,
> >                            uint32_t aLoadType)
> > {
> >     if (!mCurrentURI) {
> >         return NS_OK;
> >     }
> > 
> >-    nsCOMPtr<nsIPresShell> shell;
> >-    nsresult rv = GetPresShell(getter_AddRefs(shell));
> >-    if (NS_FAILED(rv) || !shell) {
> >+    nsCOMPtr<nsIPresShell> shell = GetPresShell();
> >+    if (!shell) {
> >         // If we failed to get the shell, or if there is no shell,
> >         // nothing left to do here.
> >-        return rv;
> >+        return NS_ERROR_FAILURE;
> >     }
> You change behavior here. Why? I'd just return NS_OK;

it seemed safer when I was writing it, but I think your right, callers had to expect NS_OK being returned here before so safer to stay with that.  I'll change it to NS_OK.

> +++ b/docshell/base/nsIDocShell.idl
> update uuid

ugh how did I forget that

Comment 4

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba035df3292b

Comment 5

[Ed Morley [:emorley]]

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