897744 - BreakpointStore should have a hasBreakpoint method

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

From bug 860035 comment 41:

> If you think it would be an improvement, consider having a separate method,
> hasBreakpoint, which returns an entry if present, or null otherwise. Then 
> getBreakpoint could always throw; it could be implemented simply in terms of 
> hasBreakpoint; and the call sites that now pass false for aShouldThrow could 
> become a little easier to read.

Comment 1

[Mason Chang]

Attachment: hasBreakpoint.patch

Created a new method in BreakpointStore called hasBreakpoint.

Comment 2

[Mason Chang]

Hi Nick, 

I'm new here and thought this would be a good starting point. I created a new method called hasBreakpoint in the Breakpoint store. Following other bugs, I think the right place to add the method was in /toolkit/devtools/server/actors/script.js b/toolkit/devtools/server/actors/script.js. I essentially took getBreakpoint and deleted the throw exception logic. Was this the right approach?

One question I had was, I couldn't quite figure out a clean way to test the method. Every location where I saw getBreakpoint was in the JIT or in a way that initially looked like it required browser support. What's the proper way to test the debugger?

Thanks for the help!

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 782567 [details] [diff] [review]
hasBreakpoint.patch

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

Hey Mason!

This is a great first stab, thanks for the patch :)

There are just a few more changes I would like to see:

* Can you redefine `getBreakpoint` in terms of `hasBreakpoint`?

  * Then, inside `getBreakpoint`, whenever we don't get a breakpoint back
    from `hasBreakpoint`, throw an error (so we remove the `aShouldThrow`
    parameter as well, since it won't do anything anymore).

  * Next, we need to replace all calls to `getBreakpoint` which suppress
    errors with calls to `hasBreakpoint`. Luckily, there is only one
    location where this happens right now:
    http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#1081

(In reply to Mason Chang from comment #2)
> One question I had was, I couldn't quite figure out a clean way to test the
> method. Every location where I saw getBreakpoint was in the JIT or in a way
> that initially looked like it required browser support. What's the proper
> way to test the debugger?

The tests for our debugger server live in the toolkit/devtools/server/tests/unit/ directory. There is a test in that directory, test_breakpointstore.js, which you can add tests for `hasBreakpoint` to. It currently has other issues (bug 899215) but nothing that should stop you from landing this patch or adding to that test.

You can run the debugger server tests with this command:

  $ ./mach build toolkit/debugger # make sure your build is up-to-date
  $ ./mach xpcshell-test toolkit/devtools/debugger/server/tests/unit # run the actual tests

Thanks again, I look forward to your updated patch!

Comment 4

[Jim Blandy :jimb]

Hi, Mason; are you on IRC? I can give you some pointers that will help you find the right code in the future.

Comment 5

[Mason Chang]

Attachment: hasBreakpoint.patch

Hi Jim and Nick,

Thanks much for the feedback and pointers! Here's another stab at fixing this bug. I replaced getBreakpoint to use hasBreakpoint and removed the optional parameter to silence the warning. I also added some test cases based on Nick's code in (Bug 899215). I ran the test cases on OS X 10.8.2 and they pass! Is there anything else I should do?

The only question I have is, I know some other parts of the codebase use getBreakpoint with the optional parameter. Should we fix those references to remove the silencing of getBreakpoint and fix them to use hasBreakpoint?

@Jim - I'm on IRC either early morning PST or late evening PST. I'm working elsewhere during the day. Thanks for your offer to help! Appreciate it!

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 782938 [details] [diff] [review]
hasBreakpoint.patch

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

(In reply to Mason Chang from comment #5)
> Created attachment 782938 [details] [diff] [review]
> hasBreakpoint.patch
> 
> Hi Jim and Nick,
> 
> Thanks much for the feedback and pointers! Here's another stab at fixing
> this bug. I replaced getBreakpoint to use hasBreakpoint and removed the
> optional parameter to silence the warning. I also added some test cases
> based on Nick's code in (Bug 899215). I ran the test cases on OS X 10.8.2
> and they pass! Is there anything else I should do?

Awesome, thank you very much!

Once you rebase and do the small changes I mention below, I will push the patch to our try server[0] which runs the tests on all of the platforms we support.

Assuming that:

  * all the tests come back green from try server,

  * and we get an official "r+" (positive review) from a module peer (Jim)

we will land the patch in fx-team. It will incubate in fx-team for a day or so before being merged into mozilla-central, and then it will show up in the next Nightly builds! :)

[0] https://wiki.mozilla.org/ReleaseEngineering/TryServer

> The only question I have is, I know some other parts of the codebase use
> getBreakpoint with the optional parameter. Should we fix those references to
> remove the silencing of getBreakpoint and fix them to use hasBreakpoint?

I think you might be mistaking a C++ method which has the same name. The only other places where getBreakpoint is called with silenced exceptions is in the test_breakpointstore.js file as of very recently. My patch from that other bug just landed, so I bit rot this one a little bit. Sorry!

You will have to pull the latest changes and re-apply your patch:

  # Assuming you are using the hg mq extension:
  $ hg qpop
  $ hg pull -u
  $ hg qpush

You will probably have to fix a minor merge conflict in test_breakpointstore.js because of my changes which landed.

Then go through the few places I added more calls to `getBreakpoint(..., false)` in test_breakpointstore.js and replace them with calls to `hasBreakpoint`.

> @Jim - I'm on IRC either early morning PST or late evening PST. I'm working
> elsewhere during the day. Thanks for your offer to help! Appreciate it!

What's your irc nick? I'm fitzgen, and Jim is jimb. Say hello! :)

::: toolkit/devtools/server/actors/script.js
@@ +105,5 @@
>    },
>  
>    /**
>     * Get a breakpoint from the breakpoint store. Will throw an error if the
> +   * breakpoint is not found. 

Nit: there is a trailing space on this line; please remove it.

@@ +132,5 @@
> +
> +  /**
> +   * Checks if the breakpoint store has a requested breakpoint
> +   * Returns true if the breakpoint store has a breakpoint
> +   * null otherwise

Nit: this comment provides misinformation: `hasBreakpoint` doesn't return true, it returns the breakpoint. Please fix the comment.

::: toolkit/devtools/server/tests/unit/test_breakpointstore.js
@@ +35,5 @@
> +  bpStore.addBreakpoint(location);
> +  do_check_eq(location, bpStore.hasBreakpoint(location), "Breakpoint added but not found in Breakpoint Store");
> +
> +  bpStore.removeBreakpoint(location);
> +  do_check_eq(null, bpStore.hasBreakpoint(location), "Breakpoint removed but still exists");

On top of testing breakpoints for whole lines, can you do the same checks for breakpoints with a column as well?

Comment 7

[Mason Chang]

Attachment: hasBreakpoint.patch

Thanks for your feedback and nit picking :) I like these code reviews heh. I did the following changes based on your feedback:

* Removed the trailing whitespace
* Fixed the comment above hasBreakpoint
* Added unit tests to include a breakpoint with a column.
* Changed unit tests to use hasBreakpoint instead of getBreakpoint
* Changed hasBreakpoint tests to fit style of other unit tests

Anything else I should fix?

> we will land the patch in fx-team. It will incubate in fx-team for a day or so > before being merged into mozilla-central, and then it will show up in the next > Nightly builds! :)

Awesome! Thanks for your guidance!

> Then go through the few places I added more calls to `getBreakpoint(..., 
> false)` in test_breakpointstore.js and replace them with calls to >`hasBreakpoint`.

Did that! Also changed the checks to see if the breakpoint exists to do_check_true, based on your usage of !! in the other tests. Cool trick, Today I learned!

> What's your irc nick? I'm fitzgen, and Jim is jimb. Say hello! :)

My nick on irc is changm. I pinged you this evening but I guess you were AFK.  hello! :)

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 783475 [details] [diff] [review]
hasBreakpoint.patch

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

(In reply to Mason Chang from comment #7)
> Created attachment 783475 [details] [diff] [review]
> hasBreakpoint.patch
> 
> Thanks for your feedback and nit picking :) I like these code reviews heh. I
> did the following changes based on your feedback:
> 
> * Removed the trailing whitespace
> * Fixed the comment above hasBreakpoint
> * Added unit tests to include a breakpoint with a column.
> * Changed unit tests to use hasBreakpoint instead of getBreakpoint
> * Changed hasBreakpoint tests to fit style of other unit tests
> 
> Anything else I should fix?

This looks great! Thanks :)

Since we last talked I have been made a module peer, so I'm r+'ing this, pushing to try, and (assuming that goes well) I will land it in fx-team for you!

> > What's your irc nick? I'm fitzgen, and Jim is jimb. Say hello! :)
> 
> My nick on irc is changm. I pinged you this evening but I guess you were
> AFK.  hello! :)

Yeah, I was at dinner. I leave my client open 24/7 so if you have any questions you can ask me and I will get back to you eventually.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Here is the try push: https://tbpl.mozilla.org/?tree=Try&rev=2f9fa7aba883

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push was green, so I am pushing to fx-team. Mason, your change will hopefully be in tomorrow's Nightly! :)

https://hg.mozilla.org/integration/fx-team/rev/992b9f11815b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/992b9f11815b

Comment 12

[Mason Chang]

Awesome! Thanks for the push :)