Bug 113613

Summary: Expose FeatureObserver data to WebKit clients
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebKit Misc.Assignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, beidson, benjamin, japhet, mjs, ossy, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch abarth: review-

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.