| Summary: | [Coverity] Address uninit constructor issues | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | kmadhusu <kmadhusu> | ||||||||||
| Component: | New Bugs | Assignee: | kmadhusu <kmadhusu> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | andersca, jamesr, tonikitoo, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
kmadhusu
2012-03-27 13:48:44 PDT
Created attachment 134130 [details]
Patch
Attachment 134130 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/ChangeLog:3: Line contains tab character. [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:4: Line contains tab character. [whitespace/tab] [5]
Source/WebCore/ChangeLog:4: Line contains tab character. [whitespace/tab] [5]
Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 4 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 134134 [details]
Fixed style errors
Comment on attachment 134130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134130&action=review Most of these changes seem fine, but some do not (see inline comments). Also please fix the tab characters. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:62 > #endif > + , m_textTrackDisplay(0) this should be inside the #if ENABLE() guard, not outside of it > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:569 > + , m_enabled(false) I'm not sure this is correct - this bool doesn't seem to actually be used. maybe it should be deleted instead? > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:130 > + , m_resolutionScale(0) i think 1 is a better value to initialize to Comment on attachment 134134 [details]
Fixed style errors
Same comments apply
Created attachment 134140 [details]
Addressed review comments.
Comment on attachment 134130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134130&action=review Addressed review comments. Please review the latest patch. Thanks. >> Source/WebCore/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=82376 > > Line contains tab character. [whitespace/tab] [5] Fixed. >> Source/WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Done >> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:62 >> + , m_textTrackDisplay(0) > > this should be inside the #if ENABLE() guard, not outside of it oops. Fixed. >> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:569 >> + , m_enabled(false) > > I'm not sure this is correct - this bool doesn't seem to actually be used. maybe it should be deleted instead? Yeah. Even I feel it should be deleted. So I made code changes accordingly. >> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:130 >> + , m_resolutionScale(0) > > i think 1 is a better value to initialize to Sure. Done >> Source/WebKit/chromium/ChangeLog:3 >> + [Coverity] Address uninit constructor issues. > > Line contains tab character. [whitespace/tab] [5] Fixed. >> Source/WebKit/chromium/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=82376 > > Line contains tab character. [whitespace/tab] [5] Fixed. Comment on attachment 134140 [details]
Addressed review comments.
Cool, looks good.
Comment on attachment 134140 [details] Addressed review comments. Rejecting attachment 134140 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12141785 jamesr@: Thanks for reviewing my code. I am not sure why the commit queue failed. Do I need to make any changes to the ChangeLog? (In reply to comment #10) > jamesr@: Thanks for reviewing my code. I am not sure why the commit queue failed. Do I need to make any changes to the ChangeLog? I think it picked up the wrong patch because patch set 2 wasn't marked "obsolete". Will try it again. Are you using webkit-patch upload to upload these patches? It takes care of many of these things for you. Comment on attachment 134140 [details] Addressed review comments. View in context: https://bugs.webkit.org/attachment.cgi?id=134140&action=review > Source/WebKit/chromium/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=82376 > + > + New tests are not required since I didn't modify any code behavior. I just initalized the member variables. actually i take that back - you need to have the "Reviewed by NOBODY (OOPS!)." line in this ChangeLog entry as well as the WebCore/ChangeLog one. Oh okay. I didn't use webkit-patch upload script. Can I modify the ChangeLog now to include that text? Yeah, just add those lines back and upload a new patch. Created attachment 134170 [details]
Patch
Comment on attachment 134170 [details] Patch Clearing flags on attachment: 134170 Committed r112343: <http://trac.webkit.org/changeset/112343> All reviewed patches have been landed. Closing bug. |