757757 - Add dexpcomed version of GetAnonymousElementByAttribute

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Boris Zbarsky [:bzbarsky]]

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)]

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 (:asurkov)]

(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)]

(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 (:asurkov)]

(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)]

> > > 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 (:asurkov)]

so I'll go with caching approach.

Comment 8

[Trevor Saunders (:tbsaunde)]

(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 9

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ccdad1d0c52

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9ccdad1d0c52