Bug 31083

Summary: EnableSecureEventInput is only called on PLATFORM(MAC)
Product: WebKit Reporter: Stuart Morgan <stuartmorgan>
Component: WebCore Misc.Assignee: Stuart Morgan <stuartmorgan>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dglazkov, mrowe
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Enable current MAC implementation for all of DARWIN
mrowe: review-
Enable for MAC || (CHROMIUM && DARWIN) none

Stuart Morgan
Reported 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.
Attachments
Enable current MAC implementation for all of DARWIN (3.82 KB, patch)
2009-11-03 14:40 PST, Stuart Morgan
mrowe: review-
Enable for MAC || (CHROMIUM && DARWIN) (3.93 KB, patch)
2009-11-03 15:29 PST, Stuart Morgan
no flags
Stuart Morgan
Comment 1 2009-11-03 14:40:39 PST
Created attachment 42420 [details] Enable current MAC implementation for all of DARWIN
Mark Rowe (bdash)
Comment 2 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.
Stuart Morgan
Comment 3 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)?
Stuart Morgan
Comment 4 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.
Dimitri Glazkov (Google)
Comment 5 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.
Darin Adler
Comment 6 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.
Darin Adler
Comment 7 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.
Stuart Morgan
Comment 8 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.
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2009-11-09 11:35:12 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.