content: convert accessibility notifications to observer usage

content: convert accessibility notifications to callbacks

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

[dmazzoni, 2013-02-08 15:55:21]

lgtm

https://codereview.chromium.org/12210082/diff/5003/content/public/browser/ren...
File content/public/browser/render_view_host_observer.h (right):

https://codereview.chromium.org/12210082/diff/5003/content/public/browser/ren...
content/public/browser/render_view_host_observer.h:45: virtual void
AccessibilityLayoutComplete();
Perhaps a single method with an argument would be cleaner?

  OnAccessibilityNotification(AccessibilityNotification notification_type);

A reason I didn't do that before (one notification with the accessibility
notification enum in the details) was so I could use
WindowedNotificationObserver.

[jam, 2013-02-08 17:59:27]

since these notifications are only used inside content, and only for testing,
they shouldn't be in the public API.

how about adding two ForTesting methods on RenderViewHostImpl, i.e.

void AddAccessibilityLayoutCompleteCallbackForTesting(const base::Closure&
callback);
void AddAccessibilityLoadCompleteCallbackForTesting(const base::Closure&
callback);

this would also make the tests simpler, since you wouldn't need to create new
observer classes.

[Paweł Hajdan Jr., 2013-02-12 14:51:33]

PTAL

[dmazzoni, 2013-02-12 16:00:52]

https://codereview.chromium.org/12210082/diff/10002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/12210082/diff/10002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:666:
std::vector<base::Closure> accessibility_layout_callbacks_;
I can't imagine we'll ever need more than one callback for tests. Just one of
each type should be fine, and the current usage pattern where it's used only to
block until the callback occurs should be fine.

How about if the callback is just called once and then deleted? Change it to:

SetOneTimeAccessibilityLoadCompleteCallbackForTesting()

...with no vector needed, and no Remove method needed.

[jam, 2013-02-12 17:48:18]

https://codereview.chromium.org/12210082/diff/10002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/12210082/diff/10002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:666:
std::vector<base::Closure> accessibility_layout_callbacks_;
On 2013/02/12 16:00:52, Dominic Mazzoni wrote:
> I can't imagine we'll ever need more than one callback for tests. Just one of
> each type should be fine, and the current usage pattern where it's used only
to
> block until the callback occurs should be fine.
> 
> How about if the callback is just called once and then deleted? Change it to:
> 
> SetOneTimeAccessibilityLoadCompleteCallbackForTesting()
> 
> ...with no vector needed, and no Remove method needed.

+1

[Paweł Hajdan Jr., 2013-02-13 15:36:12]

PTAL

[dmazzoni, 2013-02-13 19:49:20]

lgtm

[jam, 2013-02-14 00:39:00]

lgtm, thank you this looks great

https://codereview.chromium.org/12210082/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/12210082/diff/12001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:425: // Set accessibility
callbacks (only for testing).
nit: (only for testing). is redundant given the method names

also can you move these to line 417? usually the convention is to put simple
getters after method declarations

[Paweł Hajdan Jr., 2013-02-14 09:28:11]

https://codereview.chromium.org/12210082/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/12210082/diff/12001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:425: // Set accessibility
callbacks (only for testing).
On 2013/02/14 00:39:01, jam wrote:
> nit: (only for testing). is redundant given the method names
> 
> also can you move these to line 417? usually the convention is to put simple
> getters after method declarations

Done.

[Paweł Hajdan Jr., 2013-02-14 13:41:19]

PTAL

Sorry I missed one windows test file in chrome. If you want me to change things
back to an observer or do something else, fine for me.

[dmazzoni, 2013-02-15 19:34:19]

https://codereview.chromium.org/12210082/diff/17001/chrome/browser/accessibil...
File chrome/browser/accessibility/accessibility_win_browsertest.cc (right):

https://codereview.chromium.org/12210082/diff/17001/chrome/browser/accessibil...
chrome/browser/accessibility/accessibility_win_browsertest.cc:224:
content::WebContents* web_contents =
How about a helper method to get view_host since this code is repeated so often?
That way we could just have:

GetViewHost()->SetAccessibilityLayoutCompleteCallbackForTesting(...)

Otherwise, these changes look fine.

[jam, 2013-02-18 03:53:30]

On 2013/02/14 13:41:19, Paweł Hajdan Jr. wrote:
> PTAL
> 
> Sorry I missed one windows test file in chrome. If you want me to change
things
> back to an observer or do something else, fine for me.

we should avoid adding these ForTesting methods in the API

why do the tests for this need to be in src/chrome? can't the content-specific
part be in src/content?

[dmazzoni, 2013-02-18 04:20:17]

On 2013/02/18 03:53:30, jam wrote:
> On 2013/02/14 13:41:19, Paweł Hajdan Jr. wrote:
> > PTAL
> > 
> > Sorry I missed one windows test file in chrome. If you want me to change
> things
> > back to an observer or do something else, fine for me.
> 
> we should avoid adding these ForTesting methods in the API
> 
> why do the tests for this need to be in src/chrome? can't the content-specific
> part be in src/content?

I think there was some reason this test wasn't moved from chrome to content a
while ago when we moved all of the other accessibility tests. I don't see any
reason it couldn't be moved now.

[Paweł Hajdan Jr., 2013-02-27 19:32:35]

Alright, with accessibility_win_browsertest.cc in content this is again a
content-only change.

PTAL

Please do a conditional positive review if there are trivial issues to fix.

[dmazzoni, 2013-02-27 19:34:53]

lgtm

[Paweł Hajdan Jr., 2013-03-01 01:44:47]

Committed patchset #11 manually as r185393 (presubmit successful).