Add routing id and notification type to accessibility logging.

Add routing id and notification type to accessibility logging.

BUG=none
TEST=manual.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102167

[David Tseng, 2011-09-13 23:15:03]

Additions to logging; either tony or dmazzoni please review.

[tony, 2011-09-13 23:22:21]

LGTM, but you probably need a content owner to approve.

http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h#...
content/common/view_messages.h:134: case TEXT_REMOVED: return "text removed";
Nit: indent seems off here

http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h#...
content/common/view_messages.h:137: return "";
Nit: NOT_REACHED()

[David Tseng, 2011-09-13 23:26:47]

+jam for owners approval.

On 9/13/11, tony@chromium.org <tony@chromium.org> wrote:
> LGTM, but you probably need a content owner to approve.
>
>
> http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h
> File content/common/view_messages.h (right):
>
>
http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h#...
> content/common/view_messages.h:134: case TEXT_REMOVED: return "text
> removed";
> Nit: indent seems off here
>
>
http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h#...
> content/common/view_messages.h:137: return "";
> Nit: NOT_REACHED()
>
> http://codereview.chromium.org/7867018/
>

[David Tseng, 2011-09-15 14:19:07]

Brett: Please rubber stamp?


On 9/13/11, David Tseng <dtseng@chromium.org> wrote:
> +jam for owners approval.
>
> On 9/13/11, tony@chromium.org <tony@chromium.org> wrote:
>> LGTM, but you probably need a content owner to approve.
>>
>>
>> http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h
>> File content/common/view_messages.h (right):
>>
>>
http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h#...
>> content/common/view_messages.h:134: case TEXT_REMOVED: return "text
>> removed";
>> Nit: indent seems off here
>>
>>
http://codereview.chromium.org/7867018/diff/1/content/common/view_messages.h#...
>> content/common/view_messages.h:137: return "";
>> Nit: NOT_REACHED()
>>
>> http://codereview.chromium.org/7867018/
>>
>

[brettw, 2011-09-15 18:05:25]

http://codereview.chromium.org/7867018/diff/5002/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7867018/diff/5002/content/common/view_messages...
content/common/view_messages.h:114: static const std::string DebugString(Value
value) {
Can this big function be not inlined? Otherwise we'll get all these strings in
each translation unit.

http://codereview.chromium.org/7867018/diff/5002/content/common/view_messages...
content/common/view_messages.h:134: case TEXT_REMOVED: return "text removed";
Indenting

[David Tseng, 2011-09-15 21:58:45]

http://codereview.chromium.org/7867018/diff/5002/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7867018/diff/5002/content/common/view_messages...
content/common/view_messages.h:114: static const std::string DebugString(Value
value) {
On 2011/09/15 18:05:25, brettw wrote:
> Can this big function be not inlined? Otherwise we'll get all these strings in
> each translation unit.

Only true of debug builds though?
Don't feel that strongly about it, so
Done.

http://codereview.chromium.org/7867018/diff/5002/content/common/view_messages...
content/common/view_messages.h:134: case TEXT_REMOVED: return "text removed";
On 2011/09/15 18:05:25, brettw wrote:
> Indenting

Done.

[brettw, 2011-09-16 04:52:23]

I rather liked the separate function in the original code review. It seems like
it resulted in about 1/3 less code. Maybe you can make the separate function in
the .cc file?

I was originally thinking we would keep it like you have originally but
implement it in view_messages.cc, but I see there is no .cc file for
view_messages.h

http://codereview.chromium.org/7867018/diff/5002/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7867018/diff/5002/content/common/view_messages...
content/common/view_messages.h:114: static const std::string DebugString(Value
value) {
On 2011/09/15 21:58:45, David Tseng wrote:
> On 2011/09/15 18:05:25, brettw wrote:
> > Can this big function be not inlined? Otherwise we'll get all these strings
in
> > each translation unit.
> 
> Only true of debug builds though?
> Don't feel that strongly about it, so

Duplicating this code all over makes the .o files bigger and makes more work for
the linker. So it "shouldn't" affect the outcome, but makes developer's lives
slightly better when we do this consistently.

http://codereview.chromium.org/7867018/diff/12001/webkit/glue/webaccessibilit...
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/7867018/diff/12001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:32: 
Extra blank line here.

http://codereview.chromium.org/7867018/diff/12001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:344: int render_routing_id,
These should be aligned with the "bool" on the previous line.

http://codereview.chromium.org/7867018/diff/12001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:352: result+= "routing id: ";
There should be a space before the "+" on each of these lines.

[brettw, 2011-09-16 04:52:40]

LGTM with my suggestions, don't want to spend too long bikeshedding.

[David Tseng, 2011-09-19 16:02:45]

http://codereview.chromium.org/7867018/diff/12001/webkit/glue/webaccessibilit...
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/7867018/diff/12001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:32: 
On 2011/09/16 04:52:23, brettw wrote:
> Extra blank line here.

Done.

http://codereview.chromium.org/7867018/diff/12001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:344: int render_routing_id,
On 2011/09/16 04:52:23, brettw wrote:
> These should be aligned with the "bool" on the previous line.

Done.

http://codereview.chromium.org/7867018/diff/12001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:352: result+= "routing id: ";
On 2011/09/16 04:52:23, brettw wrote:
> There should be a space before the "+" on each of these lines.

Done; had done this to minimize spacing (bad style).

[commit-bot: I haz the power, 2011-09-20 00:57:12]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux, revision is 101877, job name
was 7867018-15002 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-09-21 00:13:58]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux, revision is 102057, job name
was 7867018-22001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-09-21 21:04:00]

Change committed as 102167