Last Comment Bug 750026 - rm some unneeded nsCOMPtrs
: rm some unneeded nsCOMPtrs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-04-29 00:57 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-05-24 09:20 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
don't use nsCOMPtrs with the presShell unless we need to (4.88 KB, patch)
2012-04-29 00:58 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review
don't keep a ref to an atom when we don't need to (1.00 KB, patch)
2012-04-29 00:59 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-04-29 00:57:47 PDT
I'm pretty sure we don't need owning refs in any of these places.
Comment 1 Trevor Saunders (:tbsaunde) 2012-04-29 00:58:34 PDT
Created attachment 619366 [details] [diff] [review]
don't use nsCOMPtrs with the presShell unless we need to
Comment 2 Trevor Saunders (:tbsaunde) 2012-04-29 00:59:25 PDT
Created attachment 619367 [details] [diff] [review]
don't keep a ref to an atom when we don't need to
Comment 3 alexander :surkov 2012-04-30 03:04:20 PDT
Comment on attachment 619367 [details] [diff] [review]
don't keep a ref to an atom when we don't need to

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

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +190,2 @@
>      if (tag == nsGkAtoms::option ||
>          tag == nsGkAtoms::optgroup) {

I'd say use Tag() directly since it seems no benefits to have a local for that.
Comment 4 Trevor Saunders (:tbsaunde) 2012-04-30 11:11:12 PDT
(In reply to alexander :surkov from comment #3)
> Comment on attachment 619367 [details] [diff] [review]
> don't keep a ref to an atom when we don't need to
> 
> Review of attachment 619367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/html/nsHTMLSelectAccessible.cpp
> @@ +190,2 @@
> >      if (tag == nsGkAtoms::option ||
> >          tag == nsGkAtoms::optgroup) {
> 
> I'd say use Tag() directly since it seems no benefits to have a local for
> that.

well, maybe a little nicer to ahve local since its shorter I can merge the two lines of the if onto 1 this way.
Comment 5 alexander :surkov 2012-04-30 20:10:38 PDT
ok, up to you, just iirc they said to avoid locals for inlines.
Comment 6 Trevor Saunders (:tbsaunde) 2012-05-23 21:41:30 PDT
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/90e9ed44fc45
Comment 7 Ed Morley [:emorley] 2012-05-24 09:20:37 PDT
https://hg.mozilla.org/mozilla-central/rev/90e9ed44fc45

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