Closed Bug 638004 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Widget: Cocoa, defect, trivial)

x86_64
macOS
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: mozilla, Assigned: Nomis101)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

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
Status: UNCONFIRMED → NEW
Component: General → Widget: Cocoa
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → cocoa
Version: unspecified → Trunk
Can the fix for that enable the "bounce" animation when reaching top and bottom part of a page?
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.
I filed bug 619354 on a similar issue I saw on 10.6.
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.
looks like it's fixed in Fx 9b3 / Tb 9b2
Duplicate of this bug: 707667
(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.
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.
Verified still exists using 10.0 on Lion. Same messages as Comment 8
Attached patch Will this do it? (obsolete) — Splinter Review
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.
(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.
Attached patch Maybe a fix, now with hg header (obsolete) — Splinter Review
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.
Attachment #613173 - Attachment is obsolete: true
Attachment #613326 - Flags: review?(smichaud)
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?
Markus, if you're willing to take over reviewing Nomis101's patch, that'd be fine by me :-)
(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?
(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
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];
}
Attached patch WIP (obsolete) — Splinter Review
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
Attachment #613326 - Attachment is obsolete: true
Attachment #613326 - Flags: review?(smichaud)
Attachment #613760 - Flags: feedback?(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".
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #613760 - Attachment is obsolete: true
Attachment #613760 - Flags: feedback?(mstange)
Attachment #613966 - Flags: review?(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.
Attached patch Patch v2Splinter Review
OK, you mean like this?
Attachment #613966 - Attachment is obsolete: true
Attachment #613966 - Flags: review?(mstange)
Attachment #627385 - Flags: feedback?(mstange)
Comment on attachment 627385 [details] [diff] [review]
Patch v2

Great! Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a1008736aede
Attachment #627385 - Flags: feedback?(mstange) → review+
(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.
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)
Attached patch Patch v3 (obsolete) — Splinter Review
OK, this patch now builds for me on i386. It should also fix the other things.
Attachment #627501 - Flags: review?(mstange)
Looks good, pushed to try again, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=39b5a664ad23
(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. :-)
Is it OK if I set checkin-needed now, because you reviewed v2 or do I need additional review for v3?
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
Updated to current trunk.
Attachment #627501 - Attachment is obsolete: true
Attachment #627501 - Flags: review?(mstange)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e08abe6f2d61
Assignee: nobody → Nomis101
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/e08abe6f2d61
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.