Last Comment Bug 638004 - Various "…deprecated for NSScrollWheel. Please use…" messages to console on first trackpad scroll
: Various "…deprecated for NSScrollWheel. Please use…" messages to console on f...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- trivial with 4 votes (vote)
: mozilla16
Assigned To: Nomis101
:
Mentors:
: 707667 (view as bug list)
Depends on:
Blocks: lion-compatibility
  Show dependency treegraph
 
Reported: 2011-03-01 20:46 PST by Doug Grinbergs
Modified: 2012-06-22 03:43 PDT (History)
17 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Will this do it? (5.20 KB, patch)
2012-04-08 03:20 PDT, Nomis101
no flags Details | Diff | Review
Maybe a fix, now with hg header (5.20 KB, patch)
2012-04-09 09:48 PDT, Nomis101
no flags Details | Diff | Review
WIP (3.43 KB, patch)
2012-04-10 14:11 PDT, Nomis101
no flags Details | Diff | Review
Patch (4.96 KB, patch)
2012-04-11 06:13 PDT, Nomis101
no flags Details | Diff | Review
Patch v2 (5.38 KB, patch)
2012-05-25 15:27 PDT, Nomis101
mstange: review+
Details | Diff | Review
Patch v3 (6.32 KB, patch)
2012-05-26 11:32 PDT, Nomis101
no flags Details | Diff | Review
Patch for checkin (6.32 KB, patch)
2012-06-20 11:38 PDT, Nomis101
no flags Details | Diff | Review

Description Doug Grinbergs 2011-03-01 20:46:31 PST
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) 2011-07-21 01:19:19 PDT
Can the fix for that enable the "bounce" animation when reaching top and bottom part of a page?
Comment 2 Markus Stange [:mstange] 2011-07-21 04:37:44 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-07-22 10:37:26 PDT
I filed bug 619354 on a similar issue I saw on 10.6.
Comment 4 Ben 2011-08-05 09:09:07 PDT
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 2011-11-26 15:39:41 PST
looks like it's fixed in Fx 9b3 / Tb 9b2
Comment 6 Matthias Versen [:Matti] 2011-12-05 09:08:55 PST
*** Bug 707667 has been marked as a duplicate of this bug. ***
Comment 7 Nomis101 2011-12-11 03:21:04 PST
(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 2011-12-11 03:54:52 PST
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 2012-02-03 17:22:07 PST
Verified still exists using 10.0 on Lion. Same messages as Comment 8
Comment 10 Nomis101 2012-04-08 03:20:50 PDT
Created attachment 613173 [details] [diff] [review]
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) 2012-04-09 07:55:26 PDT
(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 2012-04-09 09:48:09 PDT
Created attachment 613326 [details] [diff] [review]
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] 2012-04-09 11:07:19 PDT
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) 2012-04-09 11:11:35 PDT
Markus, if you're willing to take over reviewing Nomis101's patch, that'd be fine by me :-)
Comment 15 Nomis101 2012-04-09 12:49:03 PDT
(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 2012-04-10 01:43:58 PDT
(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] 2012-04-10 06:41:58 PDT
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 2012-04-10 14:11:35 PDT
Created attachment 613760 [details] [diff] [review]
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] 2012-04-11 00:31:04 PDT
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 2012-04-11 06:13:34 PDT
Created attachment 613966 [details] [diff] [review]
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] 2012-05-24 14:38:32 PDT
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 2012-05-25 15:27:13 PDT
Created attachment 627385 [details] [diff] [review]
Patch v2

OK, you mean like this?
Comment 23 Markus Stange [:mstange] 2012-05-25 17:28:26 PDT
Comment on attachment 627385 [details] [diff] [review]
Patch v2

Great! Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a1008736aede
Comment 24 Nomis101 2012-05-26 01:49:18 PDT
(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] 2012-05-26 09:27:11 PDT
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 2012-05-26 11:32:40 PDT
Created attachment 627501 [details] [diff] [review]
Patch v3

OK, this patch now builds for me on i386. It should also fix the other things.
Comment 27 Markus Stange [:mstange] 2012-05-26 14:54:05 PDT
Looks good, pushed to try again, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=39b5a664ad23
Comment 28 Nomis101 2012-05-26 16:50:11 PDT
(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 2012-05-31 15:15:03 PDT
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] 2012-06-20 06:28:36 PDT
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 2012-06-20 11:38:33 PDT
Created attachment 634989 [details] [diff] [review]
Patch for checkin

Updated to current trunk.
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-06-21 17:47:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e08abe6f2d61
Comment 33 Ed Morley [:emorley] 2012-06-22 03:43:59 PDT
https://hg.mozilla.org/mozilla-central/rev/e08abe6f2d61

Note You need to log in before you can comment on or make changes to this bug.