Last Comment Bug 714164 - cmd_scrollLineDown/Up have disappeared
: cmd_scrollLineDown/Up have disappeared
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 669026
  Show dependency treegraph
 
Reported: 2011-12-29 13:10 PST by Dietrich Ayala (:dietrich)
Modified: 2012-01-05 13:46 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (17.89 KB, patch)
2011-12-31 14:11 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Possible patch (23.66 KB, patch)
2012-01-01 04:12 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Proposed patch (24.33 KB, patch)
2012-01-04 15:25 PST, neil@parkwaycc.co.uk
ehsan: review+
roc: superreview+
Details | Diff | Splinter Review

Description Dietrich Ayala (:dietrich) 2011-12-29 13:10:19 PST
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) 2011-12-30 09:54:28 PST
regression range check-ins: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c5b90ea7e475&tochange=f63a99195987
Comment 2 Mozilla RelEng Bot 2011-12-30 17:40:42 PST
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
Comment 3 Mozilla RelEng Bot 2011-12-30 19:30:34 PST
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
Comment 4 Mozilla RelEng Bot 2011-12-31 08:00:54 PST
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
Comment 5 neil@parkwaycc.co.uk 2011-12-31 14:11:12 PST
Created attachment 585166 [details] [diff] [review]
Proposed patch

I tried various ways to reduce code duplication but I kept on failing tests :-(
Comment 6 Mozilla RelEng Bot 2011-12-31 15:40:26 PST
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
Comment 7 Mozilla RelEng Bot 2011-12-31 18:00:37 PST
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
Comment 8 Mozilla RelEng Bot 2011-12-31 21:10:28 PST
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
Comment 9 neil@parkwaycc.co.uk 2012-01-01 04:12:27 PST
Created attachment 585189 [details] [diff] [review]
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 :Ehsan Akhgari 2012-01-03 11:51:37 PST
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 neil@parkwaycc.co.uk 2012-01-03 13:46:46 PST
(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 :Ehsan Akhgari 2012-01-03 14:12:21 PST
(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.  :-)
Comment 13 neil@parkwaycc.co.uk 2012-01-04 12:05:39 PST
(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 :Ehsan Akhgari 2012-01-04 12:49:55 PST
(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.
Comment 15 neil@parkwaycc.co.uk 2012-01-04 15:25:49 PST
Created attachment 585919 [details] [diff] [review]
Proposed patch
Comment 16 neil@parkwaycc.co.uk 2012-01-05 13:46:15 PST
Pushed changeset ada466fe633f to mozilla-central.

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