Closed Bug 1243268 Opened 4 years ago Closed 4 years ago

Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process

Categories

(Core :: Plug-ins, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

Details

Attachments

(3 files, 1 obsolete file)

3rd party plugin vendor requires it...
Comment on attachment 8713058 [details] [diff] [review]
Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process

Upcoming Flash 21 will use CFS_EXCLUDE for ImmSetCandidateWindow.  So we need support it by API hook on plugin process.
Attachment #8713058 - Flags: review?(masayuki)
(In reply to Makoto Kato [:m_kato] from comment #2)
> Upcoming Flash 21 will use CFS_EXCLUDE for ImmSetCandidateWindow.  So we
> need support it by API hook on plugin process.

Oh, really? IIRC, Google Japanese Input doesn't support CFS_EXCLUDE... I'll check it since I need to file it to issues of them.
Comment on attachment 8713058 [details] [diff] [review]
Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process

Okay, nsIMM32Handler already uses CFS_EXCLUDE for calling ImmSetCandidateWindow() in horizontal writing mode. So, it must not be no problem flash to use CFS_EXCLUDE.

>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
>--- a/dom/ipc/PBrowser.ipdl
>+++ b/dom/ipc/PBrowser.ipdl
>@@ -279,17 +279,20 @@ parent:
>      *
>      * aFocused  Whether or not a plugin is focused
>      */
>     prio(urgent) async SetPluginFocused(bool aFocused);
> 
>     /**
>      * Set IME candidate window by windowless plugin if plugin has focus.
>      */
>-    async SetCandidateWindowForPlugin(int32_t aX, int32_t aY);
>+    async SetCandidateWindowForPlugin(int32_t aX, int32_t aY,
>+                                      bool aExclude,
>+                                      int32_t aLeft, int32_t aTop,
>+                                      int32_t aRight, int32_t aBottom);

Hmm, too many arguments... How about to create a struct in IMEData.h? (and making it IPC-aware in nsGUIEventsIPC.h)

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -1830,17 +1830,23 @@ public:
>     bool PluginHasFocus() 
>     {
>       return GetInputContext().mIMEState.mEnabled == IMEState::PLUGIN;
>     }
> 
>     /**
>      * Set IME candidate window position by windowless plugin.
>      */
>-    virtual void SetCandidateWindowForPlugin(int32_t aX, int32_t aY) = 0;
>+    virtual void SetCandidateWindowForPlugin(int32_t aX,
>+                                             int32_t aY,
>+                                             bool aExclude,
>+                                             int32_t aLeft,
>+                                             int32_t aTop,
>+                                             int32_t aRight,
>+                                             int32_t aBottom) = 0;

Don't you need to update the UUID?

Otherwise, looks good to me.


I'm thinking the struct is:

namespace mozilla {
namespace widget {

struct CandidateWindowPosition
{
  // Upper left corner of the candidate window if mExcludeRect is false.
  // Otherwise, the position currently interested.  E.g., caret position.
  LayoutDeviceIntPoint mPoint;
  // Rect which shouldn't be overlapped with the candidate window.
  // This is valid only when mExcludeRect is true.
  LayoutDeviceIntRect mRect;
  // See explanation of mPoint and mRect.
  bool mExcludeRect;
};

} // namespace widget
} // namespace mozilla

How about you?
FYI: document of CANDIDATEFORM structure: https://msdn.microsoft.com/en-us/library/windows/desktop/dd317738%28v=vs.85%29.aspx
Flags: needinfo?(m_kato)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #5)
> Comment on attachment 8713058 [details] [diff] [review]
> Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
...
> Don't you need to update the UUID?

Ehsan has posted to dev-platform for unnecessary UUID change.  We should still change it for nsIWidget too?


> I'm thinking the struct is:
> 
> namespace mozilla {
> namespace widget {
> 
> struct CandidateWindowPosition
> {
>   // Upper left corner of the candidate window if mExcludeRect is false.
>   // Otherwise, the position currently interested.  E.g., caret position.
>   LayoutDeviceIntPoint mPoint;
>   // Rect which shouldn't be overlapped with the candidate window.
>   // This is valid only when mExcludeRect is true.
>   LayoutDeviceIntRect mRect;
>   // See explanation of mPoint and mRect.
>   bool mExcludeRect;
> };
> 
> } // namespace widget
> } // namespace mozilla
> 
> How about you?
> FYI: document of CANDIDATEFORM structure:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd317738%28v=vs.
> 85%29.aspx

I will update this using your idea
Flags: needinfo?(m_kato)
Attachment #8713058 - Flags: review?(masayuki)
(In reply to Makoto Kato [:m_kato] from comment #6)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #5)
> > Comment on attachment 8713058 [details] [diff] [review]
> > Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
> ...
> > Don't you need to update the UUID?
> 
> Ehsan has posted to dev-platform for unnecessary UUID change.  We should
> still change it for nsIWidget too?

Got it, thanks. You don't need to change that.
Comment on attachment 8714235 [details] [diff] [review]
Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process

Use single parameter for SetCandidateWindow IPC
Attachment #8714235 - Flags: review?(masayuki)
Comment on attachment 8714236 [details] [diff] [review]
Part 2. Adjust ATOK workaround

Always set caret if ATOK
Attachment #8714236 - Flags: review?(masayuki)
Comment on attachment 8714235 [details] [diff] [review]
Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process

># HG changeset patch
># User Makoto Kato <m_kato@ga2.so-net.ne.jp>
># Parent  4d0e60e0a2b3bdb7ec5f66d4733cc0df898273da
>Bug 1243268 - Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
>
>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
>+using mozilla::widget::CandidateWindowPosition from "ipc/nsGUIEventIPC.h";

from "widget/IMEData.h"?
Attachment #8714235 - Flags: review?(masayuki) → review+
Comment on attachment 8714236 [details] [diff] [review]
Part 2. Adjust ATOK workaround

Did you test this with ATOK actually? If not, I'll check it.

r+ if you've already tested.
Attachment #8714236 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #12)
> Comment on attachment 8714235 [details] [diff] [review]
> Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
> 
> ># HG changeset patch
> ># User Makoto Kato <m_kato@ga2.so-net.ne.jp>
> ># Parent  4d0e60e0a2b3bdb7ec5f66d4733cc0df898273da
> >Bug 1243268 - Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
> >
> >diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
> >+using mozilla::widget::CandidateWindowPosition from "ipc/nsGUIEventIPC.h";
> 
> from "widget/IMEData.h"?

ParamTraits is defined on nsGUIEventIPC.h.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #13)
> Did you test this with ATOK actually? If not, I'll check it.
> 
> r+ if you've already tested.

I tested it on ATOK 2015.
(In reply to Makoto Kato [:m_kato] from comment #14)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #12)
> > Comment on attachment 8714235 [details] [diff] [review]
> > Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
> > 
> > ># HG changeset patch
> > ># User Makoto Kato <m_kato@ga2.so-net.ne.jp>
> > ># Parent  4d0e60e0a2b3bdb7ec5f66d4733cc0df898273da
> > >Bug 1243268 - Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
> > >
> > >diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
> > >+using mozilla::widget::CandidateWindowPosition from "ipc/nsGUIEventIPC.h";
> > 
> > from "widget/IMEData.h"?
> 
> ParamTraits is defined on nsGUIEventIPC.h.

Then, widget/nsGUIEventIPC.h?

> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #13)
> > Did you test this with ATOK actually? If not, I'll check it.
> > 
> > r+ if you've already tested.
> 
> I tested it on ATOK 2015.

Okay, fine!
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #15)
> (In reply to Makoto Kato [:m_kato] from comment #14)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #12)
> > > Comment on attachment 8714235 [details] [diff] [review]
> > > Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
> > > 
> > > ># HG changeset patch
> > > ># User Makoto Kato <m_kato@ga2.so-net.ne.jp>
> > > ># Parent  4d0e60e0a2b3bdb7ec5f66d4733cc0df898273da
> > > >Bug 1243268 - Support ImmSetCandidateWindow(CFS_EXCLUDE) on plugin process
> > > >
> > > >diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
> > > >+using mozilla::widget::CandidateWindowPosition from "ipc/nsGUIEventIPC.h";
> > > 
> > > from "widget/IMEData.h"?
> > 
> > ParamTraits is defined on nsGUIEventIPC.h.
> 
> Then, widget/nsGUIEventIPC.h?

all is the following.

using class mozilla::ContentCache from "ipc/nsGUIEventIPC.h";
using class mozilla::WidgetKeyboardEvent from "ipc/nsGUIEventIPC.h";
using class mozilla::WidgetMouseEvent from "ipc/nsGUIEventIPC.h";
using class mozilla::WidgetWheelEvent from "ipc/nsGUIEventIPC.h";
using class mozilla::WidgetDragEvent from "ipc/nsGUIEventIPC.h";
using struct nsRect from "nsRect.h";
Sigh... I think that you should keep current your code. And file a bug :-(
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #17)
> Sigh... I think that you should keep current your code. And file a bug :-(

Due to EXPORTS.ipc = ['nsGUIEventIPC.h'] on moz.build, ipc/nsGUIEventIPC.h correct.  Should we move it to widget/nsGUIEventIPC.h?
https://hg.mozilla.org/mozilla-central/rev/661ff05ff47a
https://hg.mozilla.org/mozilla-central/rev/b436fe906468
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.