Closed Bug 532130 Opened 12 years ago Closed 12 years ago

[IMM32] Support IMR_DOCUMENTFEED of WM_IME_REQUEST

Categories

(Core :: Widget: Win32, enhancement)

x86
Windows 7
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(1 file, 7 obsolete files)

https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-try-f1bc0ae98456/

test builds here.
# c8329307431e failed to build the win32 build by tryserver problem.
Comment on attachment 415415 [details] [diff] [review]
Patch v1.0

+        selectionLength -=
+          mCompositionString.Length() - (selectionOffset - mCompositionStart);
Will selectionLength never become negative?

+    } else if (selectionLength > 0) {
+      // If selection starts before composition string but the end is in the
+      // composition string, shrink the selection.
+      selectionLength -= mCompositionStart - selectionOffset;
+    }
Is selectionOffset + selectionLength always larger than mCompositionStart?

+  if (!pReconv) {
+    PRUint32 len = paragraph.Length();
+    *oResult = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR);
:
+  PRUint32 len = paragraph.Length();
+  PRUint32 needSize = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR);
Place this two lines above the if statement to avoid unnecessary duplication.

> +  DWORD tmpSize = pReconv->dwSize;
> +  ::ZeroMemory(pReconv, tmpSize);
> +  pReconv->dwSize            = tmpSize;
* Are you sure pReconv->dwSize is set by caller? Both of two sample codes from comment #0 don't preserve dwSize.
* Is it required to clear the buffer in the first place? All structure members are explicitely set below.
* ::ZeroMemory(reinterpret_cast<LPVOID>(lParam + sizeof(pReconv->dwSize)),
               pReconv->dwSize - sizeof(pReconv->dwSize));
  even if it's required.

> +  pReconv->dwTargetStrLen    = selectionLength * sizeof(WCHAR);
Per MSDN, length should be character counts.
http://msdn.microsoft.com/en-us/library/dd319107%28VS.85%29.aspx
Attachment #415415 - Flags: review?(VYV03354) → review-
(In reply to comment #3)
> (From update of attachment 415415 [details] [diff] [review])
> +        selectionLength -=
> +          mCompositionString.Length() - (selectionOffset - mCompositionStart);
> Will selectionLength never become negative?

Yes. The selectionOffset is:

selectionOffset > mCompositionStart &&
selectionOffset <= mCompositionStart + mCompositionString.Length()

> +    } else if (selectionLength > 0) {
> +      // If selection starts before composition string but the end is in the
> +      // composition string, shrink the selection.
> +      selectionLength -= mCompositionStart - selectionOffset;
> +    }
> Is selectionOffset + selectionLength always larger than mCompositionStart?

Ah, maybe the condition of the block is wrong.

> > +  DWORD tmpSize = pReconv->dwSize;
> > +  ::ZeroMemory(pReconv, tmpSize);
> > +  pReconv->dwSize            = tmpSize;
> * Are you sure pReconv->dwSize is set by caller? Both of two sample codes from
> comment #0 don't preserve dwSize.

I think it should be so because the caller can limit the buffer size. We should ignore such cases.

> > +  pReconv->dwTargetStrLen    = selectionLength * sizeof(WCHAR);
> Per MSDN, length should be character counts.
> http://msdn.microsoft.com/en-us/library/dd319107%28VS.85%29.aspx

Thanks!
> * Is it required to clear the buffer in the first place? All structure members
> are explicitely set below.

Yes, the caller can send a fixed length buffer always. So, dwSize can be larger than the needed size. We need to clear the all buffer in such case.
(In reply to comment #4)
> > +        selectionLength -=
> > +          mCompositionString.Length() - (selectionOffset - mCompositionStart);
> > Will selectionLength never become negative?
> 
> Yes. The selectionOffset is:
> 
> selectionOffset > mCompositionStart &&
> selectionOffset <= mCompositionStart + mCompositionString.Length()

Is the following condition also guaranteed?
selectionOffset + selectionLength >= mCompositionStart + mCompositionString.Length()

(In reply to comment #5)
> > * Is it required to clear the buffer in the first place? All structure members
> > are explicitely set below.
> 
> Yes, the caller can send a fixed length buffer always. So, dwSize can be larger
> than the needed size. We need to clear the all buffer in such case.

Why the unused buffer needs to be cleared? I think it's harmless to leave the unused garbage if it is not referred by offset/len pairs.
(In reply to comment #6)
> (In reply to comment #4)
> > > +        selectionLength -=
> > > +          mCompositionString.Length() - (selectionOffset - mCompositionStart);
> > > Will selectionLength never become negative?
> > 
> > Yes. The selectionOffset is:
> > 
> > selectionOffset > mCompositionStart &&
> > selectionOffset <= mCompositionStart + mCompositionString.Length()
> 
> Is the following condition also guaranteed?
> selectionOffset + selectionLength >= mCompositionStart +
> mCompositionString.Length()

Yes, because there is previous if statement.
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > > +        selectionLength -=
> > > > +          mCompositionString.Length() - (selectionOffset - mCompositionStart);
> > > > Will selectionLength never become negative?
> > > 
> > > Yes. The selectionOffset is:
> > > 
> > > selectionOffset > mCompositionStart &&
> > > selectionOffset <= mCompositionStart + mCompositionString.Length()
> > 
> > Is the following condition also guaranteed?
> > selectionOffset + selectionLength >= mCompositionStart +
> > mCompositionString.Length()
> 
> Yes, because there is previous if statement.

Er, I misread your comment, please wait a moment.
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #415415 - Attachment is obsolete: true
Attachment #416222 - Flags: review?(VYV03354)
Comment on attachment 416222 [details] [diff] [review]
Patch v1.1

er, there is mistake.
Attachment #416222 - Flags: review?(VYV03354) → review-
Attached patch Patch v1.2 (obsolete) — Splinter Review
I hope this is correct...
Attachment #416222 - Attachment is obsolete: true
Attachment #416226 - Flags: review?(VYV03354)
Comment on attachment 416226 [details] [diff] [review]
Patch v1.2

Um... Something was broken.
Attachment #416226 - Flags: review?(VYV03354) → review-
You will need to consider the following five cases.
1. mCompositionStart + mCompositionString.Length() < selectionOffset
2. mCompositionStart <= selectionOffset && selectionOffset < mCompositionStart + mCompositionString.Length()
2.1. mCompositionStart + mCompositionString.Length() < selectionOffset + selectionLength
2.2. selectionOffset + selectionLength <= mCompositionStart + mCompositionString.Length()
3. selectionOffset < mCompositionStart
3.1. mCompositionStart < selectionOffset + selectionLength
3.2. selectionOffset + selectionLength <= mCompositionStart

> +  PRUint32 needSize = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR);
> +  *oResult = needSize;
> +
> +  if (!pReconv) {
Don't set the return value here because the method can fail after that.
Attached patch Patch v1.3 (obsolete) — Splinter Review
ok, this is simple.

We don't need to cut the composition string if we set the dwCompStrLen and dwCompStrOffset correctly.

And I changed another point. If the selection covers two or more paragraphs, this patch returns all paragraphs which include the selection range.
Attachment #416226 - Attachment is obsolete: true
Attachment #416386 - Flags: review?(VYV03354)
Comment on attachment 416386 [details] [diff] [review]
Patch v1.3

> +  if (mIsComposing && ShouldDrawCompositionStringOurselves()) {
> +    pReconv->dwCompStrLen      = mCompositionString.Length();
> +    pReconv->dwCompStrOffset   =
> +      (mCompositionStart - paragraphStart) * sizeof(WCHAR);
Is (mCompositionStart - paragraphStart) always positive? I heard that WXG could have line separators in the composition string. Could you pass all contents of the editor instead of only a paragraph?

> +    // Set composition string information to the target string.
> +    pReconv->dwTargetStrLen    = pReconv->dwCompStrLen;
> +    pReconv->dwTargetStrOffset = pReconv->dwCompStrOffset;
Is this always equal to the entire composition string? I thought it means "対象文節".
Attached patch Patch v1.4 (obsolete) — Splinter Review
How about this?

This patch has two modes:
1. When it's not in composition, or it's in composition but we don't manage composition string, we use the selection range.
2. Otherwise, ignores the selection. Instead, uses the composition string range.

This patch defines that the current paragraph starts from the next character of the previous LF of the target offset and ends at the previous character of the next CR of the target end offset.
Attachment #416386 - Attachment is obsolete: true
Attachment #416870 - Flags: review?(VYV03354)
Attachment #416386 - Flags: review?(VYV03354)
Attached patch Patch v1.4.1 (obsolete) — Splinter Review
Uses nsDependentSubstring for the paragraph. That doesn't allocate/copy the memory.
Attachment #416870 - Attachment is obsolete: true
Attachment #416894 - Flags: review?(VYV03354)
Attachment #416870 - Flags: review?(VYV03354)
Attachment #416894 - Flags: review?(VYV03354) → review+
http://hg.mozilla.org/mozilla-central/rev/d029f744960d

Thank you, Kimura-san.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
oops, backed-out the patch because I found a bug in the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v1.5 (obsolete) — Splinter Review
Sorry for the re-review.

In the previous patch, nsIMM32Handler::GetTargetClauseRange doesn't return correct length. See the difference between this patch and the previous one.
Attachment #416894 - Attachment is obsolete: true
Attachment #417645 - Flags: review?(VYV03354)
Comment on attachment 417645 [details] [diff] [review]
Patch v1.5

No, this is still buggy.
Attachment #417645 - Flags: review?(VYV03354) → review-
Attached patch Patch v1.6Splinter Review
> +  if (hasCompositionString) {
> +    pReconv->dwCompStrLen      = targetLength;
> +    pReconv->dwCompStrOffset   =
> +      (targetOffset - paragraphStart) * sizeof(WCHAR);
> +    // Set composition target clause information
> +    PRUint32 offset, length;
> +    if (!GetTargetClauseRange(&offset, &length)) {
> +      PR_LOG(gIMM32Log, PR_LOG_ALWAYS,
> +        ("IMM32: HandleDocumentFeed, FAILED, by GetTargetClauseRange\n"));
> +      return PR_FALSE;
> +    }
> +    pReconv->dwTargetStrLen    = offset - mCompositionStart;
> +    pReconv->dwTargetStrOffset = length;

Here are three bugs:
1. Sets to wrong variable (offset -> len and len -> offset).
2. The offset should be byte size (needs |* sizeof(WCHAR)|)
3. The offset should be subtracted with paragraphStart, not mCompositionStart.

In nsIMM32Handler::GetTargetClauseRange:

>   if (!found) {
>     // The all composition string is targetted when there is no ATTR_TARGET_*
>     // clause. E.g., there is only ATTR_INPUT
>     *aLength = mCompositionString.Length();
>     return PR_TRUE;
>   }

I added this. When there are no ATTR_TARGET_*, we should just return the length of composition string, not 0.
Attachment #417645 - Attachment is obsolete: true
Attachment #417649 - Flags: review?(VYV03354)
Comment on attachment 417649 [details] [diff] [review]
Patch v1.6

Sorry to overlook the bugs.
Off-topic: Google IME didn't use context even if native TSF support was enabled.
Attachment #417649 - Flags: review?(VYV03354) → review+
http://hg.mozilla.org/mozilla-central/rev/49e75469c968
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.