Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Description • 13 years ago

      No description provided.

	AkhilNaik
	Comment 1 • 10 years ago

Hi. I'm a newbie and I want to contribute to this. Can you give me more information about the bug?

	Mihai Sucan [:msucan]
	Comment 2 • 10 years ago

(In reply to akhilnaik.aries from comment #1)
> Hi. I'm a newbie and I want to contribute to this. Can you give me more
> information about the bug?

This is great, however please note this is not a good first bug - fixing this bug requires a bit of experience. You may need to read documentation of advanced difficulty, and in some cases docs may not even be available for things you need to do. Reading the existing code and understanding how it works is usually needed. You will also need to experiment, to try things until you get the code to work as desired - for sufficiently complex bugs we are not always able to provide you with direct answers to questions on how to fix them.

If you feel confident you can fix this bug, please let us know. Otherwise, please search for bugs with [good first bug] in their whiteboard field.

A summary of how this bug can be fixed:

- the webconsole client receives pageError packets that include the columnNumber property - see webconsole.js from browser/. You need to update WCF_reportPageError() to use columnNumber when it creates the error messages that is displayed in the console output.

- reportPageError() still uses the old console code for message output. You need to update the function to use the new ConsoleOutput API. We need to create two new Messages.ScriptError and Messages.StyleError that both extend Messages.Simple - see console-output.js from browser/.

If you have further questions, please let us know. Thanks!

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 3 • 10 years ago

Hi Mihai,

I'm interested in this bug, can you please assign it to me?

Thanks!

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 4 • 10 years ago

I took a look at the bug and something I find weird is that the columnNumber starts with 0 while the lineNumber property starts with 1.

Do you think we should change the provider to make this right?

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 5 • 10 years ago

Okay, I guess it's fixed.

I worked around the column number by simply adding a +1 inside the `createLocationNode`.

Security errors got a little tricky about the <unknown> source, I passed a space just to escape the <unknown>.

About the category switch, I didn't find any function to reverse what `categoryForScriptError` does, maybe we can make that function work both ways with a conditional argument.

I ran mach mochitest browser/devtools and everything passed.

Thanks!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 6 • 10 years ago

Comment on attachment 8492644 [details] [diff] [review]
bug684096_reportPageError-newapi-column.diff

Reassigning since Mihai is out.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 7 • 10 years ago

Comment on attachment 8492644 [details] [diff] [review]
bug684096_reportPageError-newapi-column.diff

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

This is a good first cut, thanks!

If you run 'mach mochitest-devtools browser/devtools/webconsole' you will see some test failures. Make sure you get those fixed first and if the webconsole directory gives a clean test run, try running the mochitest-devtools on the entire browser/devtools directory.

Also, recently the console API calls (console.log, etc.) started including the column number as well. Do you think you can add support for those, too?

::: browser/devtools/webconsole/webconsole.js
@@ +1358,5 @@
>    reportPageError: function WCF_reportPageError(aCategory, aScriptError)
>    {
>      // Warnings and legacy strict errors become warnings; other types become
>      // errors.
> +    let severity = 'ERROR';

Why uppercase for severity and category? I'm pretty sure severity is lowercase and I think the same holds true for category, too.

@@ +1389,5 @@
>        errorMessage = errorMessage.initial;
>      }
>  
> +    // Create a new message
> +    // Security messages don't have a URL and we don't want them to have <unknown> source

That's not true, some security messages have URLs. Otherwise how would this method have worked so far?

@@ +2624,5 @@
>     * @return nsIDOMNode
>     *         The new anchor element, ready to be added to the message node.
>     */
>    createLocationNode:
> +  function WCF_createLocationNode(aSourceURL, aSourceLine, aTarget, aSourceColumn)

I'd prefer to use just two arguments here, "location" and "target", instead of this. In that case "location" would be an object that contains the individual bits of information, like we do elsewhere in devtools:

location: { url: "foo", line: 1, column: 2 }

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 8 • 10 years ago

Thanks for your review, I fixed the problems mentioned by you and the problems seen in `mochitest-devtools browser/devtools/webconsole`.

Just something I'm not totally sure about, in order to pass two tests about repeated warnings, I had to pass `filterDuplicates: true`, should I apply a condition to that? or do all pageErrors have this attribute?

I wanted to ask something, I have a legacy vouch right now, and I want to get a new vouch, may you vouch me please? https://mozillians.org/en-US/u/Mahdi/

Apart from contributing to Firefox (mostly DevTools), I'm coordinator of Persian translation team of Webmaker at Transifex:

https://www.transifex.com/projects/p/webmaker/language/fa/

Thanks.

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 9 • 10 years ago

(In reply to Panos Astithas [:past] from comment #7)

> Also, recently the console API calls (console.log, etc.) started including
> the column number as well. Do you think you can add support for those, too?

I was in a hurry and forgot to answer this, yes, I'll try to create a patch for that as soon as possible, I'm a little busy these days so it might take a few days.

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 10 • 10 years ago

I took a look at |WCF_logConsoleAPIMessage| and it seems the argument it takes |aMessage| doesn't include columnNumber. Am I missing something?

I tried looking for a bug in which this was / is being implemented, but no luck.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 11 • 10 years ago

Comment on attachment 8493868 [details] [diff] [review]
bug684096_reportPageError-newapi-column.diff

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

I have a couple of additional comments, but most importantly this needs a test to make sure we will not regress it in the future. I think you could just add column number checking to some existing tests, or create a new one from scratch. Up to you.

::: browser/devtools/webconsole/console-output.js
@@ +947,5 @@
>      // The ConsoleOutput owner is a WebConsoleFrame instance from webconsole.js.
>      // TODO: move createLocationNode() into this file when bug 778766 is fixed.
> +    return this.output.owner.createLocationNode({url: url,
> +                                                 line: line,
> +                                                 column: column + 1});

I don't think we should modify the column number here. For one thing it will not be consistent with other code paths that use a column number. If we were to make it consistent, we should increase the number either in the spot where the column number is received from the platform API or right before it's being displayed on the screen.

But even that wouldn't be consistent across all output formats. The terminal output for example would still show 0-based columns, leading to different stacktraces between terminal and console.

So the best course of action here seems to be to just leave columns as the platform sends them and if there is strong reason to change them, do it at the platform level.

::: browser/devtools/webconsole/webconsole.js
@@ +2621,5 @@
>     * @return nsIDOMNode
>     *         The new anchor element, ready to be added to the message node.
>     */
>    createLocationNode:
> +  function WCF_createLocationNode(aLocation, aTarget)

This method would be a little cleaner if you used object destructuring:

https://developer.mozilla.org/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

The above would then become:

function WCF_createLocationNode({url, line, column}, target)

and you would avoid the extra property lookups you have added.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 12 • 10 years ago

(In reply to Mahdi Dibaiee [:mahdi] from comment #8)
> Just something I'm not totally sure about, in order to pass two tests about
> repeated warnings, I had to pass `filterDuplicates: true`, should I apply a
> condition to that? or do all pageErrors have this attribute?

We always want to coalesce duplicate errors by default, so we need that flag.

> I wanted to ask something, I have a legacy vouch right now, and I want to
> get a new vouch, may you vouch me please?
> https://mozillians.org/en-US/u/Mahdi/

Done!

(In reply to Mahdi Dibaiee [:mahdi] from comment #10)
> I took a look at |WCF_logConsoleAPIMessage| and it seems the argument it
> takes |aMessage| doesn't include columnNumber. Am I missing something?
> 
> I tried looking for a bug in which this was / is being implemented, but no
> luck.

This was implemented in bug 1069490 and you are looking at the right place. aMessage.columnNumber will be present in a very recent Nightly build (I just verified it).

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 13 • 10 years ago

Thanks for your feedback.

> I have a couple of additional comments, but most importantly this needs a
> test to make sure we will not regress it in the future. I think you could
> just add column number checking to some existing tests, or create a new one
> from scratch. Up to you.
> 

Sure, I'll probably write a new test.

> So the best course of action here seems to be to just leave columns as the
> platform sends them and if there is strong reason to change them, do it at
> the platform level.

Having column numbers start at 1 is the behaviour used in almost all Code editors, this is what people are used to, so I guess we have to do it at platform level(?), but I'll remove that +1.

> ::: browser/devtools/webconsole/webconsole.js
> @@ +2621,5 @@
> >     * @return nsIDOMNode
> >     *         The new anchor element, ready to be added to the message node.
> >     */
> >    createLocationNode:
> > +  function WCF_createLocationNode(aLocation, aTarget)
> 
> This method would be a little cleaner if you used object destructuring:
> 
> https://developer.mozilla.org/docs/Web/JavaScript/Reference/Operators/
> Destructuring_assignment
> 
> The above would then become:
> 
> function WCF_createLocationNode({url, line, column}, target)
> 
> and you would avoid the extra property lookups you have added.

Thanks for this, I didn't know I could use it with arguments!

> This was implemented in bug 1069490 and you are looking at the right place. aMessage.columnNumber will
> be present in a very recent Nightly build (I just verified it).

Right, I'll see if it's pushed yet, or I'll apply the patch myself and do my job until it's in Nightly.

> Done!

Thank you very much!

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 14 • 10 years ago

Okay, here it is, adding columnNumber to console messages was nothing more than adding a line.

I also wrote the test, first I tried to test the expected results against |<span.lineNumber>| but for some reason, |console.error| didn't have the right textContent there, so I just used |msg.contains|.

Thanks.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 15 • 10 years ago

Comment on attachment 8496176 [details] [diff] [review]
bug684096_columnNumber_console_newapi.diff

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

Sorry for the delay, but I'm really swamped with other work these days.

::: browser/devtools/webconsole/test/browser.ini
@@ +324,5 @@
>  [browser_webconsole_cd_iframe.js]
>  [browser_webconsole_autocomplete_crossdomain_iframe.js]
>  [browser_webconsole_console_custom_styles.js]
>  [browser_webconsole_console_api_stackframe.js]
> +[browser_webconsole_bug_684096_columnNumber.js]

These test files are not present in the patch. Perhaps you forgot to do 'hg addremove'?

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 16 • 10 years ago

Oh, darn, yes I forgot to add the files, sorry about it.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 17 • 10 years ago

Comment on attachment 8501907 [details] [diff] [review]
bug684096_columnNumber_console_newapi.diff

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

Getting really close, but unfortunately the patch breaks test browser_repeated_messages_accuracy.js. Did you run all the webconsole tests locally like I suggested in comment 7?

::: browser/devtools/webconsole/test/browser.ini
@@ +324,5 @@
>  [browser_webconsole_cd_iframe.js]
>  [browser_webconsole_autocomplete_crossdomain_iframe.js]
>  [browser_webconsole_console_custom_styles.js]
>  [browser_webconsole_console_api_stackframe.js]
> +[browser_webconsole_bug_684096_columnNumber.js]

We don't put the bug number in the test name any more. browser_webconsole_column_numbers.js would be fine.

::: browser/devtools/webconsole/test/browser_webconsole_bug_684096_columnNumber.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

For tests we use the CC0 license:

/*
 * Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/
 */

@@ +37,5 @@
> +    let msg = messages[i].textContent;
> +
> +    if(!msg.contains(expected[i])) {
> +      valid = false;
> +      break;

Instead of a single is() check, you could move it inside the loop and have it print the result for every case. Prematurely terminating the loop is also not a good idea, the test harness can run all checks and report the results so in general you don't go fixing one bug, run test, fix another bug, run test, etc.

::: browser/devtools/webconsole/test/test-console-column.html
@@ +1,2 @@
> +<!DOCTYPE HTML>
> +<html dir="ltr" xml:lang="en-US" lang="en-US"><head>

Add this license text somewhere inside <head>:

<!-- Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/publicdomain/zero/1.0/ -->

@@ +11,5 @@
> +    </script>
> +  </head>
> +  <body>
> +    <h1 id="header">Here come da column numbers</h1>
> +    <div id="myDiv"></div>

The h1 and div tags aren't used, so better remove them.

::: browser/devtools/webconsole/webconsole.js
@@ +2621,5 @@
>     * Creates the anchor that displays the textual location of an incoming
>     * message.
>     *
> +   * @param object aLocation
> +   *        An object containing url, line and column number of the message source (destructed).

Typo: destructured

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 18 • 10 years ago

> the patch breaks test browser_repeated_messages_accuracy.js. Did you run all the webconsole tests locally like I suggested in comment 7?

I ran the tests but I guess there was a mistake in my pop/push or probably adding files, sorry.

Seems like the problem was the test not putting the console logs in the same column number, I used a loop to log twice.

Fixed everything you mentioned, I hope it's okay now, thanks for your time, I've learned a lot from you!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 19 • 10 years ago

Mahdi, is your patch ready for feedback / review?  If so, set the corresponding flag so :past will take a look.

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 20 • 10 years ago

Comment on attachment 8501907 [details] [diff] [review]
bug684096_columnNumber_console_newapi.diff

Oh, I forgot this. This is the result of being in hurry all the time, again, sorry and thanks for noting me.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 21 • 10 years ago

(In reply to Mahdi Dibaiee [:mahdi] from comment #20)
> Comment on attachment 8501907 [details] [diff] [review]
> bug684096_columnNumber_console_newapi.diff
> 
> Oh, I forgot this. This is the result of being in hurry all the time, again,
> sorry and thanks for noting me.

Hmm, I think you want to mark the new one for review?  Currently the old is marked for re-review.

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 22 • 10 years ago

Comment on attachment 8501907 [details] [diff] [review]
bug684096_columnNumber_console_newapi.diff

right. I'm going to sleep, feeling dizzy. D:

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 23 • 10 years ago

Comment on attachment 8505478 [details] [diff] [review]
bug684096_columnNumber_console_newapi.diff

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

Sorry for the long wait and thanks for the updated patch, this is almost ready to land! There are just a couple of minor changes still needed and then we can land this!

I've already marked it as r+, so you don't need to ask me for review again. Once you have uploaded the new patch, set the checkin-needed keyword and our sheriffs will land it. Make sure you add a proper comment that ends with r=past, as it's explained here:

https://developer.mozilla.org/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I've sent it to our try server, to make sure tests pass in all platforms:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84232597f34d

::: browser/devtools/webconsole/test/browser_webconsole_column_numbers.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-console-column.html";

Add a brief comment about the purpose of this test, like we do in other test files.

@@ +36,5 @@
> +
> +  for(let i = 0, len = messages.length; i < len; i++) {
> +    let msg = messages[i].textContent;
> +
> +    is(!msg.contains(expected[i]), true, 'column number starting from 0 is as expected ' + expected[i])

The message is slightly misleading. Better use something like:

"Found expected line:column of " + expected[i]

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 24 • 10 years ago

Thanks for your review!

Added a simple comment and yup, you were right about the message being misleading, I felt it too.

It seems the try server passed all the tests.

Thanks for your time! I guess I'll take on another bug soon.

	Carsten Book [:Tomcat]
	Comment 25 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/4206d18e42c0

	Carsten Book [:Tomcat]
	Comment 26 • 10 years ago

sorry had to back this out for test bustage like f8ccf285f454

	Carsten Book [:Tomcat]
	Comment 27 • 10 years ago

(In reply to Carsten Book [:Tomcat] from comment #26)
> sorry had to back this out for test bustage like f8ccf285f454

ups copy and paste fail :) correct link is https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1101124&repo=fx-team

	Mahdi Dibaiee [:mahdi][:mdibaiee] Assignee
	Comment 28 • 10 years ago

Yup, sorry about that, forgot to run tests for relevant tools. Edited the test so it works.

	Carsten Book [:Tomcat]
	Comment 29 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/5634e9cd4807

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 30 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/5634e9cd4807