Fix BrowserAccessibilityManagerMac: set empty selected text range.

Fix BrowserAccessibilityManagerMac: set empty selected text range.

Raises an NSInvalidArgumentException if call setObject for NSMutableDictionary
with a nil value. Dictionary uses NSNull instead of nil.
Selected text marker range may be nil sometimes
(see BrowserAccessibilityCocoa::selectedTextMarkerRange), that leads to a crashe
in BrowserAccessibilityManagerMac::GetUserInfoForSelectedTextChangedNotification().

R=aboxhall@chromium.org, dmazzoni@chromium.org, dtseng@chromium.org
BUG=

Committed: https://crrev.com/70d267e4de6002e24be82eddbb99337ab9e37362
Cr-Commit-Position: refs/heads/master@{#407194}

[a-a-l, 2016-07-18 14:48:17]

Raises an NSInvalidArgumentException if call setObject for NSMutableDictionary
with a nil value. Dictionary uses NSNull instead of nil.
Selected text marker range may be nil sometimes
(see BrowserAccessibilityCocoa::selectedTextMarkerRange), that leads to a crashe
in
BrowserAccessibilityManagerMac::GetUserInfoForSelectedTextChangedNotification().

[nektarios, 2016-07-18 15:39:35]

nektar@chromium.org changed reviewers:
+ mark@chromium.org, nektar@chromium.org

[nektarios, 2016-07-18 15:40:34]

I was the original author of this code and mark@chromium one of the reviewers.

[nektarios, 2016-07-18 15:44:37]

I would prefer if the key was not set in the dictionary if selection comes back
as nil. This is what generally happens with dictionaries in accessibility APIs.
If you have something put it in, otherwise don't set the key at all.

[a-a-l, 2016-07-19 14:36:51]

On 2016/07/18 15:44:37, nektarios wrote:
> I would prefer if the key was not set in the dictionary if selection comes
back
> as nil. This is what generally happens with dictionaries in accessibility
APIs.
> If you have something put it in, otherwise don't set the key at all.

I've changed according to your recommendations.  Please take another look.

[nektarios, 2016-07-19 14:59:50]

I don't think we need to check if the object responds to selector.
I believe that we don't need to compare to nil but simply do:
if (native_focus_object && native_focus_object->instanceActive() &&
[native_focus_object selectedTextRange]) {
...
}
I wouldn't worry about saving results in local variables. Compilers should be
good at optimizing this kind of behavior.
You might not even need to call instanceActive since selectedTextRange already
does that check.

[a-a-l, 2016-07-21 15:21:53]

On 2016/07/19 14:59:50, nektarios wrote:
> I don't think we need to check if the object responds to selector.
> I believe that we don't need to compare to nil but simply do:
> if (native_focus_object && native_focus_object->instanceActive() &&
> [native_focus_object selectedTextRange]) {
> ...
> }
> I wouldn't worry about saving results in local variables. Compilers should be
> good at optimizing this kind of behavior.
> You might not even need to call instanceActive since selectedTextRange already
> does that check.

Thank you for your comments.
1. I'll remove check if the object responds to selector.
2. I mean that check instanceActive need for a NSAccessibilityTextChangeElement.
Isn't it?
3. 
if (native_focus_object && native_focus_object->instanceActive() &&
[native_focus_object selectedTextRange]) {
...
}

Do you really mean using [native_focus_object selectedTextRange] in the check
(perhaps it is a type and you meant selectedTextMarkerRange)? In
src/content/browser/accessibility/browser_accessibility_cocoa.mm you marked this
code as deprecated.

[chromium-reviews, 2016-07-21 15:33:44]

Let's leave it as it is. Just remove the respondsToSelector and put 
braces around the following:
       if (selected_text != nil) {
         [user_info setObject:selected_text
forKey:NSAccessibilitySelectedTextMarkerRangeAttribute];
}

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nektarios, 2016-07-21 15:38:55]

Also, you don't need to say != nil simply do:
      if (selected_text) {...}

The stylistic rule says that if the condition or the body spans more than one
line, or if there is an else clause, braces should be used.
Tip: Use "git cl format" to automatically run format checks on your code.

[AGBondareva, 2016-07-22 15:00:43]

On 2016/07/21 15:33:44, chromium-reviews wrote:
> Let's leave it as it is. Just remove the respondsToSelector and put 
> braces around the following:
>        if (selected_text != nil) {
>          [user_info setObject:selected_text
> forKey:NSAccessibilitySelectedTextMarkerRangeAttribute];
> }
ok.
Please take another look.

[AGBondareva, 2016-07-22 15:00:59]

agbondareva@gmail.com changed reviewers:
+ AGBondareva@gmail.com

[nektarios, 2016-07-22 15:58:31]

nektar@chromium.org changed reviewers:
+ agbondareva@gmail.com
- AGBondareva@gmail.com

[nektarios, 2016-07-22 15:58:31]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-07-22 15:58:32]

lgtm

[commit-bot: I haz the power, 2016-07-22 15:58:48]

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, 2016-07-22 16:04:27]

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

[commit-bot: I haz the power, 2016-07-22 16:04:28]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2016-07-22 17:43:34]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-07-22 17:43:35]

lgtm

[commit-bot: I haz the power, 2016-07-22 17:44:07]

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, 2016-07-22 17:47:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-22 17:49:24]

Description was changed from

==========
Fix BrowserAccessibilityManagerMac: set empty selected text range.

Raises an NSInvalidArgumentException if call setObject for NSMutableDictionary
with a nil value. Dictionary uses NSNull instead of nil.
Selected text marker range may be nil sometimes
(see BrowserAccessibilityCocoa::selectedTextMarkerRange), that leads to a crashe
in
BrowserAccessibilityManagerMac::GetUserInfoForSelectedTextChangedNotification().

R=aboxhall@chromium.org, dmazzoni@chromium.org, dtseng@chromium.org
BUG=
==========

to

==========
Fix BrowserAccessibilityManagerMac: set empty selected text range.

Raises an NSInvalidArgumentException if call setObject for NSMutableDictionary
with a nil value. Dictionary uses NSNull instead of nil.
Selected text marker range may be nil sometimes
(see BrowserAccessibilityCocoa::selectedTextMarkerRange), that leads to a crashe
in
BrowserAccessibilityManagerMac::GetUserInfoForSelectedTextChangedNotification().

R=aboxhall@chromium.org, dmazzoni@chromium.org, dtseng@chromium.org
BUG=

Committed: https://crrev.com/70d267e4de6002e24be82eddbb99337ab9e37362
Cr-Commit-Position: refs/heads/master@{#407194}
==========

[commit-bot: I haz the power, 2016-07-22 17:49:25]

Patchset 3 (id:??) landed as
https://crrev.com/70d267e4de6002e24be82eddbb99337ab9e37362
Cr-Commit-Position: refs/heads/master@{#407194}