Add dexpcomed version of GetAnonymousElementByAttribute

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 626349 [details] [diff] [review]
patch
Attachment #626349 - Flags: review?(trev.saunders)
Attachment #626349 - Flags: review?(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.
Attachment #626349 - Flags: review?(bzbarsky) → review+
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?
Attachment #626349 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 3

5 years ago
(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? :)
(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.
(Assignee)

Comment 5

5 years ago
(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).
> > > 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.
(Assignee)

Comment 7

5 years ago
so I'll go with caching approach.
(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.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ccdad1d0c52
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/9ccdad1d0c52
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.