Summary:
A personal rule only has actions targeting the owner. Likewise, only they can
edit the rule. OTOH, a global may affect any target and is editable by anyone.
There are no new action types. Instead, type of the rule modifies the available
targets and the messaging in the ui. This is beneficial because herald rule
adapters don't need to be aware of the difference between emailing the owner of
a personal rule and emailing an arbitrary user.
This diff sets up the logic and ui for creating personal/global rules. All
existing rules have been defaulted to global.
TODO: Filter all existing rules into personal/global
TODO: Create a UI for surfacing (relevant?) global rules.
Test Plan:
1. Created a personal rule to email myself. Created a dumby revision satisfying
the conditions of that rule. Verified that I recieved a herald email.
2. Removed my adminship, change the owner of a personal rule. verified that I
couldn't edit the rule.
3.Changed rule type to global. verified that I could edit the rule.
4. Verified that admins can edit both global and personal rules.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, zizzy
Differential Revision: https://secure.phabricator.com/D1449
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.
Test Plan:
Alter database.
Open revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1437
Summary: /xhprof/ doesn't exist at all, /mail/ is only for admins
Test Plan: Display directory
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1405
databases
Summary: This is a stopgap patch until we fix T345/T327 properly, but you can't
add a 512-character key to a UTF-8 column because it creates an index on more
than 767 bytes, which is the key length limit.
Test Plan: Ran schmea upgrade script.
Reviewers: indiefan, btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, xela, jungejason
Maniphest Tasks: T327
Differential Revision: https://secure.phabricator.com/D1391
Summary:
See T624. I originally wrote this to require an explicit remote, but this
creates an ugly "origin:" in all the URIs and makes T270 more difficult.
Treat all branch names as implying 'origin/'.
Test Plan:
- Pulled and imported a fresh copy of libphutil without issues.
- Browsed various git repositories.
- Browsed Javelin's various branches.
- Ran upgrade script, got a bunch of clean 'origin/master' -> 'master'
conversions.
- Tried to specify an explicit remote in a default branch name.
- Unit tests.
Reviewers: nh, jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T624
Differential Revision: https://secure.phabricator.com/D1269
Summary:
Sometimes the SQL account may not have CREATE DATABASE privileges, so try to
fail this script immediately rather than having it work halfway if the
permission is missing.
This is just a minor tweak to try to slightly improve some cases, the long term
fix is T345 but that's a good deal more work than this.
Test Plan: Visually inspected patch; trivial change.
Reviewers: jungejason, btrahan, skrul
Reviewed By: skrul
CC: aran, skrul
Differential Revision: https://secure.phabricator.com/D1287
Summary:
- These never actually did anything.
- I don't even really remember why I built them, maybe the Open Source team
was pushing for more GitHub integration or something? I really have no idea.
- Anyway, repository tailers do everything these could do (and much more).
Test Plan:
- Ran tailers off GitHub for many months without needing post-receive hooks.
- Grepped for relevant strings, couldn't find any references.
- Used "Repository" edit interface for a Git repository.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T706
Differential Revision: https://secure.phabricator.com/D1273
Summary:
Provide tighter integration between Projects and Phriction. Partly, I have most
of a rewrite for the Projects homepage ready but it's not currently possible to
publish feed stories about a project so all the feeds are empty/boring. This
partly makes them more useful and partly just provides a tool integration point.
- When you create a project, all the wiki pages in projects/<project_name>/*
are associated with it.
- Publish updates to those pages as being related to the project so they'll
show up in project feeds.
- Show a project link on those pages.
This is very "convention over configuration" but I think it's the right
approach. We could provide some sort of, like, "@project=derp" tag to let you
associated arbitrary pages to projects later, but just letting you move pages is
probably far better.
Test Plan:
- Ran upgrade scripts against stupidly named projects ("der", " der", " der
", "der (2)", " der (2) (2)", etc). Ended up with uniquely named projects.
- Ran unit tests.
- Created /projects/ wiki documents and made sure they displayed correctly.
- Verified feed stories publish as project-related.
- Edited projects, including perfomring a name-colliding edit.
- Created projects, including performing a name-colliding create.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T681
Differential Revision: 1231
Summary:
add basic auditing functionalities. For the related commits for a
package, we detect the following conditions which might be suspicious to the
owners of the package:
* no revision specified
* revision not found
* author not match
* reviewedby not match
* owners not involved
* commit author not recognized
The owners of the package can change the status of the audit entries by
accepting it or specify concern.
The owner can turn on/off the auditing for a package.
Test Plan:
* verified that non-owner cannot see the details of the audit and cannot modify
it
* verified that all the audit reasons can be detected
* tested dropdown filtering and package search
* verified really normal change not detected
* verified accept/concern a commit
* tested enable/disable a package for auditing
* verified one audit applies to all <commit, packages> to the packages the
auditor owns
* verified that re-parsing a commit won't have effect if there exists a
relationship for <commit, package> already
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, benmathews, btrahan, mpodobnik, prithvi, TomL, epriestley
Differential Revision: 1242
Summary:
- Add a "delete" operation. Delete is just a special edit which removes the
page from indexes and shows a notice that the document has been deleted.
- When a user deletes all the content on a page, treat it as a delete.
- When a conduit call deletes all the content on a page, treat it as a delete.
- Add page status to Conduit.
- Add change type field to history.
- Added a couple of constants to support a future 'move' change, which would
move content from one document to another.
Test Plan:
- Verified deleted pages vanish from the document index (and restoring them
puts them back).
- Verified deleted pages show "This page has been deleted...".
- Created, edited and deleted a document via Conduit.
- Deleted pages via "delete" button.
- Deleted pages via editing content to nothing.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: skrul, aran, btrahan, epriestley
Maniphest Tasks: T680
Differential Revision: 1230
Summary:
This is a needlessly confusing/complex feature that I originally wrote sort of
speculativley. I think we can better serve what little need may exist here with
project feeds.
I'm probably going to get rid of or deemphasize "role" too and just add "Join
Project" and "Leave Project" buttons.
Test Plan: Viewed project list, project profile. Edited project profile and
affiliation.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1228
Summary:
For each commit, find the affected packages, and provide a way to
search by package.
Test Plan:
create commits that touch and don't touch two packages, and verify
that they display correctly in all the UI pages.
Reviewers: epriestley, blair, nh, tuomaspelkonen
Reviewed By: epriestley
CC: benmathews, aran, epriestley, btrahan, jungejason, mpodobnik, prithvi
Maniphest Tasks: T83
Differential Revision: 1208
Test Plan: ran upgrade_schema.php on my local install. re-examined SQL query
"explain select * from phabricator_pastebin.pastebin_paste where authorPHID =
"PHID-USER-xee4ju2teq7mflitwfcs" ORDER BY id DESC;" on my local install and
noted fewer rows scanned, index used, etc
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1218
Summary:
This landed during my review drama embargo and is a generally good idea but had
some implementation issues.
@elynde reports it has been broken for some time, although it still works on
secure.phabricator.com so I'm guessing it's just taking a zillion years to run
at Facebook. It's up to more than a second for me on secure.phabricator.com:
https://secure.phabricator.com/file/view/PHID-FILE-v4ql4c66u3xnkarmrpm4/
The basic problem is that some of the data architecture around this
implementation is hard to scale. I want to pursue a similar feature eventually,
but drive it off notifications that we'll ship through real-time infrastructure
too.
I'm also trying to get rid of DifferentialRevisionListData and this simplifies
that somewhat.
Test Plan:
- Grepped for table name, table constant, query constant, and class name; no
hits.
- Applied SQL patch.
- Verified that Differential no longer shows "Updated".
Reviewers: elynde, btrahan, jungejason
Reviewed By: elynde
CC: aran, elynde
Differential Revision: 1178
Summary:
Caught a bad (8ms) query that didn't have a good key available. Add an
appropriate key.
The query in question is:
SELECT * FROM `maniphest_transaction` WHERE taskID = 262 ORDER BY id ASC;
...which is used in the task detail view.
Test Plan: Ran EXPLAIN on this query before/after upgrading, it now uses the
newly available key.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran
Differential Revision: 1037
Summary:
See T549. Under configurations where files are served from an alternate domain
which does not have cookie credentials, we use random keys to prevent browsing,
similar to how Facebook relies on pseudorandom information in image URIs (we
could some day go farther than this and generate file sessions on the alternate
domain or something, I guess).
Currently, we generate these random keys in a roundabout manner. Instead, use a
real entropy source and store the key on the object. This reduces the number of
sha1() calls in the codebase as per T547.
Test Plan: Ran upgrade scripts, verified database was populated correctly.
Configured alternate file domain, uploaded file, verified secret generated and
worked properly. Changed secret, was given 404.
Reviewers: jungejason, benmathews, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: 1036
Summary: Allow configuration of a default author for bugs@ emails which don't
correspond to a known system user.
Test Plan: Configured a default author, sent some mails from nonsense addresses,
tasks were created.
Reviewers: davidreuss, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley, ide
Differential Revision: 1013
Summary:
Current scripts make it hard to administer Phabricator instance while not having
direct (priviledged) access to the database. This change allows scenario where
DB administrator creates the databases for you before you run update_schema
script.
Test Plan:
Create the databases before running the update_schema script - it shouldn't
complain that the databases already exist.
Reviewers: aran, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1014
Summary:
This makes symbol cross-references work in Differential. You need to do a little
legwork but I'll document that once the change has baked for a little while.
Basically:
- Projects are annotated with indexed languages, and "shared library" projects
(for example, symbols in Phabricator should be searched for in Arcanist and
libphutil).
- When we render a changeset, we check if its language is an indexed one. If
it is, we invoke the decorator Javascript.
- The Javascript takes you to a lookup page, which either gives you a list of
matching symbols (if several match) or redirects you instantly to the
definition.
Test Plan: Clicked class and function symbols in a diff, got jumped into
sensible sorts of places in Diffusion.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 980
Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.
Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.
Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.
Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran
CC:
Differential Revision: 931
Summary: See T315 for an extensive description of this feature. Adds the
descibed storage table.
Test Plan: Used phpsh to read/write symbol objects.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 897
Summary:
These fields use auxiliary storage now. Migrate the data and get rid of the
columns in the main table.
- This might take a little while to run, although there are <500k rows so
probably not too long.
- Maybe grab a backup of the table first, if I screwed something up this will
delete the data in these fields.
Test Plan:
- Ran migration locally.
- Browsed Differential.
- Grepped for "revertPlan" and "blameRevision".
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 832
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary:
Precursor to building this out to solve T343. This is similar to the Maniphest
fields we landed recently, although I think they're dissimilar enough that it
isn't worth going crazy trying to make them share code, at least for now.
This doesn't really do anything yet, just adds a storage object and a couple of
selector/field indirection classes.
Test Plan: Ran SQL upgrade script, created an aux field.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 798
Summary: This has to table scan a ginormous table right now, give it a fighting
chance with a more usable key.
Test Plan:
- Launched GC daemon, no errors.
- Used test console to create a new transcript.
- Viewed some old transcripts.
- Ran EXPLAIN on the SELECT and verified it was utilizing the garbageCollected
key.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 735
Test Plan: Look at a task detail. Some dummy attributes are automatically added in ManiphestTaskDetailController.
Reviewed By: epriestley
Differential Revision: 730
Summary:
It makes more sense to just make this a settings panel rather than a standalone
app, particularly since setting panels are relatively well separated now.
Also default-disabled the SSH Keys interface since it won't currently be useful
for most installs.
Test Plan: Edited preferences.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, jungejason
Differential Revision: 716
Summary:
Add a new column to PhrictionContent called 'comment' or 'description' or
something
Add an optional field to the Phriction document editing interface that
allows you to add a comment
Test Plan:
Run the sql patch to modify phriction_content table
Create a new wiki page in Phriction
Type in words in description field and save the page
Visit this page and click "Edit Page" button
The content in the desciption field is saved
Reviewed By: epriestley
Reviewers: epriestley, hsb, codeblock
Commenters: codeblock
CC: aran, codeblock, hwang, epriestley
Differential Revision: 709
Summary:
With the sshd-vcs thing I hacked together, this will enable Phabricator to host
repositories without requiring users to have SSH accounts.
I also fixed "subporjects" and added an explicit ENGINE to it.
Test Plan: Created, edited and deleted public keys. Attempted to add the same
public key twice. Attempted to add invalid and unnamed public keys.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran, cadamo, codeblock
CC: aran, epriestley
Differential Revision: 711
Summary:
Quite basic subproject attachment.
Test Plan:
Go to some proj
ect and add another as a subproject.
Play around the ui.
Reviewers: epr
iestley
CC:
Summary:
Added a 'parent' field which stores a PHID of another paste. If it is not NULL show a list of children pastes on view.
Also did some misc. refactoring to clean up the code a bit, specifically in the Create controller.
Test Plan:
- Checked old pastes, they were not affected.
- Added a paste, successfully.
- Forked it, successfully.
- Went to the original paste, saw the child paste listed.
- Forked it again, saw the new one added to the list.
Reviewers:
epriestley
CC:
Differential Revision: 672
Summary:
A few minor things:
- Add Phriction to the homepage.
- Link to remarkup reference on the edit screen.
- Add a help tab with a super basic help document.
Test Plan: Cursory inspection.
Reviewed By: codeblock
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, codeblock
Differential Revision: 655
Summary:
This is another chunk of D636, I just simplified it a bit and added slugs.
When you go to a page like /w/pokemon/, it allows you to create or edit the
page.
Title vs slug stuff is a little funky but I think mostly-reasonable.
Test Plan: Created and edited /w/, /w/pokemon/, etc.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, aran, tuomaspelkonen
CC: aran, hsb
Differential Revision: 643
Summary:
Basically a copy/paste of parts of D636, but with two changes:
- Fully separate the index table ("document") from the content table
("content"). I think this will be a cleaner solution in the long run.
- Build slugs into the document structure.
This doesn't do anything useful, it just normalizes slugs and lays some
groundwork.
Test Plan:
- Visited various /w/ pages and saw them normalize correctly.
- Verified the DAO works by inserting dummy rows.
Reviewed By: codeblock
Reviewers: hsb, codeblock, jungejason, aran, tuomaspelkonen
CC: aran, codeblock, epriestley
Differential Revision: 638
Summary:
A bunch of the .sql patch files don't explicitly specify the table engine, but
we should always use InnoDB with the exception of one table which needs MyISAM
for FULLTEXT.
MySQL doesn't no-op an ALTER TABLE statment that changes the engine back to
itself and converting large tables can be time consuming, so convert only the
required tables.
Test Plan: Ran on secure.phabricator.com and my local box, it fixed all the
issues in about 3 seconds on secure.phabricator.com and <<1 second on my local.
Reviewed By: codeblock
Reviewers: codeblock, tuomaspelkonen, jungejason, aran
CC: aran, epriestley, codeblock
Differential Revision: 641
Summary:
Add the differential parse cache to the GC. This is the largest object in the
system by a wide margin, I think.
This table is potentially gigantic which is why the script truncates it before
doing a schema change.
Test Plan: Ran the GC daemon, it cleaned up some parse caches.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: tuomaspelkonen
CC: aran, jungejason, tuomaspelkonen, epriestley
Differential Revision: 620
Summary: Port slowvote. This has some style/layout roughness but gets us most of
the way there. I'll followup to fix some of the markup issues.
Test Plan: Created and voted in several different kinds of poll.
Reviewed By: codeblock
Reviewers: codeblock, tomo, jungejason, aran, tuomaspelkonen
Commenters: aran, jungejason
CC: aran, codeblock, jungejason, epriestley
Differential Revision: 613
Summary:
- have files be uploaded by drag+drop instead of browse.
- Files are named by their uploaded filename, the user isn't given a chance to enter a file name. Is this bad?
- Store author PHID now with files
- Allow an ?author=<username> to limit the /files/ list by author.
- If one file is uploaded, the user is taken to its info page.
- If several are uploaded, they are taken to a list of their files.
Test Plan:
- Quickly tested everything and it still worked, I'd recommend some people try this out before it gets committed though. It's a rather huge revision.
Reviewers:
epriestley, Ttech
CC:
Differential Revision: 612
Summary:
People want to see all the tasks they have subscribed to in one
view. A new table was added for this to make queries faster.
Test Plan:
Tested that the view was initially empty. After running the
reindex_maniphest.php script, I saw the correct tasks there. Added
myself as a subscriber to one task and made sure the view was
updated. Removed myself as a subscriber from one task and made sure
the view was updated again.
Reviewed By: epriestley
Reviewers: epriestley, jungejason, codeblock
CC: aran, rm, epriestley
Differential Revision: 603
Summary: This defines an extremely basic version of an activity feed, like
Facebook's news feed. It doesn't do much of interest yet.
Test Plan: Published some feed stories:
https://secure.phabricator.com/file/view/PHID-FILE-5061aa72105bbdc05b21/
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
Commenters: codeblock, jungejason
CC: aran, epriestley, codeblock, tuomaspelkonen, jungejason
Differential Revision: 593
Summary:
- Add a default list of supported languages to default.conf.php
and make the initial/default value customizable.
- Store a '' in the database to infer the language from the filename/title.
Test Plan:
Tested in my sandbox with pygments enabled and disabled and various
combinations of filename/extension/dropdown selection.
Reviewers:
epriestley
CC:
Differential Revision: 587
Summary:
Major things taking place here:
- A new table for storing <task, project> relationships.
- Moved all task query logic into a dedicated class.
- Added a "projects" filter to the UI.
I was originally going to try to drive this off the main search index but the
perf benefits of a custom schema make an overwhelming argument in favor of doing
it this way.
Test Plan:
Filtered tasks by author and owner and zero, one, and more than one project.
Exercised all the group/sort options. Ran the index script over my 100k task
corpus. Edited task-project membership and verified the index updated.
Reviewed By: cadamo
Reviewers: gc3, jungejason, cadamo, tuomaspelkonen, aran
CC: aran, cadamo, epriestley
Differential Revision: 556
Summary:
Maniphest is missing some keys and some query strategy which will make it
cumbersome to manage more than a few tens of thousands of tasks.
Test Plan:
Handily manipulated 100k-scale task groups. Maniphest takes about 250ms to
select and render pages of 1,000 tasks and has no problem paging and filtering
them, etc. We should be good to scale to multiple millions of tasks with these
changes.
Reviewed By: gc3
Reviewers: fratrik, jungejason, aran, tuomaspelkonen, gc3
Commenters: jungejason
CC: anjali, aran, epriestley, gc3, jungejason
Differential Revision: 534
images correctly
Summary:
This is sort of doing two things at once:
- Add an "isOwner" flag to Project Affiliation to lay the groundwork for T237.
- Rename the "QuickCreate" workflow to "Create" and funnel all creation
through it.
- Reorganize the image transformation stuff and use it to correctly
crop/resize uploaded images.
Test Plan:
Created and edited projects and affailiations. Uploaded project, user, and
profile photos. Verified existing thumbnailing in Maniphest still works
properly.
Reviewed By: cadamo
Reviewers: cadamo, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, cadamo
Differential Revision: 529
Summary:
D477 added functionality to the project list view but had a couple of
performance issues that I missed in review, because it took the query count for
the page from around 3 to as many as 300, including up to 100 heavyweight search
index queries.
This fixes the two simple N+1 query problems. This general pattern of data
access often occurs:
COUNTEREXAMPLE
$cats = load_cats();
foreach ($cats as $cat) {
$cats_hats = load_hats_for_cat($cat);
// ...
}
But this issues "N+1" queries, i.e. if you load 100 cats you issue 101 queries.
It is faster to group the queries instead:
$cats = load_cats();
$hats = load_all_hats_for_these_cats($cats);
foreach ($cats as $cat) {
$cats_hats = $hats[$cat->getID()];
}
MySQL can execute one query which returns all the results much faster than 100
queries which return one result, especially if the database is not local (i.e.,
over the network).
However, this doesn't save a ton of time. The bigger issue is that I didn't have
the right keys on the relationship tables in the search engine. This adds them,
and reduces the search engine lookup cost from 25-80ms (for
secure.phabricator.com) down to 1-3ms.
I still probably want to get this out of the loop at some point but it's okay
for now and the page loads in a few ms rather than taking more than a second.
Test Plan:
Used "services" tab, "xhprof" and "EXPLAIN" to analyze page performance. I
measured these changes:
- Query count: 1 + (3 * N projects) -> 3 + (N projects) (e.g., 301 -> 103)
- Total time spent querying, ignoring search indexes: 40ms (local.aprhont.com)
-> 20ms (local.aphront.com)
- Cost for search index query: 25-80ms (secure.phabricator.com) -> 1-3ms
Reviewed By: cadamo
Reviewers: cadamo, aran, jungejason, tuomaspelkonen
CC: aran, cadamo, epriestley
Differential Revision: 485
