Bug 83106 - Call histogramEnumeration directly
Summary: Call histogramEnumeration directly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Pilgrim (Google)
URL:
Keywords:
Depends on:
Blocks: 82948
  Show dependency treegraph
 
Reported: 2012-04-03 19:40 PDT by Mark Pilgrim (Google)
Modified: 2012-04-04 15:55 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.08 KB, patch)
2012-04-03 19:41 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (6.96 KB, patch)
2012-04-04 07:37 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (6.14 KB, patch)
2012-04-04 14:38 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2012-04-03 19:40:33 PDT
Call histogramEnumeration directly
Comment 1 Mark Pilgrim (Google) 2012-04-03 19:41:05 PDT
Created attachment 135488 [details]
Patch
Comment 2 Mark Pilgrim (Google) 2012-04-03 19:42:04 PDT
Fails to compile WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp, can not find <public/Platform.h>
Comment 3 WebKit Review Bot 2012-04-03 20:00:57 PDT
Comment on attachment 135488 [details]
Patch

Attachment 135488 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12325463
Comment 4 Adam Barth 2012-04-03 21:28:02 PDT
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.
Comment 5 Mark Pilgrim (Google) 2012-04-04 07:37:35 PDT
Created attachment 135587 [details]
Patch
Comment 6 Mark Pilgrim (Google) 2012-04-04 07:38:20 PDT
(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 7 Adam Barth 2012-04-04 10:34:10 PDT
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 8 Adam Barth 2012-04-04 10:35:32 PDT
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.
Comment 9 Mark Pilgrim (Google) 2012-04-04 14:38:50 PDT
Created attachment 135687 [details]
Patch
Comment 10 Mark Pilgrim (Google) 2012-04-04 14:39:18 PDT
(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.
Comment 11 Mark Pilgrim (Google) 2012-04-04 14:39:29 PDT
(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 12 Adam Barth 2012-04-04 14:46:17 PDT
Comment on attachment 135687 [details]
Patch

Thanks!
Comment 13 WebKit Review Bot 2012-04-04 15:55:26 PDT
Comment on attachment 135687 [details]
Patch

Clearing flags on attachment: 135687

Committed r113255: <http://trac.webkit.org/changeset/113255>
Comment 14 WebKit Review Bot 2012-04-04 15:55:42 PDT
All reviewed patches have been landed.  Closing bug.