replace nsAccessNode::GetPresContext by nsDocAccessible::PresContext

RESOLVED FIXED

Status

()

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

People

(Reporter: surkov, Assigned: lavina)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=hub@mozilla.com][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Summary: repalce nsAccessNode::GetPresContext by nsDocAccessible::PresContext → replace nsAccessNode::GetPresContext by nsDocAccessible::PresContext
(Assignee)

Comment 1

5 years ago
I'll take this.
(Reporter)

Comment 2

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

cool, done
Assignee: nobody → orangedaylily
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

5 years ago
Created attachment 613074 [details] [diff] [review]
Bug741681 patch
Attachment #613074 - Flags: review?
Attachment #613074 - Flags: feedback?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #613074 - Flags: review? → review?(hub)
(Reporter)

Comment 6

5 years ago
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
Attachment #613074 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 7

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

Comment 8

5 years ago
(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?
(Assignee)

Comment 9

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

Comment 10

5 years ago
(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).
(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 on attachment 613074 [details] [diff] [review]
Bug741681 patch

Please address all the comments and submit a new patch. I'll gladly review it again.
Attachment #613074 - Flags: review?(hub) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 613477 [details] [diff] [review]
Revised per feedback
Attachment #613074 - Attachment is obsolete: true
Attachment #613477 - Flags: review?(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.
Attachment #613477 - Flags: review?(hub) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 613499 [details] [diff] [review]
revised per comment
Attachment #613477 - Attachment is obsolete: true

Updated

5 years ago
Attachment #613499 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/78bd7974ba72
Target Milestone: --- → mozilla14
backed out. will re-land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df10e8b5ce2
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.
Attachment #613499 - Flags: review+ → review-

Updated

5 years ago
Target Milestone: mozilla14 → ---
You can review the build log for the build failure here:

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

Comment 20

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

Comment 22

5 years ago
Created attachment 613862 [details] [diff] [review]
address windows build issue (GetPresContext() in nsTextAccessibleWrap::GetCharacterExtents() was not replaced)
(Reporter)

Updated

5 years ago
Attachment #613862 - Attachment is patch: true
(Reporter)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/259e39b355cf
(Assignee)

Updated

5 years ago
Attachment #613499 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.