The default bug view has changed. See this FAQ.

optimize AccEvent::GetDocAccessible

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: tbsaunde, Assigned: Ye Kaiqi)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
see accessible/src/base/AccEvent.cpp
if mAccessible is not null we can get the document with mAccessible->Document() which should be faster than getting it from the accessibility Service.

Comment 1

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #0)
> see accessible/src/base/AccEvent.cpp

http://hg.mozilla.org/mozilla-central/annotate/c3cb87871829/accessible/src/base/AccEvent.cpp#l105
Blocks: 389800
(Assignee)

Comment 2

5 years ago
Hello. I am a beginner of fixing bugs, and I am interested in this one. I want to ask a question that how I can test my result (modified code for this bug), because I am not sure if it is correct after I modify it. Thank you!

Comment 3

5 years ago
(In reply to Ye Kaiqi from comment #2)
> Hello. I am a beginner of fixing bugs, and I am interested in this one. I
> want to ask a question that how I can test my result (modified code for this
> bug), because I am not sure if it is correct after I modify it. Thank you!

Hi, thank you for working on it.
I think you can see if the patch has any problems by running our mochitests. I do this:
cd your-obj-dir/_tests/testing/mochitest; python runtests.py --a11y --autorun
If you don't see any failures then your patch probably ok.

Otherwise you can just upload the patch and ask the mentor for feedback (feedback flag on the patch).
(Assignee)

Comment 4

5 years ago
Created attachment 610423 [details] [diff] [review]
Just do it as instruction

Hope it is correct for creating a patch
(Assignee)

Comment 5

5 years ago
Ah. I uploaded it so fast. Haven't seen your comment. Thank you for your comment and I will do as what you said.:)

Comment 6

5 years ago
(In reply to Ye Kaiqi from comment #4)
> Created attachment 610423 [details] [diff] [review]
> Just do it as instruction

please follow instructions https://developer.mozilla.org/En/Creating_a_patch to create a patch
(Assignee)

Comment 7

5 years ago
(In reply to alexander :surkov from comment #6)
 
> please follow instructions https://developer.mozilla.org/En/Creating_a_patch
> to create a patch

So I should use "hg diff -p -U 8 FILENAME > OUT_FILE" instead of "hg diff -p -U 8 FILENAME". Sorry for that. I misunderstood the instruction last time. Hope it is correct now.
(Assignee)

Comment 8

5 years ago
Created attachment 610432 [details] [diff] [review]
Modified one

Comment 9

5 years ago
I think you can do: hg diff -p -U 8 > OUT_FILE
(Assignee)

Comment 10

5 years ago
(In reply to alexander :surkov from comment #9)
> I think you can do: hg diff -p -U 8 > OUT_FILE

This command will include all the changes, right? But actually I also created some patches for other bugs. If I did so, all the changes of other bugs will also be included. So I should use "hg diff -p -U 8 > OUT_FILE". Sorry I am quite new to fixing bugs. Just start it today.

Comment 11

5 years ago
(In reply to Ye Kaiqi from comment #10)
> (In reply to alexander :surkov from comment #9)
> > I think you can do: hg diff -p -U 8 > OUT_FILE
> 
> This command will include all the changes, right? 

right

> But actually I also
> created some patches for other bugs. If I did so, all the changes of other
> bugs will also be included. So I should use "hg diff -p -U 8 > OUT_FILE".
> Sorry I am quite new to fixing bugs. Just start it today.

I'd suggest to use mercurial queues https://developer.mozilla.org/en/Mercurial_Queues
(Assignee)

Comment 12

5 years ago
What you suggested is quite useful for working on several bugs at the same time. I am trying to activate this feature. Is the previous patch OK?

Comment 13

5 years ago
(In reply to Ye Kaiqi from comment #12)
> Is the previous patch OK?

No, you should do if (msAccessible) return mAccessible->Document(); otherwise get document accessible from mNode.
(Assignee)

Comment 14

5 years ago
(In reply to alexander :surkov from comment #13)
> 
> No, you should do if (msAccessible) return mAccessible->Document();
> otherwise get document accessible from mNode.

Ah, so I should use if(msAccessible) ... else if(node) ... Is it correct? I also uploaded another patch.
(Assignee)

Comment 15

5 years ago
Created attachment 610466 [details] [diff] [review]
new_patch_2
Attachment #610423 - Attachment is obsolete: true
Attachment #610432 - Attachment is obsolete: true

Comment 16

5 years ago
yes but instead of if else if construction I would use if () return; // do the rest.
(Assignee)

Comment 17

5 years ago
Created attachment 610474 [details] [diff] [review]
patch 3
Attachment #610466 - Attachment is obsolete: true
Attachment #610474 - Flags: feedback+

Comment 18

5 years ago
Comment on attachment 610474 [details] [diff] [review]
patch 3

you shouldn't f+ but you should f? from mentor

Trevor, please feel free to review instead feedback
Attachment #610474 - Flags: feedback+ → feedback?(trev.saunders)
(Assignee)

Comment 19

5 years ago
(In reply to alexander :surkov from comment #18)
> Comment on attachment 610474 [details] [diff] [review]
> patch 3
> 
> you shouldn't f+ but you should f? from mentor
> 
> Trevor, please feel free to review instead feedback

SO sorry for the wrong tag
(Reporter)

Comment 20

5 years ago
Comment on attachment 610474 [details] [diff] [review]
patch 3

>diff --git a/accessible/src/base/AccEvent.cpp b/accessible/src/base/AccEvent.cpp
>--- a/accessible/src/base/AccEvent.cpp
>+++ b/accessible/src/base/AccEvent.cpp
>@@ -103,20 +103,18 @@ AccEvent::GetNode()
> 
> nsDocAccessible*
> AccEvent::GetDocAccessible()
> {
>   nsINode *node = GetNode();
>   if (mAccessible)
>     return mAccessible->Document();

no point in getting the node until you know  you'll use it.

> 
>-  else if(node)
>-  {
>+  if(node)
>     return GetAccService()->GetDocAccessible(node->OwnerDoc());

wrong indentation, 4 spaces before return not 5

btw  it looks like you produced a diff against something other than trunk.
Attachment #610474 - Flags: feedback?(trev.saunders)
(Assignee)

Comment 21

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #20)
> Comment on attachment 610474 [details] [diff] [review]
> patch 3
> 
> >diff --git a/accessible/src/base/AccEvent.cpp b/accessible/src/base/AccEvent.cpp
> >--- a/accessible/src/base/AccEvent.cpp
> >+++ b/accessible/src/base/AccEvent.cpp
> >@@ -103,20 +103,18 @@ AccEvent::GetNode()
> > 
> > nsDocAccessible*
> > AccEvent::GetDocAccessible()
> > {
> >   nsINode *node = GetNode();
> >   if (mAccessible)
> >     return mAccessible->Document();
> 
> no point in getting the node until you know  you'll use it.
> 
> > 
> >-  else if(node)
> >-  {
> >+  if(node)
> >     return GetAccService()->GetDocAccessible(node->OwnerDoc());
> 
> wrong indentation, 4 spaces before return not 5
> 
> btw  it looks like you produced a diff against something other than trunk.

Thank you for your comments! I will change it! I followed someone's instructions in the IRC and it seems that it is not from another person who wants to fix bugs, now I will change it due to the instruction.
(Assignee)

Comment 22

5 years ago
Created attachment 611284 [details] [diff] [review]
modified patch

I modified using mq this time.
Attachment #610474 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #611284 - Flags: feedback?

Comment 23

5 years ago
Comment on attachment 611284 [details] [diff] [review]
modified patch

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

I'll fix nits before landing

::: accessible/src/base/AccEvent.cpp
@@ +103,5 @@
>  
>  nsDocAccessible*
>  AccEvent::GetDocAccessible()
>  {
> +  if(mAccessible)

nit: space after if

@@ +108,3 @@
>      return mAccessible->Document();
>  
> +  nsINode *node = GetNode();

nit: nsINode* node
Attachment #611284 - Flags: feedback? → review+

Updated

5 years ago
Assignee: nobody → nickyekaiqi

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/621550442931

Updated

5 years ago
Target Milestone: --- → mozilla14
This patch has landed in mozilla-central for Firefox 14, and will be included in tomorrow's nightly build.  Thanks!
https://hg.mozilla.org/mozilla-central/rev/621550442931
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.