cmd_scrollLineDown/Up have disappeared

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dietrich, Assigned: neil)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

8 years ago
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
Reporter

Updated

8 years ago
Keywords: regression
Reporter

Updated

8 years ago
Blocks: 669026
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/neil@parkwaycc.co.uk-1e753045190c
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/neil@parkwaycc.co.uk-ff06a9a9d700
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/neil@parkwaycc.co.uk-ef7f940d9839
Assignee

Comment 5

8 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
I tried various ways to reduce code duplication but I kept on failing tests :-(
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #585166 - Flags: review?(ehsan)
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/neil@parkwaycc.co.uk-f9afc3e98c60
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/neil@parkwaycc.co.uk-c2b4dc4e2227
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/neil@parkwaycc.co.uk-dd75f788e19c
Assignee

Comment 9

8 years ago
Posted patch Possible patch (obsolete) — Splinter Review
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.
Attachment #585189 - Flags: review?(ehsan)
Product: Firefox → Core
QA Contact: general → general

Comment 10

8 years ago
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.
Attachment #585189 - Flags: review?(ehsan)

Updated

8 years ago
Attachment #585166 - Attachment is obsolete: true
Attachment #585166 - Flags: review?(ehsan)
Assignee

Comment 11

8 years ago
(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

8 years ago
(In reply to neil@parkwaycc.co.uk 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.  :-)
Assignee

Comment 13

8 years ago
(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

8 years ago
(In reply to neil@parkwaycc.co.uk 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.
Assignee

Comment 15

8 years ago
Attachment #585189 - Attachment is obsolete: true
Attachment #585919 - Flags: superreview?(roc)
Attachment #585919 - Flags: review?(ehsan)
Attachment #585919 - Flags: superreview?(roc) → superreview+

Updated

8 years ago
Attachment #585919 - Flags: review?(ehsan) → review+
Assignee

Comment 16

8 years ago
Pushed changeset ada466fe633f to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.