Closed
Bug 980307
Opened 8 years ago
Closed 8 years ago
Remove DataContainerEvent dependency from accessibility
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: emk, Assigned: emk)
References
Details
Attachments
(1 file, 1 obsolete file)
15.22 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #8386756 -
Flags: review?(trev.saunders)
Comment 1•8 years ago
|
||
Comment on attachment 8386756 [details] [diff] [review] patch I was hoping for something that didn't use dom events at all, but this is fine for now. >+ customEvent->GetDetail(getter_AddRefs(detailVariant)); >+ if (!detailVariant) > return; > >- nsCOMPtr<nsIVariant> countVariant; >- dataEvent->GetData(NS_LITERAL_STRING("count"), >- getter_AddRefs(countVariant)); >- if (!countVariant) >+ nsCOMPtr<nsIPropertyBag2> propBag; >+ detailVariant->GetAsISupports(getter_AddRefs(propBag)); doesn't that use the operator nsISupports** on getter_AddRefs we'd like to get rid of? any way I think it would be clearer if you did the QI manually. >+ nsCOMPtr<nsIPropertyBag2> propBag; >+ detailVariant->GetAsISupports(getter_AddRefs(propBag)); same might not hurt to have someone review the layout bits, but they lgtm
Attachment #8386756 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 2•8 years ago
|
||
An automated test (test_tree.xul) caught errors of the patch. (In reply to Trevor Saunders (:tbsaunde) from comment #1) > I was hoping for something that didn't use dom events at all, but this is > fine for now. Maybe it is impossible unless we rewrite the test using C++. > >+ customEvent->GetDetail(getter_AddRefs(detailVariant)); > >+ if (!detailVariant) > > return; > > > >- nsCOMPtr<nsIVariant> countVariant; > >- dataEvent->GetData(NS_LITERAL_STRING("count"), > >- getter_AddRefs(countVariant)); > >- if (!countVariant) > >+ nsCOMPtr<nsIPropertyBag2> propBag; > >+ detailVariant->GetAsISupports(getter_AddRefs(propBag)); > > doesn't that use the operator nsISupports** on getter_AddRefs we'd like to > get rid of? any way I think it would be clearer if you did the QI manually. It was not a style nit but an actual fatal bug. We can't receive the interface pointer without QI'ing. This bug caused a cryptic test crash. Thanks to pointing out. I wish nsCOMPtr could have caught this... I think I made enough changes that are qualified to have another round of review. Try runs with the new patch: https://tbpl.mozilla.org/?tree=Try&rev=60b58c4ea9e2 https://tbpl.mozilla.org/?tree=Try&rev=81580be40d79
Attachment #8386756 -
Attachment is obsolete: true
Attachment #8388028 -
Flags: review?(trev.saunders)
Assignee | ||
Updated•8 years ago
|
Attachment #8388028 -
Attachment is patch: true
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #2) > I wish nsCOMPtr could have caught this... I found bug 345123.
Updated•8 years ago
|
Attachment #8388028 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 4•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0216df6e2a
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment 5•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9b0216df6e2a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•