793996 - Create reload marker in the Web Console

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Rob Campbell [:rc] (:robcee)]

Web Console should have an indicator to mark where reload or navigation occurred in the output area.

Spin off from bug 705921.

Comment 2

[Mihai Sucan [:msucan]]

Started work on this patch.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This is the proposed patch.

Screenshot: http://img.i7m.de/show/drlpt.png

Notes and concerns:

- the new ConsoleOutput needs to be a weird cross between the current web console code and the new API. I did minimal bits of the proposed API for ConsoleOutput, BaseMessage and NavigationMarker (renamed from ReloadMarker).

- filtering, removal, and all of the other aspects related to output are still handled by the main web console code. Tried to keep the patch small. We will move more functionality over to the new APIs in other bugs and patches.

- the navigation marker behaves as a kind of a network message, which means it is filtered and pruned in the same way. Ideally, the marker would be a visual indicator of a group of all kinds of messages that happen between different page navigations. One that is not pruned until its last child is pruned, and so on.

I did avoid diving into a potential fix for this issue. We need more of the new APIs before we can do this in a good way. We can do it later in bug 655700.


Looking forward to your comments. Thank you!


Patch queue: bugs 887273, 812618, 877262 then this one.

Comment 4

[Victor Porof [:vporof][:vp]]

Whoa!

Comment 5

[Mihai Sucan [:msucan]]

This patch removes WCF_regroupOutput() which wasn't useful for a long time, thus fixing bug 746871.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Comment 7

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 775702 [details] [diff] [review]
rebased patch

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

::: browser/devtools/webconsole/console-output.js
@@ +72,5 @@
> +   * @param object message...
> +   *        Any number of Message objects.
> +   * @return this
> +   */
> +  addMessage: function()

no parameters here.

if you want to use var args style arbitrary arguments, you can use rest parameters.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/rest_parameters

e.g., addMessage: function(...args)

@@ +74,5 @@
> +   * @return this
> +   */
> +  addMessage: function()
> +  {
> +    for (let i = 0; i < arguments.length; i++) {

could use for...of with rest parameters! (ok, I'll shut up about rest parameters now, they're pretty-much equivalent except arguments isn't a real array)

@@ +327,5 @@
> +      continue;
> +    }
> +
> +    result["$" + key] = fn;
> +  }

Asked Irakli about this and his recommendation is to call the parent object's prototype method directly where needed.

For example, from https://addons.mozilla.org/en-US/developers/docs/sdk/1.14/modules/sdk/core/heritage.html

var Pet = Class({
  extends: Dog,              // should inherit from Dog
  initialize: function initialize(breed, name) {
    // To call ancestor methods you will have to access them
    // explicitly
    Dog.prototype.initialize.call(this, name);
    this.breed = breed;
  },
  call: function call(name) {
    return this.name === name ? this.bark() : '';
  }
});


In places where you're going to need to call the super method, you can give it a convenient alias.

Just sayin', I don't think it's necessary to really wrapper Heritage.extend().

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> Comment on attachment 775702 [details] [diff] [review]
> rebased patch
> 
> Review of attachment 775702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/console-output.js
> @@ +72,5 @@
> > +   * @param object message...
> > +   *        Any number of Message objects.
> > +   * @return this
> > +   */
> > +  addMessage: function()
> 
> no parameters here.
> 
> if you want to use var args style arbitrary arguments, you can use rest
> parameters.
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> rest_parameters
> 
> e.g., addMessage: function(...args)

Good catch. I forgot about the rest parameters. Fixing this in the updated patch.


> @@ +74,5 @@
> > +   * @return this
> > +   */
> > +  addMessage: function()
> > +  {
> > +    for (let i = 0; i < arguments.length; i++) {
> 
> could use for...of with rest parameters! (ok, I'll shut up about rest
> parameters now, they're pretty-much equivalent except arguments isn't a real
> array)
> 
> @@ +327,5 @@
> > +      continue;
> > +    }
> > +
> > +    result["$" + key] = fn;
> > +  }
> 
> Asked Irakli about this and his recommendation is to call the parent
> object's prototype method directly where needed.
> 
> For example, from
> https://addons.mozilla.org/en-US/developers/docs/sdk/1.14/modules/sdk/core/
> heritage.html
> 
> var Pet = Class({
>   extends: Dog,              // should inherit from Dog
>   initialize: function initialize(breed, name) {
>     // To call ancestor methods you will have to access them
>     // explicitly
>     Dog.prototype.initialize.call(this, name);
>     this.breed = breed;
>   },
>   call: function call(name) {
>     return this.name === name ? this.bark() : '';
>   }
> });
> 
> 
> In places where you're going to need to call the super method, you can give
> it a convenient alias.
> 
> Just sayin', I don't think it's necessary to really wrapper
> Heritage.extend().

Good point. Better to use what others also use, for consistency. Fix coming in the updated patch.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: address review comments and rebase

Comment 10

[Mihai Sucan [:msucan]]

Green try push: https://tbpl.mozilla.org/?tree=Try&rev=0258b12aba0b

The push includes this patch and the patches from bug 877262.

Comment 11

[Mihai Sucan [:msucan]]

Landed: https://hg.mozilla.org/integration/fx-team/rev/9a1e79294a20

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9a1e79294a20