Closed Bug 741681 Opened 8 years ago Closed 8 years ago

replace nsAccessNode::GetPresContext by nsDocAccessible::PresContext

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: orangedaylily)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 3 obsolete files)

we have several places where we call GetPresContext() on nsAccessNode, it's worth to move this method into nsDocAccessible where PresShell() is.
Summary: repalce nsAccessNode::GetPresContext by nsDocAccessible::PresContext → replace nsAccessNode::GetPresContext by nsDocAccessible::PresContext
I'll take this.
(In reply to lavina from comment #1)
> I'll take this.

cool, done
Assignee: nobody → orangedaylily
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!
(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)
Attached patch Bug741681 patch (obsolete) — Splinter Review
Attachment #613074 - Flags: review?
Attachment #613074 - Flags: feedback?(surkov.alexander)
Attachment #613074 - Flags: review? → review?(hub)
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+
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.
(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?
(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?
(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-
Attached patch Revised per feedback (obsolete) — Splinter Review
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+
Attached patch revised per comment (obsolete) — Splinter Review
Attachment #613477 - Attachment is obsolete: true
Attachment #613499 - Flags: review+
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-
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
(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.
Attachment #613862 - Attachment is patch: true
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
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.