don't use nsIWeakShell

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: hub)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla13
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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?
(Reporter)

Updated

6 years ago
Depends on: 672504
(Assignee)

Updated

6 years ago
Assignee: nobody → hub
(Assignee)

Comment 1

6 years ago
Created attachment 592960 [details] [diff] [review]
use nsIPresShell instead of nsIWeakShell. r=
(Assignee)

Comment 2

6 years ago
patch to be applied on top of bug 673405
(Assignee)

Updated

6 years ago
Attachment #592960 - Flags: review?(surkov.alexander)
(Reporter)

Comment 3

6 years ago
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)
(Reporter)

Comment 4

6 years ago
changing summary because actually we need to replace nsIWeakShell on nsDocAccessible
Summary: use nsIPresShell instead of nsIWeakShell → don't use nsIWeakShell
(Assignee)

Updated

6 years ago
Blocks: 672504
No longer depends on: 672504
(Assignee)

Comment 5

6 years ago
Created attachment 595422 [details] [diff] [review]
use nsIPresShell instead of nsIWeakShell. r=
(Assignee)

Updated

6 years ago
Attachment #592960 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #595422 - Flags: review?(surkov.alexander)
(Reporter)

Comment 6

6 years ago
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+
(Assignee)

Comment 7

6 years ago
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
(Reporter)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
File bug 725572 to do that cleanup in comment 8
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a93dc11d7b7
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/7a93dc11d7b7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.