Closed Bug 1375484 Opened 2 years ago Closed 2 years ago

ScrollSelectionIntoViewEvent should be called during refresh driver tick

Categories

(Core :: Selection, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

ScrollSelectionIntoViewEvent runs at random times and flushes layout. I believe it could be called very early in refresh driver tick, before scroll event dispatching. 
Scroll event dispatch happens when WillRefresh is called
http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/layout/generic/nsGfxScrollFrame.cpp#4718
Assignee: nobody → bugs
Let's see what tryserver says about this.
In theory this should be fine, since the call was async already, but one never knows.

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/d322fb360a3306bdd3847b0c2ef486d70ea1efe5
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d322fb360a3306bdd3847b0c2ef486d70ea1efe5
remote: recorded changegroup in replication log in 0.119s
remote: View the pushlog for these changes here:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=d18869c80a5052d136b689ea1109b2eb1643326e
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d18869c80a5052d136b689ea1109b2eb1643326e
remote: recorded changegroup in replication log in 0.123s

Fixed one assertion
Attachment #8880485 - Attachment is obsolete: true
Attached patch + test fixesSplinter Review
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/3595c44cca2f398b834262c25e64c809af86e9cf
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=3595c44cca2f398b834262c25e64c809af86e9cf
Attachment #8880541 - Attachment is obsolete: true
Comment on attachment 8880602 [details] [diff] [review]
+ test fixes

I'm reusing the ScrollSelectionIntoViewEvent class here, since they are nicely revoked etc.
We may want to change AddPendingSelectionScroll to some more generic name later (I assume we may want to reuse it for IME stuff for example), but for now, I'd prefer this very specific name.
We need mPendingSelectionScrolls, because the pending scroll should be called before WillRefresh (mObservers), which ends up dispatching scroll event.

AutoTArray<, 16> is perhaps a tad big, but we may end up having several runnables there per tick, and the older ones are possibly revoked.

The MOZ_ASSERTION change is fine, since mPendingSelectionScrolls happen to be internally 

Still waiting some more test results, but doesn't look too bad.

With this patch the issue-showing-patch for bug 1373023 could be simpler. The Selection related change wouldn't be needed.
Based on profiler this behaves as expected. We flush during tick and then paint.
There is always the risk that scroll event listener causes more stuff to flush, but we can't really prevent that.
Attachment #8880602 - Flags: review?(ehsan)
Comment on attachment 8880602 [details] [diff] [review]
+ test fixes

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

::: layout/base/nsRefreshDriver.cpp
@@ +1816,5 @@
>    if (gfxPrefs::APZPeekMessages()) {
>      nsLayoutUtils::UpdateDisplayPortMarginsFromPendingMessages();
>    }
>  
> +  AutoTArray<nsCOMPtr<nsIRunnable>, 16> pendingSelectionScrolls;

Nit: do you mind making a typedef for it in the header to avoid repeating the type here and for the member?

@@ +1820,5 @@
> +  AutoTArray<nsCOMPtr<nsIRunnable>, 16> pendingSelectionScrolls;
> +  pendingSelectionScrolls.SwapElements(mPendingSelectionScrolls);
> +  for (uint32_t i = 0; i < pendingSelectionScrolls.Length(); ++i) {
> +    pendingSelectionScrolls[i]->Run();
> +  }

I think it's probably worth adding a comment here explaining that there is the risk of these listeners themselves causing more stuff to flush, but we're OK with it since there isn't anything better to do, since without such a comment the next person to read the code may think that this is an oversight...
Attachment #8880602 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #5)

> > +  AutoTArray<nsCOMPtr<nsIRunnable>, 16> pendingSelectionScrolls;
> 
> Nit: do you mind making a typedef for it in the header to avoid repeating
> the type here and for the member?
I do mind. Such typedefs just make code harder to read ;) They hide possibly useful information without not good reasaon. And here it is rather useful to know that the array elements are nsCOMPtr and not *.

> 
> @@ +1820,5 @@
> > +  AutoTArray<nsCOMPtr<nsIRunnable>, 16> pendingSelectionScrolls;
> > +  pendingSelectionScrolls.SwapElements(mPendingSelectionScrolls);
> > +  for (uint32_t i = 0; i < pendingSelectionScrolls.Length(); ++i) {
> > +    pendingSelectionScrolls[i]->Run();
> > +  }
> 
> I think it's probably worth adding a comment here explaining that there is
> the risk of these listeners themselves causing more stuff to flush,
Eh, isn't that super obvious. We don't have such comment for all the other possible callback we have.

> but
> we're OK with it since there isn't anything better to do, since without such
> a comment the next person to read the code may think that this is an
> oversight...
really? Then someone really isn't familiar with refreshdriver.
Attached patch test tweaksSplinter Review
Trying to still tweak that one failing test (can't reproduce locally).

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/d6b16daf66212404e4c50b8d7d21bd3c315f7f01
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6b16daf66212404e4c50b8d7d21bd3c315f7f01
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d1fb7eaac4
ScrollSelectionIntoViewEvent should be called during refresh driver tick, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/a3d1fb7eaac4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1461139
You need to log in before you can comment on or make changes to this bug.