Last Comment Bug 757757 - Add dexpcomed version of GetAnonymousElementByAttribute
: Add dexpcomed version of GetAnonymousElementByAttribute
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 00:16 PDT by alexander :surkov
Modified: 2012-05-28 09:41 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (16.57 KB, patch)
2012-05-23 00:16 PDT, alexander :surkov
tbsaunde+mozbugs: review+
bzbarsky: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-05-23 00:16:40 PDT
Created attachment 626349 [details] [diff] [review]
patch
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-05-23 11:26:29 PDT
Comment on attachment 626349 [details] [diff] [review]
patch

>+      mContent->OwnerDoc()->GetAnonymousElementByAttribute(bindingParent,

You want bindingParent->OwnerDoc() here.

>+              nsIContent* possibleButtonEl = mContent->OwnerDoc()->
>+                GetAnonymousElementByAttribute(rootElm, nsGkAtoms::_default,

rootElm->OwnerDoc().

>+  NS_ENSURE_ARG_POINTER(aResult);

Please don't.  It should never be null.

r=me with those changes.
Comment 2 Trevor Saunders (:tbsaunde) 2012-05-24 06:00:19 PDT
Comment on attachment 626349 [details] [diff] [review]
patch

>+              buttonEl = do_QueryInterface(possibleButtonEl);

any reason we don't make a nsCOMPtr<nsIContent> ? since we always end up QI to that at the end anyway?

>+  nsIContent* sliderContent(GetSliderNode());

nit, sliderContent = GetSliderContent()?

>+  if (!sliderContent)
>+    return state;
> 
>   nsIFrame *frame = sliderContent->GetPrimaryFrame();
>   if (frame && frame->IsFocusable())
>-    states |= states::FOCUSABLE;
>+    state |= states::FOCUSABLE;
> 
>   if (FocusMgr()->IsFocused(this))
>-    states |= states::FOCUSED;
>+    state |= states::FOCUSED;

btw shouldn't we only check that if its focusable?

> nsXULSliderAccessible::GetSliderNode()
> {
>-  if (IsDefunct())
>-    return nsnull;
>-
>   if (!mSliderNode) {
>-    nsCOMPtr<nsIDOMDocumentXBL> xblDoc(do_QueryInterface(mContent->OwnerDoc()));
>-    if (!xblDoc)
>-      return nsnull;
>-
>     // XXX: we depend on anonymous content.
>-    nsCOMPtr<nsIDOMElement> domElm(do_QueryInterface(mContent));
>-    if (!domElm)
>-      return nsnull;
>-
>-    xblDoc->GetAnonymousElementByAttribute(domElm, NS_LITERAL_STRING("anonid"),
>-                                           NS_LITERAL_STRING("slider"),
>-                                           getter_AddRefs(mSliderNode));
>+    mSliderNode = mContent->OwnerDoc()->
>+      GetAnonymousElementByAttribute(mContent, nsGkAtoms::anonid,
>+                                     NS_LITERAL_STRING("slider"));

btw, I wonder if caching still makes sense?
Comment 3 alexander :surkov 2012-05-25 01:19:22 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> >+              buttonEl = do_QueryInterface(possibleButtonEl);
> 
> any reason we don't make a nsCOMPtr<nsIContent> ? since we always end up QI
> to that at the end anyway?

It'd be nice but I like to avoid the change at this point since it's not equivalent.

> >+  nsIContent* sliderContent(GetSliderNode());
> 
> nit, sliderContent = GetSliderContent()?

GetSliderElement() then

> btw shouldn't we only check that if its focusable?

yes, not in this patch since I have related stuffs in my queue.

> btw, I wonder if caching still makes sense?

why not and what was changed to not do that? :)
Comment 4 Trevor Saunders (:tbsaunde) 2012-05-25 13:31:07 PDT
(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > >+              buttonEl = do_QueryInterface(possibleButtonEl);
> > 
> > any reason we don't make a nsCOMPtr<nsIContent> ? since we always end up QI
> > to that at the end anyway?
> 
> It'd be nice but I like to avoid the change at this point since it's not
> equivalent.

ok

> > >+  nsIContent* sliderContent(GetSliderNode());
> > 
> > nit, sliderContent = GetSliderContent()?
> 
> GetSliderElement() then

err, I meant using nsIContent* foo = bla; instead of nsIContent* foo(blah); since its not an object.  I guess if you think renaming it makes sense I don't really mind though.

> > btw shouldn't we only check that if its focusable?
> 
> yes, not in this patch since I have related stuffs in my queue.
> 
> > btw, I wonder if caching still makes sense?
> 
> why not and what was changed to not do that? :)

well, caching has some amount of cost, and you just reduced the time it takes to get what your caching.
Comment 5 alexander :surkov 2012-05-26 06:32:48 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> > > nit, sliderContent = GetSliderContent()?
> > 
> > GetSliderElement() then
> 
> err, I meant using nsIContent* foo = bla; instead of nsIContent* foo(blah);
> since its not an object.  I guess if you think renaming it makes sense I
> don't really mind though.

got it

> > why not and what was changed to not do that? :)
> 
> well, caching has some amount of cost, and you just reduced the time it
> takes to get what your caching.

memory cost I guess. I don't think we should have big amount of XUL scales, so caching shouldn't really harm us (as not caching though as well).
Comment 6 Trevor Saunders (:tbsaunde) 2012-05-26 17:41:05 PDT
> > > why not and what was changed to not do that? :)
> > 
> > well, caching has some amount of cost, and you just reduced the time it
> > takes to get what your caching.
> 
> memory cost I guess. I don't think we should have big amount of XUL scales,
> so caching shouldn't really harm us (as not caching though as well).

well, and refcounting and cc etc, but still probably true its not a big deal.
Comment 7 alexander :surkov 2012-05-27 21:49:20 PDT
so I'll go with caching approach.
Comment 8 Trevor Saunders (:tbsaunde) 2012-05-27 21:53:32 PDT
(In reply to alexander :surkov from comment #7)
> so I'll go with caching approach.

sure, meant it more as something to think about in future than as something to block this.
Comment 10 Ed Morley [:emorley] 2012-05-28 09:41:59 PDT
https://hg.mozilla.org/mozilla-central/rev/9ccdad1d0c52

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