Relanding http://codereview.chromium.org/8416034

Relanding http://codereview.chromium.org/8416034
TBR=dtseng@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112885

[dmazzoni, 2011-12-02 23:33:41]

lgtm

Happy with this change, please just make a few quick fixes below:

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

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:96: instance_active_ =
true;
You shouldn't need to do this in PostInitialize.

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.h (right):

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:59: virtual void
PreInitialize();
I think you can make this one protected now, right? The idea is for the Windows
implementation to extend this behavior without needing to take all of the
arguments.

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:65: virtual void
PostInitialize();
You don't need a non-argument version and a version that takes arguments now,
just one PostInitialize.

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:83: void
PostInitialize(BrowserAccessibilityManager* manager,
This shouldn't take any arguments.

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

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:249:
current->SendNodeUpdateEvents();
You should call PostInitialize here, right?

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:348:
instance->SendNodeUpdateEvents();
Can you combine SendNodeUpdateEvents with PostInitialize? Just stick the code in
SendNodeUpdateEvents into PostInitialize's implementation and delete
SendNodeUpdateEvents. But then please call it here, at the bottom of
CreateAccessibilityTree.

[David Tseng, 2011-12-03 00:17:13]

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

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:96: instance_active_ =
true;
On 2011/12/02 23:33:42, Dominic Mazzoni wrote:
> You shouldn't need to do this in PostInitialize.

Done.

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.h (right):

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:59: virtual void
PreInitialize();
Done.
On 2011/12/02 23:33:42, Dominic Mazzoni wrote:
> I think you can make this one protected now, right? The idea is for the
Windows
> implementation to extend this behavior without needing to take all of the
> arguments.

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:65: virtual void
PostInitialize();
First went for the symetry with PreInitialize. Kept this one and removed the
other.
On 2011/12/02 23:33:42, Dominic Mazzoni wrote:
> You don't need a non-argument version and a version that takes arguments now,
> just one PostInitialize.

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:83: void
PostInitialize(BrowserAccessibilityManager* manager,
Done; (still needs to be virtual though) so keeping the  other).

On 2011/12/02 23:33:42, Dominic Mazzoni wrote:
> This shouldn't take any arguments.

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

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:249:
current->SendNodeUpdateEvents();
On 2011/12/02 23:33:42, Dominic Mazzoni wrote:
> You should call PostInitialize here, right?

Done.

http://codereview.chromium.org/8775059/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:348:
instance->SendNodeUpdateEvents();
Done, though this doesn't exactly fit your usage of this routine in
BrowserAccessibility::InternalReleaseReference? I don't think that's a problem
though.

On 2011/12/02 23:33:42, Dominic Mazzoni wrote:
> Can you combine SendNodeUpdateEvents with PostInitialize? Just stick the code
in
> SendNodeUpdateEvents into PostInitialize's implementation and delete
> SendNodeUpdateEvents. But then please call it here, at the bottom of
> CreateAccessibilityTree.

[commit-bot: I haz the power, 2011-12-03 03:58:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/8775059/3010

[commit-bot: I haz the power, 2011-12-03 03:58:35]

Can't apply patch for file
content/browser/accessibility/browser_accessibility.cc.
While running patch -p1 --forward --force;
patching file content/browser/accessibility/browser_accessibility.cc
Hunk #3 succeeded at 179 (offset 6 lines).
Hunk #4 FAILED at 263.
1 out of 4 hunks FAILED -- saving rejects to file
content/browser/accessibility/browser_accessibility.cc.rej

[dmazzoni, 2011-12-03 08:44:32]

CQ failed due to a merge error after I committed a change to the same files.

I fixed the merge and I'm going to land manually now.