Closed
Bug 739253
Opened 13 years ago
Closed 13 years ago
optimize AccEvent::GetDocAccessible
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
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)
847 bytes,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
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•13 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: 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!
Comment 3•13 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).
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•13 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
(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 9•13 years ago
|
||
I think you can do: hg diff -p -U 8 > OUT_FILE
Assignee | ||
Comment 10•13 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•13 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•13 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•13 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•13 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•13 years ago
|
||
Attachment #610423 -
Attachment is obsolete: true
Attachment #610432 -
Attachment is obsolete: true
Comment 16•13 years ago
|
||
yes but instead of if else if construction I would use if () return; // do the rest.
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #610466 -
Attachment is obsolete: true
Attachment #610474 -
Flags: feedback+
Comment 18•13 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•13 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•13 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•13 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•13 years ago
|
||
I modified using mq this time.
Attachment #610474 -
Attachment is obsolete: true
Attachment #611284 -
Flags: feedback?
Comment 23•13 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•13 years ago
|
Assignee: nobody → nickyekaiqi
Comment 24•13 years ago
|
||
Updated•13 years ago
|
Target Milestone: --- → mozilla14
Comment 25•13 years ago
|
||
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.
Description
•