Bug 31083 – EnableSecureEventInput is only called on PLATFORM(MAC)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Stuart Morgan]

Secure input mode isn't being enabled in Chromium on the Mac; it turns out the code to do so is currently in FrameMac.cpp, rather than being enabled for all PLATFORM(DARWIN) builds.

Patch to follow shortly.

[Stuart Morgan]

Created attachment 42420 [details]
Enable current MAC implementation for all of DARWIN

[Mark Rowe (bdash)]

Comment on attachment 42420 [details]
Enable current MAC implementation for all of DARWIN

Carbon is above the Darwin level so it is not right wrap Carbon-specific code in a check for Darwin.

[Stuart Morgan]

Ah, I didn't realize that PLATFORM(DARWIN) was meant that literally. So is there a define that does the right thing, or does each port need to opt in individually (I guess MAC || CHROMIUM for now)?

[Stuart Morgan]

Created attachment 42429 [details]
Enable for MAC || (CHROMIUM && DARWIN)

Word on the street is that there's no good macro for this. This seems ugly, but it's apparently the best option; cloning the implementation to FrameChromium.cpp certainly seems worse.

Suggestions for better options are more than welcome.

[Dimitri Glazkov (Google)]

I think the patch is alright. I just don't like moving this to Frame.cpp.
Adding Darin, because he usually have good ideas.

[Darin Adler]

Comment on attachment 42429 [details]
Enable for MAC || (CHROMIUM && DARWIN)

Clearly all the ports that don't compile on Mac OS X don't want this code. Since Mac is taken to mean the native Mac WebKit, we may just need a filename suffix that means anything running on the Mac platform, which could include Qt or GTK or Chromium when compiling on Mac OS X.

One of the cleanest ways to refactor this would be to move the code into WebCore/platform and just call it from Frame or SelectionController, since it's definitely a bunch of "talking to the underlying platform" code just inline there because it was written *so* long ago.

Side note: If we are touching this anyway, it would be good to move it to SelectionController. As you can see in Frame.h, this is supposed to be moved out of the Frame class.

[Darin Adler]

Comment on attachment 42429 [details]
Enable for MAC || (CHROMIUM && DARWIN)

Despite my suggestions for how to do this better, I'll say r=me

This is a step in the wrong direction, though. A file like Frame needs to be small and not have a lot of platform-specific gunk in it. So lets keep working on this, and not just stop at what it takes to fix this Chromium issue.

[Stuart Morgan]

I've filed bug 31265 for finding this code a good home, and will start working on that; in the meantime we'll land this so Chromium doesn't have this hole during the process.

[WebKit Commit Bot]

Comment on attachment 42429 [details]
Enable for MAC || (CHROMIUM && DARWIN)

Clearing flags on attachment: 42429

Committed r50672: <http://trac.webkit.org/changeset/50672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.