Bug 73026 - [MutationObservers] Remove platform-dependent code in Document.cpp resulting from Mutation Event histogram collection
Summary: [MutationObservers] Remove platform-dependent code in Document.cpp resulting ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on: 72316
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-11-23 10:04 PST by Rafael Weinstein
Modified: 2011-12-16 16:58 PST (History)
7 users (show)

See Also:


Attachments
Patch (14.85 KB, patch)
2011-12-16 13:59 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (20.53 KB, patch)
2011-12-16 15:10 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2011-11-23 10:04:20 PST
This is cleanup from bug 72316
Comment 1 Rafael Weinstein 2011-12-16 13:59:09 PST
Created attachment 119665 [details]
Patch
Comment 2 Ryosuke Niwa 2011-12-16 14:16:21 PST
Comment on attachment 119665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119665&action=review

> Source/WebCore/ChangeLog:11
> +        No tests need. This patch is just a refactor. (OOPS!)

You need to remove (OOPS!).

> Source/WebCore/platform/chromium/HistogramSupportChromium.cpp:41
> +void HistogramSupport::histogramEnumeration(const char* name, int sample, int boundaryValue)
> +{
> +    PlatformSupport::histogramEnumeration(name, sample, boundaryValue);
> +}

Is it really okay to add an extra function call? Can't we make this function inline?
Comment 3 Rafael Weinstein 2011-12-16 14:23:35 PST
This only gets called in the document dtor.
Comment 4 Rafael Weinstein 2011-12-16 14:38:28 PST
Comment on attachment 119665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119665&action=review

>> Source/WebCore/ChangeLog:11
>> +        No tests need. This patch is just a refactor. (OOPS!)
> 
> You need to remove (OOPS!).

oops.
Comment 5 Adam Barth 2011-12-16 14:39:54 PST
Comment on attachment 119665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119665&action=review

> Source/WebCore/platform/HistogramSupport.h:42
> +#if PLATFORM(CHROMIUM)
> +    static void histogramEnumeration(const char* name, int sample, int boundaryValue);
> +#else
> +    static void histogramEnumeration(const char*, int, int) { }
> +#endif

This header shouldn't have an ifdef.  We should have a HistogramSupportNone.cpp with the stub in it.
Comment 6 Ryosuke Niwa 2011-12-16 14:46:34 PST
Comment on attachment 119665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119665&action=review

> Source/WebCore/platform/HistogramSupport.h:47
> +#endif // FileSystem_h

Wrong comment.
Comment 7 Rafael Weinstein 2011-12-16 15:10:25 PST
Created attachment 119682 [details]
Patch
Comment 8 WebKit Review Bot 2011-12-16 16:58:48 PST
Comment on attachment 119682 [details]
Patch

Clearing flags on attachment: 119682

Committed r103131: <http://trac.webkit.org/changeset/103131>
Comment 9 WebKit Review Bot 2011-12-16 16:58:55 PST
All reviewed patches have been landed.  Closing bug.