741681 - replace nsAccessNode::GetPresContext by nsDocAccessible::PresContext

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

we have several places where we call GetPresContext() on nsAccessNode, it's worth to move this method into nsDocAccessible where PresShell() is.

Comment 1

[lavina]

I'll take this.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to lavina from comment #1)
> I'll take this.

cool, done

Comment 3

[lavina]

How do I test the changes?
I set breakpoints at the places I changed the call to GetPresContext in nsHyperTextAccessible, nsXULTreeGridAccessible and nsXULTreeAccessible. Under what scenario can I expect them to be hit? So far, I haven't been successful at it.

I have Universal Access turned on.

Please advise. Thanks!

Comment 4

[alexander :surkov (:asurkov)]

(In reply to lavina from comment #3)
> How do I test the changes?
> I set breakpoints at the places I changed the call to GetPresContext in
> nsHyperTextAccessible, nsXULTreeGridAccessible and nsXULTreeAccessible.
> Under what scenario can I expect them to be hit? So far, I haven't been
> successful at it.

I guess you need to keep screen reader running like Orca if you're on Linux.

> Please advise. Thanks!

also you can try to run a11y mochitests (cd yourobjdir/_tests/testing/mochitest; python runtests.py --a11y)

Comment 5

[lavina]

Attachment: Bug741681 patch

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 613074 [details] [diff] [review]
Bug741681 patch

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

cool, thank you, f=me

::: accessible/src/base/nsAccessNode.h
@@ +160,5 @@
>    nsCOMPtr<nsIContent> mContent;
>    nsDocAccessible* mDoc;
>  
> +  /**
> +   * Notify global nsIObserver's that a11y is getting init'd or shutdown

nit: dot in the end please

@@ +167,3 @@
>  
> +  // Static data, we do our own refcounting for our static data
> +  static nsIStringBundle *gStringBundle;

nit: type* name

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +208,5 @@
>    NS_ENSURE_SUCCESS(rv, screenRect);
>  
>    NS_ENSURE_TRUE(mDoc, screenRect);
> +  nsPresContext* context = mDoc->PresContext();
> +  NS_ENSURE_TRUE(context, screenRect);

context is not null, no check is needed

@@ -1294,5 @@
>                                          PRUint32 aCoordType, PRInt32 *aOffset)
>  {
>    *aOffset = -1;
> -  if (!mDoc)
> -    return NS_ERROR_FAILURE;

it makes sense to keep instead !mDoc check
if (IsDefunct())
  return NS_ERRROR_FAILURE;

@@ +1310,5 @@
>      return NS_OK;   // Not found, will return -1
>    }
>    nsIntPoint pxInHyperText(coords.x - frameScreenRect.x,
>                             coords.y - frameScreenRect.y);
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);

and then you don't need to check mDoc

@@ +1311,5 @@
>    }
>    nsIntPoint pxInHyperText(coords.x - frameScreenRect.x,
>                             coords.y - frameScreenRect.y);
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);
> +  nsPresContext *context = mDoc->PresContext();

type* name

@@ +1316,1 @@
>    NS_ENSURE_TRUE(context, NS_ERROR_FAILURE);

no need to check context

::: accessible/src/xul/nsXULTreeAccessible.cpp
@@ +804,5 @@
>  
>    x = tcX;
>    y += tcY;
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);

you don't need to have this check here

@@ +806,5 @@
>    y += tcY;
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);
> +  nsPresContext* presContext = mDoc->PresContext();
> +  NS_ENSURE_TRUE(presContext, NS_ERROR_FAILURE);

and this one too (if mDoc is not null then PresContext() is not null too

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +925,5 @@
>    y += tcY;
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);
> +  nsPresContext* presContext = mDoc->PresContext();
> +  NS_ENSURE_TRUE(presContext, NS_ERROR_FAILURE);

same here

::: toolkit/crashreporter/google-breakpad/src/client/mac/Breakpad.xcodeproj/project.pbxproj
@@ +1336,5 @@
>  		0867D690FE84028FC02AAC07 /* Project object */ = {
>  			isa = PBXProject;
>  			buildConfigurationList = 1DEB91B108733DA50010E9CD /* Build configuration list for PBXProject "Breakpad" */;
>  			compatibilityVersion = "Xcode 3.1";
> +			developmentRegion = English;

looks like a part of different story

Comment 7

[lavina]

So, when mDoc is not null, mDoc's shell will not be null and mDoc's shell's context will not be null. That's why there is no need to ensure true for context. Is that correct?

> :::
> toolkit/crashreporter/google-breakpad/src/client/mac/Breakpad.xcodeproj/
> project.pbxproj
> @@ +1336,5 @@
> >  		0867D690FE84028FC02AAC07 /* Project object */ = {
> >  			isa = PBXProject;
> >  			buildConfigurationList = 1DEB91B108733DA50010E9CD /* Build configuration list for PBXProject "Breakpad" */;
> >  			compatibilityVersion = "Xcode 3.1";
> > +			developmentRegion = English;
> 
> looks like a part of different story


I don't understand this...can you explain?

Thanks.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to lavina from comment #7)
> So, when mDoc is not null, mDoc's shell will not be null and mDoc's shell's
> context will not be null. That's why there is no need to ensure true for
> context. Is that correct?

yes, and mDoc is not null when IsDefunct() is false

> > :::
> > toolkit/crashreporter/google-breakpad/src/client/mac/Breakpad.xcodeproj/
> > project.pbxproj
> > @@ +1336,5 @@
> > >  		0867D690FE84028FC02AAC07 /* Project object */ = {
> > >  			isa = PBXProject;
> > >  			buildConfigurationList = 1DEB91B108733DA50010E9CD /* Build configuration list for PBXProject "Breakpad" */;
> > >  			compatibilityVersion = "Xcode 3.1";
> > > +			developmentRegion = English;
> > 
> > looks like a part of different story
> I don't understand this...can you explain?

it doesn't seem that this change is related with this bug, right?

Comment 9

[lavina]

(In reply to alexander :surkov from comment #8)
> (In reply to lavina from comment #7)
> > So, when mDoc is not null, mDoc's shell will not be null and mDoc's shell's
> > context will not be null. That's why there is no need to ensure true for
> > context. Is that correct?
> 
> yes, and mDoc is not null when IsDefunct() is false
> 
> > > :::
> > > toolkit/crashreporter/google-breakpad/src/client/mac/Breakpad.xcodeproj/
> > > project.pbxproj
> > > @@ +1336,5 @@
> > > >  		0867D690FE84028FC02AAC07 /* Project object */ = {
> > > >  			isa = PBXProject;
> > > >  			buildConfigurationList = 1DEB91B108733DA50010E9CD /* Build configuration list for PBXProject "Breakpad" */;
> > > >  			compatibilityVersion = "Xcode 3.1";
> > > > +			developmentRegion = English;
> > > 
> > > looks like a part of different story
> > I don't understand this...can you explain?
> 
> it doesn't seem that this change is related with this bug, right?

Oh, ok. I don't know how this happened. I didn't change anything apart from those for this bug. How can I remove it from the patch? Do I do another "hg qnew" instead of "hg qrefresh" after I make the necessary changes?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to lavina from comment #9)
> Oh, ok. I don't know how this happened. I didn't change anything apart from
> those for this bug. How can I remove it from the patch?

I think you can do hg revert or just replace a file.

> Do I do another "hg
> qnew" instead of "hg qrefresh" after I make the necessary changes?

hg qrefresh (or hg qref).

Comment 11

[Hubert Figuiere [:hub]]

(In reply to lavina from comment #7)
> So, when mDoc is not null, mDoc's shell will not be null and mDoc's shell's
> context will not be null. That's why there is no need to ensure true for
> context. Is that correct?
> 
> > :::
> > toolkit/crashreporter/google-breakpad/src/client/mac/Breakpad.xcodeproj/
> > project.pbxproj
> > @@ +1336,5 @@
> > >  		0867D690FE84028FC02AAC07 /* Project object */ = {
> > >  			isa = PBXProject;
> > >  			buildConfigurationList = 1DEB91B108733DA50010E9CD /* Build configuration list for PBXProject "Breakpad" */;
> > >  			compatibilityVersion = "Xcode 3.1";
> > > +			developmentRegion = English;
> > 
> > looks like a part of different story
> 
> 
> I don't understand this...can you explain?


Typically you opened that file in Xcode, and Xcode saved it back with this added to it. This is a known feature of Xcode.

hg revert it and hg qref

Comment 12

[Hubert Figuiere [:hub]]

Comment on attachment 613074 [details] [diff] [review]
Bug741681 patch

Please address all the comments and submit a new patch. I'll gladly review it again.

Comment 13

[lavina]

Attachment: Revised per feedback

Comment 14

[Hubert Figuiere [:hub]]

Comment on attachment 613477 [details] [diff] [review]
Revised per feedback

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

Pleas resubmit a patch using addressing that small comment.
  
Thanks.

::: accessible/src/base/nsDocAccessible.h
@@ +144,5 @@
>  
>    /**
> +   * Return the presentation shell's context.
> +   */
> +  nsPresContext* PresContext() const { return mPresShell->GetPresContext(); };

No semicolon after the closing brace.

Comment 15

[lavina]

Attachment: revised per comment

Comment 16

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78bd7974ba72

Comment 17

[Hubert Figuiere [:hub]]

backed out. will re-land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df10e8b5ce2

Comment 18

[Hubert Figuiere [:hub]]

Comment on attachment 613499 [details] [diff] [review]
revised per comment

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

removing the r+. Patch has been backed out as it break the windows build.

Comment 19

[Hubert Figuiere [:hub]]

You can review the build log for the build failure here:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10790926&tree=Try&full=1

Comment 20

[lavina]

(In reply to Hub Figuiere [:hub] from comment #19)
> You can review the build log for the build failure here:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=10790926&tree=Try&full=1

Thanks, I'll revise the patch. 
Is the accessible/src/msaa directory the windows equivalent of accessible/src/mac?

Comment 21

[Hubert Figuiere [:hub]]

> Thanks, I'll revise the patch. 
> Is the accessible/src/msaa directory the windows equivalent of
> accessible/src/mac?

Yes it is what Windows use.

Comment 22

[lavina]

Attachment: address windows build issue (GetPresContext() in nsTextAccessibleWrap::GetCharacterExtents() was not replaced)

Comment 23

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/259e39b355cf

Comment 24

[Matt Brubeck (:mbrubeck)]

This patch has landed for Firefox 14 and will be included in tomorrow's nightly builds.  Thanks!
https://hg.mozilla.org/mozilla-central/rev/259e39b355cf