Bug 109874 - Update FeatureObserver on top level navigation
Summary: Update FeatureObserver on top level navigation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-14 16:21 PST by Kassy Coan
Modified: 2013-02-19 11:20 PST (History)
4 users (show)

See Also:

Attachments
Patch (4.21 KB, patch)
2013-02-14 16:23 PST, Kassy Coan 		no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2013-02-18 16:51 PST, Kassy Coan 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kassy Coan 2013-02-14 16:21:22 PST 
Update FeatureObserver on top level navigation
Comment 1 Kassy Coan 2013-02-14 16:23:33 PST 
Created attachment 188448 [details]
Patch
Comment 2 Kassy Coan 2013-02-14 16:28:52 PST 
abarth@ could you take a look at this?

With this update, we are flushing the histogram on top level navigation, in addition to page destruction, in order to get closer to "per page" tracking.

The new enum is for tracking number of pages visited (i.e. total number of histogram flushes). It is in a separate enum, because this statistic is also relevant to CSS tracking, which will be added to FeatureObserver in a separate patch.
There is a place holder in the new enum due do a restriction in histogramEnumeration, which requires there to be 3 or more buckets.
Comment 3 Adam Barth 2013-02-14 16:36:34 PST 
Comment on attachment 188448 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:3272
> +        m_frame->page()->setFeatureObserver(adoptPtr(reinterpret_cast<FeatureObserver*>(0)));

You can write:

m_frame->page()->setFeatureObserver(nullptr)

which is a bit cleaner.

> Source/WebCore/page/FeatureObserver.cpp:45
> +    HistogramSupport::histogramEnumeration("WebCore.FeatureObserver.TrackingStats", PageVisits, NumberOfStatistics);

I'd prefer to use the same histogram for the baseline stats as for the other stats.  That makes sure they stay in sync.

> Source/WebCore/page/FeatureObserver.cpp:66
> +    if (!page->featureObserver())
> +        page->setFeatureObserver(adoptPtr(new FeatureObserver()));

Rather than calling "new" and "delete" all the time, we can just trigger measurement on navigation and reset the m_featureBits.
Comment 4 Kassy Coan 2013-02-18 16:51:31 PST 
Created attachment 188961 [details]
Patch
Comment 5 Kassy Coan 2013-02-18 16:59:15 PST 
@abarth
I've uploaded a new patch with all the improvements you suggested:
-simplification in FrameLoader.cpp
-keeping pagevisit stats in existing enum for consistency
-no longer calling 'new'
Thank you!
Comment 6 Adam Barth 2013-02-19 11:01:12 PST 
Comment on attachment 188961 [details]
Patch

Looks great.  Thanks!
Comment 7 WebKit Review Bot 2013-02-19 11:20:44 PST 
Comment on attachment 188961 [details]
Patch

Clearing flags on attachment: 188961

Committed r143363: <http://trac.webkit.org/changeset/143363>
Comment 8 WebKit Review Bot 2013-02-19 11:20:47 PST 
All reviewed patches have been landed.  Closing bug.