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
Alexey Proskuryakov
2013-03-29 15:05:13 PDT
Created attachment 195810 [details]
proposed patch
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? Committed <http://trac.webkit.org/changeset/147260>. (In reply to comment #3) > Committed <http://trac.webkit.org/changeset/147260>. and buildfix landed in http://trac.webkit.org/changeset/147277 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.
Please feel free to refactor. Please do not roll out the patch. 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? 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). (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". Sorry for the typo, Adam is of course correct. |