Last Comment Bug 804845 - CTRL+P and CTRL+N should work the same as up arrow and down arrow respectively.
: CTRL+P and CTRL+N should work the same as up arrow and down arrow respectively.
Status: RESOLVED FIXED
[good first bug][mentor=msucan][lang=js]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 20
Assigned To: zmgmoz
:
Mentors:
Depends on:
Blocks: 817834
  Show dependency treegraph
 
Reported: 2012-10-23 17:07 PDT by Brian Brennan [:brianloveswords]
Modified: 2012-12-21 15:22 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implements CTRL+P and CTRL+N for history up and down, respectively, in webconsole (3.78 KB, patch)
2012-11-26 18:03 PST, zmgmoz
mihai.sucan: feedback+
Details | Diff | Review
Better fix for CTRL+P/N/A/E navigation within webconsole input (17.52 KB, patch)
2012-11-29 13:19 PST, zmgmoz
no flags Details | Diff | Review
more better fix for CTRL+P/N/A/E navigation within webconsole input (14.34 KB, patch)
2012-12-03 14:49 PST, zmgmoz
no flags Details | Diff | Review
fix for CTRL+P/N/A/E navigation within webconsole input (14.23 KB, patch)
2012-12-05 13:59 PST, zmgmoz
no flags Details | Diff | Review
fix for CTRL+P/N/A/E navigation within webconsole input (14.13 KB, patch)
2012-12-11 17:33 PST, zmgmoz
mihai.sucan: review+
Details | Diff | Review
final patch (14.23 KB, patch)
2012-12-17 06:02 PST, Mihai Sucan [:msucan]
mihai.sucan: checkin+
Details | Diff | Review

Description Brian Brennan [:brianloveswords] 2012-10-23 17:07:57 PDT
I'm on 18.0a2 (2012-10-23).

In the operating system, CTRL+P and CTRL+N move one line up and one line down. This is not the behavior that's exhibited in the web console – instead, CTRL+P goes to the beginning of the line and CTRL+N goes to the end of the line (which CTRL+A and CTRL+E already do).
Comment 1 Mihai Sucan [:msucan] 2012-10-24 01:47:25 PDT
To fix this bug you need to edit browser/devtools/webconsole/webconsole.js. Search for JSTF_keyPress() and add the two new keyboard shortcuts. You also need to update one of the web console history tests to check that new shortcuts work as expected.

Thank you!
Comment 2 Paul Rouget [:paul] 2012-10-24 01:58:09 PDT
To clarify things, ctrl-p should do this:
- go up and keep cursor position;
- if there's nothing to go up to, go to the beginning of the line;

In case of single line textboxes (url bar, search box), it always go to the beginning.

The console has history, so yeah, we should do as suggested by brian (but keep the current behavior if there's no history).
Comment 3 Brian Brennan [:brianloveswords] 2012-10-24 09:39:48 PDT
Ah ha, if this is JS I can try to tackle it myself.

paul, I'm not sure it should keep cursor position. In fields with history, the native behavior when moving through the history (at least in OS X) is to always put the cursor at the end of the line. This is also how up arrow/down arrow works.
Comment 4 zmgmoz 2012-11-26 18:03:21 PST
Created attachment 685429 [details] [diff] [review]
implements CTRL+P and CTRL+N for history up and down, respectively, in webconsole

hey folk, saw this and thought I'd give it crack.
Unlike arrow up/down keys, this responds to CTRL+P/N by canceling out of any autocomplete suggestions. I'm not 100% sure this is the intuitive behaviour, though, so happy to change. Let me know!
Comment 5 zmgmoz 2012-11-26 18:04:59 PST
sorry, haven't got to adding tests for it yet, but looking into that side of things now :)
Comment 6 Panos Astithas [:past] 2012-11-26 23:59:25 PST
Comment on attachment 685429 [details] [diff] [review]
implements CTRL+P and CTRL+N for history up and down, respectively, in webconsole

Note that review+ means that someone has reviewed the patch and approved it. You want to use review? when requesting review. But since you are still working on tests and just need some feedback, setting feedback? is what you were looking for.
Comment 7 Mihai Sucan [:msucan] 2012-11-27 03:35:28 PST
Zoe: thank you for taking this bug. I'm going to look into your patch really soon.
Comment 8 Mihai Sucan [:msucan] 2012-11-27 03:57:08 PST
Comment on attachment 685429 [details] [diff] [review]
implements CTRL+P and CTRL+N for history up and down, respectively, in webconsole

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

This is a good patch. Thank you! It did apply cleanly and all tests pass.

Below I have some suggestions for improvements.

::: browser/devtools/webconsole/webconsole.js
@@ +3190,5 @@
> +            // find index of closest newline <= to cursor
> +            let str = this.inputNode.value.slice(0, 
> +                                                 this.inputNode.selectionStart);
> +            str = str.split("").reverse().join("");
> +            let newlineIndex = str.search(/[\r\n]/);

It might be more efficient to just have a while loop that checks every char starting from selectionStart, going backwards, until it finds a new line.

(please excuse my pseudo-code :) )

@@ +3217,5 @@
> +        case 110:
> +          // control-n differs from down arrow; ignores autocomplete state
> +          if (this.canCaretGoNext()) {
> +            if (this.historyPeruse(HISTORY_FORWARD)) {
> +              aEvent.preventDefault();

I think we want to always call preventDefault() for these new keyboard shortcuts. If I reach the end of the history preventDefault is not called and I get the Print dialog (for ctrl-p) or a new window (for ctrl-n) - both equally disruptive for the user.

@@ +3223,5 @@
> +          }
> +          closePopup = true;
> +          break;
> +	    case 112:
> +		  // control-p differs from up arrow; ignores autocomplete state 

nit: please correctly align these lines - same style as above.

@@ +3363,5 @@
>    /**
> +   * Test for multiline input
> +   * 
> +   * @return boolean
> +   * 		 true if CR or LF found in node value; else false

nit: please align return description with return type and start with upper case, end with a period.
Comment 9 zmgmoz 2012-11-28 01:08:54 PST
(In reply to Mihai Sucan [:msucan] from comment #8)
> Below I have some suggestions for improvements.

Thanks Mihai!

> It might be more efficient to just have a while loop that checks every char
> starting from selectionStart, going backwards, until it finds a new line.
> 
> (please excuse my pseudo-code :) )

Funny, I initially implemented it that way. I'll drop it back in :)
Actually, I just ran some tests in jsperf and it looks like Firefox also does better with the explicit comparisons (as opposed to slice then regex) on the forwards-search (ctrl-e) scenario. So I'll change that, too.

> @@ +3217,5 @@
> > +        case 110:
> > +          // control-n differs from down arrow; ignores autocomplete state
> > +          if (this.canCaretGoNext()) {
> > +            if (this.historyPeruse(HISTORY_FORWARD)) {
> > +              aEvent.preventDefault();
> 
> I think we want to always call preventDefault() for these new keyboard
> shortcuts. If I reach the end of the history preventDefault is not called
> and I get the Print dialog (for ctrl-p) or a new window (for ctrl-n) - both
> equally disruptive for the user.

Oh, that's interesting! I'm on a mac, so didn't notice (since Print is command-p, and my ctrl-n is otherwise unmapped).
And in fact I was just relying on the default behaviour for inter-text line navigation when in multiline mode.
Will have to revisit this, tomorrow!

What's your feeling about the appropriate behaviour of ctrl-p/n when autocompletePopup.isOpen? I'm now wondering that if people are using ctrl-p/n as alternatives to up/down arrows, then we should maintain the same dialogue navigation (ie. they'd need to press ESC, to close).
Comment 10 Mihai Sucan [:msucan] 2012-11-28 01:32:57 PST
(In reply to zmgmoz from comment #9)
> Oh, that's interesting! I'm on a mac, so didn't notice (since Print is
> command-p, and my ctrl-n is otherwise unmapped).
> And in fact I was just relying on the default behaviour for inter-text line
> navigation when in multiline mode.
> Will have to revisit this, tomorrow!

Thank you!

> What's your feeling about the appropriate behaviour of ctrl-p/n when
> autocompletePopup.isOpen? I'm now wondering that if people are using
> ctrl-p/n as alternatives to up/down arrows, then we should maintain the same
> dialogue navigation (ie. they'd need to press ESC, to close).

I agree with the current behavior you implemented in the patch. The popup needs to close when you use ctrl-p/n.

I'm looking forward for the updated patch!
Comment 11 zmgmoz 2012-11-28 11:22:43 PST
I've been working on tests, and reading the bugs they reference (https://bugzilla.mozilla.org/show_bug.cgi?id=619598)...

Sorry if this is getting off topic, but how would I write a test for what the console inputNode.value actually looks like, on different systems? I'm wondering if it would be workable to normalize the inputNode.value in multiline scenarios by always inserting "\n" (and calling preventDefault()) on shift+ENTER. (This works fine for me on my mac)
Then all the checks & munging done with respect to linebreaks will be much cleaner - no more checking for '\r' and/or '\n'.
(I don't want to propagate a needless inefficiency!)
Comment 12 Mihai Sucan [:msucan] 2012-11-28 11:34:04 PST
No problem. It's good to ask questions!

Perhaps you don't need to bother changing how shift-enter works - users expect shift-enter to work as usual. In your mochitest you can normalize the input, when you read it - replace all \r\n with \n, or similar. To keep the test clean you make it a helper function - like getInputValue(), then you do all checks assuming only \n is used, on all systems.
Comment 13 zmgmoz 2012-11-28 12:27:39 PST
Actually, the change to shift-enter that I was thinking of would have been a behind-the-scenes change, just to simplify all the rest of the code.
(Yes, I've noticed that none of the tests actually check for handling of \r ;)

I'll leave it alone for now, but (especially if it a simple fix like substituting "\n" in the inputNode.value, instead of for "\r" or "\r\n", etc) it seemed like an opportunity for removing unnecessary weight from many places. But I don't yet know enough about how the inputNode.value is displayed, based on that value, and don't have any other OSes on hand to do a quick check.
Comment 14 Mihai Sucan [:msucan] 2012-11-28 12:40:38 PST
Yeah, don't worry, leave that alone for now. Don't bother about making a test run on multiple systems before you get it to run well on yours. I can test on Linux, and we have try servers to push the patch to - they run tests on multiple systems.
Comment 15 zmgmoz 2012-11-29 13:19:42 PST
Created attachment 686769 [details] [diff] [review]
Better fix for CTRL+P/N/A/E navigation within webconsole input

Tidied up, and new test included.

NOTE: One thing I haven't resolved is up/down (using CTRL+P/CTRL+N) navigation within multiline input for setups which use these shortcuts to print/open new window. This isn't a regression - it just isn't an improvement.
If preventDefault on must be always called on these, I haven't figured out how to preserve the desired up/down default navigation behaviour.
Is it possible to dispatch a subsequent keypress event (say to trigger up/down key) to the webconsole?
Failing that, I could synthesize up/down behaviour by calculating the appropriate offset at which to reposition the caret... but this seems a bit like opening a can of worms in trying to redefine each system's 'natural' pattern of modifier keys.

Advice?
Comment 16 Mihai Sucan [:msucan] 2012-11-30 09:58:53 PST
Comment on attachment 686769 [details] [diff] [review]
Better fix for CTRL+P/N/A/E navigation within webconsole input

(please make sure the attachment is marked as a patch - this allows us to review the patch more easily. thanks! ;) )
Comment 17 Mihai Sucan [:msucan] 2012-11-30 10:52:00 PST
Comment on attachment 686769 [details] [diff] [review]
Better fix for CTRL+P/N/A/E navigation within webconsole input

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

Thanks for the updated patch! I like the test - it's looks well done.

Two things stand out:

- the test currently fails - Ctrl-P and Ctrl-N show those dialogs on my system. I believe this was expected, so I am not complaining.
- you've made changes to how the history behaves. I'd like us to avoid doing that, for now, in this bug. Usually we keep patches well scoped, so if there's a regression we can point to the bug. If you want to make history changes, let's open a follow up bug and we can work on a separate patch and test there.

To answer your question: current behavior for ctrl-p/n is correct. I'd like to know on which systems does Ctrl-P/N move within lines? Macs only? On Ubuntu Linux and Windows 7 I don't see this behavior. So, limit these new ctrl-p/n shortcuts only to Macs.

if (Services.appinfo.OS == "Darwin") {
   ... what you have now ...
}

Also limit the test to Macs only:

ifeq ($(OS_ARCH),Darwin)
  MOCHITEST_BROWSER_FILES += testfile.js
endif

Thanks again!
Comment 18 zmgmoz 2012-12-03 14:49:26 PST
Created attachment 687980 [details] [diff] [review]
more better fix for CTRL+P/N/A/E navigation within webconsole input

As per review, limit all custom handling of CTRL+P/N to Darwin OS, and just preventDefault for all other OSes.
Likewise, new test is also limited to Macs.
Stashing the latest 'working' command has been stripped out - I'll open another bug for that :)
Comment 19 Mihai Sucan [:msucan] 2012-12-05 12:27:37 PST
Comment on attachment 687980 [details] [diff] [review]
more better fix for CTRL+P/N/A/E navigation within webconsole input

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

Thank you for the updated patch!

Just minor comments below. After your next update I will also test the patch on a Mac system and I will push it to the Mozilla try servers which will run the tests on multiple systems and configurations.

::: browser/devtools/webconsole/test/Makefile.in
@@ +123,5 @@
>  	head.js \
>  	$(NULL)
>  
> +ifeq ($OS_ARCH), Darwin)
> +MOCHITEST_BROWSER_FILES += browser_webconsole_bug_804845_ctrl_key_nav.js

Does this work? Tested on your Mac?

::: browser/devtools/webconsole/test/browser_webconsole_bug_804845_ctrl_key_nav.js
@@ +146,5 @@
> +}
> +
> +function testNavWithHistory() {
> +  // NOTE: Tests does NOT currently define behaviour for ctrl-p/ctrl-n with 
> +  // caret placed _within_ single line input

Why not?

::: browser/devtools/webconsole/webconsole.js
@@ +3285,5 @@
> +                aEvent.preventDefault();
> +              }
> +            }
> +          } else {
> +            aEvent.preventDefault();

We do not need to prevent ctrl-p and ctrl-n from showing the print dialog / opening a new window on Linux and Windows. I suggest we remove the preventDefault() here.

I know, this is slightly confusing because the initial bug report wasn't clear to us (my bad!), but the idea is that ctrl-p/n behavior changes must be limited only to Macs. So, simply remove this |else preventDefault| and you'll be fine.

@@ +3300,5 @@
> +                aEvent.preventDefault();
> +              }
> +            }
> +          } else {
> +            aEvent.preventDefault();

Same as above: please remove these two lines.
Comment 20 zmgmoz 2012-12-05 13:31:50 PST
responses inline below...

(In reply to Mihai Sucan [:msucan] from comment #19)
> Comment on attachment 687980 [details] [diff] [review]
> more better fix for CTRL+P/N/A/E navigation within webconsole input
> 
> Review of attachment 687980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the updated patch!
> 
> Just minor comments below. After your next update I will also test the patch
> on a Mac system and I will push it to the Mozilla try servers which will run
> the tests on multiple systems and configurations.
> 
> ::: browser/devtools/webconsole/test/Makefile.in
> @@ +123,5 @@
> >  	head.js \
> >  	$(NULL)
> >  
> > +ifeq ($OS_ARCH), Darwin)
> > +MOCHITEST_BROWSER_FILES += browser_webconsole_bug_804845_ctrl_key_nav.js
> 
> Does this work? Tested on your Mac?
>

Works for me on my Mac :)

> :::
> browser/devtools/webconsole/test/browser_webconsole_bug_804845_ctrl_key_nav.
> js
> @@ +146,5 @@
> > +}
> > +
> > +function testNavWithHistory() {
> > +  // NOTE: Tests does NOT currently define behaviour for ctrl-p/ctrl-n with 
> > +  // caret placed _within_ single line input
> 
> Why not?

Because reading through this bug (& especially the history of bug 619598, where it seems like the behaviour for up/down first reached a compromise to allow for multiline input) it's not clear what the specification should be for single-line internal ctrl+n/ctrl+p.
And although I have my preferences - I think it should also trigger historyPeruse(), the same way that my system commandline prompt works, and it would be a simple change in canCaretGoNext()/canCaretGoPrevious(), without breaking any current tests - for the purpose of this patch, I didn't want to open up another can of worms!

So the way it's implemented, it will just drop through to the default behaviour, thus not affecting a change, but therefore not absolutely testable. (fwiw, on my Mac, result is go to start/end of line, the same as up/down.)

(And interestingly, this is the same conclusion reached for up/down in the comment: https://bugzilla.mozilla.org/show_bug.cgi?id=619598#c4)

I figured I could always open up a follow-up bug to argue for revised ctrl+p/ctrl+n behaviour, on a Mac, if it started to bug me that much :)


> ::: browser/devtools/webconsole/webconsole.js
> @@ +3285,5 @@
> > +                aEvent.preventDefault();
> > +              }
> > +            }
> > +          } else {
> > +            aEvent.preventDefault();
> 
> We do not need to prevent ctrl-p and ctrl-n from showing the print dialog /
> opening a new window on Linux and Windows. I suggest we remove the
> preventDefault() here.
> 
> I know, this is slightly confusing because the initial bug report wasn't
> clear to us (my bad!), but the idea is that ctrl-p/n behavior changes must
> be limited only to Macs. So, simply remove this |else preventDefault| and
> you'll be fine.

Ah, ok. I took your earlier comment of "I think we want to always call preventDefault() for these new keyboard shortcuts..." to still hold for non-Macs. Will drop.

> @@ +3300,5 @@
> > +                aEvent.preventDefault();
> > +              }
> > +            }
> > +          } else {
> > +            aEvent.preventDefault();
> 
> Same as above: please remove these two lines.

Gotcha. :)
Comment 21 zmgmoz 2012-12-05 13:59:06 PST
Created attachment 688951 [details] [diff] [review]
fix for CTRL+P/N/A/E navigation within webconsole input
Comment 22 Mihai Sucan [:msucan] 2012-12-06 13:52:52 PST
Comment on attachment 688951 [details] [diff] [review]
fix for CTRL+P/N/A/E navigation within webconsole input

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

Thanks for the patch update.

You are right about avoiding the need to open a can of worms. Let it be - let's see if others want to open a bug about the issue.

Just a couple of concerns remain, see below.

::: browser/devtools/webconsole/test/Makefile.in
@@ +123,5 @@
>  	head.js \
>  	$(NULL)
>  
> +ifeq ($OS_ARCH), Darwin)
> +MOCHITEST_BROWSER_FILES += browser_webconsole_bug_804845_ctrl_key_nav.js

Tested on my Mac: this file doesn't get added to the list of tests, so it never runs. This should be:

ifeq ($(OS_ARCH), Darwin)
...

(please double-check the test actually runs after you make the changes)

Also, please correctly indent MOCHITEST_BROWSER_FILES to match the rest of the file. Thanks!

::: browser/devtools/webconsole/webconsole.js
@@ +3280,5 @@
> +            // control-n differs from down arrow; ignores autocomplete state
> +            if (this.canCaretGoNext()) {
> +              // NOTE: preserve default 'down' navigation within multiline text
> +              if (this.historyPeruse(HISTORY_FORWARD) ||
> +                  (inputNode.selectionStart == inputNode.value.length)) {

is the selection check needed? isn't this condition already covered by canCaretGoNext?

@@ +3293,5 @@
> +            // control-p differs from up arrow; ignores autocomplete state
> +            if (this.canCaretGoPrevious()) {
> +              // NOTE: preserve default 'up' navigation within multiline text
> +              if (this.historyPeruse(HISTORY_BACK) ||
> +                  (inputNode.selectionEnd == 0)) {

same question as above
Comment 23 zmgmoz 2012-12-06 15:41:38 PST
(In reply to Mihai Sucan [:msucan] from comment #22)
> Comment on attachment 688951 [details] [diff] [review]
> fix for CTRL+P/N/A/E navigation within webconsole input
> 
> Review of attachment 688951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch update.
> 
> You are right about avoiding the need to open a can of worms. Let it be -
> let's see if others want to open a bug about the issue.
> 
> Just a couple of concerns remain, see below.
> 
> ::: browser/devtools/webconsole/test/Makefile.in
> @@ +123,5 @@
> >  	head.js \
> >  	$(NULL)
> >  
> > +ifeq ($OS_ARCH), Darwin)
> > +MOCHITEST_BROWSER_FILES += browser_webconsole_bug_804845_ctrl_key_nav.js
> 
> Tested on my Mac: this file doesn't get added to the list of tests, so it
> never runs. This should be:
> 
> ifeq ($(OS_ARCH), Darwin)
> ...
> 
> (please double-check the test actually runs after you make the changes)

Huh. Of course it's not correct now that I look at it closely :) Although it seems I had a different problem, too: 

The symlink to the test from the 'installed' mochitests dir remained, and was therefore run, even after my sequence of: 
<alter Makefile.in, in this case to completely blow away the addition of the test>
$ (cd obj-x86_64-apple-darwin11.4.2 && make -s -C browser/devtools/ && make -s -C browser/app)
creating test/Makefile.
$ TEST_PATH=browser/devtools/webconsole/test make -C obj-x86_64-apple-darwin11.4.2 mochitest-browser-chrome
...

Seems like a bug/omission in the Make system to me. Is there in fact a target to clean links from the mochitests dir?

From the $OBJDIR/browser/devtools/webconsole/test directory, both make clobber and clean just give me:
rm -f .md core     Templates.DB           LOGS TAGS a.out          so_locations _gen _stubs               
rm -f -r  _java /no-such-file

and, that's about the extent that I'm about to go diving in this Make system right now :)
(In the end, I just manually removed it, before revising the Makefile.in, and repeating the sequence again.)


> 
> Also, please correctly indent MOCHITEST_BROWSER_FILES to match the rest of
> the file. Thanks!

To be honest, the indentation of Makefile.in as of changeset 114145:07b710216328 (parent to my working at the time) was a complete mess, with a mix of tab/spaces. I had no idea which pattern you'd prefer I follow so I just used your suggested one-liner, but kept MOCHITEST_BROWSER_FILES beginning on col0 like the rest of the file!
What exactly would you like to see here?

> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +3280,5 @@
> > +            // control-n differs from down arrow; ignores autocomplete state
> > +            if (this.canCaretGoNext()) {
> > +              // NOTE: preserve default 'down' navigation within multiline text
> > +              if (this.historyPeruse(HISTORY_FORWARD) ||
> > +                  (inputNode.selectionStart == inputNode.value.length)) {
> 
> is the selection check needed? isn't this condition already covered by
> canCaretGoNext?
> 
> @@ +3293,5 @@
> > +            // control-p differs from up arrow; ignores autocomplete state
> > +            if (this.canCaretGoPrevious()) {
> > +              // NOTE: preserve default 'up' navigation within multiline text
> > +              if (this.historyPeruse(HISTORY_BACK) ||
> > +                  (inputNode.selectionEnd == 0)) {
> 
> same question as above

Sigh. It's probably a left-over from when I implemented some revised behaviour for canCaretGoPrevious/canCaretGoNext(). Will look again.
Comment 24 Mihai Sucan [:msucan] 2012-12-07 08:32:52 PST
(In reply to zmgmoz from comment #23)
> > Tested on my Mac: this file doesn't get added to the list of tests, so it
> > never runs. This should be:
> > 
> > ifeq ($(OS_ARCH), Darwin)
> > ...
> > 
> > (please double-check the test actually runs after you make the changes)
> 
> Huh. Of course it's not correct now that I look at it closely :) Although it
> seems I had a different problem, too: 
> 
> The symlink to the test from the 'installed' mochitests dir remained, and
> was therefore run, even after my sequence of: 
> <alter Makefile.in, in this case to completely blow away the addition of the
> test>
> $ (cd obj-x86_64-apple-darwin11.4.2 && make -s -C browser/devtools/ && make
> -s -C browser/app)
> creating test/Makefile.
> $ TEST_PATH=browser/devtools/webconsole/test make -C
> obj-x86_64-apple-darwin11.4.2 mochitest-browser-chrome
> ...
> 
> Seems like a bug/omission in the Make system to me. Is there in fact a
> target to clean links from the mochitests dir?

Not sure if there is. I haven't seen this problem. I just rerun make. If your builds get badly damaged, just clobber it (rm obj-dir).


> > Also, please correctly indent MOCHITEST_BROWSER_FILES to match the rest of
> > the file. Thanks!
> 
> To be honest, the indentation of Makefile.in as of changeset
> 114145:07b710216328 (parent to my working at the time) was a complete mess,
> with a mix of tab/spaces. I had no idea which pattern you'd prefer I follow
> so I just used your suggested one-liner, but kept MOCHITEST_BROWSER_FILES
> beginning on col0 like the rest of the file!
> What exactly would you like to see here?

I see your point. You could use col 0 for the variable name and 8 spaces for the test file name (on a new line), like the others do. Whatever looks similar - don't bother much. (the main point was about the non-working ifeq condition)


Thanks!
Comment 25 zmgmoz 2012-12-11 17:33:34 PST
Created attachment 691158 [details] [diff] [review]
fix for CTRL+P/N/A/E navigation within webconsole input

fixed makefile inclusion, cleaned up some superfluous condition handling in webconsole.js
Comment 26 Mihai Sucan [:msucan] 2012-12-13 12:04:33 PST
Comment on attachment 691158 [details] [diff] [review]
fix for CTRL+P/N/A/E navigation within webconsole input

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

Thank you. Patch looks good, r+.

Couple of nits: you have some ifs that could be merged into one, and the patch has trailing whitespaces. I can fix these, no worries.

Pushed the patch to the try servers:
https://tbpl.mozilla.org/?tree=Try&rev=5a3925511056

If results are green, I will land the patch.
Comment 27 Mihai Sucan [:msucan] 2012-12-17 06:02:26 PST
Created attachment 692917 [details] [diff] [review]
final patch

Try push results are green.

Removed trailing whitespaces and coalesced if conditions in webconsole.js.

Landed the patch:
https://hg.mozilla.org/integration/fx-team/rev/a5793af7e70c

Thank you for your contribution Zoe!
Comment 28 Panos Astithas [:past] 2012-12-19 00:10:30 PST
https://hg.mozilla.org/mozilla-central/rev/a5793af7e70c
Comment 29 Mozilla RelEng Bot 2012-12-21 15:22:50 PST
Try run for 5a3925511056 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5a3925511056
Results (out of 52 total builds):
    exception: 1
    success: 50
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-5a3925511056

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