Bug 113613 - Expose FeatureObserver data to WebKit clients
Summary: Expose FeatureObserver data to WebKit clients
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
Depends on:
Reported: 2013-03-29 15:05 PDT by Alexey Proskuryakov
Modified: 2013-04-02 08:29 PDT (History)
9 users (show)

See Also:

proposed patch (15.16 KB, patch)
2013-03-29 15:21 PDT, Alexey Proskuryakov
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2013-03-29 15:05:13 PDT
Currently, FeatureObserver depends on chromium-only HistogramSupport, which is not really usable on Mac at least.

Instead of adding parallel feature reporting machinery, I'm adding a way to generically relay the data from FeatureObserver to port code.
Comment 1 Alexey Proskuryakov 2013-03-29 15:21:26 PDT
Created attachment 195810 [details]
proposed patch
Comment 2 Sam Weinig 2013-03-29 15:24:58 PDT
Comment on attachment 195810 [details]
proposed patch

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

> Source/WebCore/loader/FrameLoader.cpp:3356
> +    info.addMember(m_previousURL, "previousUrl");

You should fix the string as well.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4007
> +    // FIXME: Feature names should not be hardcoded.

Bug number?
Comment 3 Alexey Proskuryakov 2013-03-29 16:29:07 PDT
Committed <http://trac.webkit.org/changeset/147260>.
Comment 4 Csaba Osztrogonác 2013-03-30 01:58:55 PDT
(In reply to comment #3)
> Committed <http://trac.webkit.org/changeset/147260>.

and buildfix landed in http://trac.webkit.org/changeset/147277
Comment 5 Adam Barth 2013-03-30 10:45:28 PDT
Comment on attachment 195810 [details]
proposed patch

Rather than adding PLATFORM #ifdefs to this file, we should refactor this code so that it can be used by the apple-mac port.  I'm surmised that Sam approved this patch given that he usually opposes adding PLATFORM ifdefs to common code.
Comment 6 Alexey Proskuryakov 2013-03-30 13:26:37 PDT
Please feel free to refactor. Please do not roll out the patch.
Comment 7 Maciej Stachowiak 2013-04-01 23:46:19 PDT
The ifdefs don't seem to have a material effect, since HistogramSupport has a no-op default implementation. Were they actually required for the patch to work?
Comment 8 Alexey Proskuryakov 2013-04-02 00:42:12 PDT
No, they were needed for it to work. As mentioned in the ChangeLog, the ifdefs were added for clarity.

I think that it's significantly better to have an ifdef than to have a seemingly complete implementation that doesn't do anything, while the actual working code is in a different project (WebKit).
Comment 9 Adam Barth 2013-04-02 00:48:16 PDT
(In reply to comment #8)
> No, they were needed for it to work. As mentioned in the ChangeLog, the ifdefs were added for clarity.

^^^ I presume you mean "not needed".
Comment 10 Alexey Proskuryakov 2013-04-02 08:29:12 PDT
Sorry for the typo, Adam is of course correct.