Last Comment Bug 741681 - replace nsAccessNode::GetPresContext by nsDocAccessible::PresContext
: replace nsAccessNode::GetPresContext by nsDocAccessible::PresContext
Status: RESOLVED FIXED
[good first bug][mentor=hub@mozilla.c...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: lavina
:
: alexander :surkov
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-04-02 21:25 PDT by alexander :surkov
Modified: 2012-04-11 09:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug741681 patch (8.74 KB, patch)
2012-04-07 00:19 PDT, lavina
hub: review-
surkov.alexander: feedback+
Details | Diff | Splinter Review
Revised per feedback (7.72 KB, patch)
2012-04-09 19:51 PDT, lavina
hub: review+
Details | Diff | Splinter Review
revised per comment (7.72 KB, patch)
2012-04-09 22:54 PDT, lavina
hub: review-
Details | Diff | Splinter Review
address windows build issue (GetPresContext() in nsTextAccessibleWrap::GetCharacterExtents() was not replaced) (8.88 KB, patch)
2012-04-10 20:37 PDT, lavina
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-04-02 21:25:23 PDT
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 2012-04-03 21:19:07 PDT
I'll take this.
Comment 2 alexander :surkov 2012-04-03 21:28:46 PDT
(In reply to lavina from comment #1)
> I'll take this.

cool, done
Comment 3 lavina 2012-04-04 23:07:13 PDT
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 2012-04-05 00:39:20 PDT
(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 2012-04-07 00:19:08 PDT
Created attachment 613074 [details] [diff] [review]
Bug741681 patch
Comment 6 alexander :surkov 2012-04-09 00:30:17 PDT
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 2012-04-09 01:35:49 PDT
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 2012-04-09 01:37:51 PDT
(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 2012-04-09 01:49:27 PDT
(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 2012-04-09 01:52:12 PDT
(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] 2012-04-09 08:39:51 PDT
(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] 2012-04-09 08:43:12 PDT
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 2012-04-09 19:51:36 PDT
Created attachment 613477 [details] [diff] [review]
Revised per feedback
Comment 14 Hubert Figuiere [:hub] 2012-04-09 22:22:17 PDT
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 2012-04-09 22:54:49 PDT
Created attachment 613499 [details] [diff] [review]
revised per comment
Comment 17 Hubert Figuiere [:hub] 2012-04-10 12:44:06 PDT
backed out. will re-land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df10e8b5ce2
Comment 18 Hubert Figuiere [:hub] 2012-04-10 15:18:50 PDT
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] 2012-04-10 17:23:56 PDT
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 2012-04-10 18:02:39 PDT
(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] 2012-04-10 18:09:52 PDT
> 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 2012-04-10 20:37:53 PDT
Created attachment 613862 [details] [diff] [review]
address windows build issue (GetPresContext() in nsTextAccessibleWrap::GetCharacterExtents() was not replaced)
Comment 24 Matt Brubeck (:mbrubeck) 2012-04-11 09:36:45 PDT
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

Note You need to log in before you can comment on or make changes to this bug.