Last Comment Bug 721947 - don't use nsIWeakShell
: don't use nsIWeakShell
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on:
Blocks: cleana11y 672504
  Show dependency treegraph
 
Reported: 2012-01-27 20:48 PST by alexander :surkov
Modified: 2012-02-10 05:06 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use nsIPresShell instead of nsIWeakShell. r= (37.99 KB, patch)
2012-01-30 19:06 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
use nsIPresShell instead of nsIWeakShell. r= (46.74 KB, patch)
2012-02-08 08:42 PST, Hubert Figuiere [:hub]
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-01-27 20:48:07 PST
when bug 672504 is fixed then we can switch to nsIPresShell, we have several instances where nsIWeakShell is used like nsAccTreeWalker and nsAccessibilityService.

Hub, wanna take it?
Comment 1 Hubert Figuiere [:hub] 2012-01-30 19:06:53 PST
Created attachment 592960 [details] [diff] [review]
use nsIPresShell instead of nsIWeakShell. r=
Comment 2 Hubert Figuiere [:hub] 2012-01-30 19:07:30 PST
patch to be applied on top of bug 673405
Comment 3 alexander :surkov 2012-01-30 20:38:24 PST
Comment on attachment 592960 [details] [diff] [review]
use nsIPresShell instead of nsIWeakShell. r=

Review of attachment 592960 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsDocAccessibleWrap.cpp
@@ +47,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsDocAccessibleWrap::
> +    nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                        nsIPresShell* aShell) :

2 space indent
aShell -> aPresShell and do that everywhere in the patch please

::: accessible/src/base/nsAccDocManager.cpp
@@ +382,5 @@
>      // XXXaaronl: ideally we would traverse the presshell chain. Since there's
>      // no easy way to do that, we cheat and use the document hierarchy.
>      // GetAccessible() is bad because it doesn't support our concept of multiple
>      // presshells per doc. It should be changed to use
> +    // GetAccessibleInShell().

get rid GetAccessibleIsShell at all, make GetAccessible take nsIPresShell as argument

::: accessible/src/base/nsAccTreeWalker.cpp
@@ +69,2 @@
>                    bool aWalkAnonContent, bool aWalkCache) :
> +  mShell(aShell), mWalkCache(aWalkCache), mState(nsnull)

use nsDocAccessible instead of nsIPresShell

::: accessible/src/base/nsAccessibilityService.cpp
@@ +941,2 @@
>                                                bool* aIsSubtreeHidden)
>  {

make it take nsDocAccessible instead nsIPresShell

::: accessible/src/base/nsTextEquivUtils.cpp
@@ +117,5 @@
>      return NS_OK;
>  
>    gInitiatorAcc = aInitiatorAcc;
>  
> +  nsIPresShell* shell = nsCoreUtils::GetPresShellFor(aContent);

it makes sense to use nsDocAcessbile instead

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +351,4 @@
>  
>      do {
>        nsAccessible* headerCell =
> +        GetAccService()->GetAccessibleInShell(headerCellElm, presShell);

use nsDocAccessible::GetAccessible()

@@ +721,5 @@
>        if (NS_SUCCEEDED(rv) && startRowIndex == rowIndex &&
>            startColIndex == columnIndex && isSelected) {
>          nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement));
>          nsAccessible *cell =
> +          GetAccService()->GetAccessibleInShell(cellContent, presShell);

same

@@ +895,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement));
>    nsAccessible* cell = 
> +    GetAccService()->GetAccessibleInShell(cellContent, mDoc->PresShell());

same

::: accessible/src/xul/XULSelectControlAccessible.cpp
@@ +140,5 @@
>      mSelectControl->GetSelectedItem(getter_AddRefs(itemElm));
>  
>    nsCOMPtr<nsINode> itemNode(do_QueryInterface(itemElm));
>    return itemNode && mDoc ?
> +    GetAccService()->GetAccessibleInShell(itemNode, mDoc->PresShell()) : nsnull;

nsDocAccessible::GetAccessible()
Comment 4 alexander :surkov 2012-01-30 20:39:20 PST
changing summary because actually we need to replace nsIWeakShell on nsDocAccessible
Comment 5 Hubert Figuiere [:hub] 2012-02-08 08:42:22 PST
Created attachment 595422 [details] [diff] [review]
use nsIPresShell instead of nsIWeakShell. r=
Comment 6 alexander :surkov 2012-02-08 20:17:19 PST
Comment on attachment 595422 [details] [diff] [review]
use nsIPresShell instead of nsIWeakShell. r=

Review of attachment 595422 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: accessible/public/nsIAccessibilityService.h
@@ +71,5 @@
>     * @param  aPresShell [in] the presentation shell which contains layout info
>     *                         for the DOM node
>     */
> +  virtual nsAccessible* GetAccessible(nsINode* aNode,
> +                                      nsIPresShell* aPresShell) = 0;

move it to nsAccessibilityService, remove existing version of nsAccessibilityService::GetAccessible

::: accessible/src/atk/nsDocAccessibleWrap.cpp
@@ +47,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsDocAccessibleWrap::
> +  nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                      nsIPresShell* aShell) :

aShell -> aPresShell

@@ +48,5 @@
>  
>  nsDocAccessibleWrap::
> +  nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                      nsIPresShell* aShell) :
> +  nsDocAccessible(aDocument, aRootContent, aShell), mActivated(false)

aShell -> aPresShell

::: accessible/src/atk/nsDocAccessibleWrap.h
@@ +50,5 @@
>  class nsDocAccessibleWrap: public nsDocAccessible
>  {
>  public:
> +  nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                      nsIPresShell* aShell);

aShell -> aPresShell

::: accessible/src/base/nsAccessibilityService.cpp
@@ +958,2 @@
>    // Check to see if we already have an accessible for this node in the cache.
> +  nsAccessible* cachedAccessible = GetAccessible(aNode, presShell);

just do aDoc->GetAccessible(aNode), it will return cached accessible if any

@@ +974,5 @@
>      NS_WARNING("Creating accessible for node with no document");
>      return nsnull;
>    }
>  
> +  if (aNode->OwnerDoc() != presShell->GetDocument()) {

aNode->OwnerDoc() != aDoc->GetDocumentNode()

@@ +1107,5 @@
>            continue;
>  
>          if (tableFrame->GetType() == nsGkAtoms::tableOuterFrame) {
>            nsAccessible *tableAccessible =
> +            GetAccessible(tableContent, presShell);

aDoc->GetAccessible

::: accessible/src/base/nsDocAccessible.cpp
@@ +102,5 @@
>  // Constructor/desctructor
>  
>  nsDocAccessible::
>    nsDocAccessible(nsIDocument* aDocument, nsIContent* aRootContent,
> +                  nsIPresShell* aShell) :

aShell -> aPresShell

::: accessible/src/base/nsDocAccessible.h
@@ +96,5 @@
>  public:
>    using nsAccessible::GetParent;
>  
>    nsDocAccessible(nsIDocument *aDocument, nsIContent *aRootContent,
> +                  nsIPresShell* aShell);

aShell -> aPresShell

::: accessible/src/base/nsRootAccessible.h
@@ +68,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>  
>  public:
> +  nsRootAccessible(nsIDocument* aDocument, nsIContent* aRootContent,
> +                   nsIPresShell* aShell);

same

::: accessible/src/mac/nsDocAccessibleWrap.h
@@ +44,5 @@
>  class nsDocAccessibleWrap: public nsDocAccessible
>  {
>  public:
> +  nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                      nsIPresShell* aShell);

same

::: accessible/src/mac/nsDocAccessibleWrap.mm
@@ +40,5 @@
>  #import "mozAccessible.h"
>  
>  nsDocAccessibleWrap::
> +  nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                      nsIPresShell* aShell) :

same

::: accessible/src/mac/nsRootAccessibleWrap.h
@@ +50,5 @@
>  class nsRootAccessibleWrap : public nsRootAccessible
>  {
>  public:
> +  nsRootAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                       nsIPresShell* aShell);

same

::: accessible/src/mac/nsRootAccessibleWrap.mm
@@ +46,5 @@
>  #include "nsIViewManager.h"
>  
>  nsRootAccessibleWrap::
> +  nsRootAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                       nsIPresShell* aShell) :

same

::: accessible/src/msaa/nsDocAccessibleWrap.cpp
@@ +68,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsDocAccessibleWrap::
> +  nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                      nsIPresShell* aShell) :

same

::: accessible/src/msaa/nsDocAccessibleWrap.h
@@ +53,5 @@
>                             public ISimpleDOMDocument
>  {
>  public:
> +  nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                      nsIPresShell* aShell);

same

::: accessible/src/msaa/nsRootAccessibleWrap.cpp
@@ +50,5 @@
>  // Constructor/desctructor
>  
>  nsRootAccessibleWrap::
>    nsRootAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                       nsIPresShell* aShell) :

same and everywhere below
Comment 7 Hubert Figuiere [:hub] 2012-02-08 20:44:18 PST
Comment on attachment 595422 [details] [diff] [review]
use nsIPresShell instead of nsIWeakShell. r=

Review of attachment 595422 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/public/nsIAccessibilityService.h
@@ +71,5 @@
>     * @param  aPresShell [in] the presentation shell which contains layout info
>     *                         for the DOM node
>     */
> +  virtual nsAccessible* GetAccessible(nsINode* aNode,
> +                                      nsIPresShell* aPresShell) = 0;

It is this method that is implemented in nsAccessibilityService. Or maybe you just want it removed from nsIAccessibleService as well?

What I was actually thinking is filing a bug to remove nsAccessibilityService::GetAccessible() completely as it requires a PresShell that we get from the nsDocAccessible, in that case all we need is nsAccDocManager::GetDocAccessible() and use that nsDocAccessible to get the accessible for the node.

::: accessible/src/atk/nsDocAccessibleWrap.cpp
@@ +47,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsDocAccessibleWrap::
> +  nsDocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
> +                      nsIPresShell* aShell) :

I will do that.

::: accessible/src/base/nsAccessibilityService.cpp
@@ +958,2 @@
>    // Check to see if we already have an accessible for this node in the cache.
> +  nsAccessible* cachedAccessible = GetAccessible(aNode, presShell);

ok
Comment 8 alexander :surkov 2012-02-08 22:13:59 PST
(In reply to Hub Figuiere [:hub] from comment #7)

> > +  virtual nsAccessible* GetAccessible(nsINode* aNode,
> > +                                      nsIPresShell* aPresShell) = 0;
> 
> It is this method that is implemented in nsAccessibilityService. Or maybe
> you just want it removed from nsIAccessibleService as well?

right

> What I was actually thinking is filing a bug to remove
> nsAccessibilityService::GetAccessible() completely as it requires a
> PresShell that we get from the nsDocAccessible, in that case all we need is
> nsAccDocManager::GetDocAccessible() and use that nsDocAccessible to get the
> accessible for the node.

that's right, up to you if you don't want to remove it from nsIAccessibilityService right now.
Comment 9 Hubert Figuiere [:hub] 2012-02-08 22:28:01 PST
File bug 725572 to do that cleanup in comment 8
Comment 11 Ed Morley [:emorley] 2012-02-10 05:06:54 PST
https://hg.mozilla.org/mozilla-central/rev/7a93dc11d7b7

Note You need to log in before you can comment on or make changes to this bug.