Bug 59100 – Web Inspector: Tiny improvement to UI for adding an XHR breakpoint

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Mike West]

Currently, adding an XHR breakpoint in the inspector is quite opaque: clicking "+" brings up an input field, but gives no information regarding what it means. After the breakpoint is added, the text is a bit more clear (`URL contains "x"`). Chrome user feedback makes it apparent that this workflow isn't clear. I'd like to add explanatory text that makes the field's purpose clear.

[Mike West]

Created attachment 90529 [details]
Strawman patch for discussion.

This is the first patch I'm attempting to contribute to WebKit, so please be gentle.  Happily, it's tiny, so I hope there's not much I could have done incorrectly. :)

Assuming that the functionality is acceptable, I'd appreciate a pointer to which layout test I ought to modify to deal with the new addition. I didn't find anything (and nothing broke... ?), but I'm happy to add tests.

Thanks!

[Pavel Feldman]

Comment on attachment 90529 [details]
Strawman patch for discussion.

View in context: https://bugs.webkit.org/attachment.cgi?id=90529&action=review

Overall looks good. r- for not localizing the string + expensive selector. Also, please attach images with screenshots when suggesting visual changes.

> Source/WebCore/ChangeLog:1
> [phone]  Mike West  <[e-mail]>

Please attach screenshot for visual changes.

> Source/WebCore/ChangeLog:11
> +        Need a short description and bug URL (OOPS!)

There should be no OOPS lines.

> Source/WebCore/inspector/front-end/BreakpointsSidebarPane.js:268
> +        inputElementContainer.textContent = WebInspector.UIString("Break when URL contains:");

New localized strings should be added into WebCore/English.lproj/localizedStrings.js

> Source/WebCore/inspector/front-end/inspector.css:1744
> +.pane > .body .breakpoint-condition span {

This selector is too expensive. Could you please assign a class name (or id) to the span?

[Mike West]

Created attachment 90535 [details]
Screenshot of new behavior.

Screenshot of new behavior.

[Mike West]

Created attachment 90545 [details]
Screenshot with fixed padding.

[Mike West]

Created attachment 90546 [details]
Patch that addresses the initial review comments.

I briefly attempted to replace the `p > span` structure with a `label > input`. I still believe that's a good idea, but I'm running into enough tiny display issues doing it that I'd like to break it out into a larger refactoring (e.g. including the other on-the-fly fields in the inspector, for instance the "Watch Expressions" panel) in another bug.

[Pavel Feldman]

Comment on attachment 90546 [details]
Patch that addresses the initial review comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=90546&action=review

One nit, otherwise looks good.

> Source/WebCore/inspector/front-end/inspector.css:1744
> +#breakpoint-condition-input {

Blank line above please.

[Mike West]

Created attachment 90550 [details]
Patch to fix a nit in CSS.

[Mike West]

(In reply to comment #7)
> Created an attachment (id=90550) [details]
> Patch to fix a nit in CSS.

Newbie question: You dropped the `review?` flag entirely, so I'm not quite sure what the next step is. `review?` or `review? + cq?` or something else entirely? :)

[WebKit Commit Bot]

Comment on attachment 90550 [details]
Patch to fix a nit in CSS.

Rejecting attachment 90550 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Pavel Feldman', u'--fo..." exit_code: 9

Parsed 4 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
Original content of Source/WebCore/English.lproj/localizedStrings.js mismatches at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 274.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Pavel Feldman', u'--fo..." exit_code: 9

Full output: http://queues.webkit.org/results/8494386

[Mike West]

Created attachment 90684 [details]
Patch rebased onto ToT.

[WebKit Commit Bot]

Comment on attachment 90684 [details]
Patch rebased onto ToT.

Clearing flags on attachment: 90684

Committed r84622: <http://trac.webkit.org/changeset/84622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84622 might have broken GTK Linux 32-bit Release
The following tests are not passing:
media/controls-strict.html