Closed Bug 793996 Opened 7 years ago Closed 6 years ago

Create reload marker in the Web Console

Categories

(DevTools :: Console, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: rcampbell, Assigned: msucan)

References

Details

Attachments

(1 file, 2 obsolete files)

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

Spin off from bug 705921.
Depends on: 705921
Priority: -- → P2
Duplicate of this bug: 879689
Started work on this patch.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #771417 - Flags: review?(rcampbell)
This patch removes WCF_regroupOutput() which wasn't useful for a long time, thus fixing bug 746871.
Blocks: 746871
Blocks: 859841
Blocks: 760876
Attached patch rebased patch (obsolete) — Splinter Review
Attachment #771417 - Attachment is obsolete: true
Attachment #771417 - Flags: review?(rcampbell)
Attachment #775702 - Flags: review?(rcampbell)
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().
Attachment #775702 - Flags: review?(rcampbell) → review+
(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.
Attachment #775702 - Attachment is obsolete: true
Green try push: https://tbpl.mozilla.org/?tree=Try&rev=0258b12aba0b

The push includes this patch and the patches from bug 877262.
Landed: https://hg.mozilla.org/integration/fx-team/rev/9a1e79294a20
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9a1e79294a20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.