Bug 31083 - EnableSecureEventInput is only called on PLATFORM(MAC)
Summary: EnableSecureEventInput is only called on PLATFORM(MAC)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Stuart Morgan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-03 14:01 PST by Stuart Morgan
Modified: 2009-11-09 11:35 PST (History)
4 users (show)

See Also:


Attachments
Enable current MAC implementation for all of DARWIN (3.82 KB, patch)
2009-11-03 14:40 PST, Stuart Morgan
mrowe: review-
Details | Formatted Diff | Diff
Enable for MAC || (CHROMIUM && DARWIN) (3.93 KB, patch)
2009-11-03 15:29 PST, Stuart Morgan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stuart Morgan 2009-11-03 14:01:06 PST
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.
Comment 1 Stuart Morgan 2009-11-03 14:40:39 PST
Created attachment 42420 [details]
Enable current MAC implementation for all of DARWIN
Comment 2 Mark Rowe (bdash) 2009-11-03 14:42:28 PST
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.
Comment 3 Stuart Morgan 2009-11-03 14:45:01 PST
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)?
Comment 4 Stuart Morgan 2009-11-03 15:29:01 PST
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.
Comment 5 Dimitri Glazkov (Google) 2009-11-09 09:54:51 PST
I think the patch is alright. I just don't like moving this to Frame.cpp.
Adding Darin, because he usually have good ideas.
Comment 6 Darin Adler 2009-11-09 10:23:53 PST
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.
Comment 7 Darin Adler 2009-11-09 10:25:13 PST
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.
Comment 8 Stuart Morgan 2009-11-09 11:22:21 PST
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.
Comment 9 WebKit Commit Bot 2009-11-09 11:35:08 PST
Comment on attachment 42429 [details]
Enable for MAC || (CHROMIUM && DARWIN)

Clearing flags on attachment: 42429

Committed r50672: <http://trac.webkit.org/changeset/50672>
Comment 10 WebKit Commit Bot 2009-11-09 11:35:12 PST
All reviewed patches have been landed.  Closing bug.