Bug 97161 - Mac Chromium: Ignore system numpad modifier
Summary: Mac Chromium: Ignore system numpad modifier
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sailesh Agrawal
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-19 18:52 PDT by Sailesh Agrawal
Modified: 2012-10-08 15:07 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.47 KB, patch)
2012-09-19 18:54 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch (5.22 KB, patch)
2012-10-04 11:07 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch (17.42 KB, patch)
2012-10-08 13:13 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch for landing (6.74 KB, patch)
2012-10-08 14:28 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sailesh Agrawal 2012-09-19 18:52:47 PDT
Mac Chromium: Ignore system numpad modifier
Comment 1 Sailesh Agrawal 2012-09-19 18:54:53 PDT
Created attachment 164821 [details]
Patch
Comment 2 Sailesh Agrawal 2012-09-19 19:05:56 PDT
This is a fix for http://crbug.com/145108
The flash app is getting confused because of the numpad modifier attached to the arrow keys.

NPAPI Flash handles this by ignoring the system numpad modifier.

We could do the same in PPAPI Flash or we could do it in WebKit.

Please take a look and let me know what you think.

Thanks,
Sailesh
Comment 3 Nico Weber 2012-09-19 19:10:30 PDT
Comment on attachment 164821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164821&action=review

> Source/WebKit/chromium/src/mac/WebInputEventFactory.mm:-139
> -        return true;

Won't this change how keyboard events are delivered to js key handlers?

If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
Comment 4 Sailesh Agrawal 2012-09-19 19:20:25 PDT
> Won't this change how keyboard events are delivered to js key handlers?
Yea it will.

> If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.

The only downside is that we'll diverge from Mac Safari. I think that's ok though.
Comment 5 Viet-Trung Luu 2012-09-19 19:37:17 PDT
(In reply to comment #4)
> > Won't this change how keyboard events are delivered to js key handlers?
> Yea it will.
> 
> > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.

With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.

> The only downside is that we'll diverge from Mac Safari. I think that's ok though.
Comment 6 Nico Weber 2012-09-19 20:02:16 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > > Won't this change how keyboard events are delivered to js key handlers?
> > Yea it will.
> > 
> > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> 
> With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.

On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.

Is doing this change in flash much harder?
Comment 7 Sailesh Agrawal 2012-09-19 21:33:59 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > > Won't this change how keyboard events are delivered to js key handlers?
> > > Yea it will.
> > > 
> > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > 
> > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> 
> On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> 
> Is doing this change in flash much harder?

The flash change is already out for review. I wanted to have a discussion about doing this on the WebKit side first.

Also, if we decide not to do this change in WebKit then should the change be in Flash or Pepper?
Comment 8 Nico Weber 2012-09-19 22:26:12 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > Yea it will.
> > > > 
> > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > 
> > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > 
> > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> > 
> > Is doing this change in flash much harder?
> 
> The flash change is already out for review. I wanted to have a discussion about doing this on the WebKit side first.
> 
> Also, if we decide not to do this change in WebKit then should the change be in Flash or Pepper?

I don't care too much. Do you expect to have any pepper plugins besides flash? I guess the pdf viewer? Does it make a difference for these plugins? Does the npapi plugin path do it, or is it done in flash itself for npapi flash? Probably doesn't matter too much either way I think.
Comment 9 Viet-Trung Luu 2012-09-19 22:34:17 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > > Won't this change how keyboard events are delivered to js key handlers?
> > > Yea it will.
> > > 
> > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > 
> > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> 
> On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.

This is an argument for everyone continuing to do UA sniffing?

By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.

> 
> Is doing this change in flash much harder?

No, but it's dumb to.

Then every NaCl game would have to UA sniff.

I guess we could do it in Pepper.

But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.
Comment 10 Viet-Trung Luu 2012-09-19 22:39:35 PDT
Also, fixing this for everyone in WebKit almost certainly only makes things better, and probably doesn't hurt anyone. What are the possibilities?

- you never look at the flag => no effect for you
- you look at the flag and but don't UA/platform sniff => you're probably broken on Mac (or on Windows, which is less likely)
- you look at the flag and do UA/platform sniff => you're probably ignoring the flag for arrow keys on Mac => no effect

(In reply to comment #9)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > Yea it will.
> > > > 
> > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > 
> > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > 
> > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> 
> This is an argument for everyone continuing to do UA sniffing?
> 
> By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.
> 
> > 
> > Is doing this change in flash much harder?
> 
> No, but it's dumb to.
> 
> Then every NaCl game would have to UA sniff.
> 
> I guess we could do it in Pepper.
> 
> But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.
Comment 11 Nico Weber 2012-09-19 23:01:52 PDT
(In reply to comment #9)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > Yea it will.
> > > > 
> > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > 
> > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > 
> > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> 
> This is an argument for everyone continuing to do UA sniffing?
> 
> By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.

[citation needed]

> > Is doing this change in flash much harder?
> 
> No, but it's dumb to.
> 
> Then every NaCl game would have to UA sniff.
> 
> I guess we could do it in Pepper.
> 
> But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.

"Adobe have reported a minor inconsistency in flapper. Let's change how keyboard events on the web are handled!" doesn't sound all that coherent to me.

If you want to change js key events, that should be done independent of random flash bugs, and in safari and chrome at the same time. We decided to be safari-compatible in our keyboard handling on mac, and so far that's worked well I think.
Comment 12 Viet-Trung Luu 2012-09-19 23:22:18 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > > Yea it will.
> > > > > 
> > > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > > 
> > > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > > 
> > > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> > 
> > This is an argument for everyone continuing to do UA sniffing?
> > 
> > By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.
> 
> [citation needed]
> 
> > > Is doing this change in flash much harder?
> > 
> > No, but it's dumb to.
> > 
> > Then every NaCl game would have to UA sniff.
> > 
> > I guess we could do it in Pepper.
> > 
> > But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.
> 
> "Adobe have reported a minor inconsistency in flapper. Let's change how keyboard events on the web are handled!" doesn't sound all that coherent to me.

No, it just shows that no one has ever paid close attention to web key events.

> 
> If you want to change js key events, that should be done independent of random flash bugs, and in safari and chrome at the same time. We decided to be safari-compatible in our keyboard handling on mac, and so far that's worked well I think.

Do you think Safari doesn't have bugs?!?!?!?

The consequence of not having consistent events is:
- WEB content won't work properly on Mac (more so in markets where Macs are less common); this will become a more frequent problem as people try to do more using JS ... unless they code SPECIFICALLY for Mac
- NaCl content won't work on Mac, again unless they code SPECIFICALLY for Mac
- Flash content won't work on Mac

You're confusing the symptom with the underlying illness. We get the event wrong in Flash because we get the event wrong in Pepper because we get the event wrong in WebKit.

Yes, we could trivially fix it in Flash, but that's dumb. It forces everyone else, at EVERY OTHER LEVEL to similarly detect that they're on a Mac, and fix the issue for themselves. The real consequence is that many people writing content on Windows will produce content that's broken on Mac, and vice versa.
Comment 13 Viet-Trung Luu 2012-09-19 23:22:57 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > (In reply to comment #4)
> > > > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > > > Yea it will.
> > > > > > 
> > > > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > > > 
> > > > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > > > 
> > > > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> > > 
> > > This is an argument for everyone continuing to do UA sniffing?
> > > 
> > > By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.
> > 
> > [citation needed]

I suppose you think Macs produce Windows virtual key codes naturally.

> > 
> > > > Is doing this change in flash much harder?
> > > 
> > > No, but it's dumb to.
> > > 
> > > Then every NaCl game would have to UA sniff.
> > > 
> > > I guess we could do it in Pepper.
> > > 
> > > But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.
> > 
> > "Adobe have reported a minor inconsistency in flapper. Let's change how keyboard events on the web are handled!" doesn't sound all that coherent to me.
> 
> No, it just shows that no one has ever paid close attention to web key events.
> 
> > 
> > If you want to change js key events, that should be done independent of random flash bugs, and in safari and chrome at the same time. We decided to be safari-compatible in our keyboard handling on mac, and so far that's worked well I think.
> 
> Do you think Safari doesn't have bugs?!?!?!?
> 
> The consequence of not having consistent events is:
> - WEB content won't work properly on Mac (more so in markets where Macs are less common); this will become a more frequent problem as people try to do more using JS ... unless they code SPECIFICALLY for Mac
> - NaCl content won't work on Mac, again unless they code SPECIFICALLY for Mac
> - Flash content won't work on Mac
> 
> You're confusing the symptom with the underlying illness. We get the event wrong in Flash because we get the event wrong in Pepper because we get the event wrong in WebKit.
> 
> Yes, we could trivially fix it in Flash, but that's dumb. It forces everyone else, at EVERY OTHER LEVEL to similarly detect that they're on a Mac, and fix the issue for themselves. The real consequence is that many people writing content on Windows will produce content that's broken on Mac, and vice versa.
Comment 14 Nico Weber 2012-09-19 23:39:22 PDT
Kinuko, Trung points out that you added this bit of code in http://trac.webkit.org/changeset/52608/trunk/WebCore/platform/mac/KeyEventMac.mm . Do you remember why it was necessary?
Comment 15 Viet-Trung Luu 2012-09-19 23:48:21 PDT
Also, the current Safari does not have this bug.
Comment 16 Sailesh Agrawal 2012-09-20 09:59:57 PDT
(In reply to comment #15)
> Also, the current Safari does not have this bug.

Safari on 10.6 doesn't have this bug either, weird!
Their code still have the numpad modifier check though:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/mac/PlatformEventFactoryMac.mm#L293
Maybe the modifier gets attached to the javascript event elsewhere.

Anyways, fixing this bug in WebKit seems like a clear choice now.
Comment 17 Alexey Proskuryakov 2012-09-20 11:24:44 PDT
Please always post all relevant information about a bug to Bugzilla. Collecting bits of information from multiple bug trackers is not fun for anyone who hasn't been on the bug from the start.

(In reply to comment #2)
> This is a fix for http://crbug.com/145108
> The flash app is getting confused because of the numpad modifier attached to the arrow keys.

URL/test case :http://www.nick.com/games/teenage-mutant-ninja-turtles-sewer-run.html

Steps:
1. let game load, then choose the following: Play ->Story -> Skip -> Leo(left most choice) -> Play
2. after character is controllable press 'z' (character should lunge forward)
3. hold down an arrow key and hit z(lunge won't occur)

Expected: Should be able to hold arrow keys and press z to lunge forward at the same time. 

Observed:Multiple button presses while holding an arrow key, that IS NOT another arrow key won't be registered within the game. You must release the arrow key to use other buttons.
Comment 18 Sailesh Agrawal 2012-10-04 11:07:25 PDT
Created attachment 167135 [details]
Patch
Comment 19 Sailesh Agrawal 2012-10-04 11:11:20 PDT
Replying to Kinuko:

> The reason I added the change is to distinguish key location (if it's from NUMPAD or not) for arrow keys.
(The issue link in the ChangeLog seems to be wrong)
https://bugs.webkit.org/show_bug.cgi?id=32602

Yea, your original patch looks good. My CL will simply suppress that behavior for arrow keys. Mac keyboards don't have arrow keys on the number pad so this should be ok.

> I don't remember the details well but I'm afraid your change may affect the layout test (fast/events/keydown-numpad-keys.html) and how we used to report KeyLocation information.  Did you see if the patched version still reports the key location correctly?

Yep it does.
I've also made a chromium-mac version of your expectations file with the num pad modifier removed from the arrow key events.
Comment 20 Sailesh Agrawal 2012-10-04 11:13:04 PDT
Hi Tony, would you be a good person to review this? Thanks
Comment 21 Eric Seidel (no email) 2012-10-04 11:16:25 PDT
Comment on attachment 167135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167135&action=review

> LayoutTests/ChangeLog:10
> +        * platform/chromium-mac/fast/events/keydown-numpad-keys-expected.txt: Added.

I'm confused why we have our own set of expecations here?  These look identical to the ones in fast/events.  platform/mac doesn't have expecxtations.  Did you try running webkit-patch optimize-baselines?
Comment 22 Sailesh Agrawal 2012-10-04 11:30:02 PDT
(In reply to comment #21)
> (From update of attachment 167135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167135&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        * platform/chromium-mac/fast/events/keydown-numpad-keys-expected.txt: Added.
> 
> I'm confused why we have our own set of expecations here?  These look identical to the ones in fast/events.

The only difference is that I removed the numpad modifier for keycodes 37 to 40 (arrow keys).

> platform/mac doesn't have expecxtations.

This is skipped for mac-wk2/Skipped.
I'm not sure about mac though. I tested Safari and the behavior matches my patch.

> Did you try running webkit-patch optimize-baselines?

I haven't seen this before. Looking into it now.

Thanks,
Sailesh
Comment 23 Tony Chang 2012-10-04 12:03:51 PDT
It sounds like the discussion is saying that Safari has the same bug as Chromium Mac.  Can we fix both?  Also, what does Firefox Mac do?
Comment 24 Sailesh Agrawal 2012-10-04 12:13:49 PDT
(In reply to comment #23)
> It sounds like the discussion is saying that Safari has the same bug as Chromium Mac.  Can we fix both?  Also, what does Firefox Mac do?

Mac Safari does not have this bug.
Pressing arrow keys never sets the number pad modifier.
Comment 25 Tony Chang 2012-10-04 13:07:41 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > It sounds like the discussion is saying that Safari has the same bug as Chromium Mac.  Can we fix both?  Also, what does Firefox Mac do?
> 
> Mac Safari does not have this bug.
> Pressing arrow keys never sets the number pad modifier.

Why is the Chromium Mac expected result for keydown-numpad-keys.html different from the Apple Mac result (or from the Chromium Windows result)?
Comment 26 Sailesh Agrawal 2012-10-08 13:13:38 PDT
Created attachment 167594 [details]
Patch
Comment 27 Sailesh Agrawal 2012-10-08 13:29:27 PDT
> Why is the Chromium Mac expected result for keydown-numpad-keys.html different from the Apple Mac result (or from the Chromium Windows result)?

Hi Tony, here's the summary of what's going on:
  #1 A normal keyboard has arrow keys on the normal keyboard and on the number pad.
  #2 Mac keyboards have arrows keys only on the normal keyboard.
  #3 Mac Safari (correctly) does not attach number pad modifier when sending arrow keys to Javascript/DOM.
  #4 Mac Chromium (incorrectly) does attach number pad modifier when sending arrow keys to Javascript/DOM.
  #5 Windows Chromium (correctly) does not attach number pad modifier for normal arrow keys and does attach it for number pad arrow keys.

At this point I fixed #4 by changing WebInputEventFactory::keyboardEvent().

There's one complication though. Layout  tests behave completely differently. Layout tests don't use WebInputEventFactory so we get a completely different behavior:
  #1 Mac Safari (incorrectly) attaches num pad modifier when sending arrow keys.
  #2 Mac Chromium (incorrectly) attaches num pad modifier when sending arrow keys.

This patch fixes #2 by changing EventSender::keyDown() to strip number pad modifier for arrow keys.

I'm going to file a separate bug for #1. The fix for #1 involves changing PlatformEventFactoryMac.mm/isKeypadEvent() and changing the relevant test.
Comment 28 Tony Chang 2012-10-08 14:07:06 PDT
Comment on attachment 167594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167594&action=review

It sounds like we can't actually test this behavior in a layout test because the event created by eventSender doesn't go through the port specific WebKit code.  I would just revert the changes to DRT and the layout test.  The unittest you added seems sufficient to test the change to Chromium's WebKit code.  Trying to make DRT smarter just makes the layout test more confusing.

> Source/WebKit/chromium/WebKit.gypi:136
> +                    'tests/WebInputEventFactoryTestMac.mm',

Nit: I think you can put this in the main webkit_unittest_files list and it'll get removed on Win/Linux because of the .mm extension.

> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:43
> +using WebKit::WebInputEvent;
> +using WebKit::WebKeyboardEvent;
> +using WebKit::WebMouseEvent;
> +using WebKit::WebInputEventFactory;

Nit: Sort.

> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:62
> +}  // namespace

Nit: WebKit style is only one space before // comments.
Comment 29 Sailesh Agrawal 2012-10-08 14:28:01 PDT
Created attachment 167609 [details]
Patch for landing
Comment 30 Sailesh Agrawal 2012-10-08 14:29:34 PDT
Comment on attachment 167594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167594&action=review

>> Source/WebKit/chromium/WebKit.gypi:136
>> +                    'tests/WebInputEventFactoryTestMac.mm',
> 
> Nit: I think you can put this in the main webkit_unittest_files list and it'll get removed on Win/Linux because of the .mm extension.

Done.

>> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:43
>> +using WebKit::WebInputEventFactory;
> 
> Nit: Sort.

Done.

>> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:62
>> +}  // namespace
> 
> Nit: WebKit style is only one space before // comments.

Done.
Comment 31 WebKit Review Bot 2012-10-08 15:07:17 PDT
Comment on attachment 167609 [details]
Patch for landing

Clearing flags on attachment: 167609

Committed r130691: <http://trac.webkit.org/changeset/130691>
Comment 32 WebKit Review Bot 2012-10-08 15:07:23 PDT
All reviewed patches have been landed.  Closing bug.