Suspend Apple Events during their processing.

Suspend Apple Events during their processing.

BUG=168168, 78283
TEST=none


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

[Avi (use Gerrit), 2013-01-03 22:31:43]

Note that this depends on the new callback mechanism in
https://codereview.chromium.org/11746019/ .

https://codereview.chromium.org/11742028/diff/1/chrome/browser/ui/cocoa/apple...
File chrome/browser/ui/cocoa/applescript/tab_applescript.mm (right):

https://codereview.chromium.org/11742028/diff/1/chrome/browser/ui/cocoa/apple...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:38:
NSAppleEventDescriptor* valueToDescriptor(const base::Value* value) {
A future CL (ETA a few days) will bring this function into its own file and unit
test it.

[Avi (use Gerrit), 2013-01-04 01:57:02]

Followup with test is at https://codereview.chromium.org/11734035 .

[Avi (use Gerrit), 2013-01-04 16:15:11]

On 2013/01/04 01:57:02, Avi wrote:
> Followup with test is at https://codereview.chromium.org/11734035 .

This followup is now folded into this CL.

[Robert Sesek, 2013-01-04 17:07:30]

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/applescript/apple_event_util.h (right):

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/applescript/apple_event_util.h:14:
NSAppleEventDescriptor* ValueToAppleEventDescriptor(const base::Value* value);
namespace chrome::mac?

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/applescript/apple_event_util.mm (right):

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/applescript/apple_event_util.mm:47: std::string
string_value;
Are these user-facing strings? If so this should probably be string16.

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/applescript/apple_event_util.mm:60: static_cast<const
base::DictionaryValue*>(value);
You should use GetAsDictionary here.

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/applescript/apple_event_util.mm:85: for (unsigned i = 0;
i < list_value->GetSize(); ++i) {
size_t

[Avi (use Gerrit), 2013-01-04 18:05:20]

here ya go

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/applescript/apple_event_util.h (right):

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/applescript/apple_event_util.h:14:
NSAppleEventDescriptor* ValueToAppleEventDescriptor(const base::Value* value);
On 2013/01/04 17:07:30, rsesek wrote:
> namespace chrome::mac?

Done.

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/applescript/apple_event_util.mm (right):

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/applescript/apple_event_util.mm:47: std::string
string_value;
On 2013/01/04 17:07:30, rsesek wrote:
> Are these user-facing strings? If so this should probably be string16.

These are strings coming back from Javascript; I don't know what they are.
Value::GetAsString returns them in UTF8. If I use string16 here, I use
SysUTF16ToNSString below and achieve zero difference.

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/applescript/apple_event_util.mm:60: static_cast<const
base::DictionaryValue*>(value);
On 2013/01/04 17:07:30, rsesek wrote:
> You should use GetAsDictionary here.

Done.

https://codereview.chromium.org/11742028/diff/9001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/applescript/apple_event_util.mm:85: for (unsigned i = 0;
i < list_value->GetSize(); ++i) {
On 2013/01/04 17:07:30, rsesek wrote:
> size_t

Done.

[Robert Sesek, 2013-01-04 19:26:42]

LGTM

https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/applescript/apple_event_util_unittest.mm (right):

https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/applescript/apple_event_util_unittest.mm:111: for (bool
b : all_bools) {
Woaaaaah

[Avi (use Gerrit), 2013-01-04 19:40:09]

https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/applescript/apple_event_util_unittest.mm (right):

https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/applescript/apple_event_util_unittest.mm:111: for (bool
b : all_bools) {
On 2013/01/04 19:26:42, rsesek wrote:
> Woaaaaah

I was originally planning on

for (bool b : { true, false })

but you can't do that without support from libstdc++ which we don't have.

[commit-bot: I haz the power, 2013-01-04 20:13:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11742028/15001

[commit-bot: I haz the power, 2013-01-04 20:13:37]

Presubmit check for 11742028-15001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.4s to calculate.

[Avi (use Gerrit), 2013-01-04 20:20:55]

Nico, can you OWNER stamp?

[Nico, 2013-01-04 20:24:45]

lgtm

https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/applescript/apple_event_util.h (right):

https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/applescript/apple_event_util.h:14: namespace chrome {
Stuff in below chrome/ doesn't usually go in a chrome namespace.

[Robert Sesek, 2013-01-04 20:31:24]

https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/applescript/apple_event_util.h (right):

https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/applescript/apple_event_util.h:14: namespace chrome {
On 2013/01/04 20:24:45, Nico wrote:
> Stuff in below chrome/ doesn't usually go in a chrome namespace.

I thought we were starting to switch to this convention.

https://code.google.com/codesearch#search/&exact_package=chromium&q=%22namesp...

[Nico, 2013-01-04 20:37:46]

On Fri, Jan 4, 2013 at 12:31 PM,  <rsesek@chromium.org> wrote:
>
>
https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
> File chrome/browser/ui/cocoa/applescript/apple_event_util.h (right):
>
>
https://codereview.chromium.org/11742028/diff/15001/chrome/browser/ui/cocoa/a...
> chrome/browser/ui/cocoa/applescript/apple_event_util.h:14: namespace
> chrome {
> On 2013/01/04 20:24:45, Nico wrote:
>>
>> Stuff in below chrome/ doesn't usually go in a chrome namespace.
>
>
> I thought we were starting to switch to this convention.
>
>
https://code.google.com/codesearch#search/&exact_package=chromium&q=%22namesp...

There were several threads on chromium-dev where people (jam / darin
or ben i think) explicitly said we weren't, but nobody reads
chromium-dev I suppose.

The last time I said this in a review (3 months ago?) there were zero
instances of this.

But meh either way.

>
> https://codereview.chromium.org/11742028/

[commit-bot: I haz the power, 2013-01-04 20:45:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11742028/15001

[commit-bot: I haz the power, 2013-01-05 02:12:54]

Change committed as 175260