Closed Bug 807893 Opened 7 years ago Closed 7 years ago

Get rid of nsIWidget::BeginSecureKeyboardInput() and nsIWidget::EndSecureKeyboardInput()

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(3 files, 2 obsolete files)

Bug 394107 added the methods 5 years ago. However, other platforms don't use them. And now, nsIWidget::OnIMEFocusChange() is always called when our editable editor gets or loses focus. So, cocoa widget can switch the secure event input mode in it instead of nsIWidget::BeginSecureKeyboardInput() and nsIWidget::EndSecureKeyboardInput().
Attached patch Patch (obsolete) — Splinter Review
I'm thinking about adding automated tests for this if it's possible.
Attached patch Patch (obsolete) — Splinter Review
oops...
Attachment #677656 - Attachment is obsolete: true
roc:

Can we dispatch custom DOM event from widget? If so, the test would be simple. We can fire custom DOM events on the DOM window when the secure input mode is changed. If it's impossible, should I made a new widget event which would cause firing DOM custom event? Or, do you have better idea?
Ah, MOZ_ASSERT() in the patch could be useful for the test.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> Can we dispatch custom DOM event from widget? If so, the test would be
> simple. We can fire custom DOM events on the DOM window when the secure
> input mode is changed.

I don't think we should fire custom DOM events from widget.
These methods are only used on Mac. And current input context has the information if the focused element is password field or not. Therefore, these methods are not necessary.
Attachment #677657 - Attachment is obsolete: true
Attachment #752755 - Flags: superreview?(roc)
Attachment #752755 - Flags: review?(roc)
This patch manages the secure input mode at setting input context and changing focus.

On current trunk build, we don't exit from secure input mode if a password field has focus and we're deactivated. Then, the secure input mode is enabled on all other applications. This patch fixes this bug too.
Attachment #752758 - Flags: review?(smichaud)
Adding tests for this. However, unfortunately, the bug mentioned in the previous comment cannot test with our test framework, though.
Attachment #752760 - Flags: review?(smichaud)
Attachment #752755 - Flags: superreview?(roc)
Attachment #752755 - Flags: superreview+
Attachment #752755 - Flags: review?(roc)
Attachment #752755 - Flags: review+
Comment on attachment 752758 [details] [diff] [review]
part.2 Cocoa widget should manage secure input mode with input context at changing the context and changing focus

This basically looks fine to me.  But I'm puzzled that you have to implement NotifyIME() and SetInputContext() in nsCocoaWindow (in addition to nsChildView).  As I understand it, text input (of any kind) can only take place when an nsChildView is focused, so why pay attention to calls that focus or unfocus an nsCocoaWindow object?

Also, some of the comments need to be clarified:

+  /**
+   * EnableSecureEventInput() and DisableSecureEventInput() warp the same
+   * name APIs.  They manage count of calls in our process.
+   * API's IsSecureEventInputEnabled() returns if the mode is enabled at
+   * that time, whereas this class's IsSecureEventInputEnabled() returns
+   * if we've made secure event input mode enabled. 
+   */

This should be something like:

  EnableSecureEventInput() and DisableSecureEventInput() wrap the Carbon
  Event Manager APIs with the same names.  In addition they keep track of
  how many times we've called them (in the same process) -- unlike the
  Carbon Event Manager APIs, which only keep track of how many times they've
  been called from any and all processes.

  The Carbon Event Manager's IsSecureEventInputEnabled() returns whether
  secure event input mode is enabled (in any process).  This class's
  IsSecureEventInputEnabled() returns whether we've made any calls to
  EnableSecureEventInput() that are not (yet) offset by the calls we've
  made to DisableSecureEventInput().

+  /**
+   * EnsrueSecureEventInputDisabled() calls DisableSecureEventInput() until
+   * our call count becomes 0.
+   */

Ensrue... -> Ensure

+  NS_ASSERTION(!!sSecureEventInputCount == !!::IsSecureEventInputEnabled(),
+    "Something other process(es) has made secure event input enabled");

The comment should be something like:

  "Some other process has enabled secure event input"
Attachment #752758 - Flags: review?(smichaud) → review+
Comment on attachment 752760 [details] [diff] [review]
part.3 Add tests for secure input mode of Cocoa widget

This looks fine to me.

But of course "Not crahsed" should be "Not crashed" :-)
Attachment #752760 - Flags: review?(smichaud) → review+
You need to log in before you can comment on or make changes to this bug.