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.
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!
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 > +2011-04-21 Mike West <mkwst@google.com> 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?
Created attachment 90535 [details] Screenshot of new behavior. Screenshot of new behavior.
Created attachment 90545 [details] Screenshot with fixed padding.
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.
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.
Created attachment 90550 [details] Patch to fix a nit in CSS.
(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? :)
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
Created attachment 90684 [details] Patch rebased onto ToT.
Comment on attachment 90684 [details] Patch rebased onto ToT. Clearing flags on attachment: 90684 Committed r84622: <http://trac.webkit.org/changeset/84622>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/84622 might have broken GTK Linux 32-bit Release The following tests are not passing: media/controls-strict.html