Summary: | Call histogramEnumeration directly | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Pilgrim (Google) <pilgrim> | ||||||||
Component: | New Bugs | Assignee: | Mark Pilgrim (Google) <pilgrim> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, haraken, japhet, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 82948 | ||||||||||
Attachments: |
|
Description
Mark Pilgrim (Google)
2012-04-03 19:40:33 PDT
Created attachment 135488 [details]
Patch
Fails to compile WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp, can not find <public/Platform.h> Comment on attachment 135488 [details] Patch Attachment 135488 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12325463 Comment on attachment 135488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135488&action=review > Source/WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp:128 > - PlatformSupport::histogramEnumeration(name, sample, boundaryValue); > + WebKit::Platform::current()->histogramEnumeration(name, sample, boundaryValue); This should use histogramEnumeration from HistogramSupport.h (because this file is outside the platform directory). > Source/WebCore/platform/chromium/PlatformSupport.h:-265 > - static void histogramEnumeration(const char* name, int sample, int boundaryValue); Please remove the implementation of this function in PlatformSupport.cpp. Created attachment 135587 [details]
Patch
(In reply to comment #4) > (From update of attachment 135488 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135488&action=review > > > Source/WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp:128 > > - PlatformSupport::histogramEnumeration(name, sample, boundaryValue); > > + WebKit::Platform::current()->histogramEnumeration(name, sample, boundaryValue); > > This should use histogramEnumeration from HistogramSupport.h (because this file is outside the platform directory). Done. > > > Source/WebCore/platform/chromium/PlatformSupport.h:-265 > > - static void histogramEnumeration(const char* name, int sample, int boundaryValue); > > Please remove the implementation of this function in PlatformSupport.cpp. Done. Comment on attachment 135587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135587&action=review > Source/WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp:37 > +#if !PLATFORM(QT) > +#include "HistogramSupport.h" > +#endif This should be included unconditionally. Comment on attachment 135587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135587&action=review > Source/WebCore/ChangeLog:7 > + Call histogramEnumeration directly > + https://bugs.webkit.org/show_bug.cgi?id=83106 > + > + Reviewed by NOBODY (OOPS!). > + It would also be good to have some text in the ChangeLog referencing either the meta bug or the webkit-dev post. It's fine to say that this change is part of a series of patches to XYZ. You just want someone who starts with this patch to have a breadcrumb trail back to understanding what's going on. Created attachment 135687 [details]
Patch
(In reply to comment #7) > (From update of attachment 135587 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135587&action=review > > > Source/WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp:37 > > +#if !PLATFORM(QT) > > +#include "HistogramSupport.h" > > +#endif > > This should be included unconditionally. Done. (In reply to comment #8) > (From update of attachment 135587 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135587&action=review > > > Source/WebCore/ChangeLog:7 > > + Call histogramEnumeration directly > > + https://bugs.webkit.org/show_bug.cgi?id=83106 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > It would also be good to have some text in the ChangeLog referencing either the meta bug or the webkit-dev post. It's fine to say that this change is part of a series of patches to XYZ. You just want someone who starts with this patch to have a breadcrumb trail back to understanding what's going on. Done. Comment on attachment 135687 [details]
Patch
Thanks!
Comment on attachment 135687 [details] Patch Clearing flags on attachment: 135687 Committed r113255: <http://trac.webkit.org/changeset/113255> All reviewed patches have been landed. Closing bug. |