Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672483002:
[DevTools] Remove other breakpoints (Closed)
|
Created:
3 years, 10 months ago by Oleksii Kadurin Modified:
3 years, 10 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Remove other breakpoints
BUG=680923
Review-Url: https://codereview.chromium.org/2672483002
Cr-Commit-Position: refs/heads/master@{#447835}
Committed: https://chromium.googlesource.com/chromium/src/+/6270410d41cb73f7251746afaa93798e62b54555
Patch Set 1 #Patch Set 2 : Code optimization #Patch Set 3 : Code optimization #
Total comments: 5
Patch Set 4 : After review #
Messages
Total messages: 23 (13 generated)
The CQ bit was checked by ovkadurin@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
ovkadurin@gmail.com changed reviewers: + kozyatinskiy@chromium.org
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
I'd leave the code that is there as is and just add anothing method in bp manager and another item in context menu. https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js (right): https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:377: toggleAllBreakpoints(breakpoints, toggleState) { I would leave this as is. https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:385: removeAllBreakpoints(breakpoints) { I would leave this as is. https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:394: removeOtherBreakpoints(allBreakpoints, selectedBreakpoints) { Here you can make it accept the one (ones) you want to preserve. Pass a Set instead of array for the faster lookup. https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js (right): https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:195: if (allBreakpoints.length > 1) { It was nice to have these items present unconditionally.
On 2017/02/01 19:41:02, pfeldman wrote: > I'd leave the code that is there as is and just add anothing method in bp > manager and another item in context menu. > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js > (right): > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:377: > toggleAllBreakpoints(breakpoints, toggleState) { > I would leave this as is. > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:385: > removeAllBreakpoints(breakpoints) { > I would leave this as is. > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:394: > removeOtherBreakpoints(allBreakpoints, selectedBreakpoints) { > Here you can make it accept the one (ones) you want to preserve. Pass a Set > instead of array for the faster lookup. > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js > (right): > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:195: > if (allBreakpoints.length > 1) { > It was nice to have these items present unconditionally.
On 2017/02/01 19:41:02, pfeldman wrote: > I'd leave the code that is there as is and just add anothing method in bp > manager and another item in context menu. > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js > (right): > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:377: > toggleAllBreakpoints(breakpoints, toggleState) { > I would leave this as is. > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:385: > removeAllBreakpoints(breakpoints) { > I would leave this as is. > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:394: > removeOtherBreakpoints(allBreakpoints, selectedBreakpoints) { > Here you can make it accept the one (ones) you want to preserve. Pass a Set > instead of array for the faster lookup. > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js > (right): > > https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sources/JavaScriptBreakpointsSidebarPane.js:195: > if (allBreakpoints.length > 1) { > It was nice to have these items present unconditionally. Hi pfeldman, thank you for the review! The thing is that it would be not nice to show "Remove other breakpoints" when there is only one breakpoint. In that case clicking on the item will do nothing. And it will be confusing for users. So not showing that item makes sense for me. Could you please comment my concerns?
https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js (right): https://codereview.chromium.org/2672483002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:394: removeOtherBreakpoints(allBreakpoints, selectedBreakpoints) { On 2017/02/01 19:41:02, pfeldman wrote: > Here you can make it accept the one (ones) you want to preserve. Pass a Set > instead of array for the faster lookup. And compare breakpoints not by _breakpointStorageId(), but by their links, right? Like this: allBreakpoints.forEach(breakpoint => { if (!selectedBreakpoints.has(breakpoint)) breakpoint.remove(); });
>> The thing is that it would be not nice to show "Remove other breakpoints" when >> there is only one breakpoint. Why is that? As long as you can't select breakpoints, I would expect all other breakpoints are deleted. You can disable items conditionally, but you should not be not adding them.
pfeldman, I've got your idea. So I changed the code correspondingly.
lgtm
The CQ bit was checked by ovkadurin@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ovkadurin@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486067933328050,
"parent_rev": "72fa3be6dffb0026b5bc50aa36e4e1f62089758b", "commit_rev":
"6270410d41cb73f7251746afaa93798e62b54555"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Remove other breakpoints BUG=680923 ========== to ========== [DevTools] Remove other breakpoints BUG=680923 Review-Url: https://codereview.chromium.org/2672483002 Cr-Commit-Position: refs/heads/master@{#447835} Committed: https://chromium.googlesource.com/chromium/src/+/6270410d41cb73f7251746afaa93... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6270410d41cb73f7251746afaa93... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
