Closed Bug 739253 Opened 13 years ago Closed 13 years ago

optimize AccEvent::GetDocAccessible

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: tbsaunde, Assigned: nickyekaiqi)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 4 obsolete files)

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.
(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: cleana11y
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!
(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).
Attached patch Just do it as instruction (obsolete) — Splinter Review
Hope it is correct for creating a patch
Ah. I uploaded it so fast. Haven't seen your comment. Thank you for your comment and I will do as what you said.:)
(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
(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.
Attached patch Modified one (obsolete) — Splinter Review
I think you can do: hg diff -p -U 8 > OUT_FILE
(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.
(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
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?
(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.
(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.
Attached patch new_patch_2 (obsolete) — Splinter Review
Attachment #610423 - Attachment is obsolete: true
Attachment #610432 - Attachment is obsolete: true
yes but instead of if else if construction I would use if () return; // do the rest.
Attached patch patch 3 (obsolete) — Splinter Review
Attachment #610466 - Attachment is obsolete: true
Attachment #610474 - Flags: feedback+
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)
(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 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)
(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.
Attached patch modified patchSplinter Review
I modified using mq this time.
Attachment #610474 - Attachment is obsolete: true
Attachment #611284 - Flags: feedback?
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+
Assignee: nobody → nickyekaiqi
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
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: