Closed Bug 721947 Opened 12 years ago Closed 12 years ago

don't use nsIWeakShell

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: surkov, Assigned: hub)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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?
Depends on: 672504
Assignee: nobody → hub
patch to be applied on top of bug 673405
Attachment #592960 - Flags: review?(surkov.alexander)
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()
Attachment #592960 - Flags: review?(surkov.alexander)
changing summary because actually we need to replace nsIWeakShell on nsDocAccessible
Summary: use nsIPresShell instead of nsIWeakShell → don't use nsIWeakShell
Blocks: 672504
No longer depends on: 672504
Attachment #592960 - Attachment is obsolete: true
Attachment #595422 - Flags: review?(surkov.alexander)
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
Attachment #595422 - Flags: review?(surkov.alexander) → review+
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
(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.
File bug 725572 to do that cleanup in comment 8
https://hg.mozilla.org/mozilla-central/rev/7a93dc11d7b7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: