Closed Bug 961703 Opened 11 years ago Closed 11 years ago

[TSF] Avoid making TIP confused by odd behavior

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86_64
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(1 file, 2 obsolete files)

Patch
(deleted), patch
m_kato
: review+
Details | Diff | Splinter Review 
If our TSF implementation behaves strange, TIP is confused. And after that, TIP may not work well. For avoiding making TIP confused, we should create some walls at:

1. sending text change notification if it occurs during flushing pending actions or composition.
2. sending selection change notification if it occurs during flushing pending actions or composition.
3. destroying text store but it has composition.
Attached patch Patch (obsolete) (deleted) — — Splinter Review 
Test case for text change:
> data:text/html,<body contenteditable>Here is editable.</body><script>setInterval(function () { document.body.innerHTML += ", updated!"; }, 1000);</script>

Test case for selection change:
> data:text/html,<body contenteditable>Here is editable.</body><script>setInterval(function () { window.getSelection().selectAllChildren(document.body); window.getSelection().collapseToStart(); }, 1000);</script>
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #8362833 - Flags: review?(m_kato)
Comment on attachment 8362833 [details] [diff] [review]
Patch

Sorry for the spam, this is older patch.
Attachment #8362833 - Flags: review?(m_kato)
Attached patch Patch (obsolete) (deleted) — — Splinter Review 

        Attachment #8362833 -
        Attachment is obsolete: true

        Attachment #8362834 -
        Flags: review?(m_kato)

    
    

    
  
Comment on attachment 8362834 [details] [diff] [review]
Patch

Review of attachment 8362834 [details] [diff] [review]:
-----------------------------------------------------------------

good, but I have a question for OnTextChange.

::: widget/windows/nsTextStore.cpp
@@ +3129,5 @@
>             ("TSF: 0x%p   nsTextStore::OnTextChangeMsg(), calling"
>              "mSink->OnTextChange(0, { acpStart=%ld, acpOldEnd=%ld, "
>              "acpNewEnd=%ld })...", this, mTextChange.acpStart,
>              mTextChange.acpOldEnd, mTextChange.acpNewEnd));
>      mSink->OnTextChange(0, &mTextChange);

Before calling ITextStoreACPSink::OnTextChange, don't we check IsReadLocked()?

OnTextChangeMsg is from message loop even if OnTextChangeInternal() is called without locked state, so OnTextChangeMsg() will be called with locked state.  If this is always called with unlocked state.  please ignore this comment.

    
    

    
  
s/OnTextChangeMsg() will be called with locked state./OnTextChangeMsg() may be called with locked state.../

    
    

    
  
True. However, I'm not sure what we should do it at that case. Discarding the notification? Repost the message?

But it's a hack of initial implementation. At that time, nsTextStateManager notifies widget of text change synchronously during mutation. If it dispatches events during mutation, it causes bugs.

However, now, nsTextStateManager uses runnable event for notifying widget of text change.
http://mxr.mozilla.org/mozilla-central/source/dom/events/nsIMEStateManager.cpp#864

So, I guess that we don't need the hack anymore. I'll check it today and update the patch if we can remove it.

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> True. However, I'm not sure what we should do it at that case. Discarding
> the notification? Repost the message?

Actually, current is discarding this now since IsReadLocked() is true. If reposting the message, we need more test for all TIPs.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (deleted)
        — 
          — Splinter Review
      

    


  
This patch depends on the patch of bug 544779.

        Attachment #8362834 -
        Attachment is obsolete: true

        Attachment #8362834 -
        Flags: review?(m_kato)

        Attachment #8364145 -
        Flags: review?(m_kato)

    
    

    
  
(In reply to Makoto Kato (:m_kato) from comment #7)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > True. However, I'm not sure what we should do it at that case. Discarding
> > the notification? Repost the message?
> 
> Actually, current is discarding this now since IsReadLocked() is true. If
> reposting the message, we need more test for all TIPs.

If it causes some bugs, let's fix them in other new bugs.

        Attachment #8364145 -
        Flags: review?(m_kato) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c2e11417e175
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: