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

The "pause on exceptions" functionality is currently hard to find, and is often something that you want to toggle on/off fairly frequently. We should move it from the settings menu to a toolbar button.

	Girish Sharma [:Optimizer]
	Comment 1 • 11 years ago

Cool this will also fix the issue mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=879408#c13

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 3 • 11 years ago

Patch bit rotted and I'm not picking this back up in the near future. Free for someone else to take if they want.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 4 • 11 years ago

Note that this should probably be a 3 state toggle since we have 3 possible states now:

* Do not pause on exceptions
* Pause on ALL exceptions
* Pause on uncaught exceptions

	Hubert B Manilla Assignee
	Comment 5 • 10 years ago

Can i work on this?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 6 • 10 years ago

Go for it!

	Hubert B Manilla Assignee
	Comment 7 • 9 years ago

Just got back to start fixing this after a while.
Just a WIP to make sure i'm on the right path.

TODO:
Add tests.
Need to add icons for the toggle button.

Question:
How do handle adding icons for the buttons?

	Hubert B Manilla Assignee
	Comment 8 • 9 years ago

Specifics about the icons: who creates the icons for buttons and what kinda icon we should use?

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

(In reply to Hubert B Manilla from comment #8)
> Specifics about the icons: who creates the icons for buttons and what kinda
> icon we should use?

I find the best way to get icons is to draw your own crappy one, land that, and then ping ui people like "hey, we need a real icon, or this is going to ship." Otherwise, you'll end up waiting forever...

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 10 • 9 years ago

Comment on attachment 8578003 [details] [diff] [review]
wip.patch

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

::: browser/devtools/debugger/debugger-commands.js
@@ +372,5 @@
>      }
>    }
>  });
>  
> +/**:

Accidental change?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 11 • 9 years ago

(In reply to Hubert B Manilla from comment #7)
> Question:
> How do handle adding icons for the buttons?

Usually we would want to use a button and toggle the "checked" attribute, but becuase we have three states rather than two, I think we need something different. Perhaps a deck[0] with 3 buttons (one for each state) and we only show one at a time?

Victor, how do you think we should do a 3-state toggle button?

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/deck

	Hubert B Manilla Assignee
	Comment 12 • 9 years ago

(In reply to Nick Fitzgerald [:fitzgen] from comment #10)
> Comment on attachment 8578003 [details] [diff] [review]
> wip.patch
> 
> Review of attachment 8578003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-commands.js
> @@ +372,5 @@
> >      }
> >    }
> >  });
> >  
> > +/**:
> 
> Accidental change?

yeah. will fix this.

	Hubert B Manilla Assignee
	Comment 13 • 9 years ago

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> (In reply to Hubert B Manilla from comment #8)
> > Specifics about the icons: who creates the icons for buttons and what kinda
> > icon we should use?
> 
> I find the best way to get icons is to draw your own **** one, land that,
> and then ping ui people like "hey, we need a real icon, or this is going to
> ship." Otherwise, you'll end up waiting forever...

Nice! will do that! Anyone in particular i can buzz?

	Hubert B Manilla Assignee
	Comment 14 • 9 years ago

(In reply to Hubert B Manilla from comment #13)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> > (In reply to Hubert B Manilla from comment #8)
> > > Specifics about the icons: who creates the icons for buttons and what kinda
> > > icon we should use?
> > 
> > I find the best way to get icons is to draw your own **** one, land that,
> > and then ping ui people like "hey, we need a real icon, or this is going to
> > ship." Otherwise, you'll end up waiting forever...
> 
> Nice! will do that! Anyone in particular i can bug when i'm done?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 15 • 9 years ago

(In reply to Hubert B Manilla from comment #13)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> > (In reply to Hubert B Manilla from comment #8)
> > > Specifics about the icons: who creates the icons for buttons and what kinda
> > > icon we should use?
> > 
> > I find the best way to get icons is to draw your own **** one, land that,
> > and then ping ui people like "hey, we need a real icon, or this is going to
> > ship." Otherwise, you'll end up waiting forever...
> 
> Nice! will do that! Anyone in particular i can buzz?

:shorlander

	Victor Porof [:vporof][:vp]
	Comment 16 • 9 years ago

(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> (In reply to Hubert B Manilla from comment #7)
> > Question:
> > How do handle adding icons for the buttons?
> 
> Usually we would want to use a button and toggle the "checked" attribute,
> but becuase we have three states rather than two, I think we need something
> different. Perhaps a deck[0] with 3 buttons (one for each state) and we only
> show one at a time?
> 
> Victor, how do you think we should do a 3-state toggle button?
> 
> [0] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/deck

Why not a button with 3 possible attributes, each styled differently in css?

button[state=whatever1]
button[state=whatever2]
button[state=whatever3]

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 17 • 9 years ago

Great solution!

	Hubert B Manilla Assignee
	Comment 18 • 9 years ago

Stephen, i just used existing icons, since i'm really **** at designing any kinda icon. We would be need proper icons for the pause on exceptions button. I was directed your way. Thanks for your help!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 19 • 9 years ago

Comment on attachment 8625718 [details] [diff] [review]
882790wip.patch

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

Looks pretty straightforward, which is always good.

We shouldn't have tests "blah-03" and "blah-04" without -01 and -02. Just reuse the -01 and -02 names.

The new test files and icons aren't attached in the patch.

::: browser/devtools/debugger/views/sources-view.js
@@ +632,5 @@
> +    let state = Number(this._togglePauseOnExceptionsButton.getAttribute("state"));
> +    let tooltip;
> +
> +    state = state === 2 ? 0 : state + 1;
> +    if (state === 0) {

Let's define constants at the top for these to make it more readable:

const DO_NOT_PAUSE_ON_EXCEPTIONS = 0;
const PAUSE_ON_UNCAUGHT_EXCEPTIONS = 1;
const PAUSE_ON_ALL_EXCEPTIONS = 2;

	Hubert B Manilla Assignee
	Comment 20 • 9 years ago

Updated based on reviews feedback.

	Hubert B Manilla Assignee
	Comment 21 • 9 years ago

Updated.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 22 • 9 years ago

Comment on attachment 8626500 [details] [diff] [review]
882790wip.patch

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

Looks great!

::: browser/devtools/debugger/views/sources-view.js
@@ +635,5 @@
> +    let state = Number(this._togglePauseOnExceptionsButton.getAttribute("state"));
> +    let tooltip;
> +
> +    state = state === PAUSE_ON_UNCAUGHT_EXCEPTIONS ?
> +                      DO_NOT_PAUSE_ON_EXCEPTIONS : state + PAUSE_ON_ALL_EXCEPTIONS;

How about this:

    state = (state + 1) % 3;

Or this:

    ++state % 3;

	Hubert B Manilla Assignee
	Comment 23 • 9 years ago

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #22)
> Comment on attachment 8626500 [details] [diff] [review]
> 882790wip.patch
> 
> Review of attachment 8626500 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great!
> 
> ::: browser/devtools/debugger/views/sources-view.js
> @@ +635,5 @@
> > +    let state = Number(this._togglePauseOnExceptionsButton.getAttribute("state"));
> > +    let tooltip;
> > +
> > +    state = state === PAUSE_ON_UNCAUGHT_EXCEPTIONS ?
> > +                      DO_NOT_PAUSE_ON_EXCEPTIONS : state + PAUSE_ON_ALL_EXCEPTIONS;
> 
> How about this:
> 
>     state = (state + 1) % 3;
> 
> Or this:
> 
>     ++state % 3;

Good idea!

	Hubert B Manilla Assignee
	Comment 25 • 9 years ago

Comment on attachment 8627058 [details] [diff] [review]
882790wip.patch

updated!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 26 • 9 years ago

Comment on attachment 8627058 [details] [diff] [review]
882790wip.patch

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

FYI, unless you make significant changes to a patch, if someone has already given r+ (or says "r=me with X, Y, and Z changes") you don't need to request review again. You can just add the r+ yourself.

Do you need me to do a try push for you? I forget if you have commit access for try.

	Hubert B Manilla Assignee
	Comment 27 • 9 years ago

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #26)
> Comment on attachment 8627058 [details] [diff] [review]
> 882790wip.patch
> 
> Review of attachment 8627058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FYI, unless you make significant changes to a patch, if someone has already
> given r+ (or says "r=me with X, Y, and Z changes") you don't need to request
> review again. You can just add the r+ yourself.
> 
Good to know! was not sure if i should add the r+ myself.

> Do you need me to do a try push for you? I forget if you have commit access
> for try.
i got commit access while back, i'm not sure if anything has changed since then.
i'll give a try n i'll let you know if i can't.

	Hubert B Manilla Assignee
	Comment 28 • 9 years ago

Hey nick, i think i might need you to do a try push for me. Thanks.

I think sometime late last year there was a mail, mentioning that i would need revouching to continue contributing, but didnt get around to asking anyone.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 29 • 9 years ago

Hm, I can't apply this on latest fx-team. Can you rebase and/or push yourself. Happy to revouch for you if you flag me.

	Hubert B Manilla Assignee
	Comment 30 • 9 years ago

Rebased and updated patch!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 31 • 9 years ago

(In reply to Hubert B Manilla from comment #30)
> Created attachment 8628753 [details] [diff] [review]
> 882790wip.patch
> 
> Rebased and updated patch!

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13e66decde48

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 32 • 9 years ago

ni?me for checking back in on the try push

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 33 • 9 years ago

Try push has a bunch of devtools mochitest failures like:


698 INFO TEST-UNEXPECTED-FAIL | browser/devtools/debugger/test/browser_dbg_server-conditional-bp-01.js | Test timed out - expected PASS
812 INFO TEST-UNEXPECTED-FAIL | browser/devtools/debugger/test/browser_dbg_server-conditional-bp-05.js | Test timed out - expected PASS
835 INFO TEST-UNEXPECTED-FAIL | browser/devtools/debugger/test/browser_dbg_source-maps-04.js | The pause-on-exceptions pref should be disabled by default. - Got true, expected false
838 INFO TEST-UNEXPECTED-FAIL | browser/devtools/debugger/test/browser_dbg_source-maps-04.js | Test timed out - expected PASS 

Hubert, can you reproduce these locally? They don't seem restricted to a single platform or anything like that. You can dig into the log by selecting a failing job and then clicking on the "log" button that is on the left in the split view that pops up after clicking on the failed job.

	Hubert B Manilla Assignee
	Comment 34 • 9 years ago

Fixed the tests!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 35 • 9 years ago

It's not applying on fx-team for me:

applying 882790wip.patch
patching file browser/devtools/debugger/debugger-controller.js
Hunk #1 FAILED at 780
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/debugger/debugger-controller.js.rej
patching file browser/devtools/debugger/views/stack-frames-view.js
Hunk #1 FAILED at 10
Hunk #2 FAILED at 106
2 out of 3 hunks FAILED -- saving rejects to file browser/devtools/debugger/views/stack-frames-view.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 882790wip.patch

Do you have try access again now? If not, please request commit level 1 again, so I don't need to do try pushes for you and then ask you to rebase and then try again anymore :P

https://www.mozilla.org/en-US/about/governance/policies/commit/

	Hubert B Manilla Assignee
	Comment 36 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8a220247a72

	Hubert B Manilla Assignee
	Comment 38 • 9 years ago

Hey Nick, 
There are a couple of other failures but they don't look like errors from our end.
Can these be ignored?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 39 • 9 years ago

Yeah that push looks fine, all those seem like existing intermittents. Ready to add the checkin-needed keyword?

	Hubert B Manilla Assignee
	Comment 40 • 9 years ago

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #39)
> Yeah that push looks fine, all those seem like existing intermittents. Ready
> to add the checkin-needed keyword?

Yeah!

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 9 years ago

Comment on attachment 8632826 [details] [diff] [review]
882790wip.patch

In the future, please just use the checkin-needed bug keyword. It works better with the automated bug marking tools we use. Thanks for the patch!

	Pulsebot
	Comment 42 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/85303b3d8d17

	Hubert B Manilla Assignee
	Comment 43 • 9 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #41)
> Comment on attachment 8632826 [details] [diff] [review]
> 882790wip.patch
> 
> In the future, please just use the checkin-needed bug keyword. It works
> better with the automated bug marking tools we use. Thanks for the patch!

Ok, noted! many thanks!

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

https://hg.mozilla.org/mozilla-central/rev/85303b3d8d17

	Gabriel [:gl] (ΦωΦ)
	Comment 45 • 9 years ago

Hubert, can you make sure to ping UX for a real icon? shorlander who usually helps with UX for us is away till 7/20. Let me know if you want me to file a new bug for this.

	Gabriel [:gl] (ΦωΦ)
	Comment 46 • 9 years ago

Oh oops just saw he was already ni?

	Gabriel [:gl] (ΦωΦ)
	Comment 47 • 9 years ago

Noticed that the new toolbar button also needs a tooltip text.

	Hubert B Manilla Assignee
	Comment 48 • 9 years ago

(In reply to Gabriel Luong [:gl] from comment #45)
> Hubert, can you make sure to ping UX for a real icon? shorlander who usually
> helps with UX for us is away till 7/20. Let me know if you want me to file a
> new bug for this.

Yeah, it would be cool to have a bug to track for this.

	Hubert B Manilla Assignee
	Comment 49 • 9 years ago

(In reply to Gabriel Luong [:gl] from comment #47)
> Noticed that the new toolbar button also needs a tooltip text.

Sorry abit confused? Is it when you mouseover the pause on exceptions button, no tooltiptext shows up?

	Gabriel [:gl] (ΦωΦ)
	Comment 50 • 9 years ago

(In reply to Hubert B Manilla from comment #49)
> (In reply to Gabriel Luong [:gl] from comment #47)
> > Noticed that the new toolbar button also needs a tooltip text.
> 
> Sorry abit confused? Is it when you mouseover the pause on exceptions
> button, no tooltiptext shows up?

Yes, this would be the tooltip text that shows up when you mouseover the button.

	Hubert B Manilla Assignee
	Comment 51 • 9 years ago

(In reply to Gabriel Luong [:gl] from comment #50)
> (In reply to Hubert B Manilla from comment #49)
> > (In reply to Gabriel Luong [:gl] from comment #47)
> > > Noticed that the new toolbar button also needs a tooltip text.
> > 
> > Sorry abit confused? Is it when you mouseover the pause on exceptions
> > button, no tooltiptext shows up?
> 
> Yes, this would be the tooltip text that shows up when you mouseover the
> button.

Interestingly, it shows up for me.

	Hubert B Manilla Assignee
	Comment 52 • 9 years ago

(In reply to Hubert B Manilla from comment #51)
> (In reply to Gabriel Luong [:gl] from comment #50)
> > (In reply to Hubert B Manilla from comment #49)
> > > (In reply to Gabriel Luong [:gl] from comment #47)
> > > > Noticed that the new toolbar button also needs a tooltip text.
> > > 
> > > Sorry abit confused? Is it when you mouseover the pause on exceptions
> > > button, no tooltiptext shows up?
> > 
> > Yes, this would be the tooltip text that shows up when you mouseover the
> > button.
> 
> Interestingly, it shows up for me.

it shows "Pause on all exceptions" initially..

	Gabriel [:gl] (ΦωΦ)
	Comment 53 • 9 years ago

(In reply to Hubert B Manilla from comment #52)
> (In reply to Hubert B Manilla from comment #51)
> > (In reply to Gabriel Luong [:gl] from comment #50)
> > > (In reply to Hubert B Manilla from comment #49)
> > > > (In reply to Gabriel Luong [:gl] from comment #47)
> > > > > Noticed that the new toolbar button also needs a tooltip text.
> > > > 
> > > > Sorry abit confused? Is it when you mouseover the pause on exceptions
> > > > button, no tooltiptext shows up?
> > > 
> > > Yes, this would be the tooltip text that shows up when you mouseover the
> > > button.
> > 
> > Interestingly, it shows up for me.
> 
> it shows "Pause on all exceptions" initially..

Oh indeed, I was looking at the source and noticed the tooltiptext attribute was not set in debugger.xul

I see you are setting the attribute in the options-view.js. Perhaps as a follow up we should remove setting these attributes in options-view.js to reduce the amount of code needed, and only have one place where the attributes need to be defined.

	Gabriel [:gl] (ΦωΦ)
	Comment 54 • 9 years ago

I think we definitely want this button to standout similar to what chrome has done with having a blue button to toggle a panel to enable pause on exception. This is a fairly important feature and we want to make sure it is discoverable now that it was removed from the setting context menu. Even with just a button it might be hard to tell what the state is for pause on exception (on/off). One idea is to have a checkbox similar to the "Browser styles" in the computed view somewhere appropriate in the Debugger.

	Saheda Reza [:Antora]
	Comment 55 • 9 years ago

I looked for pause on exceptions button on toolbar menu,but didn't found on Firefox nightly windows 8.1(32bit):

Build ID 	20150613030206
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:41.0) Gecko/20100101 Firefox/41.0

I found this pause on exceptions button on Latest nightly:

Build ID 	20150722030205
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:42.0) Gecko/20100101 Firefox/42.0

[bugday-20150723]

	Gabriel [:gl] (ΦωΦ)
	Comment 56 • 9 years ago

Backed out until we address Bug 1184160

https://hg.mozilla.org/integration/fx-team/rev/789ccabdbadc

	Tim Nguyen :ntim
	Comment 57 • 9 years ago

Stephen is already ni? in Bug 1184160

	Tom Tromey :tromey
	Comment 58 • 7 years ago

This is done in the new debugger front end.