638004 - Various "…deprecated for NSScrollWheel. Please use…" messages to console on first trackpad scroll

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Doug Grinbergs]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-us) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

blah detail

Reproducible: Always

Steps to Reproduce:
1 Open app
2 Load page with enough content to enable v-scroller
3 Scroll using trackpad gesture
Actual Results:  
Messages to Console system.log, like:

Feb 28 18:10:26 107-11A390-LaCie250 firefox-bin[965]: -_continuousScroll is deprecated for NSScrollWheel. Please use -hasPreciseScrollingDeltas.
Feb 28 18:10:26 107-11A390-LaCie250 firefox-bin[965]: -deviceDeltaY is deprecated for NSScrollWheel. Please use -scrollingDeltaY.
Feb 28 18:10:26 107-11A390-LaCie250 firefox-bin[965]: -_scrollPhase is deprecated for NSScrollWheel. Please use -momentumPhase.
Feb 28 18:10:26 107-11A390-LaCie250 firefox-bin[965]: -deviceDeltaX is deprecated for NSScrollWheel. Please use -scrollingDeltaX.

Mar  1 21:34:43 107-11A390-LaCie250 firefox-bin[6721]: -_continuousScroll is deprecated for NSScrollWheel. Please use -hasPreciseScrollingDeltas.
Mar  1 21:34:43 107-11A390-LaCie250 firefox-bin[6721]: -deviceDeltaY is deprecated for NSScrollWheel. Please use -scrollingDeltaY.
Mar  1 21:34:43 107-11A390-LaCie250 firefox-bin[6721]: -deviceDeltaX is deprecated for NSScrollWheel. Please use -scrollingDeltaX.
Mar  1 21:34:43 107-11A390-LaCie250 firefox-bin[6721]: -_scrollPhase is deprecated for NSScrollWheel. Please use -momentumPhase.

Expected Results:  
No console log messages

4.0b12, 10.7 11A390, MacBook Pro/2.2 GHz Core 2 Duo

Comment 1

[Anthony Ricaud (:rik)]

Can the fix for that enable the "bounce" animation when reaching top and bottom part of a page?

Comment 2

[Markus Stange [:mstange]]

No. Actual scrolling is completely handled inside Gecko, and that doesn't allow scrolling outside the traditional scroll bounds yet. OS X can't do the bounce-back animation for us because it doesn't know when scrolling reaches the end of the scrollable area.

Comment 3

[(not currently active) Ted Mielczarek]

I filed bug 619354 on a similar issue I saw on 10.6.

Comment 4

[Ben]

I am seeing this too with thunderbird 5.0 on Lion

05/08/2011 15:01:06.165 thunderbird-bin: -_continuousScroll is deprecated for NSScrollWheel. Please use -hasPreciseScrollingDeltas.
05/08/2011 15:01:06.165 thunderbird-bin: -deviceDeltaY is deprecated for NSScrollWheel. Please use -scrollingDeltaY.
05/08/2011 15:01:06.166 thunderbird-bin: -_scrollPhase is deprecated for NSScrollWheel. Please use -momentumPhase.
05/08/2011 15:01:06.168 thunderbird-bin: -deviceDeltaX is deprecated for NSScrollWheel. Please use -scrollingDeltaX.

Comment 5

[Florian Bender]

looks like it's fixed in Fx 9b3 / Tb 9b2

Comment 7

[Nomis101]

(In reply to Florian Bender from comment #5)
> looks like it's fixed in Fx 9b3 / Tb 9b2
Hmmm, I still see this with TB 9.0b and TB 10.0a on my MacBook Pro.

Comment 8

[Florian Bender]

Yep, you're right, still there. But I /believe/ significantly less. 

Appears once about 1 min after Fx 9b5 startup: 
-----
11.12.11 11:40:20,262 firefox: -_scrollPhase is deprecated for NSScrollWheel. Please use -momentumPhase.
11.12.11 11:40:20,263 firefox: -_continuousScroll is deprecated for NSScrollWheel. Please use -hasPreciseScrollingDeltas.
11.12.11 11:40:20,263 firefox: -deviceDeltaY is deprecated for NSScrollWheel. Please use -scrollingDeltaY.
11.12.11 11:40:20,264 firefox: -deviceDeltaX is deprecated for NSScrollWheel. Please use -scrollingDeltaX.
-----

I have a lot of tabs opened and I restart Fx from time to time to "unload" background tabs and thus free memory.

Comment 9

[Jeraimee Hughes]

Verified still exists using 10.0 on Lion. Same messages as Comment 8

Comment 10

[Nomis101]

Attachment: Will this do it?

Will something like this do it? This builds for me on Lion and I than don't see any of this "... is deprecated" messages anymore in the console. But I can't test on 10.5/10.6.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #10)

You should ask someone to review your patch.  Like me.  I can test on 10.5, 10.6 and 10.7.

I can't promise to do the review right away.  But I should be able to get to it in the next few weeks.

By the way, this problem is a minor annoyance, and then only to people who look at console output.  The deprecated methods are still there, and will be in all future Lion versions.  But they might disappear in Mountain Lion, so we do want to get to this before too long.

Comment 12

[Nomis101]

Attachment: Maybe a fix, now with hg header

Yes, I know, this is currently a minor annoyance, but I had some time and this bug seemed easy enough for me to fix. :-)
For reviewing I've made a proper HG changeset patch with header.

Comment 13

[Markus Stange [:mstange]]

Thanks for the patch, Simon. I can give you a preliminary review:
You're using #ifdefs, which means that the decision is made at compile time. So the OS X version that is used when evaluating the ifdefs is fixed at compile time. Specifically, it's determined by the SDK that you specify when compiling.
That means that with your patch, the new paths are only taken when compiling with the 10.7 SDK, but not when compiling with the 10.6 SDK. The normal Firefox universal binaries use the 64 bit binary for both 10.6 and 10.7, so they're compiled with the 10.6 SDK, and thus your patch won't work in normal Firefox builds (or rather it won't make a difference).

The decision which method is used should be made at runtime, so without #ifdefs. A good way to do that is using the respondsToSelector method.

So things would look like this:

if (new method is available)
  return [call new method];

if (old method is available)
  return [call old method];

Do you want to try to make a new patch with that approach?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Markus, if you're willing to take over reviewing Nomis101's patch, that'd be fine by me :-)

Comment 15

[Nomis101]

(In reply to Markus Stange from comment #13)
> So things would look like this:
> 
> if (new method is available)
>   return [call new method];
> 
> if (old method is available)
>   return [call old method];
> 
> Do you want to try to make a new patch with that approach?

I will try that other approach, but this seems a bit more advanced coding than my try. Is there any example for this approach in the mozilla code which I can adopt?

Comment 16

[Nomis101]

(In reply to Markus Stange from comment #13)
> Do you want to try to make a new patch with that approach?
You mean with nsToolkit::OnLionOrLater, like in nsLookAndFeel.mm#352, right?
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsLookAndFeel.mm#352

Comment 17

[Markus Stange [:mstange]]

That would be one way, but I'd prefer if you used respondsToSelector. For example like this:

if ([event respondsToSelector:@selector(scrollingDeltaX)]) {
  scrollDeltaPixels = -[event scrollingDeltaX];
} else {
  scrollDeltaPixels = -[event deviceDeltaX];
}

Comment 18

[Nomis101]

Attachment: WIP

OK, this was a bit harder for me to do. But after a few build errors, this is now the first patch that builds for me on 10.7 and removes the deprecated messages for deviceDelta from the console. Is this the way you wanted it?
And I'm a bit unsure what I should do for _scrollPhase, because there is already a respondsToSelector line?
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaUtils.mm#140

Comment 19

[Markus Stange [:mstange]]

I'd rewrite it like this:

BOOL nsCocoaUtils::IsMomentumScrollEvent(NSEvent* aEvent)
{
  if ([aEvent type] != NSScrollWheel)
    return NO;

  if ([anEvent respondsToSelector:@selector(momentumPhase)])
    return ([anEvent momentumPhase] & NSEventPhaseChanged) != 0;

  if ([aEvent respondsToSelector:@selector(_scrollPhase)])
    return [aEvent _scrollPhase] != 0;

  return NO;
}

You'll also need to insert this in nsChildView.h above the @interface with the momentumPhase declaration:

#if !defined(MAC_OS_X_VERSION_10_7) || \
    MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
enum {
    NSEventPhaseNone        = 0,
    NSEventPhaseBegan       = 0x1 << 0,
    NSEventPhaseStationary  = 0x1 << 1,
    NSEventPhaseChanged     = 0x1 << 2,
    NSEventPhaseEnded       = 0x1 << 3,
    NSEventPhaseCancelled   = 0x1 << 4,
};
typedef NSUInteger NSEventPhase;
#endif

And then change the return type of momentumPhase from "long long" to "NSEventPhase".

Comment 20

[Nomis101]

Attachment: Patch

So, that patch builds on Lion (10.7 SDK) and removes all deprecated messages from the console. :-) Not tested on 10.5 and 10.6.

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 613966 [details] [diff] [review]
Patch

Oh man, I'm really sorry for the delay here.

>diff --git a/widget/cocoa/nsChildView.h b/widget/cocoa/nsChildView.h

> // Undocumented scrollPhase flag that lets us discern between real scrolls and
> // automatically firing momentum scroll events.
>+#if !defined(MAC_OS_X_VERSION_10_7) || \
>+MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
>+enum {
>+    NSEventPhaseNone        = 0,
>+    NSEventPhaseBegan       = 0x1 << 0,
>+    NSEventPhaseStationary  = 0x1 << 1,
>+    NSEventPhaseChanged     = 0x1 << 2,
>+    NSEventPhaseEnded       = 0x1 << 3,
>+    NSEventPhaseCancelled   = 0x1 << 4,
>+};
>+typedef NSUInteger NSEventPhase;
>+#endif

I'm not sure if this has changed in the meantime, but on current trunk this part is already present a few lines further down. But it needs to be shifted out of the #ifdef __LP64__ section.

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

>   if (inAxis & nsMouseScrollEvent::kIsVertical) {
>     scrollDelta       = -[theEvent deltaY];
>     if (checkPixels && (scrollDelta == 0 || scrollDelta != floor(scrollDelta))) {
>-      scrollDeltaPixels = -[theEvent deviceDeltaY];
>+        if ([theEvent respondsToSelector:@selector(scrollingDeltaY)]) {
>+            scrollDeltaPixels = -[theEvent scrollingDeltaY];
>+        } else {
>+            scrollDeltaPixels = -[theEvent deviceDeltaY];
>+        }


You're using 4 spaces indentation, but the rest of the file uses 2. Please changes this to use 2 spaces.

The same needs to be done in nsCocoaUtils.mm.

Comment 22

[Nomis101]

Attachment: Patch v2

OK, you mean like this?

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 627385 [details] [diff] [review]
Patch v2

Great! Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a1008736aede

Comment 24

[Nomis101]

(In reply to Markus Stange from comment #23)
> Comment on attachment 627385 [details] [diff] [review]
> Patch v2
> 
> Great! Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a1008736aede

Thanks. Try says "error: expected type-specifier before 'NSEventPhase'". Why is that? Is builds fine for me without any errors. But I build in x86_64, the error was for i386.

Comment 25

[Markus Stange [:mstange]]

Oh, I should have noticed that. It's because the line
- (NSEventPhase)momentumPhase;
uses "NSEventPhase" before its definition. You'll need to move the @interface NSEvent (ScrollPhase) ... @end part down, below the definition of NSEventPhase, and in a part that's outside the #ifdefs. So, ideally, just above the @interface ChildView line.

The compile error doesn't occur in a 64bit build because there NSEventPhase is defined by #import <Cocoa/Cocoa.h> (which is at the top of the file, and thus before the line that shows the error).

Also, when you post a new patch, can you correct the whitespace mistake here:

+      if ([theEvent respondsToSelector:@selector(scrollingDeltaY)]) {
+          scrollDeltaPixels = -[theEvent scrollingDeltaY];
+        } else {
+          scrollDeltaPixels = -[theEvent deviceDeltaY];
+        }
     }

(After the if, the indentation is 4 instead of 2, and then there's a 4 back indentation between the closing braces at the end)

Comment 26

[Nomis101]

Attachment: Patch v3

OK, this patch now builds for me on i386. It should also fix the other things.

Comment 27

[Markus Stange [:mstange]]

Looks good, pushed to try again, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=39b5a664ad23

Comment 28

[Nomis101]

(In reply to Markus Stange from comment #27)
> Looks good, pushed to try again, just to be sure:
> https://tbpl.mozilla.org/?tree=Try&rev=39b5a664ad23

Cool, the build succeeded this time. :-)

Comment 29

[Nomis101]

Is it OK if I set checkin-needed now, because you reviewed v2 or do I need additional review for v3?

Comment 30

[Markus Stange [:mstange]]

Ahh, sorry for the delay again. You don't need another review, I meant to check this in ages ago, sorry. But yeah, just use checkin-needed, and follow the instructions here: https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

Comment 31

[Nomis101]

Attachment: Patch for checkin

Updated to current trunk.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Ed Morley [:emorley]]

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