[webkit-reviews] review requested: [Bug 26145] WebInspector halts when opening with debugger enabled globally : [Attachment 30911] fix

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has asked  for review:
Bug 26145: WebInspector halts when opening with debugger enabled globally
https://bugs.webkit.org/show_bug.cgi?id=26145

Attachment 30911: fix
https://bugs.webkit.org/attachment.cgi?id=30911&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
(In reply to comment #2)
> (From update of attachment 30909 [review])
> > -		 enableDebugger();
> > +		 m_frontend->attachDebuggerWhenShown();
> 
> Can't you just leave this calling enableDebugger() since you fixed that
> function?
> 

Done

> >	     disableDebugger();
> > +	     m_attachDebuggerWhenShown = true;
> 
> Why is this needed? Shoudln't this be m_attachDebuggerWhenShown = false? If
it
> is supose to be false, should disableDebugger() set it to false? If it isn't
> those things, maybe a comment is needed to explain.
> 
> > -	     enableDebugger();
> > +	     m_attachDebuggerWhenShown = true;
> 

I added a comment for this. The scenario is: close window when debugger is
enabled, expect it to be enabled when the window is being reopened. The hidden
thing is that debugger is being disabled on window close unconditionally.

> Same as before. Can't you just leave this calling enableDebugger() since you
> fixed that function?
> 

Done. It is just that I fixed the function after I changed these.

> > +		 this._attachDebuggerWhenShown = false;
> 
> I like to use delete this._attachDebuggerWhenShown for cases like this, since

> there is no benifit to keeping this property around.
> 

Done.

> r+, but maybe this can be a smaller change based on the comments above.
> 

Thanks!