714164 - cmd_scrollLineDown/Up have disappeared

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

Filing in General until I narrow down the regression...

I have an add-on called VimKeybindings which recently stopped working, partially. The scroll up/down bindings no longer work in Nightly. They still work on Aurora.

Here's the relevant code:

https://raw.github.com/autonome/vimkeybindings/master/content/vimkeybindings.xml

Here's the add-on:

https://addons.mozilla.org/en-US/firefox/addon/vimkeybindings/

Here's the mxr search, showing the command now only exists in the editor:

http://mxr.mozilla.org/mozilla-central/search?string=cmd_scrollLineDown

Comment 1

[Dietrich Ayala (:dietrich)]

regression range check-ins: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c5b90ea7e475&tochange=f63a99195987

Comment 2

[Mozilla RelEng Bot]

Try run for 1e753045190c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1e753045190c
Results (out of 20 total builds):
    success: 8
    warnings: 10
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1e753045190c

Comment 3

[Mozilla RelEng Bot]

Try run for ff06a9a9d700 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ff06a9a9d700
Results (out of 6 total builds):
    success: 2
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ff06a9a9d700

Comment 4

[Mozilla RelEng Bot]

Try run for ef7f940d9839 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ef7f940d9839
Results (out of 94 total builds):
    success: 61
    warnings: 33
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ef7f940d9839

Comment 5

[[e-mail]]

Attachment: Proposed patch

I tried various ways to reduce code duplication but I kept on failing tests :-(

Comment 6

[Mozilla RelEng Bot]

Try run for f9afc3e98c60 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f9afc3e98c60
Results (out of 94 total builds):
    success: 83
    warnings: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9afc3e98c60

Comment 7

[Mozilla RelEng Bot]

Try run for c2b4dc4e2227 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c2b4dc4e2227
Results (out of 94 total builds):
    success: 82
    warnings: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c2b4dc4e2227

Comment 8

[Mozilla RelEng Bot]

Try run for dd75f788e19c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=dd75f788e19c
Results (out of 94 total builds):
    success: 81
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dd75f788e19c

Comment 9

[[e-mail]]

Attachment: Possible patch

This is a much nicer patch because of the code simplification, and I finally managed to find my error that was causing all the tests to fail.

Comment 10

[(no longer active)]

Comment on attachment 585189 [details] [diff] [review]
Possible patch

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

::: content/base/public/nsISelectionController.idl
@@ +263,2 @@
>     */
> +    void scrollHorizontal(in boolean right);

I was going to suggest splitting this into two methods:

void scrollLeft();
void scrollRight();

But you did this for the browserCommands logic to work, right?  Couldn't you just swap the back/forward strings there and leave this method as it is?

::: dom/base/nsGlobalWindowCommands.cpp
@@ +186,5 @@
>  NS_IMETHODIMP
>  nsSelectionCommandsBase::DoCommand(const char *aCommandName, nsISupports *aCommandContext)
>  {
> +  // implemented by subclasses
> +  return NS_ERROR_NOT_IMPLEMENTED;

Can't you just not define this method in this class, and let the children override it?

@@ +288,5 @@
>        }
>      }
>    }
>  
> +  for (int i = 0; i < NS_ARRAY_LENGTH(browseCommands); i++) {

Use mozilla::ArrayLength please.

Comment 11

[[e-mail]]

(In reply to Ehsan Akhgari from comment #10)
> (From update of attachment 585189 [details] [diff] [review])
> > +    void scrollHorizontal(in boolean right);
> I was going to suggest splitting this into two methods:
> 
> void scrollLeft();
> void scrollRight();
> 
> But you did this for the browserCommands logic to work, right?  Couldn't you
> just swap the back/forward strings there and leave this method as it is?
No, because the movement commands map "true" as forward, which (at least in LTR) maps to "right". (I don't know whether it makes sense to call this variable "forward" rather than "right"...)

> >  NS_IMETHODIMP
> >  nsSelectionCommandsBase::DoCommand(const char *aCommandName, nsISupports *aCommandContext)
> >  {
> > +  // implemented by subclasses
> > +  return NS_ERROR_NOT_IMPLEMENTED;
> Can't you just not define this method in this class, and let the children
> override it?
Sure, but be warned that this involves copying and pasting the declarations instead of using the NS_DECL_NSICONTROLLERCOMMAND macro.

> > +  for (int i = 0; i < NS_ARRAY_LENGTH(browseCommands); i++) {
> Use mozilla::ArrayLength please.
Bah, that sucks for debugging - it's a function call.

Comment 12

[(no longer active)]

(In reply to [e-mail] from comment #11)
> (In reply to Ehsan Akhgari from comment #10)
> > (From update of attachment 585189 [details] [diff] [review])
> > > +    void scrollHorizontal(in boolean right);
> > I was going to suggest splitting this into two methods:
> > 
> > void scrollLeft();
> > void scrollRight();
> > 
> > But you did this for the browserCommands logic to work, right?  Couldn't you
> > just swap the back/forward strings there and leave this method as it is?
> No, because the movement commands map "true" as forward, which (at least in
> LTR) maps to "right". (I don't know whether it makes sense to call this
> variable "forward" rather than "right"...)

The main objection that I have to this change is that it changes the semantics of the method with no way for callers to realize that.

Let's do something else instead.  Add another bool field to the struct, called reverseScrollSemantics, which would be false for everything except ScrollHorizontal.  Then you can check for that and reverse the bool you pass to the scroll function when you call it.

That's a terrible hack, I know, but I think it's better than silently changing the semantics of a method.

/me notes how much he hates bool arguments used in this way.

> > >  NS_IMETHODIMP
> > >  nsSelectionCommandsBase::DoCommand(const char *aCommandName, nsISupports *aCommandContext)
> > >  {
> > > +  // implemented by subclasses
> > > +  return NS_ERROR_NOT_IMPLEMENTED;
> > Can't you just not define this method in this class, and let the children
> > override it?
> Sure, but be warned that this involves copying and pasting the declarations
> instead of using the NS_DECL_NSICONTROLLERCOMMAND macro.

Not worth it, nevermind.  :-)

> > > +  for (int i = 0; i < NS_ARRAY_LENGTH(browseCommands); i++) {
> > Use mozilla::ArrayLength please.
> Bah, that sucks for debugging - it's a function call.

Yes, but only if you step into, in which case you're already used to dealing with other problems (hello nsCOMPtr::operator-> and friends!).  Also, see bug 693469.  :-)

Comment 13

[[e-mail]]

(In reply to Ehsan Akhgari from comment #12)
> (In reply to from comment #11)
> > (In reply to Ehsan Akhgari from comment #10)
> > > (From update of attachment 585189 [details] [diff] [review])
> > > > +    void scrollHorizontal(in boolean right);
> > > I was going to suggest splitting this into two methods:
> > > 
> > > void scrollLeft();
> > > void scrollRight();
> > > 
> > > But you did this for the browserCommands logic to work, right?  Couldn't you
> > > just swap the back/forward strings there and leave this method as it is?
> > No, because the movement commands map "true" as forward, which (at least in
> > LTR) maps to "right". (I don't know whether it makes sense to call this
> > variable "forward" rather than "right"...)
> The main objection that I have to this change is that it changes the
> semantics of the method with no way for callers to realize that.
I guess changing the IID doesn't help because everyone will just recompile their binary components against the new SDK anyway. Maybe I should rename the method?

Comment 14

[(no longer active)]

(In reply to [e-mail] from comment #13)
> (In reply to Ehsan Akhgari from comment #12)
> > (In reply to from comment #11)
> > > (In reply to Ehsan Akhgari from comment #10)
> > > > (From update of attachment 585189 [details] [diff] [review])
> > > > > +    void scrollHorizontal(in boolean right);
> > > > I was going to suggest splitting this into two methods:
> > > > 
> > > > void scrollLeft();
> > > > void scrollRight();
> > > > 
> > > > But you did this for the browserCommands logic to work, right?  Couldn't you
> > > > just swap the back/forward strings there and leave this method as it is?
> > > No, because the movement commands map "true" as forward, which (at least in
> > > LTR) maps to "right". (I don't know whether it makes sense to call this
> > > variable "forward" rather than "right"...)
> > The main objection that I have to this change is that it changes the
> > semantics of the method with no way for callers to realize that.
> I guess changing the IID doesn't help because everyone will just recompile
> their binary components against the new SDK anyway. Maybe I should rename
> the method?

Yes, and js callers would also not know.  Changing the name of the method does help, but I'm curious to know why the solution I suggested in comment 12 would not work.

Comment 15

[[e-mail]]

Attachment: Proposed patch

Comment 16

[[e-mail]]

Pushed changeset ada466fe633f to mozilla-central.