Split out the MSCOM pieces of BrowserAccessibilityWin into a seperate class.

Split out the MSCOM pieces of BrowserAccessibilityWin into a seperate class.

This CL splits out all of the MSCOM stuff in BrowserAccessibilityWin into to a
new class BrowserAccessibilityComWin. This allows us to more easily to reuse
the AXPlatformNodeWin.

The way this works is that BrowserAccessibilityWin creates and owns a COM
object named |browser_accessibility_com_|.  BrowserAccessibilityWin continues
to implement BrowserAccessibility but all interactions with Windows is done
through the BrowserAccessibilityComWin object.

BrowserAccessibilityComWin is created with both a delegate for
AXPlatformNodeWin usage as well as a backpointer to BrowserAccessibilityWin so
it can call back into BrowserAccessibility. Ownership is such that:

BrowserAccessibilityWin owns one reference on BrowserAccessibilityComWin.  When
BrowserAccessibilityWin goes away, it nulls out the backpointer and delegate
pointer rendering the BrowserAccessibilityComWin potentially still alive but
basically useless.

Note that this split is similar to how MacOS is implemented.

BUG=703369
Review-Url: https://codereview.chromium.org/2864953002
Cr-Commit-Position: refs/heads/master@{#470493}
Committed: https://chromium.googlesource.com/chromium/src/+/6b5b4cd8b3f76c0c4ad01e69b3f137604a9a21ee

[dougt, 2017-05-07 18:42:54]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-07 18:42:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-05-07 18:49:08]

dougt@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dougt, 2017-05-07 18:49:09]

dmazzoni, ptal. This will make reusing AXPlatformNodeWin much easier.

[commit-bot: I haz the power, 2017-05-07 20:05:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-07 20:05:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2017-05-08 04:45:24]

lgtm!!!

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_win_browsertest.cc (right):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_win_browsertest.cc:119: IAccessible*
AccessibilityWinBrowserTest::GetRendererAccessible() {
I love that this file didn't need any major changes, this function
just continues to work. Great for test coverage.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.cc (right):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:815: if
(!GetOwner() || !GetOwner()->instance_active())
I don't see any purpose for BrowserAccessibility::instance_active anymore.

We should just check GetOwner here and if it's null, we're not active,
then get rid of BrowserAccessibility::Destroy() and replace it with just a
simple public destructor.

Oh, and we should probably have AXPlatformNodeWin own an
AXPlatformNodeComWin to match this.

Most of that can wait for a follow-up, but I think it would be safe to only
check GetOwner() and not bother checking instance_active(). If you look at
the code, there should never be a case where instance_active() returns
false, but GetOwner() is still valid.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:967:
->ToBrowserAccessibilityManagerWin()
Optional:
Since GetOwner()->manager() is used a lot, and sometimes
ToBrowserAccessibilityManagerWin(), how about a
convenience method GetManager() that returns a
BrowserAccessibilityManagerWin*?

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:1110:
->PlatformGetParent()
I think we want braces for readability since this wraps onto multiple lines?

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:1245: // No need
to check |GetOwner()->manager()| because this interface is
This is slightly confusing, how about just |GetOwner()|

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.h (left):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:979:
ToBrowserAccessibilityWin(BrowserAccessibility* obj);
Is there really no time we'll ever also want ToBrowserAccessibilityWin?

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.h (right):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:44: // a
BrowserAccessibilityCom |owner_|.  When this owner goes away, the
BrowserAccessibilityCom -> BrowserAccessibilityWin?

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:45: //
BrowserAccessibilityComWin objects may continue to exists being held onto by
Mention due to reference counting

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:46: // MSCOM.
However, such objects are invalid and should gracefully fail.
Should return E_FAIL from everything when instance_active_ is false

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:745: void
SetOwner(BrowserAccessibilityWin* owner) { owner_ = owner; }
I wonder if this should be called Init() or merged into an existing Init
function
to reinforce the idea that this is something that should be done once on
creation
and not ever changed

[dougt, 2017-05-08 16:42:19]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 16:43:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-05-08 16:53:35]

dougt@chromium.org changed reviewers:
+ brettw@chromium.org

[dougt, 2017-05-08 16:53:36]

brettw@chromium.org: Please review changes in 

content/browser/BUILD.gn
  adds two new files for the MSCOM stuff we factored out.

content/browser/renderer_host/legacy_render_widget_host_win.cc
content/browser/renderer_host/render_widget_host_view_aura.cc
  need to return the COM object now

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_win_browsertest.cc (right):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_win_browsertest.cc:119: IAccessible*
AccessibilityWinBrowserTest::GetRendererAccessible() {
On 2017/05/08 04:45:24, dmazzoni wrote:
> I love that this file didn't need any major changes, this function
> just continues to work. Great for test coverage.

+1000

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.cc (right):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:815: if
(!GetOwner() || !GetOwner()->instance_active())
I would like to only test GetOwner() in a follow up.  This is a pretty big
change and I was hoping to keep the logic nearly identical.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:967:
->ToBrowserAccessibilityManagerWin()
On 2017/05/08 04:45:24, dmazzoni wrote:
> Optional:
> Since GetOwner()->manager() is used a lot, and sometimes
> ToBrowserAccessibilityManagerWin(), how about a
> convenience method GetManager() that returns a
> BrowserAccessibilityManagerWin*?

Acknowledged.  Good follow up.  Noted.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:1110:
->PlatformGetParent()
On 2017/05/08 04:45:24, dmazzoni wrote:
> I think we want braces for readability since this wraps onto multiple lines?

Done.  Two places fixed.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:1245: // No need
to check |GetOwner()->manager()| because this interface is
yeah, this comment is useless.

Basically what it's saying is that "unlike all of the other methods we've
implemented... in this method, since we're not touching owner, or manager, or
whatever, we don't have to test for owner, or manager, or whatever.".  It's sort
of obvious.

I think this comment exists because up above all of the IAccessibile method
impls, there is a comment section called "Conventions" which demands that we
call GetOwner().  What I am going to do is remove these two confusing comments,
and update the Conventions comment block with an exception.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.h (left):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:979:
ToBrowserAccessibilityWin(BrowserAccessibility* obj);
On 2017/05/08 04:45:24, dmazzoni wrote:
> Is there really no time we'll ever also want ToBrowserAccessibilityWin?

Tests use it.  But, by far, the most common cast is to go from
BrowserAccessibility - > BrowserAccessibilityCOMWin.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.h (right):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:44: // a
BrowserAccessibilityCom |owner_|.  When this owner goes away, the
On 2017/05/08 04:45:24, dmazzoni wrote:
> BrowserAccessibilityCom -> BrowserAccessibilityWin?

Done.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:45: //
BrowserAccessibilityComWin objects may continue to exists being held onto by
On 2017/05/08 04:45:24, dmazzoni wrote:
> Mention due to reference counting

Done.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:46: // MSCOM.
However, such objects are invalid and should gracefully fail.
On 2017/05/08 04:45:24, dmazzoni wrote:
> Should return E_FAIL from everything when instance_active_ is false

Done.

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:745: void
SetOwner(BrowserAccessibilityWin* owner) { owner_ = owner; }
On 2017/05/08 04:45:24, dmazzoni wrote:
> I wonder if this should be called Init() or merged into an existing Init
> function
> to reinforce the idea that this is something that should be done once on
> creation
> and not ever changed

I do agree with you that this is a bit funky.  Similarly we have a ::Init method
on the AXPlatformNodeWin side of things.  I think we want to see if we can
*somehow* share these two things.

[dmazzoni, 2017-05-08 18:31:04]

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.h (left):

https://codereview.chromium.org/2864953002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:979:
ToBrowserAccessibilityWin(BrowserAccessibility* obj);
On 2017/05/08 04:45:24, dmazzoni wrote:
> Is there really no time we'll ever also want ToBrowserAccessibilityWin?

Nevermind, I see now it's just in a different file.

[brettw, 2017-05-09 17:45:47]

lgtm

[dougt, 2017-05-09 17:55:31]

The CQ bit was checked by dougt@chromium.org

[dougt, 2017-05-09 17:55:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2864953002/#ps40001
(title: "follow up comment")

[commit-bot: I haz the power, 2017-05-09 17:57:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 18:35:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 18:35:20]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougt, 2017-05-09 19:40:48]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2017-05-09 19:41:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 20:31:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 20:31:21]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dougt, 2017-05-10 03:09:31]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2017-05-10 03:10:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 06:04:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494385771763940,
"parent_rev": "b0d8e514b70f72945520e48b18bcc759db47caed", "commit_rev":
"6b5b4cd8b3f76c0c4ad01e69b3f137604a9a21ee"}

[commit-bot: I haz the power, 2017-05-10 06:04:21]

Description was changed from

==========
Split out the MSCOM pieces of BrowserAccessibilityWin into a seperate class.

This CL splits out all of the MSCOM stuff in BrowserAccessibilityWin into to a
new class BrowserAccessibilityComWin. This allows us to more easily to reuse
the AXPlatformNodeWin.

The way this works is that BrowserAccessibilityWin creates and owns a COM
object named |browser_accessibility_com_|.  BrowserAccessibilityWin continues
to implement BrowserAccessibility but all interactions with Windows is done
through the BrowserAccessibilityComWin object.

BrowserAccessibilityComWin is created with both a delegate for
AXPlatformNodeWin usage as well as a backpointer to BrowserAccessibilityWin so
it can call back into BrowserAccessibility. Ownership is such that:

BrowserAccessibilityWin owns one reference on BrowserAccessibilityComWin.  When
BrowserAccessibilityWin goes away, it nulls out the backpointer and delegate
pointer rendering the BrowserAccessibilityComWin potentially still alive but
basically useless.

Note that this split is similar to how MacOS is implemented.

BUG=703369
==========

to

==========
Split out the MSCOM pieces of BrowserAccessibilityWin into a seperate class.

This CL splits out all of the MSCOM stuff in BrowserAccessibilityWin into to a
new class BrowserAccessibilityComWin. This allows us to more easily to reuse
the AXPlatformNodeWin.

The way this works is that BrowserAccessibilityWin creates and owns a COM
object named |browser_accessibility_com_|.  BrowserAccessibilityWin continues
to implement BrowserAccessibility but all interactions with Windows is done
through the BrowserAccessibilityComWin object.

BrowserAccessibilityComWin is created with both a delegate for
AXPlatformNodeWin usage as well as a backpointer to BrowserAccessibilityWin so
it can call back into BrowserAccessibility. Ownership is such that:

BrowserAccessibilityWin owns one reference on BrowserAccessibilityComWin.  When
BrowserAccessibilityWin goes away, it nulls out the backpointer and delegate
pointer rendering the BrowserAccessibilityComWin potentially still alive but
basically useless.

Note that this split is similar to how MacOS is implemented.

BUG=703369

Review-Url: https://codereview.chromium.org/2864953002
Cr-Commit-Position: refs/heads/master@{#470493}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b5b4cd8b3f76c0c4ad01e69b3f1...
==========

[commit-bot: I haz the power, 2017-05-10 06:04:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6b5b4cd8b3f76c0c4ad01e69b3f1...