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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
proposed patch	abarth: review-

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-	Details Formatted Diff Diff
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.