Bug 112740 - Add a feature observer for RenderDeprecatedFlexibleBox
Summary: Add a feature observer for RenderDeprecatedFlexibleBox
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: Christian Biesinger
URL:
Keywords:
Depends on: 112958
Blocks:
  Show dependency tree / graph
 
Reported: 2013-03-19 13:49 PDT by Christian Biesinger
Modified: 2013-03-21 13:30 PDT (History)
7 users (show)

See Also:

Attachments
Patch (2.82 KB, patch)
2013-03-19 13:51 PDT, Christian Biesinger 		no flags Details | Formatted Diff | Diff
Patch (2.84 KB, patch)
2013-03-19 16:03 PDT, Christian Biesinger 		no flags Details | Formatted Diff | Diff
Patch (2.94 KB, patch)
2013-03-20 11:45 PDT, Christian Biesinger 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) 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 Christian Biesinger 2013-03-19 13:49:10 PDT 
Add a feature observer for RenderDeprecatedFlexibleBox
Comment 1 Christian Biesinger 2013-03-19 13:51:53 PDT 
Created attachment 193914 [details]
Patch
Comment 2 Adam Barth 2013-03-19 13:54:06 PDT 
LGTM, but Ojan should give you the official review.
Comment 3 Tony Chang 2013-03-19 14:10:31 PDT 
Comment on attachment 193914 [details]
Patch

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

> Source/WebCore/page/FeatureObserver.h:104
> +        DeprecatedFlexbox,

Nit: I would probably name this DeprecatedFlexboxWebContent or something.
Comment 4 Christian Biesinger 2013-03-19 16:03:37 PDT 
Created attachment 193944 [details]
Patch
Comment 5 Eric Seidel (no email) 2013-03-19 20:54:27 PDT 
Ojan was telling me today that FeatureObserver depends on normal-page shutdown, which is not how chromium renderers commonly shut down?  Suggesting we're not getting very good data from these metrics?
Comment 6 WebKit Review Bot 2013-03-19 20:56:57 PDT 
Comment on attachment 193944 [details]
Patch

Rejecting attachment 193944 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 193944, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
ebkit-commit-queue

Parsed 3 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/page/FeatureObserver.h
Hunk #1 FAILED at 101.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/page/FeatureObserver.h.rej
patching file Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17191542
Comment 7 Ojan Vafai 2013-03-19 20:59:37 PDT 
(In reply to comment #5)
> Ojan was telling me today that FeatureObserver depends on normal-page shutdown, which is not how chromium renderers commonly shut down?  Suggesting we're not getting very good data from these metrics?

Chrome only does normal shutdown of the page if is being navigated to a same-domain page, it has beforeunload/unload listeners registered or there are multiple pages in the same render process (in all other cases, we can safely just kill the render process). It's kind of a bummer, but I'm not sure how we would be able to get better metrics. Hopefully this doesn't skew our numbers too much.
Comment 8 Christian Biesinger 2013-03-20 11:45:38 PDT 
Created attachment 194092 [details]
Patch

Rebased
Comment 9 WebKit Review Bot 2013-03-20 12:22:17 PDT 
Comment on attachment 194092 [details]
Patch

Clearing flags on attachment: 194092

Committed r146375: <http://trac.webkit.org/changeset/146375>
Comment 10 WebKit Review Bot 2013-03-20 12:22:21 PDT 
All reviewed patches have been landed.  Closing bug.