Last Comment Bug 739253 - optimize AccEvent::GetDocAccessible
: optimize AccEvent::GetDocAccessible
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Ye Kaiqi
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-03-26 08:44 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-04-02 11:07 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Just do it as instruction (13.12 KB, patch)
2012-03-28 21:26 PDT, Ye Kaiqi
no flags Details | Diff | Review
Modified one (588 bytes, patch)
2012-03-28 22:28 PDT, Ye Kaiqi
no flags Details | Diff | Review
new_patch_2 (604 bytes, patch)
2012-03-29 00:05 PDT, Ye Kaiqi
no flags Details | Diff | Review
patch 3 (581 bytes, patch)
2012-03-29 00:39 PDT, Ye Kaiqi
no flags Details | Diff | Review
modified patch (847 bytes, patch)
2012-04-01 09:17 PDT, Ye Kaiqi
surkov.alexander: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-03-26 08:44:10 PDT
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 alexander :surkov 2012-03-26 20:20:31 PDT
(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
Comment 2 Ye Kaiqi 2012-03-28 21:18:06 PDT
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 alexander :surkov 2012-03-28 21:22:22 PDT
(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).
Comment 4 Ye Kaiqi 2012-03-28 21:26:01 PDT
Created attachment 610423 [details] [diff] [review]
Just do it as instruction

Hope it is correct for creating a patch
Comment 5 Ye Kaiqi 2012-03-28 21:27:32 PDT
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 alexander :surkov 2012-03-28 21:33:34 PDT
(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
Comment 7 Ye Kaiqi 2012-03-28 22:26:28 PDT
(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.
Comment 8 Ye Kaiqi 2012-03-28 22:28:38 PDT
Created attachment 610432 [details] [diff] [review]
Modified one
Comment 9 alexander :surkov 2012-03-28 22:29:43 PDT
I think you can do: hg diff -p -U 8 > OUT_FILE
Comment 10 Ye Kaiqi 2012-03-28 22:35:25 PDT
(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 alexander :surkov 2012-03-28 22:43:49 PDT
(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
Comment 12 Ye Kaiqi 2012-03-28 23:25:40 PDT
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 alexander :surkov 2012-03-28 23:28:45 PDT
(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.
Comment 14 Ye Kaiqi 2012-03-29 00:03:09 PDT
(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.
Comment 15 Ye Kaiqi 2012-03-29 00:05:44 PDT
Created attachment 610466 [details] [diff] [review]
new_patch_2
Comment 16 alexander :surkov 2012-03-29 00:06:37 PDT
yes but instead of if else if construction I would use if () return; // do the rest.
Comment 17 Ye Kaiqi 2012-03-29 00:39:21 PDT
Created attachment 610474 [details] [diff] [review]
patch 3
Comment 18 alexander :surkov 2012-03-29 00:47:51 PDT
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
Comment 19 Ye Kaiqi 2012-03-29 01:04:48 PDT
(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
Comment 20 Trevor Saunders (:tbsaunde) 2012-03-29 04:12:22 PDT
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.
Comment 21 Ye Kaiqi 2012-04-01 08:41:33 PDT
(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.
Comment 22 Ye Kaiqi 2012-04-01 09:17:08 PDT
Created attachment 611284 [details] [diff] [review]
modified patch

I modified using mq this time.
Comment 23 alexander :surkov 2012-04-01 20:29:23 PDT
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
Comment 25 Matt Brubeck (:mbrubeck) 2012-04-02 11:07:59 PDT
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

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