Closed Bug 1208900 Opened 6 years ago Closed 6 years ago

Fix a null check in sdnAccessible::get_computedStyle

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: ehsan.akhgari, Unassigned)

References

Details

Attachments

(1 file)

No description provided.
Found by Viva64.
Attachment #8666517 - Flags: review?(tbsaunde+mozbugs)
Blocks: 710966
Duplicate of this bug: 1208856
Comment on attachment 8666517 [details] [diff] [review]
Fix a null check in sdnAccessible::get_computedStyle

so, I think we generally agree this API should go away (meaning all of ISimpleDOM).  this method hasn't usefully worked since nov 2012, and nobody has complained.  Alex David do you see any reason we should make it work now instead of just having this method return E_NOTIMPL?
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(dbolter)
I defer to surkov for ISimpleDOM but if it truly isn't used then fine with me.
Flags: needinfo?(dbolter)
(In reply to David Bolter [:davidb] from comment #4)
> I defer to surkov for ISimpleDOM but if it truly isn't used then fine with
> me.

if you look at the code you'll see its hard to imagine someone doing something useful with it.
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 8666517 [details] [diff] [review]
> Fix a null check in sdnAccessible::get_computedStyle
> 
> so, I think we generally agree this API should go away (meaning all of
> ISimpleDOM).  this method hasn't usefully worked since nov 2012, and nobody
> has complained.  Alex David do you see any reason we should make it work now
> instead of just having this method return E_NOTIMPL?

I'm ok with both ways, and they are much equivalent as long as the method doesn't have a consumer.
Flags: needinfo?(surkov.alexander)
"Unassigning" from myself (even though I was never assigned) because I lack the test setup to write the patch that Trevor wants.
Comment on attachment 8666517 [details] [diff] [review]
Fix a null check in sdnAccessible::get_computedStyle

Review of attachment 8666517 [details] [diff] [review]:
-----------------------------------------------------------------

my understanding is that nothing prevents us to take this patch, correct, Trev?
Attachment #8666517 - Flags: review?(tbsaunde+mozbugs) → review+
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to alexander :surkov from comment #8)
> Comment on attachment 8666517 [details] [diff] [review]
> Fix a null check in sdnAccessible::get_computedStyle
> 
> Review of attachment 8666517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> my understanding is that nothing prevents us to take this patch, correct,
> Trev?

I'd rather not make this method work and thus encourage people to use it unless we have a good reason.  is there any reason *to* take this patch?  It seems to me it doesn't improve anything so I'd tend to say its not ok lacking other evidence.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #7)
> "Unassigning" from myself (even though I was never assigned) because I lack
> the test setup to write the patch that Trevor wants.

honestly all I'd do is replace the method body with return E_NOTIMPL and check it compiles on windows.  There are no tests for this.
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> I'd rather not make this method work and thus encourage people to use it
> unless we have a good reason.  is there any reason *to* take this patch?

* ISimpleDOM is still alive API, so it'd be good if its methods were worked properly. We don't encourage people to use it, but that doesn't mean we have to break stuff (or keep broken) intentionally.

* you won't be longer pointed to the problem by viva64.
(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > I'd rather not make this method work and thus encourage people to use it
> > unless we have a good reason.  is there any reason *to* take this patch?
> 
> * ISimpleDOM is still alive API, so it'd be good if its methods were worked
> properly. We don't encourage people to use it, but that doesn't mean we have
> to break stuff (or keep broken) intentionally.

I'd say its unfortunate its still alive and its use is to be discouraged.  So while maybe we shouldn't break it intentionally there's no reason we have to fix bugs that clearly nobody cares about, or keep around implementations of methods that are clearly unused.

> * you won't be longer pointed to the problem by viva64.

I don't think we're running it ourselves so that seems of very low value.
Comment on attachment 8666517 [details] [diff] [review]
Fix a null check in sdnAccessible::get_computedStyle

I'm willing to reconsider if someone wants to convince me there's some value in making this work again.
Attachment #8666517 - Flags: review-
anyway, as I said I'm good with both options. Minor stuff like this shouldn't strike 10+ comments.
We don't know if one of the proprietary ATs atually relies on this function to give their users formatting information. Granted, IA2 provides that, but given their legacy code they carry around, we don't know that they're not calling into this particular method, too.

I vote for fixing it so the warning goes away and not risk breaking things. If we deprecate ISimpleDOM, we should do it all at once and not bit by bit.
(In reply to Marco Zehe (:MarcoZ) from comment #14)
> We don't know if one of the proprietary ATs atually relies on this function
> to give their users formatting information. Granted, IA2 provides that, but
> given their legacy code they carry around, we don't know that they're not
> calling into this particular method, too.
> 
> I vote for fixing it so the warning goes away and not risk breaking things.
> If we deprecate ISimpleDOM, we should do it all at once and not bit by bit.

the bug is almost 3 years old (http://hg.mozilla.org/mozilla-central/rev/19eb532fad5a), no high risk that someone depends on it
Sorry guys, looks like I accidentally hit the hornet's nest here.  Let's just not do anything here and get back to the stuff that really matters.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.