Summary: | Web Inspector: Tiny improvement to UI for adding an XHR breakpoint | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Enhancement | CC: | abarth, apavlov, bweinstein, commit-queue, eric, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||
Priority: | P5 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Mike West
2011-04-21 06:43:01 PDT
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 |