RESOLVED FIXED Bug 113613
Expose FeatureObserver data to WebKit clients 
https://bugs.webkit.org/show_bug.cgi?id=113613
Summary Expose FeatureObserver data to WebKit clients
Alexey Proskuryakov
Reported 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.
Attachments
proposed patch (15.16 KB, patch)
2013-03-29 15:21 PDT, Alexey Proskuryakov 		abarth: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2013-03-29 15:21:26 PDT
Created attachment 195810 [details] proposed patch
Sam Weinig
Comment 2 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?
Alexey Proskuryakov
Comment 3 2013-03-29 16:29:07 PDT
Committed <http://trac.webkit.org/changeset/147260>.
Csaba Osztrogonác
Comment 4 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
Adam Barth
Comment 5 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.
Alexey Proskuryakov
Comment 6 2013-03-30 13:26:37 PDT
Please feel free to refactor. Please do not roll out the patch.
Maciej Stachowiak
Comment 7 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?
Alexey Proskuryakov
Comment 8 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).
Adam Barth
Comment 9 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".
Alexey Proskuryakov
Comment 10 2013-04-02 08:29:12 PDT
Sorry for the typo, Adam is of course correct.
Note You need to log in before you can comment on or make changes to this bug.