838518 - C++ createTreeWalker callers broken by API change

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The API change in bug 698384 didn't update any callers to the new API.  That doesn't matter too much for JS callers, since the extra argument is ignored.  But for C++ callers what used to be the "entityReferenceExpansion" boolean is now treated as the optional_argc.

There are two C++ callers.  One was passing true, which becomes 1, and it was passing a null filter so it's ok.  The other one, though, was passing false, so now its whatToShow argument is being ignored.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Fix treewalker callers per the API change that was made.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Fix nodeiterator callers per the API change that was made.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we add a test that would have caught this?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't know.  The only real difference in behavior was for the accessibility treewalker...

Trevor, any idea how to test that?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 710591 [details] [diff] [review]
part 1.  Fix treewalker callers per the API change that was made.

Ugh.

Comment 6

[Trevor Saunders (:tbsaunde)]

> Trevor, any idea how to test that?

Well, testing this will suck, and I should probably rip the nsIDOM crud there out so any test you write will hopefully stop testing anything soon.  However if you really want to test it I think something like <div>foo bar</div> selecting "bar" then getting the accessible for that div and asking its nsIAccessibleText impl for the selection bounds should work.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I doubt that will work.  The bug here was that the treewalker was using SHOW_ALL instead of SHOW_ELEMENT|SHOW_TEXT.  So to trigger it we'd need a tree that has other things in there (comments, processing instructions, something)...

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> I doubt that will work.  The bug here was that the treewalker was using
> SHOW_ALL instead of SHOW_ELEMENT|SHOW_TEXT.  So to trigger it we'd need a

yeah, oops, thanks xpidl

> tree that has other things in there (comments, processing instructions,
> something)...

In that case I'm not sure it has an observable effect since that loop ignores things that don't have accessibles, and all we use it for is text offset stuff.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Great, thanks!

Comment 10

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e47cdd7b68
   https://hg.mozilla.org/integration/mozilla-inbound/rev/adcbf76cd586

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1e47cdd7b68
https://hg.mozilla.org/mozilla-central/rev/adcbf76cd586