868648 - new scroll bars should be shown temporarily on two-finger hold

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Cameron McCormack (:heycam)]

In other applications, doing a two-finger tap on a scrollable area will temporarily show the scroll bars.  We should do that too.

Comment 1

[Jesse Ruderman]

On my Mac, only a two-finger *hold* makes scrollbars appear.  A quick two-finger tap makes a context menu appear instead.

Comment 2

[Cameron McCormack (:heycam)]

Do you have "Tap to click" enabled?  There might be a difference when that is set (which I don't).

Comment 3

[Jesse Ruderman]

Yes, I have both "tap to click" and "two-finger tap/click to right-click" enabled.

Comment 4

[Markus Stange [:mstange]]

Attachment: demo app for scroll phase debugging

André, I think we can observe fingers-on-trackpad state just by handling the events that arrive in -[ChildView scrollWheel:]. Here's a small demo app that prints [event phase] and [event momentumPhase], and I can see events coming through when I two-finger-tap the window.

Comment 5

[André Reinald]

(In reply to Markus Stange from comment #4)

Thanks Markus! By reading Apple's doc, I could more or less figure out a few ways of detecting the 2 finger touch. I think yours is the right way as it detects changing the number of fingers on the trackpad. I would have tried intercepting begin/endgesture events, but it would probably not have detected this.

Actually my main concern is I don't know how to pass those events to the right place in the code, which is likely in a window object. Bug 412486 looks like a good starting point for me to understand event handling in gecko, so that's where I asked for suggestions (in comment 100 of that bug).

Comment 6

[André Reinald]

Attachment: Part 1: generate events when 2 fingers touch/release trackpad

Tried to follow the path of patch for bug 412486. Is this the right way?

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 754489 [details] [diff] [review]
Part 1: generate events when 2 fingers touch/release trackpad

This looks great!

Comment 8

[Markus Stange [:mstange]]

The processing of these events should probably happen in nsEventStateManager::PostHandleEvent. You could, for example, add methods ScrollbarActivityStarted and ScrollbarActivityStopped to nsIScrollbarOwner (which would call the right methods on the scroll frame's ScrollbarActivity) and call them from the event state manager.

Comment 9

[André Reinald]

If we want to mimic the behavior of other applications, the show-scrollbars-on-two-finger-down only applies to the enclosing window, not to inner scroll views.

Should we stick to this, or should we follow our own logic, which to me makes more sense, and show the scrollbars that will actually be moved by the gesture?

Comment 10

[Markus Stange [:mstange]]

(In reply to André Reinald from comment #9)
> Should we stick to this, or should we follow our own logic, which to me
> makes more sense, and show the scrollbars that will actually be moved by the
> gesture?

I think this is what we should do. But it's not going to be perfect, assuming we keep the current behavior, because it's possible that a different scrollbox is selected depending on the scroll direction. For example, say we have an outer scrollbox and an inner scrollbox, and the outer scrollbox is scrolled to the middle (i.e. scrollable both up and down), and the inner scrollbox is scrolled to the top (i.e. can only be scrolled down). In that case, scrolling down in the inner box will select the inner box, but scrolling up will select the outer box. The scroll direction is not known yet when the fingers are placed on the trackpad, so we might show the wrong scrollbars.

Masayuki, do you have an opinion on how this case should be handled?

Comment 11

[André Reinald]

(In reply to Markus Stange from comment #10)
> 
> ...
> scrolled down). In that case, scrolling down in the inner box will select
> the inner box, but scrolling up will select the outer box. The scroll
> direction is not known yet when the fingers are placed on the trackpad, so
> we might show the wrong scrollbars.

You are right.

I see no sensible way to decide which scrollbars have to be shown in this situation until the user actaully scrolls one way or another. So it seems Apple's solution, even if not perfect, is still the best way to go. Anyway, I still have things to understand and code whatever behavior we end up choosing.

Meanwhile I assigned bug 868659 to myself because it's so closely related. Would it make sense to mark it as duplicate of this one?

Comment 12

[Stephen A Pohl [:spohl]]

(In reply to André Reinald from comment #11)
> Meanwhile I assigned bug 868659 to myself because it's so closely related.
> Would it make sense to mark it as duplicate of this one?

Since the reporter is the same for these two bugs I believe this was deliberate. And although very similar, the bugs describe two different problems. You'd probably want to keep both bugs open to make it easier to track. It'll also allow you to check in a patch for one or the other bug should one of them take a bit longer to fix.

Comment 13

[Jesse Ruderman]

I'd mark it as a dependency (assuming this patch fixes both issues) rather than a duplicate.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Sorry for the long delay.

It's very difficult issue. I think that the purpose of showing scrollbars is telling which area is scrollable and its scrolled position. With this angle, I feel that we should show all scrollable areas scrollbars at the point. From the inner-most element at the point, climb up the frames, and record the scrollable direction. Then, stop climbing up the frames when all directions are found, and show the scrollbars of found scrollable frames.

Comment 15

[André Reinald]

Attachment: WIP: generate "wheel" events when 2 fingers touch/release trackpad instead of "gesture" events.

Generating wheel events makes more sense here than gesture events: all the logic to detect which areas are scrollable (and which scrollbars to show) is already implemented for those.

WIP because I'm getting compile errors in nsGlobalWindow.cpp for some missing definitions. Trying to figure out why and how to define/implement those.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 765356 [details] [diff] [review]
WIP: generate "wheel" events when 2 fingers touch/release trackpad instead of "gesture" events.

Is this the right patch?

Comment 17

[André Reinald]

No wait a mn.

Comment 18

[André Reinald]

Attachment: WIP: generate "wheel" events when 2 fingers touch/release trackpad instead of "gesture" events.

It was the wrong patch. There were some leftovers from previous gesture event variant.

Comment 19

[André Reinald]

Attachment: WIP: generate "wheel" events when 2 fingers touch/release trackpad instead of "gesture" events.

3rd version. There were still inconsistencies.

Comment 20

[Markus Stange [:mstange]]

Using wheel events instead of gesture events sounds good to me. I'll leave the decision whether / how they should be exposed as DOM events to Olli.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Wheel events are from DOM 3 Events, so exposing them to web page is ok.
(We already do dispatch wheel events when scrolling using touchpad/mouse wheel/etc)

Comment 22

[Markus Stange [:mstange]]

(In reply to Olli Pettay [:smaug] from comment #21)
> Wheel events are from DOM 3 Events

The patch is adding to new messages for NS_WHEEL_EVENT: NS_WHEEL_SCROLL_STARTED and NS_WHEEL_SCROLL_STOPPED (which indicate whether the finger is on the touchpad). These events are probably not part of DOM 3 Events.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Oh, I didn't look at the patch. The names are certainly wrong on DOM side if it about use of
wheel/touchpad. Could be wheelstart, wheelend or some such.

Can we support something like wheelstart/end on all the platforms? If so, perhaps such could be
spec'ed.
But, do we really need them?

Comment 24

[Markus Stange [:mstange]]

We certainly need them as nsEvents but not necessarily as DOM events, at least not yet. Web pages might want them if they want to implement a UI where you can swipe between elastic pages, like in the OS X Launchpad UI, using scroll events. I don't know whether that's something people want.

Comment 25

[André Reinald]

Attachment: WIP : trying to use the Wheel{Start|Stop} events to show/hide scrollbars

Wire Wheel{Start|Stop} events to calls to nsMouseWheelTrnsaction::{Begin|End}Transaction methods. Doesn't work yet. Trying to figure out why.

Feedback welcome from anyone on the CC list. Meanwhile, using debugger and traces.

Comment 26

[Stephen A Pohl [:spohl]]

Overlay scrollbars have been disabled in FF23 per bug 895203. Removing tracking flag.

Comment 27

[Stephen A Pohl [:spohl]]

I'm not sure if you're still in need of feedback, but here are my thoughts:

For NS_WHEEL_START, you're checking if the phase has the NSEventPhaseMayBegin flag set. This phase is only available on 10.8 and up [1], so you may want to check for NSEventPhaseBegan instead. I've noticed that you will get both NSEventPhaseMayBegin and NSEventPhaseBegan events on 10.8, so you will want to ignore one of them. You may also want to verify that you can build on OSX below 10.8 if you're using NSEventPhaseMayBegin, since it may not be defined. At a more abstract level, I'm not sure you need to detect these phases at all. Isn't the first NS_WHEEL_WHEEL event synonymous with the start of the gesture?

For NS_WHEEL_STOP, you're checking if the phase has the NSEventPhaseCancelled flag set. However, according to [1], I don't think you will ever get a scroll event with NSEventPhaseCancelled. You would probably want to check for NSPhaseEnded. I've confirmed this locally.

In order to suppress the fading of the scrollbars, you would want to delay the call to ScrollbarActivity::StartFadeBeginTimer until you've received an NS_WHEEL_STOP event. In order to do that, I suggest that you follow the path of NS_WHEEL_WHEEL. From a quick look, the execution path appears to be something like:
[2]
[3]
[4]

So, you could expand the nsIScrollableFrame interface with a method similar to ScrollBy with the purpose of notifying the scrollable frame when the scroll gesture has ended. You could then call ScrollbarActivity::StartFadeBeginTimer from there.


[1] https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSEvent_Class/Reference/Reference.html#//apple_ref/occ/instm/NSEvent/phase
[2] http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#3317
[3] http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#2898
[4] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2434

Comment 28

[Markus Stange [:mstange]]

(In reply to Stephen Pohl [:spohl] from comment #27)
> So, you could expand the nsIScrollableFrame interface with a method similar
> to ScrollBy with the purpose of notifying the scrollable frame when the
> scroll gesture has ended. You could then call
> ScrollbarActivity::StartFadeBeginTimer from there.

I wouldn't go so far into ScrollbarActivity. Instead, I would have NS_WHEEL_START call ActivityStarted() and NS_WHEEL_STOP call ActivityStopped(). ScrollbarActivity has a nested activity counter which should do the rest for you. At least that was my original idea behind ActivityStarted and ActivityStopped; their effects may have changed since then.

Comment 29

[Stephen A Pohl [:spohl]]

Right, I didn't convey this properly: I meant to clarify where the fading of the scrollbars is happening if there was any confusion, i.e. it is ScrollbarActivity::StartFadeBeginTimer and not nsMouseWheelTransaction::EndTransaction that's handling this. Having NS_WHEEL_STOP call ActivityStopped seems like the right thing to do.

The nested activity counter is still present in the same form. I've never seen it get past a value of 1 though. Maybe it's no longer necessary? Especially in the case where we delay the fading of scrollbars it may be preferable to ignore/remove this counter. For example, if a user scrolls the page and then moves the cursor off the browser window we will not receive an NS_WHEEL_STOP event. This would leave the scrollbars visible and seems to match what Safari is doing (activityCounter=1). However, it's expected that when the user scrolls again (activityCounter=2) and an NS_WHEEL_STOP event is received (activityCounter=1), that the scrollbars disappear. With the nested activity counter at a value above 0 we may not do that properly.

Comment 30

[Markus Stange [:mstange]]

(In reply to Stephen Pohl [:spohl] from comment #29)
> Right, I didn't convey this properly: I meant to clarify where the fading of
> the scrollbars is happening if there was any confusion, i.e. it is
> ScrollbarActivity::StartFadeBeginTimer and not
> nsMouseWheelTransaction::EndTransaction that's handling this. Having
> NS_WHEEL_STOP call ActivityStopped seems like the right thing to do.

Oh I see.

> The nested activity counter is still present in the same form. I've never
> seen it get past a value of 1 though. Maybe it's no longer necessary?

This very bug was actually the primary reason for the nested activity counter. So it's more the case that it's not really necessary yet, but will become necessary through this bug :)
Still, even now you can temporarily go over a value of 1 if you drag the scrollbar. Having the mouse button pressed on the scrollbar keeps it at 1, and moving during that time calls ActivityOccurred() which (very briefly) sets the value to 2.

> Especially in the case where we delay the fading of scrollbars it may be
> preferable to ignore/remove this counter. For example, if a user scrolls the
> page and then moves the cursor off the browser window we will not receive an
> NS_WHEEL_STOP event.

That's really unfortunate.

> This would leave the scrollbars visible and seems to
> match what Safari is doing (activityCounter=1). However, it's expected that
> when the user scrolls again (activityCounter=2) and an NS_WHEEL_STOP event
> is received (activityCounter=1), that the scrollbars disappear. With the
> nested activity counter at a value above 0 we may not do that properly.

I think that should be handled at the widget level where NS_WHEEL_* events are sent. I'd like the sending of NS_WHEEL_START to guarantee a future balanced NS_WHEEL_STOP event. So for example nsChildView could remember whether it sent an NS_WHEEL_STOP event and if it hasn't send one before the next NS_WHEEL_START event, it just sends NS_WHEEL_STOP right then and there. Does that sound sensible?

Comment 31

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #30)
> (In reply to Stephen Pohl [:spohl] from comment #29)
> > The nested activity counter is still present in the same form. I've never
> > seen it get past a value of 1 though. Maybe it's no longer necessary?
> 
> This very bug was actually the primary reason for the nested activity
> counter. So it's more the case that it's not really necessary yet, but will
> become necessary through this bug :)
> Still, even now you can temporarily go over a value of 1 if you drag the
> scrollbar. Having the mouse button pressed on the scrollbar keeps it at 1,
> and moving during that time calls ActivityOccurred() which (very briefly)
> sets the value to 2.

Ah, that makes sense! I didn't notice this before. Thanks!
 
> > This would leave the scrollbars visible and seems to
> > match what Safari is doing (activityCounter=1). However, it's expected that
> > when the user scrolls again (activityCounter=2) and an NS_WHEEL_STOP event
> > is received (activityCounter=1), that the scrollbars disappear. With the
> > nested activity counter at a value above 0 we may not do that properly.
> 
> I think that should be handled at the widget level where NS_WHEEL_* events
> are sent. I'd like the sending of NS_WHEEL_START to guarantee a future
> balanced NS_WHEEL_STOP event. So for example nsChildView could remember
> whether it sent an NS_WHEEL_STOP event and if it hasn't send one before the
> next NS_WHEEL_START event, it just sends NS_WHEEL_STOP right then and there.
> Does that sound sensible?

Great idea! This should work around this problem nicely.

Comment 32

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #30)
> I think that should be handled at the widget level where NS_WHEEL_* events
> are sent. I'd like the sending of NS_WHEEL_START to guarantee a future
> balanced NS_WHEEL_STOP event. So for example nsChildView could remember
> whether it sent an NS_WHEEL_STOP event and if it hasn't send one before the
> next NS_WHEEL_START event, it just sends NS_WHEEL_STOP right then and there.
> Does that sound sensible?

I figured out that there should be some global place where we track the "state" of the wheel (which is actually 2 fingers down). nsMouseWheelTransaction seemed to be a nice place since it's a singleton. I'm not sure though that it does intercept all events like the "test app" you made.

There is some confusion as to which scroll bars should disappear and which should appear when we have nested scroll frames. It seemed to me that nsMouseWheelTransaction somehow was keeping track of this, and that's why I started to use those NS_WHEEL_START/STOP events there.

Now I'm looking for a way to bridge nsMouseWheelTransaction and the right ScrollbarActivity instances, and call its ActivityStarted/Stopped methods.

Comment 33

[Markus Stange [:mstange]]

(In reply to André Reinald from comment #32)
> (In reply to Markus Stange [:mstange] from comment #30)
> > I think that should be handled at the widget level where NS_WHEEL_* events
> > are sent. I'd like the sending of NS_WHEEL_START to guarantee a future
> > balanced NS_WHEEL_STOP event. So for example nsChildView could remember
> > whether it sent an NS_WHEEL_STOP event and if it hasn't send one before the
> > next NS_WHEEL_START event, it just sends NS_WHEEL_STOP right then and there.
> > Does that sound sensible?
> 
> I figured out that there should be some global place where we track the
> "state" of the wheel (which is actually 2 fingers down).
> nsMouseWheelTransaction seemed to be a nice place since it's a singleton.

Sounds good to me.

> I'm not sure though that it does intercept all events like the "test app"
> you made.

You mean with your current patch? I'd have to test.

> There is some confusion as to which scroll bars should disappear and which
> should appear when we have nested scroll frames. It seemed to me that
> nsMouseWheelTransaction somehow was keeping track of this, and that's why I
> started to use those NS_WHEEL_START/STOP events there.

Yes, sounds good.

> Now I'm looking for a way to bridge nsMouseWheelTransaction and the right
> ScrollbarActivity instances, and call its ActivityStarted/Stopped methods.

I think the frames that have a ScrollbarActivity implement nsIScrollbarOwner, you should be able to QueryFrame to that. You can add ScrollbarActivityStarted() and ScrollbarActivityStopped() to the nsIScrollbarOwner interface and implement that in all nsIScrollbarOwner subclasses by just calling the right methods of the attached ScrollbarActivity instance.

I hope that my model of this matches the current reality, it's been a while since I've looked at this code.

Comment 34

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #33)
> (In reply to André Reinald from comment #32)
> > Now I'm looking for a way to bridge nsMouseWheelTransaction and the right
> > ScrollbarActivity instances, and call its ActivityStarted/Stopped methods.
> 
> I think the frames that have a ScrollbarActivity implement
> nsIScrollbarOwner, you should be able to QueryFrame to that. You can add
> ScrollbarActivityStarted() and ScrollbarActivityStopped() to the
> nsIScrollbarOwner interface and implement that in all nsIScrollbarOwner
> subclasses by just calling the right methods of the attached
> ScrollbarActivity instance.
> 
> I hope that my model of this matches the current reality, it's been a while
> since I've looked at this code.

Your model still matches. :-) Those frames implement nsIScrollbarOwner by implementing nsIScrollableFrame, which extends nsIScrollbarOwner.

Comment 35

[André Reinald]

Sorry I overwrote the flags. I put them back to what they should be. Someone may change ? to + where appropriate.

Basically I think we have 2 "models" for showing/hiding scrollbars:
1. if we get a NS_WHEEL_START event, disable all timers and wait for a NS_WHEEL_STOP event to hide the scrollbars.
2. if we get no NS_WHEEL_START event, rely on the current model, which is timer based and relies on NS_WHEEL_WHEEL events occurring frequently to keep the scrollbars visible.

Comment 36

[Markus Stange [:mstange]]

If you're referring to the mouse transaction timers, then I agree completely.

Comment 37

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #36)
> If you're referring to the mouse transaction timers, then I agree completely.

I was also considering skipping the mFadeBeginTimer. But that's arguable as I notice there is a delay between my raising fingers from the touchpad and scrollbars starting to fade (in Xcode).

Comment 38

[Markus Stange [:mstange]]

Right. I wouldn't do anything about the mFadeBeginTimer. Just call ActivityStopped() and let the rest handle itself.

Comment 39

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #38)
> Right. I wouldn't do anything about the mFadeBeginTimer. Just call
> ActivityStopped() and let the rest handle itself.

In case we are in mode 1 (comment 35), I think we should skip calls to ActivityOccured on mousemove events. Am I right?

Comment 40

[Markus Stange [:mstange]]

Why?

Comment 41

[André Reinald]

Attachment: WIP: attempt to link nsMouseWheelTransaction with ScrollbarActivity

Not yet tested.
Have some suggestions for a small refactoring as I had to copy/paste code across a few files, and noticed I was not the only one who did that!

Comment 42

[André Reinald]

Attachment: bugzilla-868648-1.patch

Still WIP, connection between nsMouseWheelTransaction and ScrollbarActivity works fine:
- the scrollbars still don't show on 2 finger down,
- once scroll started scrollbars show and stay visible while 2 fingers hold,
- if raising fingers then touching again (without moving), the scrollbars will hide anyway.

Though it's an improvement over past behavior, it's still not on par with native scrollbars. As it is now, this patch is closer to address bug 868659 than this one.

I would appreciate some feedback at this point.

Comment 43

[Stephen A Pohl [:spohl]]

Haven't had much time to look into this yet, but I'm surprised to still see the checks for NSEventPhaseMayBegin and NSEventPhaseCancelled in nsChildView.mm (see comment 27). Particularly NSEventPhaseCancelled. I could never observe that phase set. Could you?

Comment 44

[André Reinald]

(In reply to Stephen Pohl [:spohl] from comment #43)
> Haven't had much time to look into this yet, but I'm surprised to still see
> the checks for NSEventPhaseMayBegin and NSEventPhaseCancelled in
> nsChildView.mm (see comment 27). Particularly NSEventPhaseCancelled. I could
> never observe that phase set. Could you?

Sure: this is actually the way to detect 2 fingers down/up, so those checks do their job. You can check in the sample app Markus posted in comment 4.

Comment 45

[Markus Stange [:mstange]]

Comment on attachment 787625 [details] [diff] [review]
bugzilla-868648-1.patch

This looks really good to me!

Some comments:

I think NS_WHEEL_START and NS_WHEEL_STOP shouldn't be exposed as DOM events in this bug. If we really want to expose them to Website JavaScript, we should propose them to the relevant standards groups first and add the DOM bits in a different bug. Can you check whether the patch still works when you revert the changes to nsGkAtomList.h, nsEventNameList.h and nsIInlineEventHandlers.idl?

nsMouseWheelTransaction doesn't have much to do with scrollbar visibility, so maybe instead of sDisappearWhenWheelStop, could you call it sStayActiveUntilWheelStop or something like that?

As far as I can tell, the change to nsEventStateManager::ComputeScrollTarget currently only serves to get far enough in nsEventStateManager::PostHandleEvent to call nsMouseWheelTransaction::EndTransaction(). Is that correct? In that case, maybe revert that change and just have a separate path for NS_WHEEL_STOP in nsEventStateManager::PostHandleEvent that just directly calls nsMouseWheelTransaction::EndTransaction().

If you want to fix this bug in the same patch, how about this:
On NS_WHEEL_START, call a new method like nsMouseWheelTransaction::ActivateAllPossibleScrollbarsUntilTransactionBegin that has a similar loop as in ComputeScrollTarget, but collects all encountered scroll frames in an nsTArray<nsWeakFrame> which is stored in nsMouseWheelTransaction (e.g. as sActivatedScrollableFrames), and call ScrollbarActivityStarted() on them all. Then, when a future NS_WHEEL_WHEEL event causes BeginTransaction to be called (through DoScrollText), clear this array again and call ScrollbarActivityStopped() on all but the now active scroll frame. Would that work?

Comment 46

[Stephen A Pohl [:spohl]]

(In reply to André Reinald from comment #44)
> Sure: this is actually the way to detect 2 fingers down/up, so those checks
> do their job. You can check in the sample app Markus posted in comment 4.

Hmm, this does work with the sample app, but I couldn't get those phases when I was debugging firefox locally. I shall check that next. Do we expect two-finger hold to work on 10.8 and up only, since NSEventPhaseMayBegin is only available on 10.8+?

Comment 47

[Markus Stange [:mstange]]

Attachment: patch with activation on NS_WHEEL_START

I've played with the patch a little and was able to get NS_WHEEL_START scrollbar activation working. Hope this helps.

Comment 48

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #47)
> Created attachment 788804 [details] [diff] [review]
> patch with activation on NS_WHEEL_START
> 
> I've played with the patch a little and was able to get NS_WHEEL_START
> scrollbar activation working. Hope this helps.

Sure it helps! You finished my work on this bug!

I see you hid the events from JavaScript. Actually I mostly added my events everywhere I saw WHEEL_WHEEL events related declarations, but not knowing exactly what every bit was for.

Now I have to understand the rest of your changes which I assume are what you suggested in comment 45, and what I was progressing on... too slowly.

There is one weird behavior: when scrolling with momentum the thumbs take longer to disappear than when lifting fingers when the scroll stopped. I'll try to figure out why, but I guess some timers are adding up when they should not.

Comment 49

[Markus Stange [:mstange]]

(In reply to André Reinald from comment #48)
> You finished my work on this bug!

Not really. Comments need to be added to the changes, some things could probably be solved a little more elegantly. For example, I don't like the nested directions array, and the new method names are a little long... moreover, it's confusing to "deactivate all activated scroll targets" on every NS_WHEEL_WHEEL, when actually only the temporarily activated NS_WHEEL_START targets are deactivated, i.e. the name is confusing.

> I see you hid the events from JavaScript. Actually I mostly added my events
> everywhere I saw WHEEL_WHEEL events related declarations, but not knowing
> exactly what every bit was for.

That's totally fine and exactly how I would have started out, too!

> Now I have to understand the rest of your changes which I assume are what
> you suggested in comment 45

The patch does it a little differently than what I suggested. The patch doesn't collect scroll targets while walking up the frame tree, but instead it finds the right scroll target for each of the 8 possible scrolling directions. That has the advantage of showing only those scrollbars that would actually be scrolled from this point. Also, it means that we can use a fixed size nsWeakFrame array. I think nsWeakFrames don't work in an nsTArray (which was what I suggested doing) since nsTArray can move its contents during resizing, which would mean that the nsIFrames' pointers to the weakframes become invalid.

>, and what I was progressing on... too slowly.

Absolutely do not worry about that! You're doing really well for somebody who hasn't been around in these parts of Gecko code for that long.

> There is one weird behavior: when scrolling with momentum the thumbs take
> longer to disappear than when lifting fingers when the scroll stopped. I'll
> try to figure out why, but I guess some timers are adding up when they
> should not.

Interesting. I don't have an idea at the moment why this would happen, but it would be good to find out. Or is this just when scrolling to the very end of the page? In that case, we may get more momentum scroll events while the page is already at the end, and these events keep the scrollbars visible, until the NS_WHEEL_STOP event starts the fadeout timer. Before this patch, when more momentum scroll events arrived that didn't move the scrollbar, we didn't keep the scrollbars visible because we only considered them active while they were moving.

Comment 50

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #49)

Among the things that bug me, is it necessary to check the 8 directions ? I believe up/down/right/left should be enough. In most cases those would return only 2 scrollbars, and at most 4 scrollbars, right ? Additionally, it may be worth avoiding duplicates in the array ?

I've also looked into nsPoint to store the 8 (or 4) directions. But as the event directions and the aDirection params are not nsPoint, it would not be a net win to do that as we'd not have the "add" method and would have to explicitly access .x and .y fields anyway.

The way you compute which scrollbars should show is, in my opinion, much better than what I originally thought based on my observations of Safari's behavior. And I believe it doesn't matter if we behave differently (and better) here. By the way, I didn't know what weakFrames were, and how they worked, but your comment on nsArrayT gives me a good hint !

Comment 51

[André Reinald]

Another question : I guess nsWeakFrame is a class so that when nsEventStateManager is created, all mActivatedScrollTargets[] objects have their default constructor called. As I understand there is a cross reference between nsIFrame and nsWeakFrame objects, is everything handled correctly when nsEventStateManager is destroyed or is this something we should take care of ?

Comment 52

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #49)
> (In reply to André Reinald from comment #48)
> > There is one weird behavior: when scrolling with momentum the thumbs take
> > longer to disappear than when lifting fingers when the scroll stopped. I'll
> > try to figure out why, but I guess some timers are adding up when they
> > should not.
> 
> Interesting. I don't have an idea at the moment why this would happen, but
> it would be good to find out. Or is this just when scrolling to the very end
> of the page? In that case, we may get more momentum scroll events while the
> page is already at the end, and these events keep the scrollbars visible,
> until the NS_WHEEL_STOP event starts the fadeout timer. Before this patch,
> when more momentum scroll events arrived that didn't move the scrollbar, we
> didn't keep the scrollbars visible because we only considered them active
> while they were moving.

The behavior doesn't depend on the scroll being at the end of its range. I guess I'll have to look into momentum scrolling and understand how it works.

Comment 53

[André Reinald]

Compile failed on try servers:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26655840&tree=Try

There is a discrepancy here: the header of the page says "OS X 10.7 try build" but it seems this is not true as the log shows paths hinting at 10.6 SDK such as:
"/Developer/SDKs/MacOSX10.6.sdk/System/Library/..."

Beyond this, some symbols are not defined there, and I didn't check yet if they are defined in 10.7. I know for sure that at least some constants are 10.8 only.

My past practice, using Xcode, was to use the latest SDK while targeting the earlier supported system for our products. And it worked as expected Objective C wise: when running on earlier systems, unimplemented methods just returned nil. Which we took care of checking at runtime.

In our case this would mean using 10.8 SDK and targeting 10.6.

I have no idea though how this translates in our own build system.

And maybe we have a good reason not to do so.

But as far as I know, there is no method of detecting "two fingers down on touchpad" before 10.8. So unless we switch to 10.8 SDK I see no way of implementing the temporary scrollbar activation (which of course will only work on 10.8 and later).

Comment 54

[André Reinald]

Maybe it would be worth trying my own build (which used 10.8 SDK I believe) on earlier systems. But I'm not sure how to do that. Install 10.6 and 10.7 in virtual machines?

Comment 55

[Markus Stange [:mstange]]

(In reply to André Reinald from comment #50)
> (In reply to Markus Stange [:mstange] from comment #49)
> 
> Among the things that bug me, is it necessary to check the 8 directions ? I
> believe up/down/right/left should be enough.

You're probably right. I haven't checked the ComputeScrollTarget code to find out whether scrolling north-east may result in a different scroll target than scrolling only north or only east.

> In most cases those would
> return only 2 scrollbars, and at most 4 scrollbars, right ?

Yes. I even expect the 1 scrollbar case to be hit in 99% of cases ;)

> Additionally, it
> may be worth avoiding duplicates in the array ?

Probably not worth it. Avoiding them would add code and do nothing about performance, I think.

> I've also looked into nsPoint to store the 8 (or 4) directions. But as the
> event directions and the aDirection params are not nsPoint, it would not be
> a net win to do that as we'd not have the "add" method and would have to
> explicitly access .x and .y fields anyway.

You can always do nsPoint point2 = point1 + nsPoint(deltaX, deltaY); or something like that. But it probably won't make the code clearer.

> The way you compute which scrollbars should show is, in my opinion, much
> better than what I originally thought based on my observations of Safari's
> behavior. And I believe it doesn't matter if we behave differently (and
> better) here.

:)
I also like our mouse transaction behavior much better than Safari's "always scroll anything that's possibly scrollable" behavior.

> By the way, I didn't know what weakFrames were, and how they
> worked, but your comment on nsArrayT gives me a good hint !

So the thing about storing references to nsIFrames is that nsIFrames are not reference-counted, so you can't "addref" them and expect them to stay around until you're done storing them. Instead, they can be deleted at any time, so any nsIFrame* pointers you have during that time can become invalid. nsWeakFrame ensures that you don't have any stale pointers by asking the nsIFrame to tell the nsWeakFrame when it's destroyed so that the nsWeakFrame can null out its pointer to it.

(In reply to André Reinald from comment #51)
> Another question : I guess nsWeakFrame is a class so that when
> nsEventStateManager is created, all mActivatedScrollTargets[] objects have
> their default constructor called.

Yes, they're initialized with null.

> As I understand there is a cross reference
> between nsIFrame and nsWeakFrame objects, is everything handled correctly
> when nsEventStateManager is destroyed or is this something we should take
> care of ?

This is handled automatically by the destructor of nsWeakFrame.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#3230

(In reply to André Reinald from comment #53)
> Compile failed on try servers:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26655840&tree=Try

Have a look at http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.h#152

You'll need to add something similar. Whenever we use constants that are only defined in a more recent SDK than what we use, we do something like
#if building with an SDK that doesn't support the new constants
enum {
  NSWhateverConstantName = the value that's defined for the contsant in the documentation
}
#endif

> In our case this would mean using 10.8 SDK and targeting 10.6.
> 
> I have no idea though how this translates in our own build system.
> 
> And maybe we have a good reason not to do so.

I don't know why we do it the way we do it. I think the reasoning is "nobody has had the time to look into doing it the other way round, we've always built with the SDK of the oldest supported OS and it has worked for us" but I don't know if that's the real answer.

> But as far as I know, there is no method of detecting "two fingers down on
> touchpad" before 10.8. So unless we switch to 10.8 SDK I see no way of
> implementing the temporary scrollbar activation (which of course will only
> work on 10.8 and later).

Luckily this is not the case :)
All you need is the constant definition. It's just a number that's set on a flag on an event object, and whether that number is set does not depend on what SDK we build with.

(In reply to André Reinald from comment #54)
> Maybe it would be worth trying my own build (which used 10.8 SDK I believe)
> on earlier systems. But I'm not sure how to do that. Install 10.6 and 10.7
> in virtual machines?

It's probably not worth it for now.

Comment 56

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #53)

Historically (with a few rare exceptions) we've always built with the SDK of the lowest version of OS X that we support.  According to Apple that's not supposed to be necessary, but (as I remember it) we found at least one case where it *was* necessary -- though I can no longer remember exactly what that was.

Markus has told you how to incorporate definitions that are only included in header files belong to more recent SDKs.

I think we should stick with the current SDK policy, and make only make exceptions to it for compelling reasons.  The most recent exception we made (and in fact the only one I'm aware of) was to start using the 10.6 SDK while we still supported OS X 10.5.  This was when the build infrastructure team needed to buy lots of Macs, but new ones only ran 10.7 and up, for which versions of XCode weren't available that included the 10.5 SDK.  Switching SDKs like this *did* cause at least one bug (which only became apparent months after the switch), but fortunately we were able to work around it.

Comment 57

[Steven Michaud [:smichaud] (Retired)]

> Switching SDKs like this *did* cause at least one bug (which only
> became apparent months after the switch), but fortunately we were
> able to work around it.

Bug 721160.

Comment 58

[André Reinald]

Attachment: bugzilla-868648-5.patch

Try server results:
https://tbpl.mozilla.org/?tree=Try&rev=20a887980c90

Also fixes bug 868659.

Comment 59

[Markus Stange [:mstange]]

This patch (and the try push) is missing the change to nsChildView.mm, as far as I can see.

Comment 60

[André Reinald]

You're right, I messed up at some point with my patch queue, and had to reapply changes by hand. I hope I didn't miss anything else. Fixed it. At least the feature is working (checked locally). I'll also take into account the "if missing wheel_start, send one right before wheel_stop" in your comment 30 before resubmitting the patch.

Comment 61

[Josh Aas]

Comment on attachment 795026 [details] [diff] [review]
bugzilla-868648-5.patch

Clearing review while waiting for a new patch. Please get review from mstange when you're ready.

Comment 62

[André Reinald]

Attachment: bugzilla-868648-6.patch

Hopefully put back pieces of the puzzle. Stuck with previous NSEventPhase related declarations in nsChildView.h although I'd have rather put them in nsChildView.mm as they are only used locally. Added pairing up/down events.

https://tbpl.mozilla.org/?tree=Try&rev=d94c2710d2d1

Comment 63

[Markus Stange [:mstange]]

Comment on attachment 796088 [details] [diff] [review]
bugzilla-868648-6.patch

Sorry for the delay.

>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp
>@@ -35,16 +35,17 @@
> #include "nsPIWindowRoot.h"
> #include "nsIWebNavigation.h"
> #include "nsIContentViewer.h"
> #include <algorithm>
> #ifdef MOZ_XUL
> #include "nsXULPopupManager.h"
> #endif
> #include "nsFrameManager.h"
>+#include "nsGfxScrollFrame.h"

Why is this necessary? Do you need it for nsIScrollbarOwner? If so, please only include nsIScrollbarOwner.h.

>+void
>+nsEventStateManager::TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
>+                                                      mozilla::widget::WheelEvent* aEvent)
>+{
>+  static const double directions[4][2] = {
>+    { 0, -1 }, { 0, +1 }, { -1, 0 }, { +1, 0 }

I still haven't read the code so I'm still not sure if four directions are actually enough. Say you have three nested scrollboxes. The innermost is only scrollable to the right, the middle one is only scrollable downwards, and the outer is scrollable both right and down. Now you do a diagonal down-right scroll motion in the innermost box. Will that scroll the outer one?

>+        // ComputeScrollTarget will often return the same target for different scroll directions
>+        // Adding a test to avoid storing duplicates in mActivatedScrollTargets[]
>+        // Quick code : we only have 4 elements in the array !
>+        bool unique = true;
>+        for (int32_t j = 0; j < i; ++j) {
>+          if (mActivatedScrollTargets[j] == targetFrame) {
>+            unique = false;
>+            break;
>+          }
>+        }
>+        if (unique) {

I still don't think this is worth it. I think you should remove these 11 lines. Calling ScrollbarActivityStarted multiple times on the same ScrollbarActivity will just increment a counter, it's not an expensive operation.

>diff --git a/content/events/src/nsEventStateManager.h b/content/events/src/nsEventStateManager.h
>--- a/content/events/src/nsEventStateManager.h
>+++ b/content/events/src/nsEventStateManager.h
>@@ -545,16 +545,26 @@ protected:
>       (PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_X_AXIS | START_FROM_PARENT),
>     COMPUTE_SCROLLABLE_ANCESTOR_ALONG_Y_AXIS     =
>       (PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_Y_AXIS | START_FROM_PARENT)
>   };
>   nsIScrollableFrame* ComputeScrollTarget(nsIFrame* aTargetFrame,
>                                           mozilla::widget::WheelEvent* aEvent,
>                                           ComputeScrollTargetOptions aOptions);
> 
>+  nsIScrollableFrame* ComputeScrollTarget(nsIFrame* aTargetFrame,
>+                                          double aDirectionX,
>+                                          double aDirectionY,
>+                                          mozilla::widget::WheelEvent* aEvent,
>+                                          ComputeScrollTargetOptions aOptions);
>+
>+  void TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
>+                                        mozilla::widget::WheelEvent* aEvent);
>+  void DeactivateAllTemporarilyActivatedScrollTargets();
>+

Document that these two methods are responsible for showing and hiding scrollbars on NS_WHEEL_START / NS_WHEEL_STOP.

>diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp
>--- a/layout/generic/nsGfxScrollFrame.cpp
>+++ b/layout/generic/nsGfxScrollFrame.cpp
>@@ -69,16 +69,28 @@ NS_NewHTMLScrollFrame(nsIPresShell* aPre
> NS_IMPL_FRAMEARENA_HELPERS(nsHTMLScrollFrame)
> 
> nsHTMLScrollFrame::nsHTMLScrollFrame(nsIPresShell* aShell, nsStyleContext* aContext, bool aIsRoot)
>   : nsContainerFrame(aContext),
>     mInner(ALLOW_THIS_IN_INITIALIZER_LIST(this), aIsRoot)
> {
> }
> 
>+void
>+nsHTMLScrollFrame::ScrollbarActivityStarted() const {
>+  if (mInner.mScrollbarActivity)
>+    mInner.mScrollbarActivity->ActivityStarted();
>+}
>+ 
>+void
>+nsHTMLScrollFrame::ScrollbarActivityStopped() const {
>+  if (mInner.mScrollbarActivity)
>+    mInner.mScrollbarActivity->ActivityStopped();
>+}

{} around the if bodies, please.

>+ 
> nsresult
> nsHTMLScrollFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
> {
>   return mInner.CreateAnonymousContent(aElements);
> }
> 
> void
> nsHTMLScrollFrame::AppendAnonymousContentTo(nsBaseContentList& aElements,
>@@ -893,16 +905,28 @@ nsXULScrollFrame::nsXULScrollFrame(nsIPr
>                                    bool aIsRoot, bool aClipAllDescendants)
>   : nsBoxFrame(aShell, aContext, aIsRoot),
>     mInner(ALLOW_THIS_IN_INITIALIZER_LIST(this), aIsRoot)
> {
>   SetLayoutManager(nullptr);
>   mInner.mClipAllDescendants = aClipAllDescendants;
> }
> 
>+void
>+nsXULScrollFrame::ScrollbarActivityStarted() const {
>+  if (mInner.mScrollbarActivity)
>+    mInner.mScrollbarActivity->ActivityStarted();
>+}
>+ 
>+void
>+nsXULScrollFrame::ScrollbarActivityStopped() const {
>+  if (mInner.mScrollbarActivity)
>+    mInner.mScrollbarActivity->ActivityStopped();
>+}

here too

>diff --git a/layout/generic/nsGfxScrollFrame.h b/layout/generic/nsGfxScrollFrame.h
>--- a/layout/generic/nsGfxScrollFrame.h
>+++ b/layout/generic/nsGfxScrollFrame.h

>@@ -636,17 +639,17 @@ public:
>    *
>    * @see nsGkAtoms::scrollFrame
>    */
>   virtual nsIAtom* GetType() const MOZ_OVERRIDE;
>   
> #ifdef DEBUG
>   NS_IMETHOD GetFrameName(nsAString& aResult) const MOZ_OVERRIDE;
> #endif
>-
>+                            

revert this

>diff --git a/layout/generic/nsIScrollbarOwner.h b/layout/generic/nsIScrollbarOwner.h
>--- a/layout/generic/nsIScrollbarOwner.h
>+++ b/layout/generic/nsIScrollbarOwner.h
>@@ -18,11 +18,18 @@ class nsIScrollbarOwner : public nsQuery
> public:
>   NS_DECL_QUERYFRAME_TARGET(nsIScrollbarOwner)
> 
>   /**
>    * Obtain the frame for the horizontal or vertical scrollbar, or null
>    * if there is no such box.
>    */
>   virtual nsIFrame* GetScrollbarBox(bool aVertical) = 0;
>+
>+  /**
>+   * Show or hide scrollbars on 2 fingers touch
>+   * subclasses should call their ScrollbarActivity's corresponding metods

Please start each sentence with a capital letter and end it with a period. Also, metods->methods

>diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp
>--- a/layout/xul/tree/nsTreeBodyFrame.cpp
>+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
>@@ -4469,16 +4469,30 @@ nsTreeBodyFrame::PostScrollEvent()
>   if (NS_FAILED(NS_DispatchToCurrentThread(ev))) {
>     NS_WARNING("failed to dispatch ScrollEvent");
>   } else {
>     mScrollEvent = ev;
>   }
> }
> 
> void
>+nsTreeBodyFrame::ScrollbarActivityStarted() const
>+{
>+  if (mScrollbarActivity)
>+    mScrollbarActivity->ActivityStarted();
>+}
>+ 
>+void
>+nsTreeBodyFrame::ScrollbarActivityStopped() const
>+{
>+  if (mScrollbarActivity)
>+    mScrollbarActivity->ActivityStopped();
>+}

{}


>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm

>+  // fire NS_WHEEL_START/STOP events when 2 fingers touch/release the touchpad

capital letter + period

>+  // TODO: NSEventPhaseMayBegin is 10.8 specific.
>+  // For earlier Mac OS versions, we still have to discover how to detect those.
>+  // See bug 868648 for more discussion on this.

Do native apps on 10.7 show scrollbars when two fingers touch the touchpad?

>+  if (phase & NSEventPhaseMayBegin) {
>+    msg = NS_WHEEL_START;
>+    mDidSendWheelStart = YES;
>+  } else if (phase & (NSEventPhaseEnded | NSEventPhaseCancelled)) {
>+    if (!mDidSendWheelStart) {
>+      WheelEvent wheelStartEvent(true, NS_WHEEL_START, mGeckoChild);
>+      [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelStartEvent];
>+      mDidSendWheelStart = YES;
>+      mGeckoChild->DispatchWindowEvent(wheelStartEvent);
>+    }
>+    msg = NS_WHEEL_STOP;
>+    mDidSendWheelStart = NO;
>+  }

Don't we want mDidSendWheelStop, too, with similar behavior? Or do we not need that? Or do you want to do that in another patch?

The rest looks good. Please request review from me on the next patch again.

Comment 64

[bhavana bajaj [:bajaj]]

Spoke offline with :spohl and this is not a blocker for Scrollbar feature going in Fx24, but a nice to have in upcoming release.Hence wontfixing it for Fx24 at this point.

Comment 65

[André Reinald]

Attachment: bugzilla-868648-7.patch

(In reply to Markus Stange [:mstange] from comment #63)
> Comment on attachment 796088 [details] [diff] [review]
> bugzilla-868648-6.patch
> 
> Sorry for the delay.

Seems this patch won't go in Fx 24 anyway, so there's no hurry.

> >diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
> >--- a/content/events/src/nsEventStateManager.cpp
> >+++ b/content/events/src/nsEventStateManager.cpp
> >@@ -35,16 +35,17 @@
> >+#include "nsGfxScrollFrame.h"
> 
> Why is this necessary? Do you need it for nsIScrollbarOwner? If so, please
> only include nsIScrollbarOwner.h.

As a matter of fact while rebasing, this very bit was rejected, and I didn't put it back, while compilation is successful anyway.

> >+void
> >+nsEventStateManager::TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
> >+                                                      mozilla::widget::WheelEvent* aEvent)
> >+{
> >+  static const double directions[4][2] = {
> >+    { 0, -1 }, { 0, +1 }, { -1, 0 }, { +1, 0 }
> 
> I still haven't read the code so I'm still not sure if four directions are
> actually enough. Say you have three nested scrollboxes. The innermost is
> only scrollable to the right, the middle one is only scrollable downwards,
> and the outer is scrollable both right and down. Now you do a diagonal
> down-right scroll motion in the innermost box. Will that scroll the outer
> one?

I didn't try but as far as I understood from the code, h and v are treated independently, which means that a diagonal motion (it's actually very hard to make a pure h or v motion anyway) will scroll the 2 inner frames, not the outer one. And to me, that behavior makes sense. Checking diagonal directions would only return the 2 inner frames multiple times.

> >+        // ComputeScrollTarget will often return the same target for different scroll directions
> >+        // Adding a test to avoid storing duplicates in mActivatedScrollTargets[]
> >+        // Quick code : we only have 4 elements in the array !
> 
> I still don't think this is worth it. I think you should remove these 11
> lines. Calling ScrollbarActivityStarted multiple times on the same
> ScrollbarActivity will just increment a counter, it's not an expensive
> operation.

Ok, removed.

> >diff --git a/content/events/src/nsEventStateManager.h b/content/events/src/nsEventStateManager.h
> >--- a/content/events/src/nsEventStateManager.h
> >+++ b/content/events/src/nsEventStateManager.h
> >@@ -545,16 +545,26 @@ protected:
> >+  void TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
> >+                                        mozilla::widget::WheelEvent* aEvent);
> >+  void DeactivateAllTemporarilyActivatedScrollTargets();
> >+
> 
> Document that these two methods are responsible for showing and hiding
> scrollbars on NS_WHEEL_START / NS_WHEEL_STOP.

Ok, though I'm not sure about the formatting of the comments. Should I assume it will be collected by Doxygen? Then I should document each method separately. But I see many undocumented methods, and the names makes those trivial once one reads the comment I just added.

> {} around the if bodies, please.

Done (on the 3 occurrences you mentioned).

> >diff --git a/layout/generic/nsGfxScrollFrame.h b/layout/generic/nsGfxScrollFrame.h
> >--- a/layout/generic/nsGfxScrollFrame.h
> >+++ b/layout/generic/nsGfxScrollFrame.h
> 
> >@@ -636,17 +639,17 @@ public:
> >    *
> >    * @see nsGkAtoms::scrollFrame
> >    */
> >   virtual nsIAtom* GetType() const MOZ_OVERRIDE;
> >   
> > #ifdef DEBUG
> >   NS_IMETHOD GetFrameName(nsAString& aResult) const MOZ_OVERRIDE;
> > #endif
> >-
> >+                            
> 
> revert this

Done.

> >diff --git a/layout/generic/nsIScrollbarOwner.h b/layout/generic/nsIScrollbarOwner.h
> Please start each sentence with a capital letter and end it with a period.
> Also, metods->methods

Done.

> >diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
> >--- a/widget/cocoa/nsChildView.mm
> >+++ b/widget/cocoa/nsChildView.mm
> 
> >+  // fire NS_WHEEL_START/STOP events when 2 fingers touch/release the touchpad
> 
> capital letter + period

Done.

> >+  // TODO: NSEventPhaseMayBegin is 10.8 specific.
> >+  // For earlier Mac OS versions, we still have to discover how to detect those.
> >+  // See bug 868648 for more discussion on this.
> 
> Do native apps on 10.7 show scrollbars when two fingers touch the touchpad?

Right, it doesn't make sense to try to do that before 10.8 as the OS doesn't.

> >+  if (phase & NSEventPhaseMayBegin) {
> >+    msg = NS_WHEEL_START;
> >+    mDidSendWheelStart = YES;
> >+  } else if (phase & (NSEventPhaseEnded | NSEventPhaseCancelled)) {
> >+    if (!mDidSendWheelStart) {
> >+      WheelEvent wheelStartEvent(true, NS_WHEEL_START, mGeckoChild);
> >+      [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelStartEvent];
> >+      mDidSendWheelStart = YES;
> >+      mGeckoChild->DispatchWindowEvent(wheelStartEvent);
> >+    }
> >+    msg = NS_WHEEL_STOP;
> >+    mDidSendWheelStart = NO;
> >+  }
> 
> Don't we want mDidSendWheelStop, too, with similar behavior? Or do we not
> need that? Or do you want to do that in another patch?

Done, I send the "other" event if needed before both start/stop events.
Changed mDidSendWheelStart to mLastWheelEventIsStart to account for the semantic change.

> The rest looks good. Please request review from me on the next patch again.

Here you are! Thanks.

Comment 66

[André Reinald]

> Also, metods->methods

Forgot to correct the spelling, doing it before asking for commit.

Comment 67

[Markus Stange [:mstange]]

(In reply to André Reinald from comment #65)
> > >+void
> > >+nsEventStateManager::TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
> > >+                                                      mozilla::widget::WheelEvent* aEvent)
> > >+{
> > >+  static const double directions[4][2] = {
> > >+    { 0, -1 }, { 0, +1 }, { -1, 0 }, { +1, 0 }
> > 
> > I still haven't read the code so I'm still not sure if four directions are
> > actually enough. Say you have three nested scrollboxes. The innermost is
> > only scrollable to the right, the middle one is only scrollable downwards,
> > and the outer is scrollable both right and down. Now you do a diagonal
> > down-right scroll motion in the innermost box. Will that scroll the outer
> > one?
> 
> I didn't try but as far as I understood from the code, h and v are treated
> independently, which means that a diagonal motion (it's actually very hard
> to make a pure h or v motion anyway) will scroll the 2 inner frames, not the
> outer one.

Okay then :)

> > >diff --git a/content/events/src/nsEventStateManager.h b/content/events/src/nsEventStateManager.h
> > >--- a/content/events/src/nsEventStateManager.h
> > >+++ b/content/events/src/nsEventStateManager.h
> > >@@ -545,16 +545,26 @@ protected:
> > >+  void TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
> > >+                                        mozilla::widget::WheelEvent* aEvent);
> > >+  void DeactivateAllTemporarilyActivatedScrollTargets();
> > >+
> > 
> > Document that these two methods are responsible for showing and hiding
> > scrollbars on NS_WHEEL_START / NS_WHEEL_STOP.
> 
> Ok, though I'm not sure about the formatting of the comments. Should I
> assume it will be collected by Doxygen? Then I should document each method
> separately. But I see many undocumented methods, and the names makes those
> trivial once one reads the comment I just added.

Yeah, comment styles are all over the place, and I don't think they're processed in an automatic fashion anywhere. The style is good the way you did it.

> > >+  // TODO: NSEventPhaseMayBegin is 10.8 specific.
> > >+  // For earlier Mac OS versions, we still have to discover how to detect those.
> > >+  // See bug 868648 for more discussion on this.
> > 
> > Do native apps on 10.7 show scrollbars when two fingers touch the touchpad?
> 
> Right, it doesn't make sense to try to do that before 10.8 as the OS doesn't.

Ok, you can remove these three comment lines then.

Comment 68

[Markus Stange [:mstange]]

Comment on attachment 799543 [details] [diff] [review]
bugzilla-868648-7.patch

Good, almost done now. Looking at this again I'm finding some things that I missed last time.

>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp

>+void
>+nsEventStateManager::TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
>+                                                      mozilla::widget::WheelEvent* aEvent)
>+{
>+  static const double directions[4][2] = {
>+    { 0, -1 }, { 0, +1 }, { -1, 0 }, { +1, 0 }
>+  };
>+  for (int32_t i = 0; i < 4; i++) {
>+    const double* dir = directions[i];
>+    nsWeakFrame& scrollTarget = mActivatedScrollTargets[i];
>+    MOZ_ASSERT(!scrollTarget, "scroll target still temporarily activated!");
>+    nsIScrollableFrame* target =
>+      ComputeScrollTarget(aTargetFrame, dir[0], dir[1], aEvent,
>+                          COMPUTE_DEFAULT_ACTION_TARGET);
>+    if (target) {
>+      nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(target);
>+      if (scrollbarOwner) {
>+        nsIFrame* targetFrame = do_QueryFrame(target);
>+        scrollTarget = targetFrame;
>+        scrollbarOwner->ScrollbarActivityStarted();
>+      }
>+    }
>+  }
>+}
>+
>+void
>+nsEventStateManager::DeactivateAllTemporarilyActivatedScrollTargets()
>+{
>+  for (int32_t i = 0; i < 4; i++) {
>+    nsWeakFrame& scrollTarget = mActivatedScrollTargets[i];
>+    if (scrollTarget) {
>+      nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(scrollTarget);
>+      if (scrollbarOwner) {
>+        scrollbarOwner->ScrollbarActivityStopped();
>+      }
>+      scrollTarget = nullptr;
>+    }
>+  }
>+}

It just occurred to me that this code would probably be a little clearer if you did
nsWeakFrame* scrollTarget = &mActivatedScrollTargets[i];
...
*scrollTarget = ...;
in both methods. That would make it more obvious that assigning to scrollTarget has a side-effect. Do you agree? (When I read this code quickly just now, I was momentarily confused why we bothered to assign to a variable that is about to go out of scope. Even though I'm the one who wrote it...)

>diff --git a/content/events/src/nsEventStateManager.h b/content/events/src/nsEventStateManager.h
>--- a/content/events/src/nsEventStateManager.h
>+++ b/content/events/src/nsEventStateManager.h
>@@ -542,16 +542,29 @@ protected:
>       (PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_X_AXIS | START_FROM_PARENT),
>     COMPUTE_SCROLLABLE_ANCESTOR_ALONG_Y_AXIS     =
>       (PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_Y_AXIS | START_FROM_PARENT)
>   };
>   nsIScrollableFrame* ComputeScrollTarget(nsIFrame* aTargetFrame,
>                                           mozilla::widget::WheelEvent* aEvent,
>                                           ComputeScrollTargetOptions aOptions);
> 
>+  nsIScrollableFrame* ComputeScrollTarget(nsIFrame* aTargetFrame,
>+                                          double aDirectionX,
>+                                          double aDirectionY,
>+                                          mozilla::widget::WheelEvent* aEvent,
>+                                          ComputeScrollTargetOptions aOptions);
>+
>+  /**
>+   * Those 2 methods are called upon NS_WHEEL_START/NS_WHEEL_STOP events to show/hide adequate scrollbars

Those->These, 2->two, adequate->right (?), period at the end, wrap at 80 characters

>diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp
>--- a/layout/generic/nsGfxScrollFrame.cpp
>+++ b/layout/generic/nsGfxScrollFrame.cpp
>@@ -69,16 +69,30 @@ NS_NewHTMLScrollFrame(nsIPresShell* aPre
> NS_IMPL_FRAMEARENA_HELPERS(nsHTMLScrollFrame)
> 
> nsHTMLScrollFrame::nsHTMLScrollFrame(nsIPresShell* aShell, nsStyleContext* aContext, bool aIsRoot)
>   : nsContainerFrame(aContext),
>     mInner(ALLOW_THIS_IN_INITIALIZER_LIST(this), aIsRoot)
> {
> }
> 
>+void
>+nsHTMLScrollFrame::ScrollbarActivityStarted() const {

Opening braces for methods should get their own line.

void
nsHTMLScrollFrame::ScrollbarActivityStarted() const
{
  ...


>diff --git a/layout/generic/nsIScrollbarOwner.h b/layout/generic/nsIScrollbarOwner.h
>--- a/layout/generic/nsIScrollbarOwner.h
>+++ b/layout/generic/nsIScrollbarOwner.h
>@@ -18,11 +18,18 @@ class nsIScrollbarOwner : public nsQuery
> public:
>   NS_DECL_QUERYFRAME_TARGET(nsIScrollbarOwner)
> 
>   /**
>    * Obtain the frame for the horizontal or vertical scrollbar, or null
>    * if there is no such box.
>    */
>   virtual nsIFrame* GetScrollbarBox(bool aVertical) = 0;
>+
>+  /**
>+   * Show or hide scrollbars on 2 fingers touch.
>+   * Subclasses should call their ScrollbarActivity's corresponding methods.
>+   */
>+  virtual void ScrollbarActivityStarted() const = 0;
>+  virtual void ScrollbarActivityStopped() const = 0;

Hmm, why const?

>diff --git a/widget/cocoa/nsChildView.h b/widget/cocoa/nsChildView.h
>--- a/widget/cocoa/nsChildView.h
>+++ b/widget/cocoa/nsChildView.h
>@@ -197,16 +197,22 @@ typedef NSInteger NSEventGestureAxis;
> - (void)trackSwipeEventWithOptions:(NSEventSwipeTrackingOptions)options
>           dampenAmountThresholdMin:(CGFloat)minDampenThreshold
>                                max:(CGFloat)maxDampenThreshold
>                       usingHandler:(void (^)(CGFloat gestureAmount, NSEventPhase phase, BOOL isComplete, BOOL *stop))trackingHandler;
> @end
> #endif // #ifdef __LP64__
> #endif // #if !defined(MAC_OS_X_VERSION_10_7) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
> 
>+#if !defined(MAC_OS_X_VERSION_10_8) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_8
>+enum {
>+  NSEventPhaseMayBegin    = 0x1 << 5
>+};
>+#endif // #if !defined(MAC_OS_X_VERSION_10_8) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_8
>+
> // Undocumented scrollPhase flag that lets us discern between real scrolls and
> // automatically firing momentum scroll events.
> @interface NSEvent (ScrollPhase)
> // Leopard and SnowLeopard
> - (long long)_scrollPhase;
> // Lion and above
> - (NSEventPhase)momentumPhase;
> @end
>@@ -243,16 +249,17 @@ typedef NSInteger NSEventGestureAxis;
> 
>   // when acceptsFirstMouse: is called, we store the event here (strong)
>   NSEvent* mClickThroughMouseDownEvent;
> 
>   // rects that were invalidated during a draw, so have pending drawing
>   NSMutableArray* mPendingDirtyRects;
>   BOOL mPendingFullDisplay;
>   BOOL mPendingDisplay;
>+  BOOL mLastWheelEventIsStart;

Hmm. During scrolling, the least wheel event is usually "wheel"... How about "mExpectingWheelStop"?

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm

>+  // TODO: NSEventPhaseMayBegin is 10.8 specific.
>+  // For earlier Mac OS versions, we still have to discover how to detect those.
>+  // See bug 868648 for more discussion on this.

remove

>+  if (phase & NSEventPhaseMayBegin) {
>+    if (mLastWheelEventIsStart) {
>+      WheelEvent wheelStopEvent(true, NS_WHEEL_STOP, mGeckoChild);
>+      [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelStopEvent];
>+      mLastWheelEventIsStart = NO;
>+      mGeckoChild->DispatchWindowEvent(wheelStopEvent);
>+    }
>+    mLastWheelEventIsStart = YES;
>+    msg = NS_WHEEL_START;
>+  } else if (phase & (NSEventPhaseEnded | NSEventPhaseCancelled)) {
>+    if (!mLastWheelEventIsStart) {
>+      WheelEvent wheelStartEvent(true, NS_WHEEL_START, mGeckoChild);
>+      [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelStartEvent];
>+      mLastWheelEventIsStart = YES;
>+      mGeckoChild->DispatchWindowEvent(wheelStartEvent);
>+    }
>+    mLastWheelEventIsStart = NO;
>+    msg = NS_WHEEL_STOP;
>+  }
>+
>+  WheelEvent wheelEvent(true, msg, mGeckoChild);
>   [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelEvent];
>+
>+  if (msg != NS_WHEEL_WHEEL) {
>+    mGeckoChild->DispatchWindowEvent(wheelEvent);
>+    return;
>+  }
>+

You could introduce a -[ChildView sendWheelStartOrStop:forEvent:] method, and transform this part into

  if (phase & NSEventPhaseMayBegin) {
    if (mExpectingWheelStop) {
      [self sendWheelStartOrStop:NS_WHEEL_STOP forEvent:theEvent];
    }
    [self sendWheelStartOrStop:NS_WHEEL_START forEvent:theEvent];
    mExpectingWheelStop = YES;
    return;
  }

  if (phase & (NSEventPhaseEnded | NSEventPhaseCancelled)) {
    if (!mExpectingWheelStop) {
      [self sendWheelStartOrStop:NS_WHEEL_START forEvent:theEvent];
    }
    [self sendWheelStartOrStop:NS_WHEEL_STOP forEvent:theEvent];
    mExpectingWheelStop = NO;
    return;
  }

Comment 69

[André Reinald]

Attachment: bugzilla-868648-8.patch

Took into account all comments but this one:

>>+  virtual void ScrollbarActivityStarted() const = 0;
>>+  virtual void ScrollbarActivityStopped() const = 0;

> Hmm, why const?

Because those methods don't modify the object itself.

I tend to use "const" anywhere I can (and wish it was used more frequently throughout our code). It's similar to all immutable cocoa classes (ie NSString, NSArray...). Use mutable (or non const) only when needed, then pass the object along as immutable (const). Make copies when needed. This discipline avoids many potential unwanted side effects. Plus the const indication helps compilers optimize the code.

In other words, I think const should be the default, and any non-const should be motivated.

Comment 70

[Markus Stange [:mstange]]

> I tend to use "const" anywhere I can (and wish it was used more frequently
> throughout our code). It's similar to all immutable cocoa classes (ie
> NSString, NSArray...). Use mutable (or non const) only when needed, then
> pass the object along as immutable (const). Make copies when needed. This
> discipline avoids many potential unwanted side effects. Plus the const
> indication helps compilers optimize the code.
> 
> In other words, I think const should be the default, and any non-const
> should be motivated.

I agree with all of this, but why don't you consider "scrollbar activity state" a property of our object?

Comment 71

[André Reinald]

Because it's really a property of a ScrollbarActivity object which nsIScrollbarOwner only has a pointer to. ScrollbarActivity::ActivityStarted/Stopped are non-const methods, but we don't inherit this "non-const-ness".

Should nsIScrollbarOwner inherit from ScrollbarActivity or embed a ScrollbarActivity object (which may result from a refactoring of the class hierarchy), things would be different.

Comment 72

[Markus Stange [:mstange]]

Comment on attachment 799824 [details] [diff] [review]
bugzilla-868648-8.patch

Alright.

Please request review from smaug for the nsEventStateManager changes now.

Comment 73

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #72)
> Comment on attachment 799824 [details] [diff] [review]
> bugzilla-868648-8.patch
> 
> Alright.
> 
> Please request review from smaug for the nsEventStateManager changes now.

Thanks, done!

Comment 74

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 799824 [details] [diff] [review]
bugzilla-868648-8.patch

Just to reduce my reviewing load a bit...Masayuki, could you perhaps review this?
If not, assign back to me.

Comment 75

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Okay, I should look it. It's really my area. However, perhaps, I can review this on Monday.

Comment 76

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 799824 [details] [diff] [review]
bugzilla-868648-8.patch

>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp
>+bool
>+nsMouseWheelTransaction::OutOfTime(uint32_t aBaseTime, uint32_t aThreshold)
>+{
>+  if (sStayActiveUntilWheelStop)
>+    return false;

nit: Use {}.

> bool
> nsMouseWheelTransaction::UpdateTransaction(widget::WheelEvent* aEvent)
> {
>   nsIScrollableFrame* sf = GetTargetFrame()->GetScrollTargetFrame();
>   NS_ENSURE_TRUE(sf, false);
> 
>+  SetTimeout();
>+
>   if (!CanScrollOn(sf, aEvent->deltaX, aEvent->deltaY)) {
>     OnFailToScrollTarget();
>     // We should not modify the transaction state when the view will not be
>     // scrolled actually.
>     return false;
>   }
> 
>-  SetTimeout();
>-

I don't understand why you need this change.

> void
> nsMouseWheelTransaction::OnEvent(nsEvent* aEvent)
> {
>-  if (!sTargetFrame)
>+  if (aEvent->message == NS_WHEEL_START) {
>+    sStayActiveUntilWheelStop = true;
>+  }
>+
>+  if (!sTargetFrame) {
>     return;
>+  }
> 
>   if (OutOfTime(sTime, GetTimeoutTime())) {
>     // Even if the scroll event which is handled after timeout, but onTimeout
>     // was not fired by timer, then the scroll event will scroll old frame,
>     // therefore, we should call OnTimeout here and ensure to finish the old
>     // transaction.
>     OnTimeout(nullptr, nullptr);
>     return;
>   }
> 
>   switch (aEvent->message) {
>+    case NS_WHEEL_STOP:
>+      sStayActiveUntilWheelStop = false;
>+      EndTransaction();
>+      break;

I don't understand well the timing of coming NS_WHEEL_STOP here.

If the event is fired when user leaves fingers from touchpad, calling EndTransaction() breaks our wheel transaction. If user tries to scroll a lot, the scroll target must not be changed at leaving the fingers from touchpad.

>@@ -496,16 +524,18 @@ nsMouseWheelTransaction::OnTimeout(nsITi
>                       NS_LITERAL_STRING("MozMouseScrollTransactionTimeout"),
>                       true, true);
>   }
> }
> 
> void
> nsMouseWheelTransaction::SetTimeout()
> {
>+  if (sStayActiveUntilWheelStop)
>+    return;

nit: Use {}.

>@@ -959,16 +989,18 @@ nsEventStateManager::PreHandleEvent(nsPr
>   case NS_KEY_UP:
>     {
>       nsIContent* content = GetFocusedContent();
>       if (content)
>         mCurrentTargetContent = content;
>     }
>     break;
>   case NS_WHEEL_WHEEL:
>+  case NS_WHEEL_START:
>+  case NS_WHEEL_STOP:
>     {
>       NS_ASSERTION(aEvent->mFlags.mIsTrusted,
>                    "Untrusted wheel event shouldn't be here");
> 
>       nsIContent* content = GetFocusedContent();
>       if (content)
>         mCurrentTargetContent = content;

This means that NS_WHEEL_START and NS_WHEEL_STOP are treated as normal wheel event. It may cause new regression in the future. I guess that you only need to initialize mCurrentTargetContent, isn't it? If so, please break after if if the event is _START or _STOP.

>+void
>+nsEventStateManager::TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
>+                                                      mozilla::widget::WheelEvent* aEvent)
>+{
>+  static const double directions[4][2] = {
>+    { 0, -1 }, { 0, +1 }, { -1, 0 }, { +1, 0 }
>+  };

I've not tested actually, this might cause some warnings at building with MSVC. Could you append .0 for all of them?

>+  for (int32_t i = 0; i < 4; i++) {
>+    const double* dir = directions[i];
>+    nsWeakFrame* scrollTarget = mActivatedScrollTargets + i;
>+    MOZ_ASSERT(!*scrollTarget, "scroll target still temporarily activated!");
>+    nsIScrollableFrame* target =
>+      ComputeScrollTarget(aTargetFrame, dir[0], dir[1], aEvent,
>+                          COMPUTE_DEFAULT_ACTION_TARGET);
>+    if (target) {
>+      nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(target);
>+      if (scrollbarOwner) {
>+        nsIFrame* targetFrame = do_QueryFrame(target);
>+        *scrollTarget = targetFrame;
>+        scrollbarOwner->ScrollbarActivityStarted();
>+      }
>+    }
>+  }
>+}
>+
>+void
>+nsEventStateManager::DeactivateAllTemporarilyActivatedScrollTargets()
>+{
>+  for (int32_t i = 0; i < 4; i++) {

4 looks like a magic number. Could you use ArrayLength(mActivatedScrollTargets), instead?

>+    nsWeakFrame* scrollTarget = mActivatedScrollTargets + i;

And using mActivatedScrollTargets[i] is better.

>@@ -3147,39 +3232,47 @@ nsEventStateManager::PostHandleEvent(nsP
>       nsIPresShell *shell = presContext->GetPresShell();
>       if (shell) {
>         nsRefPtr<nsFrameSelection> frameSelection = shell->FrameSelection();
>         frameSelection->SetMouseDownState(false);
>       }
>     }
>     break;
>   case NS_WHEEL_WHEEL:
>+  case NS_WHEEL_START:
>+  case NS_WHEEL_STOP:
>     {
>       MOZ_ASSERT(aEvent->mFlags.mIsTrusted);
> 
>       if (*aStatus == nsEventStatus_eConsumeNoDefault) {
>         break;
>       }
> 
>       widget::WheelEvent* wheelEvent = static_cast<widget::WheelEvent*>(aEvent);
>       switch (WheelPrefs::GetInstance()->ComputeActionFor(wheelEvent)) {
>         case WheelPrefs::ACTION_SCROLL: {
>-          if (!wheelEvent->deltaX && !wheelEvent->deltaY) {
>+          if (aEvent->message == NS_WHEEL_WHEEL &&
>+              !wheelEvent->deltaX && !wheelEvent->deltaY) {
>             break;
>           }
>           // For scrolling of default action, we should honor the mouse wheel
>           // transaction.
>+          if (aEvent->message == NS_WHEEL_START) {
>+            TemporarilyActivateAllPossibleScrollTargets(aTargetFrame, wheelEvent);
>+          } else {
>+            DeactivateAllTemporarilyActivatedScrollTargets();
>+          }

I guess that this should be preformed even if the event is consumed by web pages, isn't it? If so, you need this move up to before:

>       if (*aStatus == nsEventStatus_eConsumeNoDefault) {

And I think that following code shouldn't be preformed with _START or _STOP, right?

>@@ -243,16 +249,17 @@ typedef NSInteger NSEventGestureAxis;
> 
>   // when acceptsFirstMouse: is called, we store the event here (strong)
>   NSEvent* mClickThroughMouseDownEvent;
> 
>   // rects that were invalidated during a draw, so have pending drawing
>   NSMutableArray* mPendingDirtyRects;
>   BOOL mPendingFullDisplay;
>   BOOL mPendingDisplay;
>+  BOOL mExpectingWheelStop;

Could you add some explanation for the new member?

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm
>@@ -4729,34 +4730,62 @@ NSEvent* gLastDragMouseDownEvent = nil;
> }
> 
> static int32_t RoundUp(double aDouble)
> {
>   return aDouble < 0 ? static_cast<int32_t>(floor(aDouble)) :
>                        static_cast<int32_t>(ceil(aDouble));
> }
> 
>+- (void)sendWheelStartOrStop:(uint32_t)msg forEvent:(NSEvent *)theEvent
>+{
>+  WheelEvent wheelEvent(true, msg, mGeckoChild);
>+  [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelEvent];
>+  mExpectingWheelStop = (msg == NS_WHEEL_START);
>+  mGeckoChild->DispatchWindowEvent(wheelEvent);
>+}

I worry this code. You don't initialize some specific members of widget::WheelEvent here. However, some developers who to change ESM may assume that they are initialized with proper values...

Comment 77

[André Reinald]

Attachment: bugzilla-868648-9.patch

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #76)
> Comment on attachment 799824 [details] [diff] [review]
> bugzilla-868648-8.patch
> 
> nit: Use {}.

Done at all mentioned places.

> > bool
> > nsMouseWheelTransaction::UpdateTransaction(widget::WheelEvent* aEvent)
> > {
> >   nsIScrollableFrame* sf = GetTargetFrame()->GetScrollTargetFrame();
> >   NS_ENSURE_TRUE(sf, false);
> > 
> >+  SetTimeout();
> >+
> >   if (!CanScrollOn(sf, aEvent->deltaX, aEvent->deltaY)) {
> >     OnFailToScrollTarget();
> >     // We should not modify the transaction state when the view will not be
> >     // scrolled actually.
> >     return false;
> >   }
> > 
> >-  SetTimeout();
> >-
> 
> I don't understand why you need this change.

To reset the timeout even if we'd return false on the previous test.
I assumed it would make more sense, but I may have been wrong.
Is this change doing any harm or is it just useless?

> > void
> > nsMouseWheelTransaction::OnEvent(nsEvent* aEvent)
> > {
> >-  if (!sTargetFrame)
> >+  if (aEvent->message == NS_WHEEL_START) {
> >+    sStayActiveUntilWheelStop = true;
> >+  }
> >+
> >+  if (!sTargetFrame) {
> >     return;
> >+  }
> > 
> >   if (OutOfTime(sTime, GetTimeoutTime())) {
> >     // Even if the scroll event which is handled after timeout, but onTimeout
> >     // was not fired by timer, then the scroll event will scroll old frame,
> >     // therefore, we should call OnTimeout here and ensure to finish the old
> >     // transaction.
> >     OnTimeout(nullptr, nullptr);
> >     return;
> >   }
> > 
> >   switch (aEvent->message) {
> >+    case NS_WHEEL_STOP:
> >+      sStayActiveUntilWheelStop = false;
> >+      EndTransaction();
> >+      break;
> 
> I don't understand well the timing of coming NS_WHEEL_STOP here.
> 
> If the event is fired when user leaves fingers from touchpad, calling
> EndTransaction() breaks our wheel transaction. If user tries to scroll a
> lot, the scroll target must not be changed at leaving the fingers from
> touchpad.

I think the scroll should not end (because of momentum), but the transaction itself should. Therefore, by design, when handling NS_WHEEL_START events, I assume a new transaction will start (which means EndTransaction() was called). So not calling EndTransaction() here would imply other changes I believe.

> >@@ -959,16 +989,18 @@ nsEventStateManager::PreHandleEvent(nsPr
> >   case NS_KEY_UP:
> >     {
> >       nsIContent* content = GetFocusedContent();
> >       if (content)
> >         mCurrentTargetContent = content;
> >     }
> >     break;
> >   case NS_WHEEL_WHEEL:
> >+  case NS_WHEEL_START:
> >+  case NS_WHEEL_STOP:
> >     {
> >       NS_ASSERTION(aEvent->mFlags.mIsTrusted,
> >                    "Untrusted wheel event shouldn't be here");
> > 
> >       nsIContent* content = GetFocusedContent();
> >       if (content)
> >         mCurrentTargetContent = content;
> 
> This means that NS_WHEEL_START and NS_WHEEL_STOP are treated as normal wheel
> event. It may cause new regression in the future. I guess that you only need
> to initialize mCurrentTargetContent, isn't it? If so, please break after if
> if the event is _START or _STOP.

Done.

> >+void
> >+nsEventStateManager::TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
> >+                                                      mozilla::widget::WheelEvent* aEvent)
> >+{
> >+  static const double directions[4][2] = {
> >+    { 0, -1 }, { 0, +1 }, { -1, 0 }, { +1, 0 }
> >+  };
> 
> I've not tested actually, this might cause some warnings at building with
> MSVC. Could you append .0 for all of them?

Haven't tested neither, I just did a trybuild and it was ok on all platforms.
I'll add .0s anyway, it can't do any harm.

> >+  for (int32_t i = 0; i < 4; i++) {
> >+    const double* dir = directions[i];
> >+    nsWeakFrame* scrollTarget = mActivatedScrollTargets + i;
> >+    MOZ_ASSERT(!*scrollTarget, "scroll target still temporarily activated!");
> >+    nsIScrollableFrame* target =
> >+      ComputeScrollTarget(aTargetFrame, dir[0], dir[1], aEvent,
> >+                          COMPUTE_DEFAULT_ACTION_TARGET);
> >+    if (target) {
> >+      nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(target);
> >+      if (scrollbarOwner) {
> >+        nsIFrame* targetFrame = do_QueryFrame(target);
> >+        *scrollTarget = targetFrame;
> >+        scrollbarOwner->ScrollbarActivityStarted();
> >+      }
> >+    }
> >+  }
> >+}
> >+
> >+void
> >+nsEventStateManager::DeactivateAllTemporarilyActivatedScrollTargets()
> >+{
> >+  for (int32_t i = 0; i < 4; i++) {
> 
> 4 looks like a magic number. Could you use
> ArrayLength(mActivatedScrollTargets), instead?

Done (in Activate and Deactivate).
Though I'm not satisfied because 4 still remains a "magic number" at directions[4][2] and mActivatedScrollTargets[4]. I'd better use a constant but I think it's overkill.

> >+    nsWeakFrame* scrollTarget = mActivatedScrollTargets + i;
> 
> And using mActivatedScrollTargets[i] is better.

Done (in Activate and Deactivate).
Though I prefer mActivatedScrollTargets + i.

> >@@ -3147,39 +3232,47 @@ nsEventStateManager::PostHandleEvent(nsP
> >       nsIPresShell *shell = presContext->GetPresShell();
> >       if (shell) {
> >         nsRefPtr<nsFrameSelection> frameSelection = shell->FrameSelection();
> >         frameSelection->SetMouseDownState(false);
> >       }
> >     }
> >     break;
> >   case NS_WHEEL_WHEEL:
> >+  case NS_WHEEL_START:
> >+  case NS_WHEEL_STOP:
> >     {
> >       MOZ_ASSERT(aEvent->mFlags.mIsTrusted);
> > 
> >       if (*aStatus == nsEventStatus_eConsumeNoDefault) {
> >         break;
> >       }
> > 
> >       widget::WheelEvent* wheelEvent = static_cast<widget::WheelEvent*>(aEvent);
> >       switch (WheelPrefs::GetInstance()->ComputeActionFor(wheelEvent)) {
> >         case WheelPrefs::ACTION_SCROLL: {
> >-          if (!wheelEvent->deltaX && !wheelEvent->deltaY) {
> >+          if (aEvent->message == NS_WHEEL_WHEEL &&
> >+              !wheelEvent->deltaX && !wheelEvent->deltaY) {
> >             break;
> >           }
> >           // For scrolling of default action, we should honor the mouse wheel
> >           // transaction.
> >+          if (aEvent->message == NS_WHEEL_START) {
> >+            TemporarilyActivateAllPossibleScrollTargets(aTargetFrame, wheelEvent);
> >+          } else {
> >+            DeactivateAllTemporarilyActivatedScrollTargets();
> >+          }
> 
> I guess that this should be preformed even if the event is consumed by web
> pages, isn't it? If so, you need this move up to before:
> 
> >       if (*aStatus == nsEventStatus_eConsumeNoDefault) {
> 
> And I think that following code shouldn't be preformed with _START or _STOP,
> right?

I have no idea of what consuming events means (though I can make some sense from English).
So please either be more precise so I can make the changes you suggest, or make them for me and I'll try to understand.

> >@@ -243,16 +249,17 @@ typedef NSInteger NSEventGestureAxis;
> > 
> >   // when acceptsFirstMouse: is called, we store the event here (strong)
> >   NSEvent* mClickThroughMouseDownEvent;
> > 
> >   // rects that were invalidated during a draw, so have pending drawing
> >   NSMutableArray* mPendingDirtyRects;
> >   BOOL mPendingFullDisplay;
> >   BOOL mPendingDisplay;
> >+  BOOL mExpectingWheelStop;
> 
> Could you add some explanation for the new member?

Done.

> >diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
> >--- a/widget/cocoa/nsChildView.mm
> >+++ b/widget/cocoa/nsChildView.mm
> >@@ -4729,34 +4730,62 @@ NSEvent* gLastDragMouseDownEvent = nil;
> > }
> > 
> > static int32_t RoundUp(double aDouble)
> > {
> >   return aDouble < 0 ? static_cast<int32_t>(floor(aDouble)) :
> >                        static_cast<int32_t>(ceil(aDouble));
> > }
> > 
> >+- (void)sendWheelStartOrStop:(uint32_t)msg forEvent:(NSEvent *)theEvent
> >+{
> >+  WheelEvent wheelEvent(true, msg, mGeckoChild);
> >+  [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelEvent];
> >+  mExpectingWheelStop = (msg == NS_WHEEL_START);
> >+  mGeckoChild->DispatchWindowEvent(wheelEvent);
> >+}
> 
> I worry this code. You don't initialize some specific members of
> widget::WheelEvent here. However, some developers who to change ESM may
> assume that they are initialized with proper values...

I made the assumption that developers would access those members only for NS_WHEEL_WHEEL events because it's the only case where they make sense.

I can think of 2 alternatives:
1. Initialize by hand all fields of the event? In my opinion it's the constructor's job to do that. Should I change the constructor?
2. Create my own event class? It would be a pity as I share lot of code with NS_WHEEL_WHEEL events.

Comment 78

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

(In reply to André Reinald from comment #77)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #76)
> > > bool
> > > nsMouseWheelTransaction::UpdateTransaction(widget::WheelEvent* aEvent)
> > > {
> > >   nsIScrollableFrame* sf = GetTargetFrame()->GetScrollTargetFrame();
> > >   NS_ENSURE_TRUE(sf, false);
> > > 
> > >+  SetTimeout();
> > >+
> > >   if (!CanScrollOn(sf, aEvent->deltaX, aEvent->deltaY)) {
> > >     OnFailToScrollTarget();
> > >     // We should not modify the transaction state when the view will not be
> > >     // scrolled actually.
> > >     return false;
> > >   }
> > > 
> > >-  SetTimeout();
> > >-
> > 
> > I don't understand why you need this change.
> 
> To reset the timeout even if we'd return false on the previous test.
> I assumed it would make more sense, but I may have been wrong.
> Is this change doing any harm or is it just useless?

I guess that this causes bug 442774 again.

> > > void
> > > nsMouseWheelTransaction::OnEvent(nsEvent* aEvent)
> > > {
> > >-  if (!sTargetFrame)
> > >+  if (aEvent->message == NS_WHEEL_START) {
> > >+    sStayActiveUntilWheelStop = true;
> > >+  }
> > >+
> > >+  if (!sTargetFrame) {
> > >     return;
> > >+  }
> > > 
> > >   if (OutOfTime(sTime, GetTimeoutTime())) {
> > >     // Even if the scroll event which is handled after timeout, but onTimeout
> > >     // was not fired by timer, then the scroll event will scroll old frame,
> > >     // therefore, we should call OnTimeout here and ensure to finish the old
> > >     // transaction.
> > >     OnTimeout(nullptr, nullptr);
> > >     return;
> > >   }
> > > 
> > >   switch (aEvent->message) {
> > >+    case NS_WHEEL_STOP:
> > >+      sStayActiveUntilWheelStop = false;
> > >+      EndTransaction();
> > >+      break;
> > 
> > I don't understand well the timing of coming NS_WHEEL_STOP here.
> > 
> > If the event is fired when user leaves fingers from touchpad, calling
> > EndTransaction() breaks our wheel transaction. If user tries to scroll a
> > lot, the scroll target must not be changed at leaving the fingers from
> > touchpad.
> 
> I think the scroll should not end (because of momentum), but the transaction
> itself should. Therefore, by design, when handling NS_WHEEL_START events, I
> assume a new transaction will start (which means EndTransaction() was
> called). So not calling EndTransaction() here would imply other changes I
> believe.

I disagree with aborting scroll transaction at leaving the fingers. E.g., when user tries to scroll the document root, then, at leaving fingers from touchpad, the mouse cursor may be on another scrollable frame such as <iframe> or <div style="overflow: auto;". Then, the next scroll target becomes the another scrollable frame, not the document root. This means your change completely breaks current wheel transaction mechanism.

So, I believe that EndTransaction() should be called only when the transaction is finished by time-out. I think the wheel transaction and activating scroll bars are similar, but not exactly same.

> > >+void
> > >+nsEventStateManager::TemporarilyActivateAllPossibleScrollTargets(nsIFrame* aTargetFrame,
> > >+                                                      mozilla::widget::WheelEvent* aEvent)
> > >+{
> > >+  static const double directions[4][2] = {
> > >+    { 0, -1 }, { 0, +1 }, { -1, 0 }, { +1, 0 }
> > >+  };
> > 
> > I've not tested actually, this might cause some warnings at building with
> > MSVC. Could you append .0 for all of them?
> 
> Haven't tested neither, I just did a trybuild and it was ok on all platforms.
> I'll add .0s anyway, it can't do any harm.

Thanks. VC sometimes outputs warnings for safe auto casting between constant value and float or double type. I don't understand well the condition, though.

> > >+void
> > >+nsEventStateManager::DeactivateAllTemporarilyActivatedScrollTargets()
> > >+{
> > >+  for (int32_t i = 0; i < 4; i++) {
> > 
> > 4 looks like a magic number. Could you use
> > ArrayLength(mActivatedScrollTargets), instead?
> 
> Done (in Activate and Deactivate).
> Though I'm not satisfied because 4 still remains a "magic number" at
> directions[4][2] and mActivatedScrollTargets[4]. I'd better use a constant
> but I think it's overkill.

Thanks!

> 
> > >+    nsWeakFrame* scrollTarget = mActivatedScrollTargets + i;
> > 
> > And using mActivatedScrollTargets[i] is better.
> 
> Done (in Activate and Deactivate).
> Though I prefer mActivatedScrollTargets + i.

I believe that |mActivateScrollTargets + i| looks like adding number at least a moment. Therefore, I believe that [] is better especially like in this case, mActivatedScrollTargets isn't defined in the method locally.

> > >@@ -3147,39 +3232,47 @@ nsEventStateManager::PostHandleEvent(nsP
> > >       nsIPresShell *shell = presContext->GetPresShell();
> > >       if (shell) {
> > >         nsRefPtr<nsFrameSelection> frameSelection = shell->FrameSelection();
> > >         frameSelection->SetMouseDownState(false);
> > >       }
> > >     }
> > >     break;
> > >   case NS_WHEEL_WHEEL:
> > >+  case NS_WHEEL_START:
> > >+  case NS_WHEEL_STOP:
> > >     {
> > >       MOZ_ASSERT(aEvent->mFlags.mIsTrusted);
> > > 
> > >       if (*aStatus == nsEventStatus_eConsumeNoDefault) {
> > >         break;
> > >       }
> > > 
> > >       widget::WheelEvent* wheelEvent = static_cast<widget::WheelEvent*>(aEvent);
> > >       switch (WheelPrefs::GetInstance()->ComputeActionFor(wheelEvent)) {
> > >         case WheelPrefs::ACTION_SCROLL: {
> > >-          if (!wheelEvent->deltaX && !wheelEvent->deltaY) {
> > >+          if (aEvent->message == NS_WHEEL_WHEEL &&
> > >+              !wheelEvent->deltaX && !wheelEvent->deltaY) {
> > >             break;
> > >           }
> > >           // For scrolling of default action, we should honor the mouse wheel
> > >           // transaction.
> > >+          if (aEvent->message == NS_WHEEL_START) {
> > >+            TemporarilyActivateAllPossibleScrollTargets(aTargetFrame, wheelEvent);
> > >+          } else {
> > >+            DeactivateAllTemporarilyActivatedScrollTargets();
> > >+          }
> > 
> > I guess that this should be preformed even if the event is consumed by web
> > pages, isn't it? If so, you need this move up to before:
> > 
> > >       if (*aStatus == nsEventStatus_eConsumeNoDefault) {
> > 
> > And I think that following code shouldn't be preformed with _START or _STOP,
> > right?
> 
> I have no idea of what consuming events means (though I can make some sense
> from English).
> So please either be more precise so I can make the changes you suggest, or
> make them for me and I'll try to understand.

"consumed" means that the event's preventDefault() method has been called by somebody. Then, typically, wheel event triggered some reactions which are performed by the event handler which called preventDefault(). So, such event handlers may want to prevent the event's default action for preventing "double" reaction.

So, if DOM wheel event's preventDefault() is called, DeactivateAllTemporarilyActivatedScrollTargets() is never called from here. Then, the scrollbars stay there, unexpectedly.

> > >diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
> > >--- a/widget/cocoa/nsChildView.mm
> > >+++ b/widget/cocoa/nsChildView.mm
> > >@@ -4729,34 +4730,62 @@ NSEvent* gLastDragMouseDownEvent = nil;
> > > }
> > > 
> > > static int32_t RoundUp(double aDouble)
> > > {
> > >   return aDouble < 0 ? static_cast<int32_t>(floor(aDouble)) :
> > >                        static_cast<int32_t>(ceil(aDouble));
> > > }
> > > 
> > >+- (void)sendWheelStartOrStop:(uint32_t)msg forEvent:(NSEvent *)theEvent
> > >+{
> > >+  WheelEvent wheelEvent(true, msg, mGeckoChild);
> > >+  [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelEvent];
> > >+  mExpectingWheelStop = (msg == NS_WHEEL_START);
> > >+  mGeckoChild->DispatchWindowEvent(wheelEvent);
> > >+}
> > 
> > I worry this code. You don't initialize some specific members of
> > widget::WheelEvent here. However, some developers who to change ESM may
> > assume that they are initialized with proper values...
> 
> I made the assumption that developers would access those members only for
> NS_WHEEL_WHEEL events because it's the only case where they make sense.
> 
> I can think of 2 alternatives:
> 1. Initialize by hand all fields of the event? In my opinion it's the
> constructor's job to do that. Should I change the constructor?
> 2. Create my own event class? It would be a pity as I share lot of code with
> NS_WHEEL_WHEEL events.

I think that some _START and _STOP event information are used by ESM. E.g., deciding its default action. So, #2 causes a lot of changes. How about to create new methods which initialize all members of WheelEvent except deltaMode and delta[X-Z]? Then, the initialization code can be shared between _WHEEL and _START/_STOP.

Comment 79

[André Reinald]

Agree with almost all points from your last comment, but need clarifications for those:

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #78)
> I disagree with aborting scroll transaction at leaving the fingers. E.g.,
> when user tries to scroll the document root, then, at leaving fingers from
> touchpad, the mouse cursor may be on another scrollable frame such as
> <iframe> or <div style="overflow: auto;". Then, the next scroll target
> becomes the another scrollable frame, not the document root. This means your
> change completely breaks current wheel transaction mechanism.
> 
> So, I believe that EndTransaction() should be called only when the
> transaction is finished by time-out. I think the wheel transaction and
> activating scroll bars are similar, but not exactly same.

The transaction and the scrollbar visibility have to always match the same scrollable frame or we'll have inconsistent UI. So they are really linked.
I guess we are discussing a UI matter here:
- 10.7 lacks the needed cocoa events and so is timer based for both transaction and visibility,
- 10.8 adds the needed cocoa events and is based on 2 fingers down when applicable.

We have bug 868659 asking to keep scrollbars visible until 2 fingers rise. That of course applies only to 10.8. In that case the scrollbars should stay visible as long as the 2 fingers are down, and the transaction should stay active too i.e. don't change the scrolled frame.

I agree the timer based mechanism makes sense in 10.7 when the OS could not transmit 2 fingers down events. That's the purpose of sStayActiveUntilWheelStop: it's set on _START events, and disables the timer.

I suggest we change to something like:

  switch (aEvent->message) {
    case NS_WHEEL_STOP:
      if (sStayActiveUntilWheelStop) {
        sStayActiveUntilWheelStop = false;
        EndTransaction();
      }
      break;

This would keep the current behavior unchanged on 10.7 while adopting the new behavior on 10.8.

To make things even clearer, I could completely disable generation of _START/_STOP events on 10.7 and earlier. How about that?

> > I can think of 2 alternatives:
> > 1. Initialize by hand all fields of the event? In my opinion it's the
> > constructor's job to do that. Should I change the constructor?
> > 2. Create my own event class? It would be a pity as I share lot of code with
> > NS_WHEEL_WHEEL events.
> 
> I think that some _START and _STOP event information are used by ESM. E.g.,
> deciding its default action. So, #2 causes a lot of changes. How about to
> create new methods which initialize all members of WheelEvent except
> deltaMode and delta[X-Z]? Then, the initialization code can be shared
> between _WHEEL and _START/_STOP.

Can you please suggest which members I should initialize and to what values for _START/_STOP events?
As far as I can tell, current ESM code only needs to know which frame those events belong to, and I can't see which other useful info they could carry.

Comment 80

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

(In reply to André Reinald from comment #79)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #78)
> > I disagree with aborting scroll transaction at leaving the fingers. E.g.,
> > when user tries to scroll the document root, then, at leaving fingers from
> > touchpad, the mouse cursor may be on another scrollable frame such as
> > <iframe> or <div style="overflow: auto;". Then, the next scroll target
> > becomes the another scrollable frame, not the document root. This means your
> > change completely breaks current wheel transaction mechanism.
> > 
> > So, I believe that EndTransaction() should be called only when the
> > transaction is finished by time-out. I think the wheel transaction and
> > activating scroll bars are similar, but not exactly same.
> 
> The transaction and the scrollbar visibility have to always match the same
> scrollable frame or we'll have inconsistent UI. So they are really linked.
> I guess we are discussing a UI matter here:
> - 10.7 lacks the needed cocoa events and so is timer based for both
> transaction and visibility,
> - 10.8 adds the needed cocoa events and is based on 2 fingers down when
> applicable.
> 
> We have bug 868659 asking to keep scrollbars visible until 2 fingers rise.
> That of course applies only to 10.8. In that case the scrollbars should stay
> visible as long as the 2 fingers are down, and the transaction should stay
> active too i.e. don't change the scrolled frame.
> 
> I agree the timer based mechanism makes sense in 10.7 when the OS could not
> transmit 2 fingers down events. That's the purpose of
> sStayActiveUntilWheelStop: it's set on _START events, and disables the timer.
> 
> I suggest we change to something like:
> 
>   switch (aEvent->message) {
>     case NS_WHEEL_STOP:
>       if (sStayActiveUntilWheelStop) {
>         sStayActiveUntilWheelStop = false;
>         EndTransaction();
>       }
>       break;
> 
> This would keep the current behavior unchanged on 10.7 while adopting the
> new behavior on 10.8.
> 
> To make things even clearer, I could completely disable generation of
> _START/_STOP events on 10.7 and earlier. How about that?

I'd like to confirm the fact that. When user's two fingers are touched to touchpad, _START is fired. And when user's two fingers are left from touchpad, _STOP is fired, right? If so, I still disagree with it. I think that the wheel transaction system is really different system from Mac OS X 10.8's new feature.

The wheel transaction mechanism isn't emulating any platform's native behavior. It was implemented for improving usability on all platforms which has mouse wheel operation. So, killed only on 10.8 doesn't make sense for me. The important point of wheel transaction mechanism is, user can scroll only one target until timeout or moving cursor.

Anyway, we should focus to change the scrollbar visibility in this bug. If you still believe that our current implementation doesn't make sense for Mac OS 10.8 users, please file a new bug. It should be able to be backed out independently.

> > > I can think of 2 alternatives:
> > > 1. Initialize by hand all fields of the event? In my opinion it's the
> > > constructor's job to do that. Should I change the constructor?
> > > 2. Create my own event class? It would be a pity as I share lot of code with
> > > NS_WHEEL_WHEEL events.
> > 
> > I think that some _START and _STOP event information are used by ESM. E.g.,
> > deciding its default action. So, #2 causes a lot of changes. How about to
> > create new methods which initialize all members of WheelEvent except
> > deltaMode and delta[X-Z]? Then, the initialization code can be shared
> > between _WHEEL and _START/_STOP.
> 
> Can you please suggest which members I should initialize and to what values
> for _START/_STOP events?
> As far as I can tell, current ESM code only needs to know which frame those
> events belong to, and I can't see which other useful info they could carry.

I don't say that current patch causes any problems now. However, it might make some developers confused in the future. Therefore, the values should be initialized as far as possible.

I think we should initialize following members in common initializer such as convertCocoaScrollWheelEvent:

* deltaMode
* isMomentum

Note that the most important thing is to make new method for initializing common members of WheelEvent.

Comment 81

[André Reinald]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #80)
> I'd like to confirm the fact that. When user's two fingers are touched to
> touchpad, _START is fired. And when user's two fingers are left from
> touchpad, _STOP is fired, right? If so, I still disagree with it. I think
> that the wheel transaction system is really different system from Mac OS X
> 10.8's new feature.
> 
> The wheel transaction mechanism isn't emulating any platform's native
> behavior. It was implemented for improving usability on all platforms which
> has mouse wheel operation. So, killed only on 10.8 doesn't make sense for
> me. The important point of wheel transaction mechanism is, user can scroll
> only one target until timeout or moving cursor.
> 
> Anyway, we should focus to change the scrollbar visibility in this bug. If
> you still believe that our current implementation doesn't make sense for Mac
> OS 10.8 users, please file a new bug. It should be able to be backed out
> independently.

If scrollwheel transaction is not to reflect any native system's behavior, I understand your concern better now.

But we can't fix scrollbar visibility independently from scrollwheel transaction without risking inconsistent UI i.e. one scrollbar is visible while another frame is scrolled. We have to make sure the two always stay in sync. Does my concern makes sense?

Maybe one solution would be, on _START events, to first check if wheel transaction is active, in which case we take it's frame. Otherwise fallback to current behavior. On _STOP events, keep doing what we do now: hide the scrollbars, but don't reset scrollwheel transaction. Do you think that would work?

> I think we should initialize following members in common initializer such as
> convertCocoaScrollWheelEvent:
> 
> * deltaMode
> * isMomentum
> 
> Note that the most important thing is to make new method for initializing
> common members of WheelEvent.

I can't see how those members have meaning for _START/_STOP events, but I'm fine initializing them. To what value should deltaMode be initialized? And why shouldn't we initialize them inside the WheelEvent constructor?

Comment 82

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

(In reply to André Reinald from comment #81)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #80)
> > I'd like to confirm the fact that. When user's two fingers are touched to
> > touchpad, _START is fired. And when user's two fingers are left from
> > touchpad, _STOP is fired, right? If so, I still disagree with it. I think
> > that the wheel transaction system is really different system from Mac OS X
> > 10.8's new feature.
> > 
> > The wheel transaction mechanism isn't emulating any platform's native
> > behavior. It was implemented for improving usability on all platforms which
> > has mouse wheel operation. So, killed only on 10.8 doesn't make sense for
> > me. The important point of wheel transaction mechanism is, user can scroll
> > only one target until timeout or moving cursor.
> > 
> > Anyway, we should focus to change the scrollbar visibility in this bug. If
> > you still believe that our current implementation doesn't make sense for Mac
> > OS 10.8 users, please file a new bug. It should be able to be backed out
> > independently.
> 
> If scrollwheel transaction is not to reflect any native system's behavior, I
> understand your concern better now.
> 
> But we can't fix scrollbar visibility independently from scrollwheel
> transaction without risking inconsistent UI i.e. one scrollbar is visible
> while another frame is scrolled. We have to make sure the two always stay in
> sync. Does my concern makes sense?
> 
> Maybe one solution would be, on _START events, to first check if wheel
> transaction is active, in which case we take it's frame. Otherwise fallback
> to current behavior. On _STOP events, keep doing what we do now: hide the
> scrollbars, but don't reset scrollwheel transaction. Do you think that would
> work?

Hmm, I might misunderstand your patch. I understood as:

1. at _START, scrollbars of all possible scrollable elements from the cursor position are activated.
2. at _WHEEL, only the scrollbars of decided scroll target element.
3. at _STOP, all scrollbars are hidden.

Isn't it?

If so, I don't understand why not calling EndTransaction() at _STOP causes mismatching actual scroll target and scrollbar visible element.

> > I think we should initialize following members in common initializer such as
> > convertCocoaScrollWheelEvent:
> > 
> > * deltaMode
> > * isMomentum
> > 
> > Note that the most important thing is to make new method for initializing
> > common members of WheelEvent.
> 
> I can't see how those members have meaning for _START/_STOP events, but I'm
> fine initializing them. To what value should deltaMode be initialized? And
> why shouldn't we initialize them inside the WheelEvent constructor?

WheelEvent constructor cannot access the native event due to abstract class for all platforms.

I think that you just move:

> 4755   [self convertCocoaMouseEvent:theEvent toGeckoEvent:&wheelEvent];
> 4756   wheelEvent.deltaMode =
> 4757     Preferences::GetBool("mousewheel.enable_pixel_scrolling", true) ?
> 4758       nsIDOMWheelEvent::DOM_DELTA_PIXEL : nsIDOMWheelEvent::DOM_DELTA_LINE;
> 4759 
> 4760   // Calling deviceDeltaX or deviceDeltaY on theEvent will trigger a Cocoa
> 4761   // assertion and an Objective-C NSInternalInconsistencyException if the
> 4762   // underlying "Carbon" event doesn't contain pixel scrolling information.
> 4763   // For these events, carbonEventKind is kEventMouseWheelMoved instead of
> 4764   // kEventMouseScroll.
> 4765   if (wheelEvent.deltaMode == nsIDOMWheelEvent::DOM_DELTA_PIXEL) {
> 4766     EventRef theCarbonEvent = [theEvent _eventRef];
> 4767     UInt32 carbonEventKind = theCarbonEvent ? ::GetEventKind(theCarbonEvent) : 0;
> 4768     if (carbonEventKind != kEventMouseScroll) {
> 4769       wheelEvent.deltaMode = nsIDOMWheelEvent::DOM_DELTA_LINE;
> 4770     }
> 4771   }

and

> 4803   wheelEvent.isMomentum = nsCocoaUtils::IsMomentumScrollEvent(theEvent);

to the new method. I.e., convertCocoaMouseEvent should be called by the new method.

Comment 83

[André Reinald]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #82)
> Hmm, I might misunderstand your patch. I understood as:
> 
> 1. at _START, scrollbars of all possible scrollable elements from the cursor
> position are activated.
> 2. at _WHEEL, only the scrollbars of decided scroll target element.
> 3. at _STOP, all scrollbars are hidden.
> 
> Isn't it?
> 
> If so, I don't understand why not calling EndTransaction() at _STOP causes
> mismatching actual scroll target and scrollbar visible element.

You understood my patch right.

Not calling EndTransaction() on _STOP events would mean that, until the timer fires, the wheel transaction will keep the focus on the same scroll frame.

If during this time, the user touches the trackpad again, scrollbar visibility will be computed again and could result in a different scroll frame's scrollbars to be shown, hence the inconsistency. This is why I suggest we just modify the computation of scrollbar visibility on _START and _WHEEL events to match the active wheel transaction if any.

Comment 84

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

(In reply to André Reinald from comment #83)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #82)
> > Hmm, I might misunderstand your patch. I understood as:
> > 
> > 1. at _START, scrollbars of all possible scrollable elements from the cursor
> > position are activated.
> > 2. at _WHEEL, only the scrollbars of decided scroll target element.
> > 3. at _STOP, all scrollbars are hidden.
> > 
> > Isn't it?
> > 
> > If so, I don't understand why not calling EndTransaction() at _STOP causes
> > mismatching actual scroll target and scrollbar visible element.
> 
> You understood my patch right.
> 
> Not calling EndTransaction() on _STOP events would mean that, until the
> timer fires, the wheel transaction will keep the focus on the same scroll
> frame.
> 
> If during this time, the user touches the trackpad again, scrollbar
> visibility will be computed again and could result in a different scroll
> frame's scrollbars to be shown, hence the inconsistency. This is why I
> suggest we just modify the computation of scrollbar visibility on _START and
> _WHEEL events to match the active wheel transaction if any.

Thanks. Then, how about following approach?

1. When ESM receives _START and there is no wheel transaction, show all possible scrollable element's scrollbars (current behavior of your patch).
2. When ESM receives _START and there is wheel transaction, update transaction and show the scrollbars of scroll target.
3. When ESM receives _STOP and there is wheel transaction, just update the transaction.
4. When ESM receives _STOP and there is no wheel transaction, just ignore.
5. When wheel transaction is timed-out, hide all scrollbars.

Then, I think that we don't meet the mismatch which you worry about. Do I misunderstand?

Comment 85

[André Reinald]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #84)
> Thanks. Then, how about following approach?
> ...
> Then, I think that we don't meet the mismatch which you worry about. Do I
> misunderstand?

Mostly agree:

1. agree: done
2. agree: to do
3. disagree: _STOP events should have no effect on wheel transaction, or did I misunderstand?
4. disagree: same reason
5. agree: if we had no _START event, scrollbars should disappear on wheel transaction timeout
6. plus: if we had a _START event, scrollbars should disappear on a _STOP event

Messages are becoming shorter, good sign!

Comment 86

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

(In reply to André Reinald from comment #85)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #84)
> > Thanks. Then, how about following approach?
> > ...
> > Then, I think that we don't meet the mismatch which you worry about. Do I
> > misunderstand?
> 
> Mostly agree:
> 
> 3. disagree: _STOP events should have no effect on wheel transaction, or did
> I misunderstand?

Although, it isn't serious matter. By dispatching _START and _STOP on 10.8, it causes a little delay between _WHEEL events. If it causes timeout of wheel transaction, 10.8 users' transaction is kept shorter than other platforms. However, it's trivial difference, I guess. So, if you don't like to do it or the code becomes complicated, I think that ignoring the event is okay.

> 4. disagree: same reason

Do you disagree with #4 due to same reason as #3? 

> 6. plus: if we had a _START event, scrollbars should disappear on a _STOP
> event

If there is wheel transaction, I think that the scrollbars of scroll target should keep visible. Then, user can understand the current scroll target visually (at timeout of the wheel transaction, the scrollbars automatically hidden).

If there is no wheel transaction, hiding all temporary scrollbars is good behavior.

Therefore, I think that _STOP should be ignored or just update the transaction.

So, _STOP should be used only for hiding all temporary scrollbars if wheel transaction hasn't been started after _START.

> Messages are becoming shorter, good sign!

Agree ;-)

Comment 87

[André Reinald]

Attachment: bugzilla-868648-11.patch

So I removed the offending nsMouseWheelTransaction::EndTransaction() calls, and the now useless nsMouseWheelTransaction::sStayActiveUntilWheelStop member.

After careful reading of nsEventStateManager::ComputeScrollTarget() I can see there is already an option PREFER_MOUSE_WHEEL_TRANSACTION which does what we want. This option is actually set when calling from nsEventStateManager::PostHandleEvent() or even from nsEventStateManager::TemporarilyActivateAllPossibleScrollTargets(). So point 2 above will be fine.

PS: I started in a wrong direction (patch 10 which I didn't send) and at some point had to revert to the previous patch (9), then hand re-applied the "good" changes. I hope everything is fine now (patch 11).

Comment 88

[André Reinald]

Comment on attachment 804482 [details] [diff] [review]
bugzilla-868648-11.patch

Cancelling review : after this patch, the scrollbars disappear after a timeout if a scroll has been performed. They still wait for WHEEL_STOP if no scroll was performed. This is true even after I've removed calls to ScrollbarActivityStarted/Stopped() from nsMouseWheelTransaction::Begin/EndTransaction(). Will work some more on it.

Comment 89

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

FYI: this landing renames mozilla::widget::WheelEvent to mozilla::WheelEvent.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dbc32740161

So, this perhaps breaks your patch. You can fix it with just removing "widget::" before WheelEvent in your patch.

Comment 90

[André Reinald]

Attachment: bugzilla-868648-12.patch

Working fine now as far as I could see. Added 2 static variables inside nsEventStateManager::PostHandleEvent(). Not happy about this, but they had nothing to do inside the nsMouseWheelTransaction class. Plus their use is very local. If you have a better suggestion...

Comment 91

[Alex Keybl [:akeybl]]

Didn't block FF24's release, so no need to track for FF25. We'll consider an uplift request when ready.

Comment 92

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 805228 [details] [diff] [review]
bugzilla-868648-12.patch

>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp
>@@ -289,69 +289,72 @@ protected:
>   static void OnFailToScrollTarget();
>   static void OnTimeout(nsITimer *aTimer, void *aClosure);
>   static void SetTimeout();
>   static uint32_t GetIgnoreMoveDelayTime();
>   static int32_t GetAccelerationStart();
>   static int32_t GetAccelerationFactor();
>   static DeltaValues OverrideSystemScrollSpeed(widget::WheelEvent* aEvent);
>   static double ComputeAcceleratedWheelDelta(double aDelta, int32_t aFactor);
>+  static bool OutOfTime(uint32_t aBaseTime, uint32_t aThreshold);
> 
>   static nsWeakFrame sTargetFrame;
>   static uint32_t    sTime;        // in milliseconds
>   static uint32_t    sMouseMoved;  // in milliseconds
>   static nsITimer*   sTimer;
>   static int32_t     sScrollSeriesCounter;
>+  // this flag indicates that we have some temporary scrollbar activity
>+  // it is set to true on WHEEL_START events (which mean 2 fingers down on touchpad)

This comment looks like not needed.

> void
> nsMouseWheelTransaction::BeginTransaction(nsIFrame* aTargetFrame,
>                                           widget::WheelEvent* aEvent)
> {
>   NS_ASSERTION(!sTargetFrame, "previous transaction is not finished!");
>   sTargetFrame = aTargetFrame;
>   sScrollSeriesCounter = 0;
>-  if (!UpdateTransaction(aEvent)) {
>+  if (aEvent->message == NS_WHEEL_WHEEL && !UpdateTransaction(aEvent)) {
>     NS_ERROR("BeginTransaction is called even cannot scroll the frame");
>     EndTransaction();
>   }
> }

BeginTransaction() is called only from DoScrollText() which is called only when _WHEEL is handled. So, I think that you should just add:
MOZ_ASSERT(aEvent->message == NS_WHEEL_WHEEL,
           "Transaction must be started with a wheel event");
at below of the NS_ASSERTION(!sTargetFrame, ...).

>@@ -387,18 +390,19 @@ nsMouseWheelTransaction::EndTransaction(
>     sTimer->Cancel();
>   sTargetFrame = nullptr;
>   sScrollSeriesCounter = 0;
> }
> 
> void
> nsMouseWheelTransaction::OnEvent(nsEvent* aEvent)
> {
>-  if (!sTargetFrame)
>+  if (!sTargetFrame) {
>     return;
>+  }

You should remove this change for clearer annotation since you don't change around here.

>@@ -3147,39 +3211,76 @@ nsEventStateManager::PostHandleEvent(nsP
>       nsIPresShell *shell = presContext->GetPresShell();
>       if (shell) {
>         nsRefPtr<nsFrameSelection> frameSelection = shell->FrameSelection();
>         frameSelection->SetMouseDownState(false);
>       }
>     }
>     break;
>   case NS_WHEEL_WHEEL:
>+  case NS_WHEEL_START:
>+  case NS_WHEEL_STOP:
>     {
>       MOZ_ASSERT(aEvent->mFlags.mIsTrusted);
> 
>       if (*aStatus == nsEventStatus_eConsumeNoDefault) {
>+        DeactivateAllTemporarilyActivatedScrollTargets();
>         break;
>       }
> 
>       widget::WheelEvent* wheelEvent = static_cast<widget::WheelEvent*>(aEvent);
>       switch (WheelPrefs::GetInstance()->ComputeActionFor(wheelEvent)) {
>         case WheelPrefs::ACTION_SCROLL: {
>-          if (!wheelEvent->deltaX && !wheelEvent->deltaY) {
>+          // For scrolling of default action, we should honor the mouse wheel
>+          // transaction.
>+          
>+          // Those 2 static variables hold the state of visible scrollbars in case
>+          // of _START/_STOP show/hide behavior. See bugs 868648 and 868659.
>+          static bool sHadWheelStart = false;
>+          static nsWeakFrame sActiveScrollbarOwner = nullptr;

Okay, then, could you create a singleton class nsEventStateManager::ScrollbarForWheel in nsEventStateManager.h, like nsEventStateManager::DeltaAccumulator class? You can capsule the messy stuff of the code for related bugs into your own class. I'm thinking it as:

class ScrollbarsForWheel MOZ_FINAL
{
public:
  static ScrollbarsForWheel* GetInstance()
  {
    if (!sInstance) {
      sInstance = new ScrollbarForWheel();
    }
    return sInstance;
  }

  void PrepareToScrollText(nsEventStateManager* aESM,
                           mozilla::WheelEvent* aEvent);
  void SetActiveScrollTarget(nsIScrollableFrame* aScrollTarget);
  // Hide all scrollbars (both mActiveOwner's and mActivatedScrollTargets')
  void Hide();

private:
  ScrollbarsForWheel() :
    mHadWheelStart(false)
  {
  }

  static ScrollbarForWheel* sInstance;

  nsWeakFrame mActiveOwner; // for sActiveScrollbarOwner
  nsWeakFrame mActivatedScrollTargets[4];

  bool mHadWheelStart; // for sHadWheelStart

  void TemporarilyActivateAllPossibleScrollTargets(
         nsEventStateManager* aESM,
         nsIFrame* aTargetFrame,
         mozilla::WheelEvent* aEvent);
  void DeactivateAllTemporarilyActivatedScrollTargets();
};

>+
>+          if (aEvent->message == NS_WHEEL_START) {
>+            TemporarilyActivateAllPossibleScrollTargets(aTargetFrame, wheelEvent);
>+            if (!sActiveScrollbarOwner)
>+              sHadWheelStart = true;
>+          } else {
>+            DeactivateAllTemporarilyActivatedScrollTargets();
>+          }
>+          
>+          if (aEvent->message == NS_WHEEL_STOP && sActiveScrollbarOwner) {
>+            nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(sActiveScrollbarOwner);
>+            if (scrollbarOwner) {
>+              scrollbarOwner->ScrollbarActivityStopped();
>+            }
>+            sActiveScrollbarOwner = nullptr;
>+          }

Move this into ScrollbarsForWheel::PrepareToScrollText().

And move the latter block into ScrollbarsForWheel::Hide(). Additionally, Hide() also calls DeactivateAllTemporarilyActivatedScrollTargets().

>+          
>+          if (aEvent->message != NS_WHEEL_WHEEL ||
>+              (!wheelEvent->deltaX && !wheelEvent->deltaY)) {
>             break;
>           }
>-          // For scrolling of default action, we should honor the mouse wheel
>-          // transaction.
>+
>           nsIScrollableFrame* scrollTarget =
>             ComputeScrollTarget(aTargetFrame, wheelEvent,
>                                 COMPUTE_DEFAULT_ACTION_TARGET);
>+
>+          if (sHadWheelStart) {
>+            nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(scrollTarget);
>+            if (scrollbarOwner) {
>+              sActiveScrollbarOwner = do_QueryFrame(scrollTarget);
>+              scrollbarOwner->ScrollbarActivityStarted();
>+              sHadWheelStart = false;
>+            }
>+          }

Move this into ScrollbarsForWheel::SetActiveScrollTarget().

>+
>           wheelEvent->overflowDeltaX = wheelEvent->deltaX;
>           wheelEvent->overflowDeltaY = wheelEvent->deltaY;
>           WheelPrefs::GetInstance()->
>             CancelApplyingUserPrefsFromOverflowDelta(wheelEvent);
>-          if (scrollTarget) {
>+          if (scrollTarget && aEvent->message == NS_WHEEL_WHEEL) {

This change must not be necessary because you check this already.

>             DoScrollText(scrollTarget, wheelEvent);
>           } else {
>             nsMouseWheelTransaction::EndTransaction();
>           }
>           break;
>         }
>         case WheelPrefs::ACTION_HISTORY: {
>           // If this event doesn't cause NS_MOUSE_SCROLL event or the direction

I think that we need to call ScrollbarsForWheel::Hide() when wheel event causes the other default action. Therefore, you need to call it at timeout of wheel transaction. (i.e., need to call from nsMouseWheelTransaction::EndTransaction(), you can call it with nsEventStateManager::ScrollbarsForWheel::GetInstance()->Hide())

Comment 93

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Oops, please don't forget to create ScrollbarsForWheel::Shutdown() for destroying the singleton instance. It should be called from nsEventStateManager::~nsEventStateManager() if it's instance count becomes 0.

Comment 94

[André Reinald]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #92)
> Comment on attachment 805228 [details] [diff] [review]
> bugzilla-868648-12.patch
> >+  // this flag indicates that we have some temporary scrollbar activity
> >+  // it is set to true on WHEEL_START events (which mean 2 fingers down on touchpad)
> 
> This comment looks like not needed.

Ok.

> BeginTransaction() is called only from DoScrollText() which is called only
> when _WHEEL is handled. So, I think that you should just add:
> MOZ_ASSERT(aEvent->message == NS_WHEEL_WHEEL,
>            "Transaction must be started with a wheel event");
> at below of the NS_ASSERTION(!sTargetFrame, ...).

Ok.

> > void
> > nsMouseWheelTransaction::OnEvent(nsEvent* aEvent)
> > {
> >-  if (!sTargetFrame)
> >+  if (!sTargetFrame) {
> >     return;
> >+  }
> 
> You should remove this change for clearer annotation since you don't change
> around here.

Ok. So you're saying that clearer annotations are a priority over respecting formatting conventions? I had to do the same change for my own patch a few lines below, so I thought correcting this one would be fine too.

> Okay, then, could you create a singleton class
> nsEventStateManager::ScrollbarForWheel in nsEventStateManager.h, like
> nsEventStateManager::DeltaAccumulator class? You can capsule the messy stuff
> of the code for related bugs into your own class. I'm thinking it as:

In this case I'd rather use an all static class similar to nsScrollWheelTransaction. Then we only have to declare it in nsEventStateManager.cpp and we won't need to allocate and deallocate anything, just keep track of a few nsWeakFrame objects.
Is this fine with you?

> Move this into ScrollbarsForWheel::PrepareToScrollText().
> 
> And move the latter block into ScrollbarsForWheel::Hide(). Additionally,
> Hide() also calls DeactivateAllTemporarilyActivatedScrollTargets().

Right, I'll put most of my new stuff inside this new class. Following this and following recommendations.

> I think that we need to call ScrollbarsForWheel::Hide() when wheel event
> causes the other default action. Therefore, you need to call it at timeout
> of wheel transaction. (i.e., need to call from
> nsMouseWheelTransaction::EndTransaction(), you can call it with
> nsEventStateManager::ScrollbarsForWheel::GetInstance()->Hide())

I'd rather not do that because of bug 868659. I think it makes sense to address both bugs in the same patch because they are closely related.
Is this fine with you?

Comment 95

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

(In reply to André Reinald from comment #94)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #92)
> > > void
> > > nsMouseWheelTransaction::OnEvent(nsEvent* aEvent)
> > > {
> > >-  if (!sTargetFrame)
> > >+  if (!sTargetFrame) {
> > >     return;
> > >+  }
> > 
> > You should remove this change for clearer annotation since you don't change
> > around here.
> 
> Ok. So you're saying that clearer annotations are a priority over respecting
> formatting conventions? I had to do the same change for my own patch a few
> lines below, so I thought correcting this one would be fine too.

Yes. I've been reviewed so many times if the changing point is far from actual code change. If you'd like to do clean up them, file a new bug only for it and do it on all over the file.

> > Okay, then, could you create a singleton class
> > nsEventStateManager::ScrollbarForWheel in nsEventStateManager.h, like
> > nsEventStateManager::DeltaAccumulator class? You can capsule the messy stuff
> > of the code for related bugs into your own class. I'm thinking it as:
> 
> In this case I'd rather use an all static class similar to
> nsScrollWheelTransaction. Then we only have to declare it in
> nsEventStateManager.cpp and we won't need to allocate and deallocate
> anything, just keep track of a few nsWeakFrame objects.
> Is this fine with you?

Yeah, it sounds okay for me.

> > I think that we need to call ScrollbarsForWheel::Hide() when wheel event
> > causes the other default action. Therefore, you need to call it at timeout
> > of wheel transaction. (i.e., need to call from
> > nsMouseWheelTransaction::EndTransaction(), you can call it with
> > nsEventStateManager::ScrollbarsForWheel::GetInstance()->Hide())
> 
> I'd rather not do that because of bug 868659. I think it makes sense to
> address both bugs in the same patch because they are closely related.
> Is this fine with you?

Hmm, so, that means expanding wheel transaction only on Mac OS X. However, I don't mind the difference because I don't think that it's odd behavior. And perhaps, I understand your idea right now.

Then:
1. We need to *keep* wheel transaction between _START and _STOP.
2. _STOP event should cause hiding active scroll target's scrollbars without aborting wheel transaction.

Is this right?

Then, I think that nsMouseWheelTransaction::OnTimeout() needs to do extra work.

  void
  nsMouseWheelTransaction::OnTimeout(nsITimer* aTimer, void* aClosure)
  {
+   if (ScrollbarsForWheel::IsActive()) {
+     ScrollbarsForWheel::CallEndTransactionAtHidingScrollbar();
+     return;
+   }
    if (!sTargetFrame) {
      // The transaction target was destroyed already
      EndTransaction();

ScrollbarsForWheel::CallEndTransactionAtHidingScrollbar() sets a flag, and ScrollbarsForWheel::OnWheelStop() should call nsMouseWheelTransaction::EndTransaction() if the flag was set. Additionally, nsMouseWheelTransaction::OnWheelStop() must be called with any default action. So, I think that you should add independent case for NS_WHEEL_STOP in nsEventStateManager::PostHandleEvent().

Comment 96

[André Reinald]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #95)
> Then:
> 1. We need to *keep* wheel transaction between _START and _STOP.
> 2. _STOP event should cause hiding active scroll target's scrollbars without
> aborting wheel transaction.
> 
> Is this right?

Right, we can do that. It would have saved me some work to go that way from the start. But it's all right, after all, I learn from any changes I make :-)

Comment 97

[André Reinald]

After tweaking the code again and seeing there is now much stronger "partnership" between nsMouseWheelTransaction and nsScrollbarsForWheel, I believe it would make sense to merge both functionality into a new nsScrollingManager (hints for the new name welcome). It may require a little more refactoring than adding nsScrollbarsForWheel and changing nsMouseWheelTransaction, but it would certainly make the code easier to read in the future. What do you think?

Comment 98

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Um, I'm not sure what you image.

The wheel transaction feature and activating scrollbar are really different feature. So, if we'd merge them, it should be done carefully.

Anyway, I think that you should create only the new class in this bug. It's easier to focus/review the new feature implementation. If you'd like to merge them, please file a new bug for it after you fix this bug. I guess that you prefer to implement this feature as soon as possible. Creating only new class helps it.

Comment 99

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

And the separated new feature has lower risk if you request to uplift it.

Comment 100

[André Reinald]

Attachment: bugzilla-868648-13.patch

13th release of the patch. Let me know if there are still things we can improve.

Comment 101

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 807780 [details] [diff] [review]
bugzilla-868648-13.patch

>+/******************************************************************/
>+/* nsScrollbarsForWheel                                           */
>+/******************************************************************/
>+
>+class nsScrollbarsForWheel {
>+public:
>+  static void PrepareToScrollText(
>+                                   nsEventStateManager* aESM,
>+                                   nsIFrame* aTargetFrame,
>+                                   WheelEvent* aEvent);

nit: The first argument should be immediately after "(".

>+  static void SetActiveScrollTarget(nsIScrollableFrame* aScrollTarget);
>+  // Hide all scrollbars (both mActiveOwner's and mActivatedScrollTargets')
>+  static void Hide();
>+  static bool IsActive();
>+  static void CallEndTransactionAtHidingScrollbar();
>+
>+private:
>+  static const size_t         kNumberOfTargets = 4;
>+  static const DeltaValues    directions[kNumberOfTargets];
>+  static nsWeakFrame          sActiveOwner;
>+  static nsWeakFrame          sActivatedScrollTargets[kNumberOfTargets];
>+  static bool                 sHadWheelStart;
>+  static bool                 sCallEndTransactionAtHidingScrollbar;
>+
>+  /**
>+   * These two methods are called upon NS_WHEEL_START/NS_WHEEL_STOP events
>+   * to show/hide the right scrollbars.
>+   */
>+  static void TemporarilyActivateAllPossibleScrollTargets(
>+                                  nsEventStateManager* aESM,
>+                                  nsIFrame* aTargetFrame,
>+                                  WheelEvent* aEvent);
>+  static void DeactivateAllTemporarilyActivatedScrollTargets();
>+};
>+
>+const DeltaValues nsScrollbarsForWheel::directions[nsScrollbarsForWheel::kNumberOfTargets] = {
>+                                    DeltaValues(-1, 0),
>+                                    DeltaValues(+1, 0),
>+                                    DeltaValues(0, -1),
>+                                    DeltaValues(0, +1)
>+                                  };

Strange indent, just 2 white-spaces are okay.

>+nsWeakFrame nsScrollbarsForWheel::sActiveOwner = nullptr;
>+nsWeakFrame nsScrollbarsForWheel::sActivatedScrollTargets[kNumberOfTargets] = {
>+                                    nullptr,
>+                                    nullptr,
>+                                    nullptr,
>+                                    nullptr
>+                                  };

Same here.

>+void nsScrollbarsForWheel::PrepareToScrollText(
>+                                  nsEventStateManager* aESM,
>+                                  nsIFrame* aTargetFrame,
>+                                  WheelEvent* aEvent)
>+{
>+  if (aEvent->message == NS_WHEEL_START) {
>+    TemporarilyActivateAllPossibleScrollTargets(aESM, aTargetFrame, aEvent);
>+    if (!sActiveOwner)
>+      sHadWheelStart = true;

nit: use {} or sHadWheelStart = (sHadWheelStart || !sActiveOwner);

>+  } else {
>+    DeactivateAllTemporarilyActivatedScrollTargets();
>+  }
>+}
>+
>+void nsScrollbarsForWheel::SetActiveScrollTarget(nsIScrollableFrame* aScrollTarget)
>+{
>+  if (sHadWheelStart) {
>+    nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(aScrollTarget);
>+    if (scrollbarOwner) {
>+      sActiveOwner = do_QueryFrame(aScrollTarget);
>+      scrollbarOwner->ScrollbarActivityStarted();
>+    }
>+    sHadWheelStart = false;
>+  }
>+}

Use early return style. I.e.,

{
  if (!sHadWheelStart) {
    return;
  }
  sHadWheelStart = false;
  nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(aScrollTarget);
  if (!scrollbarOwner) {
    return;
  }
  sActiveOwner = do_QueryFrame(aScrollTarget);
  scrollbarOwner->ScrollbarActivityStarted();
}

>@@ -3147,36 +3321,52 @@ nsEventStateManager::PostHandleEvent(nsP
>       nsIPresShell *shell = presContext->GetPresShell();
>       if (shell) {
>         nsRefPtr<nsFrameSelection> frameSelection = shell->FrameSelection();
>         frameSelection->SetMouseDownState(false);
>       }
>     }
>     break;
>   case NS_WHEEL_WHEEL:
>+  case NS_WHEEL_START:
>+  case NS_WHEEL_STOP:
>     {
>       MOZ_ASSERT(aEvent->mFlags.mIsTrusted);
> 
>       if (*aStatus == nsEventStatus_eConsumeNoDefault) {
>+        nsScrollbarsForWheel::Hide();
>         break;
>       }
> 
>       WheelEvent* wheelEvent = static_cast<WheelEvent*>(aEvent);
>       switch (WheelPrefs::GetInstance()->ComputeActionFor(wheelEvent)) {
>         case WheelPrefs::ACTION_SCROLL: {
>-          if (!wheelEvent->deltaX && !wheelEvent->deltaY) {
>+          // For scrolling of default action, we should honor the mouse wheel
>+          // transaction.
>+          
>+          nsScrollbarsForWheel::PrepareToScrollText(this, aTargetFrame, wheelEvent);
>+          
>+          if (aEvent->message == NS_WHEEL_STOP) {
>+            nsScrollbarsForWheel::Hide();
>+          }

I still feel this odd. Shouldn't _STOP event *always* cause calling nsScrollbarsForWheel::Hide()? I mean, it shouldn't depend on the default action. For example, if user presses a modifier key during two finger swipe and leaves their fingers from touchpad before releasing the modifier key, then, nsScrollbarsForWheel::Hide() won't called until next two finger leaving from touchpad. I think that this should be out of switch or in all cases.

Otherwise, looks good to me.

Comment 102

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

FYI:

I landed a lot of patches for refactoring event stuff on mozilla-inbound.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=008ba688cd5a

You need to discards the change of nsGUIEvent.h and add same code to widget/BasicEvents.h.

Comment 103

[André Reinald]

Attachment: bugzilla-868648-14.patch

> nit: The first argument should be immediately after "(".

Ok: the intent was to match the indent of the definition of the same method.

> Strange indent, just 2 white-spaces are okay.

Ok: thought of it more like avoiding long lines, and resembling long method parameters lists.

> nit: use {} or sHadWheelStart = (sHadWheelStart || !sActiveOwner);

Done.

> Use early return style. I.e.,

Done, though I dislike early return style, especially when the method is short enough to fit on screen.
Your proposed change also sets sHadWheelStart to false earlier than I intended originally, so I put it after the 2nd return, keeping the original behavior.

> I still feel this odd. Shouldn't _STOP event *always* cause calling nsScrollbarsForWheel::Hide()? I mean, it shouldn't depend on the default action. For example, if user presses a modifier key during two finger swipe and leaves their fingers from touchpad before releasing the modifier key, then, nsScrollbarsForWheel::Hide() won't called until next two finger leaving from touchpad. I think that this should be out of switch or in all cases.

Done, I handle _STOP events in a separate case now.

> Otherwise, looks good to me.

Maybe our last iteration on this bug? :-)

Comment 104

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 810519 [details] [diff] [review]
bugzilla-868648-14.patch

>+void nsScrollbarsForWheel::PrepareToScrollText(
>+                                  nsEventStateManager* aESM,
>+                                  nsIFrame* aTargetFrame,
>+                                  WheelEvent* aEvent)
>+{
>+  if (aEvent->message == NS_WHEEL_START) {
>+    TemporarilyActivateAllPossibleScrollTargets(aESM, aTargetFrame, aEvent);
>+    sHadWheelStart ||= !sActiveOwner;
>+  } else {
>+    DeactivateAllTemporarilyActivatedScrollTargets();
>+  }
>+}

Sorry, perhaps, I missed this point. If this is called with _START event while there is an active wheel scroll transaction, we should not call TemporarilyActivateAllPossibleScrollTargets() because the scroll target has already been decided.

So, 

>+  if (aEvent->message == NS_WHEEL_START) {

should be:

if (aEvent->message == NS_WHEEL_START &&
    nsMouseWheelTransaction::GetTargetFrame()) {

?

If you agree with this, r=masayuki

Otherwise, let me know what you think.

> I dislike early return style, especially when the method is short enough to fit on screen.

But the method *might* become longer in the future.

Comment 105

[André Reinald]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #104)
> Comment on attachment 810519 [details] [diff] [review]
> Sorry, perhaps, I missed this point. If this is called with _START event
> while there is an active wheel scroll transaction, we should not call
> TemporarilyActivateAllPossibleScrollTargets() because the scroll target has
> already been decided.
> 
> So, 
> 
> >+  if (aEvent->message == NS_WHEEL_START) {
> 
> should be:
> 
> if (aEvent->message == NS_WHEEL_START &&
>     nsMouseWheelTransaction::GetTargetFrame()) {
> 
> ?
> 
> If you agree with this, r=masayuki
> 
> Otherwise, let me know what you think.

I changed to this, keeping the nsScrollbarsForWheel more self-contained:

void nsScrollbarsForWheel::PrepareToScrollText(
                                  nsEventStateManager* aESM,
                                  nsIFrame* aTargetFrame,
                                  WheelEvent* aEvent)
{
  if (aEvent->message == NS_WHEEL_START) {
    if (!IsActive()) {
      TemporarilyActivateAllPossibleScrollTargets(aESM, aTargetFrame, aEvent);
      sHadWheelStart = true;
    }
  } else {
    DeactivateAllTemporarilyActivatedScrollTargets();
  }
}

> But the method *might* become longer in the future.

In my opinion, methods should never grow longer than one page, they should be split before that. And you probably agree as you rightfully asked me to group all my code in a new class, and call its methods from existing code, instead of adding my code inline as I first did. As an excuse, the methods I was lengthening were already much longer than one page :-)

On the other side, I find early returns almost as bad as gotos. I can bear them in existing code, but I'd rather avoid them in new code that I write. Nevertheless, if so you prefer, I'll use early returns in this patch.

Comment 106

[André Reinald]

Attachment: bugzilla-868648-15.patch

Comment 107

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

(In reply to André Reinald from comment #105)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #104)
> > Comment on attachment 810519 [details] [diff] [review]
> > Sorry, perhaps, I missed this point. If this is called with _START event
> > while there is an active wheel scroll transaction, we should not call
> > TemporarilyActivateAllPossibleScrollTargets() because the scroll target has
> > already been decided.
> > 
> > So, 
> > 
> > >+  if (aEvent->message == NS_WHEEL_START) {
> > 
> > should be:
> > 
> > if (aEvent->message == NS_WHEEL_START &&
> >     nsMouseWheelTransaction::GetTargetFrame()) {
> > 
> > ?
> > 
> > If you agree with this, r=masayuki
> > 
> > Otherwise, let me know what you think.
> 
> I changed to this, keeping the nsScrollbarsForWheel more self-contained:
> 
> void nsScrollbarsForWheel::PrepareToScrollText(
>                                   nsEventStateManager* aESM,
>                                   nsIFrame* aTargetFrame,
>                                   WheelEvent* aEvent)
> {
>   if (aEvent->message == NS_WHEEL_START) {
>     if (!IsActive()) {
>       TemporarilyActivateAllPossibleScrollTargets(aESM, aTargetFrame,
> aEvent);
>       sHadWheelStart = true;
>     }
>   } else {
>     DeactivateAllTemporarilyActivatedScrollTargets();
>   }
> }

I don't think that it's same as my suggestion. IsActive() returns false after Hide() is called by _STOP event. However, the wheel transaction is still kept even if fingers are left from touchpad.

Comment 108

[André Reinald]

Right, it's not exactly your suggestion, but I kept your idea: not show new temporary scrollbars when we have an ongoing scroll activity.

Maybe we should reciprocate what we did previously: if _STOP event comes while a transaction is still active, set a bool inside nsMouseWheelTransaction to true, then if this bool is true when EndTransaction is called, also call Hide() from there.

I still think we should merge nsMouseWheelTransaction and nsScrollbarsForWheel because the latter is mostly reflecting in the UI what is happening in the former. In other words, make nsMouseWheelTransaction aware of _START and _STOP events (while still keeping the timer based feature) should be enough.

Comment 109

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

(In reply to André Reinald from comment #108)
> Maybe we should reciprocate what we did previously: if _STOP event comes
> while a transaction is still active, set a bool inside
> nsMouseWheelTransaction to true, then if this bool is true when
> EndTransaction is called, also call Hide() from there.

I don't understand what you meant.

I'm thinking that TemporarilyActivateAllPossibleScrollTargets() is useful when scroll target hasn't been decided because it lets users know what scrollable area can be scrollable.

However, if nsMouseWheelTransaction has active transaction, the scroll target has already decided. Therefore, new _START event doesn't need to let users know other scrollable areas.

So, I guess that your patch causes flicking unnecessary scrollbars at swiping with two fingers continuously. Isn't it?

Comment 110

[André Reinald]

Attachment: bugzilla-868648-16.patch

I brought the changes I wanted in order to ensure consistence between the "transaction life" and the "scrollbars visibility". This consistence is my main concern from the very start.

To really "finish", each one is waiting for the other to "want to finish". When both "want to finish" then both will "finish".

As you'll see, it was mostly about reciprocating what we already did with the sCallHideScrollbarsAtEndingTransaction. I had to add a little complexity to handle the order the 2 "finish" were called (I made the "finish" methods not public).

I added a "want to finish" method to each class to make things clear (which call the "finish" method if needed). The "force" flag in each "want to finish" method allows skipping the other class to "want to finish".

Comment 111

[André Reinald]

Attachment: bugzilla-868648-17.patch

Removed a debug macro I used to track execution flow. Sorry for patch 16.

Comment 112

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Oh, I'm surprised at the big change of the patch... I don't understand why you do such big change in this stage yet.

Anyway, I go to bed soon. If I could review it tomorrow morning, I'd do it. But perhaps, I can review it next Monday because a lot of part are changed.

Looks like your new patch breaks our coding rules:

1. Don't break after "}" if "else" follows it.

if () {

} else if {) {

} else {

}

2. Use a prefix for argument of any methods, i.e., force should be aForce.

3. Break after result type at implementing a method. E.g.,

void
ClassName::MethodName()
{

}

Comment 113

[André Reinald]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #112)
> Oh, I'm surprised at the big change of the patch... I don't understand why
> you do such big change in this stage yet.

Thanks for the quick reply. Indeed I now feel quite comfortable changing this source as I got to better understanding of it. My first patches were smaller and took me much longer.

> Anyway, I go to bed soon. If I could review it tomorrow morning, I'd do it.
> But perhaps, I can review it next Monday because a lot of part are changed.

Have a good night ! No hurry I have things to work on meanwhile.

> Looks like your new patch breaks our coding rules:

I'll take care of those right now and post patch 18.

Comment 114

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

And use "{}" even if its content is only one line.

Comment 115

[André Reinald]

Attachment: bugzilla-868648-18.patch

Modified formatting to match coding rules.

Comment 116

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 811192 [details] [diff] [review]
bugzilla-868648-18.patch

Sorry for the delay.

Looks good to me this patch in logical level but I don't like some points this in coding level.

First, nsScrollbarsForWheel class has IsActive(). I think that this name is good because except Mac OS X, IsVisible() returns strange result. IsActive() can sound if the class is active or not.

So, I think that Hide*() methods are not good name. I recommend that let's use Inactive().

Next, this patch looks pretty complicated and I'm afraid that it would be broken in the future because this patch makes the classes stateful with two or more flags　which are named as CallSomething. The names feel too directly to me. This indicates very clear the class what needs to do. However, it may be difficult to understand what the requesting class want the other class to do.

I believe that what you attempt to do with them is, you decide and recode which class (nsMouseWheelTransaction or nsScrollbarsForWheel) is responsible for ending transaction and hiding scrollars. So, I think that it's better to name "Call*()" methods as some other names which means "you own my work", e.g., "nsScrollbarsForWheel::OwnWheelTransaction()" and "nsMouseWheelTransaction::OwnActivatedScrollbars()"? If you have better idea, let's use them.

Additionally, ShouldDoSomething(bool aForce) isn't good because it's not easy to read. Please separate them as ShouldDoSomething() or MayDoSomething() and just DoSomething().

Finally, some members of nsScrollbarsForWheel has "scrollbars" in their names. However, this is redundant. Let's drop it.

>+class nsScrollbarsForWheel {
>+public:
>+  static void PrepareToScrollText(nsEventStateManager* aESM,
>+                                  nsIFrame* aTargetFrame,
>+                                  WheelEvent* aEvent);
>+  static void SetActiveScrollTarget(nsIScrollableFrame* aScrollTarget);
>+  // Hide all scrollbars (both mActiveOwner's and mActivatedScrollTargets')
>+  static void ShouldHideScrollbars(bool aForce);

I think this should be separated as "MayInactivate()" or "ShouldInactivate()" and "Inactivate()". The former method should call Inactivate() if it actually performs it.

>+  static bool IsActive();
>+  static void CallEndTransactionAtHidingScrollbars(bool callEndTransaction);

I think this should be "OwnWheelTransaction()".

And add "static void NotifyStartWheelTransaction()" for clearing the flag.

>+
>+protected:
>+  static const size_t         kNumberOfTargets = 4;
>+  static const DeltaValues    directions[kNumberOfTargets];

kDirections

>+  static nsWeakFrame          sActiveOwner;
>+  static nsWeakFrame          sActivatedScrollTargets[kNumberOfTargets];
>+  static bool                 sHadWheelStart;
>+  static bool                 sCallEndTransactionAtHidingScrollbars;

I think that this should be "sOwningWheelTransaction". This should be initialize as false at NotifyStartWheelTransaction() which should be called by nsMouseWheelTransaction::BeginTransaction(). And Inactivate() should call nsMouseWheelTransaction::EndTransaction() if this is true.

>+/******************************************************************/
>+/* nsMouseWheelTransaction                                        */
>+/******************************************************************/
>+
> class nsMouseWheelTransaction {
> public:
>   static nsIFrame* GetTargetFrame() { return sTargetFrame; }
>   static void BeginTransaction(nsIFrame* aTargetFrame,
>                                WheelEvent* aEvent);
>   // Be careful, UpdateTransaction may fire a DOM event, therefore, the target
>   // frame might be destroyed in the event handler.
>   static bool UpdateTransaction(WheelEvent* aEvent);
>-  static void EndTransaction();
>+  static void ShouldEndTransaction(bool aForce);

I think that this should be "MayEndTransaction()" or "ShouldEndTransaction()" and "EndTransaction()". The former should call the latter when it actually performs it.

>   static void OnEvent(nsEvent* aEvent);
>   static void Shutdown();
>   static uint32_t GetTimeoutTime();
>-
>+  static void CallHideScrollbarsAtEndingTransaction(bool callHideScrollbars);

I think that this should be "OwnActivatedScrollbars()". And add "RenounceActivatedScrollbars()" for clearing the flag.

>   static nsWeakFrame sTargetFrame;
>   static uint32_t    sTime;        // in milliseconds
>   static uint32_t    sMouseMoved;  // in milliseconds
>   static nsITimer*   sTimer;
>   static int32_t     sScrollSeriesCounter;
>+  static bool        sCallHideScrollbarsAtEndingTransaction;

I think that this should be "sOwningActivatedScrollbars". This should be initialized at RenounceActivatedScrollbars() which should be called by nsScrollbarsForWheel::PrepareToScrollText(). And also it's initialized at nsMouseWheelTransaction::BeginTransaction().

> void
>+nsMouseWheelTransaction::ShouldEndTransaction(bool aForce)
>+{
>+  if (sCallHideScrollbarsAtEndingTransaction || aForce) {
>+    EndTransaction();
>+    sCallHideScrollbarsAtEndingTransaction = false;
>+    nsScrollbarsForWheel::CallEndTransactionAtHidingScrollbars(false);
>+    nsScrollbarsForWheel::ShouldHideScrollbars(false);
>+  } else if (nsScrollbarsForWheel::IsActive()) {
>+    nsScrollbarsForWheel::CallEndTransactionAtHidingScrollbars(true);
>+    return;
>+  } else {
>+    EndTransaction();
>+  }
>+}

So, this should be:

void
nsMouseWheelTransaction::MayEndTransaction()
{
  if (!sOwningActivatedScrollbars && nsScrollbarsForWheel::IsActive()) {
    nsScrollbarsForWheel::OwnWheelTransaction();
    return;
  }
  EndTransaction();
}

>+
>+void
> nsMouseWheelTransaction::EndTransaction()
> {
>   if (sTimer)
>     sTimer->Cancel();
>   sTargetFrame = nullptr;
>   sScrollSeriesCounter = 0;
> }

Add:

if (sOwningWheelTransaction) {
  sOwningWheelTransaction = false;
  nsScrollbarsForWheel::Inactivate();
}

>@@ -443,17 +522,17 @@ nsMouseWheelTransaction::OnEvent(nsEvent
>     case NS_KEY_UP:
>     case NS_KEY_DOWN:
>     case NS_MOUSE_BUTTON_UP:
>     case NS_MOUSE_BUTTON_DOWN:
>     case NS_MOUSE_DOUBLECLICK:
>     case NS_MOUSE_CLICK:
>     case NS_CONTEXTMENU:
>     case NS_DRAGDROP_DROP:
>-      EndTransaction();
>+      ShouldEndTransaction(true);

This change isn't necessary.

>@@ -470,32 +549,32 @@ nsMouseWheelTransaction::OnFailToScrollT
>                       sTargetFrame->GetContent()->OwnerDoc(),
>                       sTargetFrame->GetContent(),
>                       NS_LITERAL_STRING("MozMouseScrollFailed"),
>                       true, true);
>   }
>   // The target frame might be destroyed in the event handler, at that time,
>   // we need to finish the current transaction
>   if (!sTargetFrame)
>-    EndTransaction();
>+    ShouldEndTransaction(true);

same.

> void
> nsMouseWheelTransaction::OnTimeout(nsITimer* aTimer, void* aClosure)
> {
>   if (!sTargetFrame) {
>     // The transaction target was destroyed already
>-    EndTransaction();
>+    ShouldEndTransaction(true);

same.

> /******************************************************************/
>+/* nsScrollbarsForWheel                                           */
>+/******************************************************************/
>+
>+void
>+nsScrollbarsForWheel::PrepareToScrollText(
>+                                  nsEventStateManager* aESM,
>+                                  nsIFrame* aTargetFrame,
>+                                  WheelEvent* aEvent)

You don't need to break the line after "(".

>+{
>+  if (aEvent->message == NS_WHEEL_START) {
>+    nsMouseWheelTransaction::CallHideScrollbarsAtEndingTransaction(false);

Call nsMouseWheelTransaction::RenounceActivatedScrollbars() instead.

>+void
>+nsScrollbarsForWheel::ShouldHideScrollbars(bool aForce)
>+{
>+  if (sCallEndTransactionAtHidingScrollbars || aForce) {
>+    HideScrollbars();
>+    sCallEndTransactionAtHidingScrollbars = false;
>+    nsMouseWheelTransaction::CallHideScrollbarsAtEndingTransaction(false);
>+    nsMouseWheelTransaction::ShouldEndTransaction(false);
>+  } else if (nsMouseWheelTransaction::GetTargetFrame()) {
>+    nsMouseWheelTransaction::CallHideScrollbarsAtEndingTransaction(true);
>+  } else {
>+    HideScrollbars();
>+  }
>+}

void
nsMouseWheelTransaction::MayInactivate()
{
  if (!sOwningWheelTransaction && nsScrollbarsForWheel::IsActive()) {
    nsMouseWheelTransaction::OwnActivatedScrollbars();
    return;
  }
  Inactivate();
}

>+
>+void
>+nsScrollbarsForWheel::HideScrollbars()
>+{
>+  nsIScrollbarOwner* scrollbarOwner = do_QueryFrame(sActiveOwner);
>+  if (scrollbarOwner) {
>+    scrollbarOwner->ScrollbarActivityStopped();
>+  }
>+  sActiveOwner = nullptr;
>+  DeactivateAllTemporarilyActivatedScrollTargets();
>+}

This also calls:

if (sOwningWheelTransaction) {
  sOwningWheelTransaction = false;
  nsMouseWheelTransaction::EndTransaction();
}

>@@ -3149,43 +3369,60 @@ nsEventStateManager::PostHandleEvent(nsP
> 
>       nsIPresShell *shell = presContext->GetPresShell();
>       if (shell) {
>         nsRefPtr<nsFrameSelection> frameSelection = shell->FrameSelection();
>         frameSelection->SetMouseDownState(false);
>       }
>     }
>     break;
>-  case NS_WHEEL_WHEEL:
>+  case NS_WHEEL_STOP:
>     {
>       MOZ_ASSERT(aEvent->mFlags.mIsTrusted);
>+      nsScrollbarsForWheel::ShouldHideScrollbars(false);
>+    }
>+    break;
>+  case NS_WHEEL_WHEEL:
>+  case NS_WHEEL_START:
>+    {
>+      MOZ_ASSERT(aEvent->mFlags.mIsTrusted);
> 
>       if (*aStatus == nsEventStatus_eConsumeNoDefault) {
>+        nsScrollbarsForWheel::ShouldHideScrollbars(true);

Just call nsScrollbarsForWheel::Inactivate();

>         break;
>       }
> 
>       WheelEvent* wheelEvent = static_cast<WheelEvent*>(aEvent);
>       switch (WheelPrefs::GetInstance()->ComputeActionFor(wheelEvent)) {
>         case WheelPrefs::ACTION_SCROLL: {
>-          if (!wheelEvent->deltaX && !wheelEvent->deltaY) {
>+          // For scrolling of default action, we should honor the mouse wheel
>+          // transaction.
>+          
>+          nsScrollbarsForWheel::PrepareToScrollText(this, aTargetFrame, wheelEvent);
>+          
>+          if (aEvent->message != NS_WHEEL_WHEEL ||
>+              (!wheelEvent->deltaX && !wheelEvent->deltaY)) {
>             break;
>           }
>-          // For scrolling of default action, we should honor the mouse wheel
>-          // transaction.
>+
>           nsIScrollableFrame* scrollTarget =
>             ComputeScrollTarget(aTargetFrame, wheelEvent,
>                                 COMPUTE_DEFAULT_ACTION_TARGET);
>+
>+          nsScrollbarsForWheel::SetActiveScrollTarget(scrollTarget);
>+
>           wheelEvent->overflowDeltaX = wheelEvent->deltaX;
>           wheelEvent->overflowDeltaY = wheelEvent->deltaY;
>+
>           WheelPrefs::GetInstance()->
>             CancelApplyingUserPrefsFromOverflowDelta(wheelEvent);
>           if (scrollTarget) {
>             DoScrollText(scrollTarget, wheelEvent);
>           } else {
>-            nsMouseWheelTransaction::EndTransaction();
>+            nsMouseWheelTransaction::ShouldEndTransaction(false);

I don't think so. If there is no scrollable target, we should discard active wheel transaction. Additionally, nsScrollbarsForWheel::Inactivate() should be called too.

>@@ -4142,17 +4379,17 @@ nsEventStateManager::SetPointerLock(nsIW
>   // NOTE: aElement will be nullptr when unlocking.
>   sIsPointerLocked = !!aElement;
> 
>   if (!aWidget) {
>     return;
>   }
> 
>   // Reset mouse wheel transaction
>-  nsMouseWheelTransaction::EndTransaction();
>+  nsMouseWheelTransaction::ShouldEndTransaction(false);

Although, I'm not so familiar with pointer lock, the wheel transaction should be aborted. I think that nsScrollbarsForWheel::Inactivate() should be called too.

Before writing new patch, let me know your objection to my idea if you have.

Comment 117

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Since I'm not a native speaker of English (and my English is not good), if you have better names for suggested methods and variables, let me know ;-)

Comment 118

[André Reinald]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #116)
> Comment on attachment 811192 [details] [diff] [review]
> bugzilla-868648-18.patch
> 
> Looks good to me this patch in logical level but I don't like some points
> this in coding level.

Yes, I think the logic is good and I'm finally almost satisfied with what I did except 2 points:

The first one went unnoticed from code analysis but is obvious when using the patch: the delay between raising fingers and scrollbars disappearing is too long. This problem arose since comment 48 of this bug and in time disappeared and reappeared, but now I can tell where it comes from and how to correct it.

That's because the delay from nsScrollwheelTransaction adds to the delay from ScrollbarActivity. I believe the right way to correct this would be to be able to call the "hide" method of ScrollbarActivity directly. But that may be beyond the scope of this patch, and justify a follow-up bug.

> Next, this patch looks pretty complicated and I'm afraid that it would be
> broken in the future because this patch makes the classes stateful with two
> or more flags　which are named as CallSomething. The names feel too directly
> to me. This indicates very clear the class what needs to do. However, it may
> be difficult to understand what the requesting class want the other class to
> do.
> 
> I believe that what you attempt to do with them is, you decide and recode
> which class (nsMouseWheelTransaction or nsScrollbarsForWheel) is responsible
> for ending transaction and hiding scrollars. So, I think that it's better to
> name "Call*()" methods as some other names which means "you own my work",
> e.g., "nsScrollbarsForWheel::OwnWheelTransaction()" and
> "nsMouseWheelTransaction::OwnActivatedScrollbars()"? If you have better
> idea, let's use them.

That's the 2nd one and you are right at pointing it out: I had to add too much complexity to have the symmetry between the 2 classes, plus there is some redundancy in the "finish" methods of each. There is clearly room for improvement here.

I suggest we create another short class which we should call something like "ScrollFinishCoordinator" (but we can change name of course) which will handle the state of both classes (timer fired and fingers raised) and call the "finish" methods when appropriate. The resulting code would be much simpler and there will be no code duplication as there is now.

What do you think?

Comment 119

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Sounds good to me. However, I don't mind even if you will do it in another bug since this bug has already been too long. If you (and Mac users) want to fix this bug as soon as possible, I'll mark r+ with my suggestions in the previous comment. But either is okay. You can choose a way you like better.

If you create such new class, I prefer "Manager" than "Coordinator". However, in design pattern, it sounds like Adopter or Bridge. Although, I'm not sure the design of new class which you imagine.

FYI: If you'd create new class, I'd post a patch which replaces mozilla::WheelEvent with mozilla::WidgetWheelEvent. I'm putting off it for you.

Comment 120

[André Reinald]

Attachment: bugzilla-868648-19.patch

I preferred to make less changes this time. The way you suggested to have "May" methods and "Own" flags simplifies the whole thing enough. I hope I didn't forget anything else from your suggestions, though I've added some changes of my own, in the same intention to reciprocate everything between the 2 classes.

Comment 121

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 812738 [details] [diff] [review]
bugzilla-868648-19.patch

Looks good to me. Thank you for your great work!

Comment 122

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

> Keywords: checkin-needed

Did you try to merge it with current m-c?

When I tried to import your patch in my queue, some hunks are rejected.

Comment 123

[Ed Morley [:emorley]]

Doesn't apply against mozilla-inbound tip, please can you rebase? :-)

Comment 124

[André Reinald]

Attachment: bugzilla-868648-20.patch

Rebased patch-19 to current m-c. Builds and works fine.
Not sure about the checkin flag I just set in this attachment page. Is it the same as previously done on the bug page?

Comment 125

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 814850 [details] [diff] [review]
bugzilla-868648-20.patch

I *think* you just want checkin-needed again.

Someone please correct me if I'm wrong.

Comment 126

[André Reinald]

Thanks Steven!

In the "help" from the checkin+ menu (on the attachment page), I read "Use this flag if you do not have VCS access and wish to request that someone check in your patch..." which exactly fits my needs. So I was wondering in what this was different from "checkin-needed".

Plus I also think that asking for checkin makes more sense for a patch than for a bug (which often references multiple patches). Or maybe search tools used by sheriffs target bugs but not patches, hence a checkin+ flag on an attachment can't show up in bug searches? But this question doesn't belong to this bug discussion I guess.

Comment 127

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d72778f6eee9

Comment 128

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d72778f6eee9

Comment 129

[Steven Michaud [:smichaud] (Retired)]

I backed out this bug's patch (on mozilla-inbound) for causing bug 925411 on OS X 10.6 (see bug 925411 comment #9).  Stephen's probably already identified the cause of bug 925411 at bug 925411 comment #6.  But bug 925411 is very bad on OS X 10.6, so I thought it best not to leave it hanging over our heads while we try to fix it.

Comment 130

[Stephen A Pohl [:spohl]]

André, this is just a nit but since you'll have to post a new patch anyway: could you remove all trailing white space in your patch before we land this again? It's probably easiest to see in Splinter review (i.e. when clicking on "Review" next to the patch). Thanks!

Comment 131

[André Reinald]

Attachment: bugzilla-868648-21.patch

Should fix bug 925411 by avoiding the call to [NSEvent phase] under 10.6.
Used regex to remove trailing whitespaces.

Comment 132

[Stephen A Pohl [:spohl]]

Hmm, the patch in bug 673875 just broke the patch here. I'll update the patch here so that it applies cleanly on inbound. Did the last change in comment 131 need another quick review before we submit it again?

Comment 133

[Stephen A Pohl [:spohl]]

Attachment: bugzilla-868648-22.patch

Updated for current inbound (i.e. with bug 673875 landed). Asking for a quick review of the rebase and the changes in comment 131 just to be safe.

Comment 134

[Stephen A Pohl [:spohl]]

Attachment: Interdiff between patch 21 and 22

Comment 135

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 815855 [details] [diff] [review]
bugzilla-868648-22.patch

This looks fine to me.

I'll test the mozilla-inbound build on 10.6 once we have it.

I do have a couple of nits, which can be addressed in a later patch:

 - (void) convertCocoaMouseEvent:(NSEvent*)aMouseEvent
-                   toGeckoEvent:(WidgetInputEvent*)outGeckoEvent;
+                   toGeckoEvent:(WidgetInputEvent*)outWheelEvent;

This change shouldn't have been made :-)

-  // Calling deviceDeltaX or deviceDeltaY on theEvent will trigger a Cocoa
-  // assertion and an Objective-C NSInternalInconsistencyException if the
-  // underlying "Carbon" event doesn't contain pixel scrolling information.
-  // For these events, carbonEventKind is kEventMouseWheelMoved instead of
-  // kEventMouseScroll.

I wonder if it's really safe/appropriate to get rid of this comment.

I'll also look more carefully through the patch, later on, to see if I have any more questions or comments.

Comment 136

[Stephen A Pohl [:spohl]]

Attachment: bugzilla-868648-23.patch

Addressed feedback. Carrying over r+. Will kick off a try build before submitting to inbound.

(In reply to Steven Michaud from comment #135)
> -  // Calling deviceDeltaX or deviceDeltaY on theEvent will trigger a Cocoa
> -  // assertion and an Objective-C NSInternalInconsistencyException if the
> -  // underlying "Carbon" event doesn't contain pixel scrolling information.
> -  // For these events, carbonEventKind is kEventMouseWheelMoved instead of
> -  // kEventMouseScroll.
> 
> I wonder if it's really safe/appropriate to get rid of this comment.
> 

This wasn't actually removed, but rather moved to convertCocoaMouseWheelEvent.

Comment 137

[Stephen A Pohl [:spohl]]

Attachment: bugzilla-868648-24.patch

Made sure to hg qrefresh this time. Carrying over r+.

Comment 138

[Stephen A Pohl [:spohl]]

https://tbpl.mozilla.org/?tree=Try&rev=c0cc3ac60243

Comment 139

[Stephen A Pohl [:spohl]]

Try is green and smichaud was able to confirm that this is no longer crashing on 10.6. Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7def963cdcc

Comment 140

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f7def963cdcc

Comment 141

[André Reinald]

As I sometimes see this happen when a patch has landed and not caused trouble in nightly for a while, I was wondering if it would make sense to ask uplifting this patch (and the one from bug 927259) to aurora ? to beta ?

It seems spohl had a conflicting patch (comment 133) and had to change this one so it could apply after his own. I'm not sure how we should handle this.

Comment 142

[Stephen A Pohl [:spohl]]

This bug is set to lion-scrollbars=, which means it isn't a must-have. It also changes quite a bit of code, so it may be safer to just let this ride the train and see if anything breaks. I'm sure smichaud will be able to make the right call here.

Comment 143

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Basically, any patches which don't fix new regression or security issue shouldn't be uplifted. However, if it's very important for marketing, it's worthwhile to do it. At this bug, I don't think that we should do it.

Comment 144

[Stephen A Pohl [:spohl]]

Removing n-i per comment 143. Thanks, Masayuki!

Comment 145

[Steven Michaud [:smichaud] (Retired)]

I agree with Stephen and Masayuki.  Originally I'd suggested that we upload this patch (plus the required additional fix) to aurora (though not beta), to get more testers.  But now (https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.planning/Rizxl_080No) it looks like aurora doesn't actually have any more users than trunk (mozilla-central).

It's true this patch does make a lot of changes -- enough that we should give it lots of time to bake before a release.  Patches that effect external code (e.g. plugin patches) often don't get reasonable levels of user-testing until they get onto the beta channel -- simply because so many users of "unusual" plugins or extensions don't test with trunk or aurora nightlies (or even with betas).  That consideration doesn't (of course) apply here.

Let's see what happens.  If there's more demand for this feature than we've anticipated, we should consider uplifting this patch one level (from trunk to aurora, or (sometime in the future) from aurora to beta).

Comment 146

[Reuben Morais [:reuben]]

Was this backed out? Or the code changed considerably in a different bug? On Nightly 2013-10-28 scrollbars don't show up when you touch the trackpad with two fingers, and scrolling is impossible.

Comment 147

[Stephen A Pohl [:spohl]]

(In reply to Reuben Morais [:reuben] from comment #146)
> Was this backed out? Or the code changed considerably in a different bug? On
> Nightly 2013-10-28 scrollbars don't show up when you touch the trackpad with
> two fingers, and scrolling is impossible.

I can't reproduce this problem on Nightly 2013-10-28 and everything still works as expected. Could it be that you're running into bug 927702? If not, please file a new bug with STR that include the page you were on. Thanks!

Comment 148

[Reuben Morais [:reuben]]

(In reply to Stephen Pohl [:spohl] from comment #147)
> Could it be that you're running into bug 927702?

Yep, that's what I'm hitting, thanks.

Comment 149

[André Reinald]

Comment on attachment 815905 [details] [diff] [review]
bugzilla-868648-24.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 868648
User impact if declined: Scrollbars don't appear when user touches trackpad.
Testing completed (on m-c, etc.): The patch has landed in m-c for over 5 weeks, no issues since then.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Note: [Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 868648
User impact if declined: Scroll target is not switched when cursor is moved.
Testing completed (on m-c, etc.): Has landed in m-c for over 5 weeks, no issue since then.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Note: bug 927259 is a follow-up of 868648 and is targeted at Mozilla 27. Hence my request to uplift this patch to Mozilla 27 too, as this patch has to be applied before attachment 817760 [details] [diff] [review] from 927259.

Comment 150

[bhavana bajaj [:bajaj]]

(In reply to André Reinald from comment #149)
> Comment on attachment 815905 [details] [diff] [review]
> bugzilla-868648-24.patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Bug 868648
> User impact if declined: Scrollbars don't appear when user touches trackpad.
> Testing completed (on m-c, etc.): The patch has landed in m-c for over 5
> weeks, no issues since then.
> Risk to taking this patch (and alternatives if risky): Low
> String or IDL/UUID changes made by this patch: None
> 
> Note: [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Bug 868648
> User impact if declined: Scroll target is not switched when cursor is moved.
> Testing completed (on m-c, etc.): Has landed in m-c for over 5 weeks, no
> issue since then.
> Risk to taking this patch (and alternatives if risky): Low
> String or IDL/UUID changes made by this patch: None
> 
> Note: bug 927259 is a follow-up of 868648 and is targeted at Mozilla 27.
> Hence my request to uplift this patch to Mozilla 27 too, as this patch has
> to be applied before attachment 817760 [details] [diff] [review] from 927259.

Looks like this attachment landed a month ago when Fx27 was on central. Am i missing something here ?

Comment 151

[André Reinald]

(In reply to bhavana bajaj [:bajaj] from comment #150)
> Looks like this attachment landed a month ago when Fx27 was on central. Am i
> missing something here ?

I thought it didn't make it to Fx27, but I just checked and it's there indeed. Sorry for the trouble.

Comment 152

[bhavana bajaj [:bajaj]]

Comment on attachment 815905 [details] [diff] [review]
bugzilla-868648-24.patch

Clearing the approval nom here as the patch already exists for Fx27.