Skip to content

Commit

Permalink
Issue 6934: A red circle should appear when checking 'Break on Proper…
Browse files Browse the repository at this point in the history 
…ty Change' in the watch panel
  • Loading branch information
@janodvarko
janodvarko committed Nov 1, 2013
1 parent 1609c9a commit bf9f139
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 6 deletions.
6 changes: 4 additions & 2 deletions extension/content/firebug/chrome/tabContext.js
View file
Expand Up @@ -8,9 +8,10 @@ define([
"firebug/lib/url",
"firebug/chrome/window",
"firebug/lib/css",
"firebug/lib/wrapper",
"firebug/chrome/plugin",
],
function(Obj, Arr, CompilationUnit, Events, Url, Win, Css) {
function(Obj, Arr, CompilationUnit, Events, Url, Win, Css, Wrapper) {

// ********************************************************************************************* //
// Constants
Expand Down Expand Up @@ -227,7 +228,8 @@ Firebug.TabContext.prototype =

getCurrentGlobal: function()
{
return this.stoppedGlobal || this.baseWindow || this.window;
var global = this.stoppedGlobal || this.baseWindow || this.window;
return Wrapper.getContentView(global);

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Nov 1, 2013

Member

This doesn't match our current usages (e.g. DebuggerLib.getDebuggeeGlobal does Wrapper.getContentView on the return value, and that's just the first one I checked). I think we should attempt to use wrapped values everywhere except the places where we handle generic content objects. So e.g. all globals and elements should be wrapped unless otherwise mentioned.

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Nov 1, 2013

Member

(This is breaking e.g. the command line.)

This comment has been minimized.

Sign in to view

Copy link
@fflorent

fflorent Nov 3, 2013

Member

I agree. If you want, we could rather be an option to pass:

    /**
     * Returns the current global (the top window or one of its frames or sub-frames).
     * Note that the current global changes when debugging scripts.
     *
     * @param {boolean} [unwrap] If set to true, return the unwrapped global object (default=false).
     *
     * @return {Window} The current global
     */
    getCurrentGlobal: function(unwrap)
    {
        var global = this.stoppedGlobal || this.baseWindow || this.window;
        return unwrap ? Wrapper.getContentView(global) : global;
    },

This comment has been minimized.

Sign in to view

Copy link
@janodvarko

janodvarko Nov 4, 2013

Author Member

Sound good to me.

@simonlindholm: do you like Florent's suggestion?

Honza

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Nov 4, 2013

Member

IMO it's nicer to make the unwrapping explicit at call site. Fewer boolean arguments, too. Where do we need it?

This comment has been minimized.

Sign in to view

Copy link
@janodvarko

janodvarko Nov 4, 2013

Author Member

  1. In the WatchPanel.showEmptyMembers()
    https://github.com/firebug/firebug/blob/jsd2/extension/content/firebug/debugger/watch/watchPanel.js#L253

  2. In the DefaultWatchPanelInput.getChildren()
    https://github.com/firebug/firebug/blob/jsd2/extension/content/firebug/debugger/watch/watchProvider.js#L235

Honza

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Nov 4, 2013

Member

Right, in those places I think we should do explicit unwrapping to symbolize that we want the raw content-scope JS object instead of a symbolic global.

This comment has been minimized.

Sign in to view

Copy link
@janodvarko

janodvarko Nov 4, 2013

Author Member

Explicit "true" argument isn't explicit enough?
(also less deps, no need for Wrapper)

Fewer boolean arguments, too.

I agree in general, but I think it's useful in this case.

Honza

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm via email Nov 4, 2013

Member

This comment has been minimized.

Sign in to view

Copy link
@fflorent

fflorent via email Nov 4, 2013

Member

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm via email Nov 4, 2013

Member

This comment has been minimized.

Sign in to view

Copy link
@janodvarko

janodvarko Nov 4, 2013

Author Member

OK, let's do quick voting.

  1. getCurrentGlobal(true)
  • it's nice and simple (+ a comment can make it clear)
  1. Explicit "firebug/lib/wrapper" module dependency
  • it's more code, but we have the explicit dep listed (which is the real difference)

I am not including {unwrap: true}, it's not really different from option #1
(#1 + a comment = the same)

I am for #1
Simon is for #2

Florent, can you pick one?

Honza

This comment has been minimized.

Sign in to view

Copy link
@fflorent

fflorent via email Nov 4, 2013

Member

This comment has been minimized.

Sign in to view

Copy link
@SebastianZ

SebastianZ Nov 5, 2013

Member
  • given how seldom we unwrap the global it shouldn't need the syntactic sugar.
  • unwrapping doesn't belong in getCurrentGlobal; it's something only the caller cares about.

If that is true, then I'm for #2, otherwise #1.

Sebastian

This comment has been minimized.

Sign in to view

Copy link
@janodvarko

janodvarko Nov 5, 2013

Author Member

Related patch at:
cb3c984

In the end I just fixed the getCurrentGlobal (do not unwrap by default) and didn't introduce any new argument.

You can also read this comment that is related to the patch.
https://code.google.com/p/fbug/issues/detail?id=6934#c4

We should probably yet discuss sometime about how to properly deal with wrappers.

Thanks to all involved in the review!

Honza
},

destroy: function(state)
Expand Down
12 changes: 10 additions & 2 deletions extension/content/firebug/dom/domBasePanel.js
View file
Expand Up @@ -394,7 +394,7 @@ Firebug.DOMBasePanel.prototype = Obj.extend(Panel,

getContextMenuItems: function(object, target)
{
Trace.sysout("dom.getContextMenuItems;", object);
Trace.sysout("dom.getContextMenuItems; " + object, object);

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Nov 1, 2013

Member

object is from content scope, don't call toString() on it unconditionally and w/o try-catch

This comment has been minimized.

Sign in to view

Copy link
@janodvarko

janodvarko Nov 1, 2013

Author Member

Removed at:
9d48d0a

Honza

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Nov 1, 2013

Member

(You could have kept the argument, because Trace.sysout makes sure not to throw exceptions - the problem is only with the stringification. But that works too.)

var row = Dom.getAncestorByClass(target, "memberRow");

Expand Down Expand Up @@ -483,13 +483,16 @@ Firebug.DOMBasePanel.prototype = Obj.extend(Panel,

if (!isDomMemeber && member && member.breakable)
{
var bps = this.context.dom.breakpoints;
var hasBreakpoint = bps.findBreakpoint(rowObject, rowName);

items.push(
"-",
{
label: "dom.label.breakOnPropertyChange",
tooltiptext: "dom.tip.Break_On_Property_Change",
type: "checkbox",
checked: this.context.dom.breakpoints.findBreakpoint(rowObject, rowName),
checked: hasBreakpoint,
command: Obj.bindFixed(this.breakOnProperty, this, row)
}
);
Expand Down Expand Up @@ -530,6 +533,8 @@ Firebug.DOMBasePanel.prototype = Obj.extend(Panel,

rebuild: function(update, scrollTop)
{
Trace.sysout("domBasePanel.rebuild; " + this.selection, this.selection);

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Nov 1, 2013

Member

same thing I believe

This comment has been minimized.

Sign in to view

Copy link
@janodvarko

janodvarko Nov 1, 2013

Author Member

Removed at:
0df6ed4

Honza

Events.dispatch(this.fbListeners, "onBeforeDomUpdateSelection", [this]);

var members = this.getMembers(this.selection, 0);
Expand Down Expand Up @@ -981,6 +986,9 @@ Firebug.DOMBasePanel.prototype = Obj.extend(Panel,
return;

// Create new or remove an existing breakpoint.
// xxxHonza: This action can be executed from within the DOM side panel
// in which case we need to ensure that the breakpoint column in the DOM
// main panel is properly updated.
var breakpoints = this.context.dom.breakpoints;
var bp = breakpoints.findBreakpoint(object, name);
if (bp)
Expand Down
8 changes: 6 additions & 2 deletions extension/content/firebug/dom/domMemberProvider.js
View file
Expand Up @@ -4,20 +4,23 @@

define([
"firebug/firebug",
"firebug/lib/trace",
"firebug/lib/object",
"firebug/lib/array",
"firebug/lib/wrapper",
"firebug/lib/dom",
"firebug/lib/trace",
"firebug/lib/locale",
"firebug/console/closureInspector",
"firebug/chrome/reps",
],
function(Firebug, Obj, Arr, Wrapper, Dom, FBTrace, Locale, ClosureInspector, FirebugReps) {
function(Firebug, FBTrace, Obj, Arr, Wrapper, Dom, Locale, ClosureInspector, FirebugReps) {

// ********************************************************************************************* //
// Constants

var Trace = FBTrace.to("DBG_DOM");
var TraceError = FBTrace.to("DBG_ERRORS");

// ********************************************************************************************* //
// DOM Member Provider

Expand Down Expand Up @@ -356,6 +359,7 @@ DOMMemberProvider.prototype =

var breakpoints = this.context.dom.breakpoints;
var bp = breakpoints.findBreakpoint(object, name);

if (bp)
{
member.breakpoint = true;
Expand Down

0 comments on commit bf9f139

Please sign in to comment.