Improve formatting of accessibility tests that dump the tree.

Re-land: Improve formatting of accessibility tests that dump the tree.

Adds indentation, uses more readable notation.
Adds one new test for static lists to illustrate the indentations.

BUG=none
TEST=Adds new test

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=125898

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126105

[dmazzoni, 2012-03-06 21:29:46]

dtseng: primary review

Suggestions from Alice & Aaron welcome.

This is just the first step, let's all work on making these tests awesome.

[David Tseng, 2012-03-06 23:49:05]

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_helper.cc (right):

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper.cc:13: 
nit: extra line.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper.cc:18: for (int i =
0; i < indent; i++)
nit: ++i

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper.cc:20:
prefix[indent] = 0;
nit: 0 -> NULL for clearity?

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper.cc:24:
RecursiveDumpAccessibilityTree(node->children()[i], contents, indent + 4);
nit: define the indent step as a constant somewhere.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_helper_mac.mm
(right):

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper_mac.mm:20:
[NSString stringWithFormat:@"%s%@ subrole=%@ title='%@' value='%@'\n",
Could you also include "role=", and perhaps we should have a cross platform
template and use vswprintf in the platform implementations.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_helper_win.cc (left):

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper_win.cc:184: const
string16 DumpAccessibilityTreeHelper::GetLineEnding() const {
I don't seem to see the browsertest file? Put this in in order to get Windows
style line endings so the expected files are notepad editable.

[dmazzoni, 2012-03-08 07:10:37]

PTAL; I addressed or responded to all of your feedback. I also added a little
bit more debugging info to the test, to ideally make it really easy to fix when
tests fail.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_helper.cc (right):

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper.cc:13: 
On 2012/03/06 23:49:05, David Tseng wrote:
> nit: extra line.

Done.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper.cc:18: for (int i =
0; i < indent; i++)
On 2012/03/06 23:49:05, David Tseng wrote:
> nit: ++i

Done.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper.cc:20:
prefix[indent] = 0;
On 2012/03/06 23:49:05, David Tseng wrote:
> nit: 0 -> NULL for clearity?

I think NULL is only supposed to be for pointers. I'll use '\0' instead to make
it more clear it's a string terminator and not an integer.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper.cc:24:
RecursiveDumpAccessibilityTree(node->children()[i], contents, indent + 4);
On 2012/03/06 23:49:05, David Tseng wrote:
> nit: define the indent step as a constant somewhere.

Done.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_helper_mac.mm
(right):

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper_mac.mm:20:
[NSString stringWithFormat:@"%s%@ subrole=%@ title='%@' value='%@'\n",
On 2012/03/06 23:49:05, David Tseng wrote:
> Could you also include "role=", and perhaps we should have a cross platform
> template and use vswprintf in the platform implementations.

Are you okay to try it without role? I tried adding role but the resulting file
is less readable, at least visually. I like that the very first thing on each
line is the role, it makes it much easier to skim a hierarchy and find a line
you're looking for.

If you still don't like it after we've added more tests, I'm fine to add role=
later, or alternatively try some other format like xml or json.

I don't think we should strive to have a strict cross-platform template, because
remember there are several fields that will only apply to either mac or win, so
we should allow them to diverge as necessary. Let's just have them use similar
syntax.

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_helper_win.cc (left):

http://codereview.chromium.org/9617019/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_helper_win.cc:184: const
string16 DumpAccessibilityTreeHelper::GetLineEnding() const {
On 2012/03/06 23:49:05, David Tseng wrote:
> I don't seem to see the browsertest file? Put this in in order to get Windows
> style line endings so the expected files are notepad editable.

It's handled directly in ToString now - the platforms still use different line
endings, I just didn't see the need for the method.

[David Tseng, 2012-03-08 17:47:41]

> [NSString stringWithFormat:@"%s%@ subrole=%@ title='%@' value='%@'\n",
> On 2012/03/06 23:49:05, David Tseng wrote:
>> Could you also include "role=", and perhaps we should have a cross
> platform
>> template and use vswprintf in the platform implementations.
>
> Are you okay to try it without role? I tried adding role but the
> resulting file is less readable, at least visually. I like that the very
> first thing on each line is the role, it makes it much easier to skim a
> hierarchy and find a line you're looking for.
>
Ok.

> If you still don't like it after we've added more tests, I'm fine to add
> role= later, or alternatively try some other format like xml or json.
>
> I don't think we should strive to have a strict cross-platform template,
> because remember there are several fields that will only apply to either
> mac or win, so we should allow them to diverge as necessary. Let's just
> have them use similar syntax.
>

I was thinking that we could have a cross platform piece (role=...
value=...) and then a platform specific one (... subrole=), but I'm
fine with the current format as long as we keep things in similar
order.

>
> const string16 DumpAccessibilityTreeHelper::GetLineEnding() const {
> On 2012/03/06 23:49:05, David Tseng wrote:
>> I don't seem to see the browsertest file? Put this in in order to get
> Windows
>> style line endings so the expected files are notepad editable.
>
> It's handled directly in ToString now - the platforms still use
> different line endings, I just didn't see the need for the method.
>
I'm probably going to use it in the comment patch at:
https://chromiumcodereview.appspot.com/9639016
(not yet ready for review) unless we can get the native line break
from somewhere else.

[dmazzoni, 2012-03-08 17:48:52]

On Thu, Mar 8, 2012 at 9:47 AM, David Tseng <dtseng@chromium.org> wrote:

> I'm probably going to use it in the comment patch at:
> https://chromiumcodereview.appspot.com/9639016
> (not yet ready for review) unless we can get the native line break
> from somewhere else.
>

Ah, ok.

[David Tseng, 2012-03-08 18:57:38]

lgtm with those changes; thanks.

On 3/8/12, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> On Thu, Mar 8, 2012 at 9:47 AM, David Tseng <dtseng@chromium.org> wrote:
>
>> I'm probably going to use it in the comment patch at:
>> https://chromiumcodereview.appspot.com/9639016
>> (not yet ready for review) unless we can get the native line break
>> from somewhere else.
>>
>
> Ah, ok.
>

[commit-bot: I haz the power, 2012-03-09 19:49:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/9617019/14001

[commit-bot: I haz the power, 2012-03-09 19:49:59]

Can't apply patch for file
content/test/data/accessibility/aria-application-expected-win.txt.
While running patch -p1 --forward --force;
patching file content/test/data/accessibility/aria-application-expected-win.txt
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
content/test/data/accessibility/aria-application-expected-win.txt.rej

[dmazzoni, 2012-03-09 21:10:45]

(Committed manually. I think "patch" had trouble with the line endings on
the win expectations file.)

On Fri, Mar 9, 2012 at 11:50 AM, <commit-bot@chromium.org> wrote:

> Can't apply patch for file
> content/test/data/**accessibility/aria-**application-expected-win.txt.
> While running patch -p1 --forward --force;
> patching file content/test/data/**accessibility/aria-**
> application-expected-win.txt
> Hunk #1 FAILED at 1.
> 1 out of 1 hunk FAILED -- saving rejects to file
> content/test/data/**accessibility/aria-**application-expected-win.txt.**
> rej
>
>
>
http://codereview.chromium.**org/9617019/<http://codereview.chromium.org/9617...
>

[Ilya Sherman, 2012-03-09 21:24:14]

This broke compile on Linux, so had to be reverted:
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28dbg%29%28s...

[dmazzoni, 2012-03-09 22:23:06]

Argh! Thanks for reverting.

I should have done a try job on Linux too. The reason CQ failed was because
it couldn't patch the file with Windows line endings. Apparently the same
issue also caused the browser test to fail on win - the expected file's
line endings got changed in the patch.

David, what would you think about just switching to unix line endings, even
on Windows, so that it isn't so much trouble to patch? Unless you have
another solution. Maybe the file could be marked as binary in svn, but can
we do that from a git patch too? Would that work when unix 'patch' is used,
like on the try servers?

- Dominic

On Fri, Mar 9, 2012 at 1:24 PM, <isherman@chromium.org> wrote:

> This broke compile on Linux, so had to be reverted:
> http://build.chromium.org/p/**chromium/builders/Linux%**
> 20Builder%20%28dbg%29%**28shared%29/builds/19714/**
>
steps/compile/logs/stdio<http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28dbg%29%28shared%29/builds/19714/steps/compile/logs/stdio>
>
>
https://chromiumcodereview.**appspot.com/9617019/<https://chromiumcodereview....
>

[Ilya Sherman, 2012-03-09 22:31:09]

On Fri, Mar 9, 2012 at 2:23 PM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> Argh! Thanks for reverting.
>
> I should have done a try job on Linux too. The reason CQ failed was
> because it couldn't patch the file with Windows line endings. Apparently
> the same issue also caused the browser test to fail on win - the expected
> file's line endings got changed in the patch.
>
> David, what would you think about just switching to unix line endings,
> even on Windows, so that it isn't so much trouble to patch?
>

Unless there's a special/unusual reason not to use Unix line endings, I'd
strongly recommend this approach.  Pretty much all of our infrastructure
expects Unix line endings, and has trouble with Windows ones.


> Unless you have another solution. Maybe the file could be marked as binary
> in svn, but can we do that from a git patch too? Would that work when unix
> 'patch' is used, like on the try servers?
>
> - Dominic
>
>
> On Fri, Mar 9, 2012 at 1:24 PM, <isherman@chromium.org> wrote:
>
>> This broke compile on Linux, so had to be reverted:
>> http://build.chromium.org/p/**chromium/builders/Linux%**
>> 20Builder%20%28dbg%29%**28shared%29/builds/19714/**
>>
steps/compile/logs/stdio<http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28dbg%29%28shared%29/builds/19714/steps/compile/logs/stdio>
>>
>>
https://chromiumcodereview.**appspot.com/9617019/<https://chromiumcodereview....
>>
>
>

[David Tseng, 2012-03-09 22:37:37]

Apparently the same
> issue also caused the browser test to fail on win - the expected file's
> line endings got changed in the patch.
>
> David, what would you think about just switching to unix line endings, even
> on Windows, so that it isn't so much trouble to patch? Unless you have
> another solution. Maybe the file could be marked as binary in svn, but can
> we do that from a git patch too? Would that work when unix 'patch' is used,
> like on the try servers?

I had lots of trouble with this which is why I explicitly added the
methods to get platform line endings. Git doesn't seem to respect the
endings for the expectations files -- it _always_ uses crlf no matter
what combination of git config autocrlf (true, false, auto, etc) I
tried in the past when paired with all either crlf or just lf's for
the expectations.


I haven't looked, but did you change
dump_accessibility_tree_browsertests accumulation of the individual
lines (including calling GetLineEnding)?

- David

[David Tseng, 2012-03-09 22:45:29]

Meant to say that the try bots always displayed the *expected.txt
files as crlf even when the patch converted everything to lf's.

Anyhow, if you can get it to pass on the try bots using unix style
line endings, please go for it.

Thanks.
On 3/9/12, David Tseng <dtseng@chromium.org> wrote:
> Apparently the same
>> issue also caused the browser test to fail on win - the expected file's
>> line endings got changed in the patch.
>>
>> David, what would you think about just switching to unix line endings,
>> even
>> on Windows, so that it isn't so much trouble to patch? Unless you have
>> another solution. Maybe the file could be marked as binary in svn, but
>> can
>> we do that from a git patch too? Would that work when unix 'patch' is
>> used,
>> like on the try servers?
>
> I had lots of trouble with this which is why I explicitly added the
> methods to get platform line endings. Git doesn't seem to respect the
> endings for the expectations files -- it _always_ uses crlf no matter
> what combination of git config autocrlf (true, false, auto, etc) I
> tried in the past when paired with all either crlf or just lf's for
> the expectations.
>
>
> I haven't looked, but did you change
> dump_accessibility_tree_browsertests accumulation of the individual
> lines (including calling GetLineEnding)?
>
> - David
>

[Ilya Sherman, 2012-03-09 22:49:32]

On Fri, Mar 9, 2012 at 2:45 PM, David Tseng <dtseng@chromium.org> wrote:

> Meant to say that the try bots always displayed the *expected.txt
> files as crlf even when the patch converted everything to lf's.
>

Yes, the try bots can't handle line-endings changes very well.  What I've
done in the past -- per maruel's recommendation -- is to create a minimal
CL that changes the line endings and nothing else, commit that to the tree
without try jobs (but with confidence based on the minimalism of the CL),
and then watch the tree closely just in case.  Once that CL lands,
everything should work much more smoothly; but unfortunately there's not
any nice smooth path to fixing the line endings to begin with.


> Anyhow, if you can get it to pass on the try bots using unix style
> line endings, please go for it.
>
> Thanks.
> On 3/9/12, David Tseng <dtseng@chromium.org> wrote:
> > Apparently the same
> >> issue also caused the browser test to fail on win - the expected file's
> >> line endings got changed in the patch.
> >>
> >> David, what would you think about just switching to unix line endings,
> >> even
> >> on Windows, so that it isn't so much trouble to patch? Unless you have
> >> another solution. Maybe the file could be marked as binary in svn, but
> >> can
> >> we do that from a git patch too? Would that work when unix 'patch' is
> >> used,
> >> like on the try servers?
> >
> > I had lots of trouble with this which is why I explicitly added the
> > methods to get platform line endings. Git doesn't seem to respect the
> > endings for the expectations files -- it _always_ uses crlf no matter
> > what combination of git config autocrlf (true, false, auto, etc) I
> > tried in the past when paired with all either crlf or just lf's for
> > the expectations.
> >
> >
> > I haven't looked, but did you change
> > dump_accessibility_tree_browsertests accumulation of the individual
> > lines (including calling GetLineEnding)?
> >
> > - David
> >
>

[dmazzoni, 2012-03-09 22:50:35]

Sounds good, will do.

I suspect the problem is if one of the test or build machines doesn't have
its line endings set the same way. For example, a trybot or buildbot could
be in a mode that converts all (non-binary) files to crlf consistently
without causing any problems elsewhere.

Maybe the easiest thing will be to just have a line-ending-aware comparison
function? You were adding a custom diff anyway, so it'd be pretty trivial.

- Dominic

On Fri, Mar 9, 2012 at 2:45 PM, David Tseng <dtseng@chromium.org> wrote:

> Meant to say that the try bots always displayed the *expected.txt
> files as crlf even when the patch converted everything to lf's.
>
> Anyhow, if you can get it to pass on the try bots using unix style
> line endings, please go for it.
>
> Thanks.
> On 3/9/12, David Tseng <dtseng@chromium.org> wrote:
> > Apparently the same
> >> issue also caused the browser test to fail on win - the expected file's
> >> line endings got changed in the patch.
> >>
> >> David, what would you think about just switching to unix line endings,
> >> even
> >> on Windows, so that it isn't so much trouble to patch? Unless you have
> >> another solution. Maybe the file could be marked as binary in svn, but
> >> can
> >> we do that from a git patch too? Would that work when unix 'patch' is
> >> used,
> >> like on the try servers?
> >
> > I had lots of trouble with this which is why I explicitly added the
> > methods to get platform line endings. Git doesn't seem to respect the
> > endings for the expectations files -- it _always_ uses crlf no matter
> > what combination of git config autocrlf (true, false, auto, etc) I
> > tried in the past when paired with all either crlf or just lf's for
> > the expectations.
> >
> >
> > I haven't looked, but did you change
> > dump_accessibility_tree_browsertests accumulation of the individual
> > lines (including calling GetLineEnding)?
> >
> > - David
> >
>

[David Tseng, 2012-03-09 23:50:26]

On 3/9/12, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> Sounds good, will do.
>
Cool; sounds good.
> I suspect the problem is if one of the test or build machines doesn't have
> its line endings set the same way. For example, a trybot or buildbot could
> be in a mode that converts all (non-binary) files to crlf consistently
> without causing any problems elsewhere.
>
> Maybe the easiest thing will be to just have a line-ending-aware comparison
> function? You were adding a custom diff anyway, so it'd be pretty trivial.
>
> - Dominic
>
> On Fri, Mar 9, 2012 at 2:45 PM, David Tseng <dtseng@chromium.org> wrote:
>
>> Meant to say that the try bots always displayed the *expected.txt
>> files as crlf even when the patch converted everything to lf's.
>>
>> Anyhow, if you can get it to pass on the try bots using unix style
>> line endings, please go for it.
>>
>> Thanks.
>> On 3/9/12, David Tseng <dtseng@chromium.org> wrote:
>> > Apparently the same
>> >> issue also caused the browser test to fail on win - the expected file's
>> >> line endings got changed in the patch.
>> >>
>> >> David, what would you think about just switching to unix line endings,
>> >> even
>> >> on Windows, so that it isn't so much trouble to patch? Unless you have
>> >> another solution. Maybe the file could be marked as binary in svn, but
>> >> can
>> >> we do that from a git patch too? Would that work when unix 'patch' is
>> >> used,
>> >> like on the try servers?
>> >
>> > I had lots of trouble with this which is why I explicitly added the
>> > methods to get platform line endings. Git doesn't seem to respect the
>> > endings for the expectations files -- it _always_ uses crlf no matter
>> > what combination of git config autocrlf (true, false, auto, etc) I
>> > tried in the past when paired with all either crlf or just lf's for
>> > the expectations.
>> >
>> >
>> > I haven't looked, but did you change
>> > dump_accessibility_tree_browsertests accumulation of the individual
>> > lines (including calling GetLineEnding)?
>> >
>> > - David
>> >
>>
>

[dmazzoni, 2012-03-12 21:18:02]

For the record: re-landed successfully.

On Fri, Mar 9, 2012 at 3:50 PM, David Tseng <dtseng@chromium.org> wrote:

> On 3/9/12, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> > Sounds good, will do.
> >
> Cool; sounds good.
> > I suspect the problem is if one of the test or build machines doesn't
> have
> > its line endings set the same way. For example, a trybot or buildbot
> could
> > be in a mode that converts all (non-binary) files to crlf consistently
> > without causing any problems elsewhere.
> >
> > Maybe the easiest thing will be to just have a line-ending-aware
> comparison
> > function? You were adding a custom diff anyway, so it'd be pretty
> trivial.
> >
> > - Dominic
> >
> > On Fri, Mar 9, 2012 at 2:45 PM, David Tseng <dtseng@chromium.org> wrote:
> >
> >> Meant to say that the try bots always displayed the *expected.txt
> >> files as crlf even when the patch converted everything to lf's.
> >>
> >> Anyhow, if you can get it to pass on the try bots using unix style
> >> line endings, please go for it.
> >>
> >> Thanks.
> >> On 3/9/12, David Tseng <dtseng@chromium.org> wrote:
> >> > Apparently the same
> >> >> issue also caused the browser test to fail on win - the expected
> file's
> >> >> line endings got changed in the patch.
> >> >>
> >> >> David, what would you think about just switching to unix line
> endings,
> >> >> even
> >> >> on Windows, so that it isn't so much trouble to patch? Unless you
> have
> >> >> another solution. Maybe the file could be marked as binary in svn,
> but
> >> >> can
> >> >> we do that from a git patch too? Would that work when unix 'patch' is
> >> >> used,
> >> >> like on the try servers?
> >> >
> >> > I had lots of trouble with this which is why I explicitly added the
> >> > methods to get platform line endings. Git doesn't seem to respect the
> >> > endings for the expectations files -- it _always_ uses crlf no matter
> >> > what combination of git config autocrlf (true, false, auto, etc) I
> >> > tried in the past when paired with all either crlf or just lf's for
> >> > the expectations.
> >> >
> >> >
> >> > I haven't looked, but did you change
> >> > dump_accessibility_tree_browsertests accumulation of the individual
> >> > lines (including calling GetLineEnding)?
> >> >
> >> > - David
> >> >
> >>
> >
>