Closed Bug 968647 Opened 6 years ago Closed 6 years ago

Use nsIReflowObserver and nsIScrollPositionListener for TSF instead of nsITimer

Categories

(Core :: Widget: Win32, defect)

x86
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(6 files, 8 obsolete files)

10.46 KB, patch
roc
: review-
Details | Diff | Splinter Review
13.33 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
6.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.33 KB, patch
Details | Diff | Splinter Review
16.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
17.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
nsITimer (100ms) is not good for battery since TSF uses on Metro mode.

But from bug 966157's discussion, we can use some interfaces instead.
Assignee: nobody → m_kato
Comment on attachment 8371246 [details] [diff] [review]
Part 2. TSF should use postion change notification instead of nsITimer

Could you remove the const of kPrefNameLayoutChangeInternal. And the pref from all.js?
Comment on attachment 8371245 [details] [diff] [review]
Part 1. Add postion change notification for IME

>+    // Add scroll position listener and reflow observer to detect position and
>+    // size changes
>+    if (mPresShell) {
>+      nsIScrollableFrame* scrollableFrame =
>+        mPresShell->GetRootScrollFrameAsScrollable();
>+      if (scrollableFrame) {
>+        scrollableFrame->AddScrollPositionListener(this);
>+      }
>+    }

Isn't this only listening the root scrollable frame's position? I.e., isn't scroll of nested scrollable frames including the editor itself ignored?

>   enum
>   {
>-    NOTIFY_NOTHING           = 0x00,
>-    NOTIFY_SELECTION_CHANGE  = 0x01,
>-    NOTIFY_TEXT_CHANGE       = 0x02,
>-    NOTIFY_DURING_DEACTIVE   = 0x80
>+    NOTIFY_NOTHING           = 0,
>+    NOTIFY_SELECTION_CHANGE  = 1 << 0,
>+    NOTIFY_TEXT_CHANGE       = 1 << 1,
>+    NOTIFY_POSITION_CHANGE   = 1 << 2,
>+    NOTIFY_DURING_DEACTIVE   = 1 << 4
>   };

Could you set max bit value to NOTIFY_DURING_DEACTIVE? Conditions shouldn't be mixed in *_CHANGE constants in the future.
Attachment #8371245 - Attachment is obsolete: true
Part 1's patch may require nsIDocumentObserver too...  But parent element is changed, IME focus is lost.  So listeners is removed.
Attachment #8374686 - Attachment is obsolete: true
Part 1 isn't reviewable yet.  When changing parent, composition string is cancel, but parent tree cannot traverse...
Since bug 965685 changes NotifyIME(), I will wait until fixing that bug...
Attachment #8374682 - Attachment is obsolete: true
Comment on attachment 8377947 [details] [diff] [review]
Part 1. Add postion change notification for IME

For IME candidate window management, it requires that we need detect window position change, reflow and scroll position.

TSF cannot detect it, so it uses nsITimer with 100ms.  It isn't good for power saving since TSF is always used by Metro version.
Attachment #8377947 - Flags: review?(roc)
Comment on attachment 8377948 [details] [diff] [review]
Part 2. TSF should use postion change notification instead of nsITimer

for TSF implementation to remove nsITimer
Attachment #8377948 - Flags: review?(masayuki)
Comment on attachment 8377948 [details] [diff] [review]
Part 2. TSF should use postion change notification instead of nsITimer

Looks good to me only this part.

There is a point which I don't like.

nsWindow::OnWindowPosChanged() directly call NotifyIME() internally. Why don't you call it from nsIWidgetListener::WindowMoved() and inherited methods (nsWebShellWindow::WindowMoved() and nsView::WindowMoved()) calls the super method? Doesn't it work in XP level? If it works, I'd like you to add new patch for it and request review to roc.

Additionally, we might need to check mLock in nsTextStore::OnLayoutChangeInternal() but it's okay for this bug since the current code doesn't check it.

Additionally, I think that nsIMM32Handler and nsGtkIMModule should handle the new notification in new bugs.
Attachment #8377948 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> Comment on attachment 8377948 [details] [diff] [review]
> Part 2. TSF should use postion change notification instead of nsITimer
> 
> Looks good to me only this part.
> 
> There is a point which I don't like.
> 
> nsWindow::OnWindowPosChanged() directly call NotifyIME() internally. Why
> don't you call it from nsIWidgetListener::WindowMoved() and inherited
> methods (nsWebShellWindow::WindowMoved() and nsView::WindowMoved()) calls
> the super method? Doesn't it work in XP level? If it works, I'd like you to
> add new patch for it and request review to roc.

Widget cannot handle multiple nsIWidgetListenr, so listener cannot be stacked up widget.  For all platform level, we implement another way or modify all this listener code and handing.

> Additionally, we might need to check mLock in
> nsTextStore::OnLayoutChangeInternal() but it's okay for this bug since the
> current code doesn't check it.

OK.

> Additionally, I think that nsIMM32Handler and nsGtkIMModule should handle
> the new notification in new bugs.

Yes.  I will file it
(In reply to Makoto Kato (:m_kato) from comment #17)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> > Comment on attachment 8377948 [details] [diff] [review]
> > Part 2. TSF should use postion change notification instead of nsITimer
> > 
> > Looks good to me only this part.
> > 
> > There is a point which I don't like.
> > 
> > nsWindow::OnWindowPosChanged() directly call NotifyIME() internally. Why
> > don't you call it from nsIWidgetListener::WindowMoved() and inherited
> > methods (nsWebShellWindow::WindowMoved() and nsView::WindowMoved()) calls
> > the super method? Doesn't it work in XP level? If it works, I'd like you to
> > add new patch for it and request review to roc.
> 
> Widget cannot handle multiple nsIWidgetListenr, so listener cannot be
> stacked up widget.  For all platform level, we implement another way or
> modify all this listener code and handing.

I think that nsBaseWidget should have a method such as OnWindowMoved() which is called from every inherited classes. Anyway, it may not be in the scope of this bug.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> (In reply to Makoto Kato (:m_kato) from comment #17)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> > > Comment on attachment 8377948 [details] [diff] [review]
> > > Part 2. TSF should use postion change notification instead of nsITimer
> > > 
> > > Looks good to me only this part.
> > > 
> > > There is a point which I don't like.
> > > 
> > > nsWindow::OnWindowPosChanged() directly call NotifyIME() internally. Why
> > > don't you call it from nsIWidgetListener::WindowMoved() and inherited
> > > methods (nsWebShellWindow::WindowMoved() and nsView::WindowMoved()) calls
> > > the super method? Doesn't it work in XP level? If it works, I'd like you to
> > > add new patch for it and request review to roc.
> > 
> > Widget cannot handle multiple nsIWidgetListenr, so listener cannot be
> > stacked up widget.  For all platform level, we implement another way or
> > modify all this listener code and handing.
> 
> I think that nsBaseWidget should have a method such as OnWindowMoved() which
> is called from every inherited classes. Anyway, it may not be in the scope
> of this bug.

OK. I will file a followed issue after this.
Comment on attachment 8378920 [details] [diff] [review]
Part 1.1 - Notice window position change

masayuki suggests moved window listener to XP code by comment #17.
Attachment #8378920 - Flags: review?(roc)
Comment on attachment 8377947 [details] [diff] [review]
Part 1. Add postion change notification for IME

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

::: dom/events/nsIMEStateManager.cpp
@@ +93,5 @@
>  private:
>    void NotifyContentAdded(nsINode* aContainer, int32_t aStart, int32_t aEnd);
>    void ObserveEditableNode();
>  
> +  nsRefPtr<nsIPresShell>         mPresShell;

You probably don't want to hang onto mPresShell since it could be destroyed and recreated if we're in an <iframe> which toggles between "display:none" and not-none.

You probably just want to hang onto the document and get the presshell when necessary.

A weak reference to the presshell you added a listener on, just for removing the listener, may be needed, however.

@@ +800,5 @@
> +        nsIScrollableFrame* scrollableFrame = do_QueryFrame(f);
> +        if (scrollableFrame) {
> +          scrollableFrame->AddScrollPositionListener(this);
> +        }
> +        f = nsLayoutUtils::GetCrossDocParentFrame(f);

This isn't going to work properly when frame ancestors of 'f' are recreated due to style changes.

We probably need a way to get notified when any scrolling happens in the presshell. I think we should add AddWeakScrollingObserver to docshell. That should be fairly easy.
Attachment #8377947 - Flags: review?(roc) → review-
I merged the part.1 patch for my tree. If you have not rebased it yet, use this.
wait for bug 961704.
Attachment #8381885 - Attachment is obsolete: true
Comment on attachment 8382099 [details] [diff] [review]
Part 1. Add postion change notification for IME v2

adding nsIScrollObserver for weak reference.
Attachment #8382099 - Flags: review?(roc)
Comment on attachment 8382099 [details] [diff] [review]
Part 1. Add postion change notification for IME v2

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

Mostly looks good.

::: docshell/base/nsIDocShell.idl
@@ +680,5 @@
> +   */
> +  [noscript] void removeWeakScrollObserver(in nsIScrollObserver obs);
> +
> +  /**
> +   * Notify all attached observer that scroll postion some element is changed

"Notify all attached observers that the scroll position of some element has changed".

::: docshell/base/nsIScrollObserver.idl
@@ +9,5 @@
> +{
> +  /**
> +   * Called when some elements is changed. 
> +   */
> +  void scrollPositionChanged();

This should just be a C++ class, not an XPCOM interface.
Attachment #8382099 - Flags: review?(roc) → review-
Attachment #8382099 - Attachment is obsolete: true
Attachment #8382858 - Flags: review?(roc)
(In reply to Carsten Book [:Tomcat] from comment #32)
> sorry had to backout this changes for
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35407228&tree=Mozilla-
> Inbound mochitest bustage on OS X

OS X and all platforms
Humm, passed on try server as same changeset.
https://tbpl.mozilla.org/?tree=Try&rev=94e3e9d85777

CLOBBBER?
I test on broken binary on m-i, function table seems to be broken.  I will reland it.
CLOBBER commit comment will no longer work. You will have to touch CLOBBER file.
https://mxr.mozilla.org/mozilla-central/source/CLOBBER
(In reply to Masatoshi Kimura [:emk] from comment #37)
> CLOBBER commit comment will no longer work. You will have to touch CLOBBER
> file.
> https://mxr.mozilla.org/mozilla-central/source/CLOBBER

thanks.  I found problem so this is backed out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/753007aca755
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a48bf9197d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/492ce94e6528
forget updating uuid of nsIDocShell.
Attachment #8384393 - Flags: review?(roc)
Blocks: 979148
Depends on: 982298
You need to log in before you can comment on or make changes to this bug.