Fix lifetime problems in AXPlatformNodeCocoa.

Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

Previously committed in r484048. Reverted due to compile errors against
more recent SDKs. Now fixed.

BUG=734939
Review-Url: https://codereview.chromium.org/2944103005
Cr-Commit-Position: refs/heads/master@{#484477}
Committed: https://chromium.googlesource.com/chromium/src/+/98c0539b58c22054eb032bde7df0648846f9acbc

[tapted, 2017-06-20 08:13:02]

Description was changed from

==========
Fix lifetime problems in AXPlatformNodeCocoa


Lifetime test


cl format

(cherry picked from commit a7c54d56e97657274d2a712ea06d8b88473d9557)

BUG=
==========

to

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

BUG=734939
==========

[tapted, 2017-06-20 08:13:02]

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

[tapted, 2017-06-20 08:29:50]

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

[commit-bot: I haz the power, 2017-06-20 08:29:57]

Dry run: CQ is trying da patch.

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

[tapted, 2017-06-20 08:40:36]

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

[commit-bot: I haz the power, 2017-06-20 08:40:49]

Dry run: 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-06-20 09:48:30]

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

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

Dry run: This issue passed the CQ dry run.

[tapted, 2017-06-20 10:51:10]

Hi Dominic, please take a look

[dmazzoni, 2017-06-20 17:47:16]

lgtm

https://codereview.chromium.org/2944103005/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2944103005/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:283: if (!node_)
Isn't this an internal helper method? It seems like it
shouldn't be necessary to check at the top of every method,
only at the top of public APIs (below where it says
"NSAccessibility informal protocol implementation".

[tapted, 2017-06-20 23:28:42]

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

[commit-bot: I haz the power, 2017-06-20 23:29:04]

Dry run: CQ is trying da patch.

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

[tapted, 2017-06-20 23:30:52]

Thanks Dominic!

Did you see the other CLs? (they're not too big :) -
 - https://codereview.chromium.org/2944083004
 - https://codereview.chromium.org/2946783003

(this CL is based on those to get coverage using NSAccessibilityShowMenuAction)

https://codereview.chromium.org/2944103005/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2944103005/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:283: if (!node_)
On 2017/06/20 17:47:16, dmazzoni wrote:
> Isn't this an internal helper method? It seems like it
> shouldn't be necessary to check at the top of every method,
> only at the top of public APIs (below where it says
> "NSAccessibility informal protocol implementation".

Removed - I think this was just left over from earlier iterations.

[commit-bot: I haz the power, 2017-06-20 23:46:16]

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

[commit-bot: I haz the power, 2017-06-20 23:46:17]

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

[tapted, 2017-07-04 01:49:00]

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

[commit-bot: I haz the power, 2017-07-04 01:49:14]

Dry run: 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-07-04 02:13:53]

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

[commit-bot: I haz the power, 2017-07-04 02:13:54]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-07-04 03:23:41]

(rebased to master now that https://codereview.chromium.org/2944083004 landed)

https://codereview.chromium.org/2944103005/diff/80001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2944103005/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:172: EXPECT_EQ(@[],
[ax_node accessibilityParameterizedAttributeNames]);
Since r482502 there now are parameterized attributes \o/. So this expectedly
started failing. Updated with the same checks we do for non-paramaterized
attributes (using NSAccessibilityStringForRangeParameterizedAttribute)

[tapted, 2017-07-04 03:24:22]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-07-04 03:24:22]

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/2944103005/#ps100001
(title: "Can now test paramaterized attributes")

[commit-bot: I haz the power, 2017-07-04 03:24:34]

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-07-04 03:31:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499138662021910,
"parent_rev": "0da1b4712dd42d51b2e38cf1031b73d7cb12ea5f", "commit_rev":
"bb756436f55c01c2066c17d5eec5aaeb1e52a120"}

[commit-bot: I haz the power, 2017-07-04 03:31:26]

Description was changed from

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

BUG=734939
==========

to

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

BUG=734939

Review-Url: https://codereview.chromium.org/2944103005
Cr-Commit-Position: refs/heads/master@{#484048}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb756436f55c01c2066c17d5eec5...
==========

[commit-bot: I haz the power, 2017-07-04 03:31:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/bb756436f55c01c2066c17d5eec5...

[Avi (use Gerrit), 2017-07-05 18:11:16]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2969293002/ by avi@chromium.org.

The reason for reverting is: Compile issues with modern SDK:

../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:176:34: error:
null passed to a callee that requires a non-null argument [-Werror,-Wnonnull]
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
                                 ^                         ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1925:29: note:
expanded from macro 'EXPECT_EQ'
                      val1, val2)
                            ^~~~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:40:
note: expanded from macro 'EXPECT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
                                       ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:43:
note: expanded from macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                                          ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:77:52:
note: expanded from macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:217:34: error:
null passed to a callee that requires a non-null argument [-Werror,-Wnonnull]
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
                                 ^                         ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1925:29: note:
expanded from macro 'EXPECT_EQ'
                      val1, val2)
                            ^~~~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:40:
note: expanded from macro 'EXPECT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
                                       ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:43:
note: expanded from macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                                          ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:77:52:
note: expanded from macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
In file included from
../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:14:
In file included from ../../testing/gtest_mac.h:8:
In file included from ../../testing/gtest/include/gtest/gtest.h:10:
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1392:11:
error: comparison of integers of different signs: 'const long' and 'const
unsigned long' [-Werror,-Wsign-compare]
  if (lhs == rhs) {
      ~~~ ^  ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1421:12: note:
in instantiation of function template specialization
'testing::internal::CmpHelperEQ<long, unsigned long>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:176:3: note:
in instantiation of function template specialization
'testing::internal::EqHelper<false>::Compare<long, unsigned long>' requested
here
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
  ^
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1924:63: note:
expanded from macro 'EXPECT_EQ'
                      EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \
                                                              ^
3 errors generated.
.

[tapted, 2017-07-06 03:57:05]

Description was changed from

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

BUG=734939

Review-Url: https://codereview.chromium.org/2944103005
Cr-Commit-Position: refs/heads/master@{#484048}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb756436f55c01c2066c17d5eec5...
==========

to

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

BUG=734939

Review-Url: https://codereview.chromium.org/2944103005
Cr-Commit-Position: refs/heads/master@{#484048}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb756436f55c01c2066c17d5eec5...
==========

[tapted, 2017-07-06 03:58:25]

Description was changed from

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

BUG=734939

Review-Url: https://codereview.chromium.org/2944103005
Cr-Commit-Position: refs/heads/master@{#484048}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb756436f55c01c2066c17d5eec5...
==========

to

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

Previously committed in r484048. Reverted due to compile errors against
more recent SDKs. Now fixed.

BUG=734939
==========

[tapted, 2017-07-06 03:59:54]

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

[commit-bot: I haz the power, 2017-07-06 04:00:03]

Dry run: 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-07-06 04:20:36]

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

[commit-bot: I haz the power, 2017-07-06 04:20:37]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-07-06 06:01:35]

(the compile fixes are pretty uninteresting - happy to follow up on any concerns
you have .. weird that Apple are still updating the signatures for these
interfaces even though the NSAccessibility informal protocol has been deprecated
since 10.10..)

[tapted, 2017-07-06 06:01:44]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-07-06 06:01:45]

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/2944103005/#ps120001
(title: "-tFix modern SDK warnings")

[commit-bot: I haz the power, 2017-07-06 06:01:52]

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-07-06 06:05:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499320904892580,
"parent_rev": "b4d8ef261ff7621c5b74a8331fc4b07e4ce23653", "commit_rev":
"98c0539b58c22054eb032bde7df0648846f9acbc"}

[commit-bot: I haz the power, 2017-07-06 06:05:36]

Description was changed from

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

Previously committed in r484048. Reverted due to compile errors against
more recent SDKs. Now fixed.

BUG=734939
==========

to

==========
Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

Previously committed in r484048. Reverted due to compile errors against
more recent SDKs. Now fixed.

BUG=734939

Review-Url: https://codereview.chromium.org/2944103005
Cr-Commit-Position: refs/heads/master@{#484477}
Committed:
https://chromium.googlesource.com/chromium/src/+/98c0539b58c22054eb032bde7df0...
==========

[commit-bot: I haz the power, 2017-07-06 06:05:37]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/98c0539b58c22054eb032bde7df0...