Closed
Bug 757757
Opened 12 years ago
Closed 12 years ago
Add dexpcomed version of GetAnonymousElementByAttribute
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: surkov, Assigned: surkov)
Details
Attachments
(1 file)
16.57 KB,
patch
|
tbsaunde
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #626349 -
Flags: review?(trev.saunders)
Attachment #626349 -
Flags: review?(bzbarsky)
Comment 1•12 years ago
|
||
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 2•12 years ago
|
||
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•12 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? :)
Comment 4•12 years ago
|
||
(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•12 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).
Comment 6•12 years ago
|
||
> > > 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•12 years ago
|
||
so I'll go with caching approach.
Comment 8•12 years ago
|
||
(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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ccdad1d0c52
Target Milestone: --- → mozilla15
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9ccdad1d0c52
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•