Summary: Ref T603. This query isn't policy-aware yet, but prepare for it to be one day.
Test Plan: Looked at: home page; differential home; differential detail; diffusion browse. Made differential.query conduit call.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6337
Summary: Ref T603. This is a very old, very bad version of `DifferentialRevisionQuery`. I want to modernize only the latter. Express the remaining callsite of the former in terms of `DifferentialRevisionQuery`.
Test Plan: Executed all four modes of `differential.find`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6335
Summary:
Ref T603.
- Primarily, this gets rid of a `DifferentialRevisionListData` callsite.
- Also modernize and clean up some UI stuff.
Test Plan:
{F48260}
{F48261}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6334
Summary: See discussion in IRC. Not 100% sure what's going on here because of email ghost theives, but conceivably a commit with no changes will end up with `null` changesets instead of `array()` changesets, which throws. Such diffs are certianly possible (`git commit --allow-empty`) even if they aren't the issue in this specific case. See T3416. Initialize changesets to `array()` to avoid throwing.
Test Plan:
Viewed some commits?
iiam
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6339
Summary: Ref T2222. Currently, we load inline comments by `commentID` here, but we always pass every commentID associated with the revision. Instead, just load non-draft comments by revision ID. This simplifies querying a little bit and is likely faster anyway (draft comments are currently loaded separately).
Test Plan: Looked at some revisions and verified inlines showed up correctly and in the right places.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6270
Summary:
Ref T2222. My path forward here wasn't very good -- I was thinking I could set `transactionPHID` for the inline comments as I migrated, but it must be unique and an individual DifferentialComment may have more than one inline comment. Dropping the unique requirement just creates more issues for us, not fewer.
So the migration in D6266 isn't actually useful. Undo it -- this can't be a straight revert because some installs may already have upgraded.
Test Plan: Ran new migrations, verified the world ended up back in the same place as before (made comments, viewed reivsions).
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6269
Summary: Ref T2222. I didn't translate this query properly; reproduce the original.
Test Plan: When viewing a revision with non-draft inline comments by a user other than the viewer, the inline comments now appear on the changesets themselves.
Reviewers: kawakami, btrahan, garoevans
Reviewed By: garoevans
CC: aran, mbishopim3
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6281
Summary: I assume this is a bug!
Test Plan: Look at it
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6279
Summary:
Ref T2222. This adds PHIDs to all Differential comments so I can migrate the inlinecommment table to transaction_comment in the next diff.
@wez, this will issue a few million queries for Facebook (roughly, one for each Differential comment ever made). It's safe to skip the `.php` half of the patch, bring Phabricator up normally, and then apply this patch with Phabricator running if that eases the migration, although the next few diffs will probably be downtime-required migrations so maybe it's easier to just schedule some downtime.
Test Plan: Ran migration locally. Verified existing comments and new comments received PHIDs.
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6266
Summary:
Ref T2222. See D6260.
Push all this junk behind a Query so I can move the storage out from underneath it.
Test Plan: Viewed home page, list view, revision. Made draft, looked at preview, submitted draft, viewed inline, replied to inline.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6262
Summary:
Ref T2222. Ref T1460. Depends on D6260.
This creates the new tables, but doesn't start using them. I added three new fields for {T1460}, to represent fixed/done/replied states.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1460, T2222
Differential Revision: https://secure.phabricator.com/D6261
Summary:
Ref T2222.
I'm thinking about how I want to approach the Asana sync, and I want to try to do T2222 first so that we can build it cleanly on top of ApplicationTransactions. I think we can at least walk down this road a little bit and if it turns out to be scary we can take another approach.
I was generally very happy with how the auth migration turned out (seemingly, it was almost completely clean), and want to pursue a similar strategy here. Basically:
- Wrap the new objects in the old objects for reads/writes.
- Migrate all the existing data to the new table.
- Everything hard is done; move things over a piece at a time at a leisurely pace in lots of smallish, relatively-easy-to-understand changes.
This deletes or abstracts all reads of the DifferentialComment table. In particular, these things are **deleted**:
- The script `undo_commits.php`, which I haven't pointed anyone at in a very long time.
- The `differential.getrevisionfeedback` Conduit method, which has been marked deprecated for a year or more.
- The `/stats/` interface in Differential, which should be rebuilt on Fact and has never been exposed in the UI. It does a ton of joins and such which are prohibitively difficult to migrate.
This leaves a small number of reading interfaces, which I replaced with a new `DifferentialCommentQuery`. Some future change will make this actually load transactions and wrap them with DifferentialComment interfaces.
Test Plan: Viewed a revision; made revision comments
Reviewers: btrahan
Reviewed By: btrahan
CC: edward, chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6260
Summary: Used more logical icons for subscribe, auto, and delete instead of the mail icons. Fixes T3329
Test Plan: Tested subscribing and unsubscribing in Maniphest.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3329
Differential Revision: https://secure.phabricator.com/D6151
Summary:
This diff covers a bit of ground.
- PHUIDocumentExample has been added
- PHUIDocument has been extended with new features
- PhabricatorMenuView is now PHUIListView
- PhabricatorMenuItemView is now PHUIItemListView
Overall - I think I've gotten all the edges covered here. There is some derpi-ness that we can talk about, comments in the code. Responsive design is missing from the new features on PHUIDocument, will follow up later.
Test Plan: Tested mobile and desktop menus, old phriction layout, new document views, new lists, and object lists.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6130
Summary:
This was mentioned in T2928 and nobody objected.
It just references the task instead of fixing it as that would be too aggressive.
It also doesn't check assignee of the task (by purpose).
Test Plan: Created diff from a branch named T2928.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2928
Differential Revision: https://secure.phabricator.com/D5640
Summary:
Kind of a quick look at an idea for T2184
Ref T2184
Test Plan: Make sure the site still loads
Reviewers: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T2184
Differential Revision: https://secure.phabricator.com/D6045
Summary: Added constants to PhabricatorEventType. Modified DifferentialRevisionEditor and DifferentialCommentEditor.
Test Plan:
Created a revision. Edited and made a comment on that revision. It's updating as usual. I think nothing broke may be it's working.
Let me know if I have done it correclty.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2899
Differential Revision: https://secure.phabricator.com/D5869
Summary:
removes the whole custom image thing, instead using a more standard application crumbs. Gives this glorious space back to the compose area which is now tens of pixels taller. Also defaults it to the people widget. Basically, fixes T3160.
For now, you **CAN NOT** edit the title of a conpherence. I didn't want to jam in too much here. Next diff will be to change the widget icons into the dropdown switcher, which will also bring back the editing of titles.
Test Plan: looked at conpherence and it was pretty. Resized it vigorously and it wasn't too bad.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3160
Differential Revision: https://secure.phabricator.com/D5998
Summary:
Fixes T3218.
- Currently, Paste pages don't clear notifications about the paste (notably, token notifications).
- Currently, Paste pages don't show tooltips on tokens.
- `buildApplicationPage()` stopped respecting `pageObjects` (which controls whether "this page has been updated" is shown). Restore that.
- Make `pageObjects` imply "clear notifications on this stuff".
Test Plan: Viewed a tokened Paste. Verified it cleared the notification and hovering over a token showed a tip.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3218
Differential Revision: https://secure.phabricator.com/D5971
Summary:
Ref T2785
Looks for hosts in `conduit.servers` config and if any exist route any conduit calls through any one of the hosts.
Test Plan:
Make some curl calls to public methods (`conduit.ping`), watch the access log for two requests. Make some calls from the UI that require authentication, watch the access log a bit more.
Also ran the unit tests.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2785
Differential Revision: https://secure.phabricator.com/D5970
Summary:
Moves all remaining mail handling into ReplyHandlers.
Farewell, `getPhabricatorToInformation()`! You were a bad method and no one liked you.
Ref T1205.
Test Plan:
- Used test console to send mail to Revisions, Tasks, Conpherences and Commits (these all actually work).
- Used test console to send mail to Requests, Macros, Questions and Mocks (these accept the mail but don't do anything with it, but didn't do anything before either).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5953
Summary: This doesn't do anything, but touches a bunch of files so I split it out to reduce the size of the next diff. Basically, make `MailReceiver` classes responsible for loading their application objects. Ref T1205.
Test Plan: Inspection / next diff / code is not reached.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5941
Summary: Ref T2784. This one was a wee bit complicated. Had to add PhabricatorUser and concept of initFromConduit (or not) to DiffusionRequest.
Test Plan: foreach repo, visited CALLSIGN and clicked a commit and verified they laoded correctly. Hacked code to hit NOT via Conduit and repeated tests to great success.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5928
Summary:
Ref T1205. Continuation of D5915.
Currently, `PhabricatorMetaMTAReceivedMail` has //all// the logic for routing mail. In particular:
- New mail receivers in applications must edit it.
- Mail receivers don't drop out when applications are uninstalled.
Applications have some logic in subclasses of `PhabricatorMailReplyHandler`, but this class is a bit of a mess. It is also heavily based on the assumption that mail receivers are objects (like revisions), but this is not true in at least two cases today (creating new tasks with `bugs@`, creating a new Conpherence thread) and likely other cases in the future (e.g., revision-by-mail).
Move this logic into a new `PhabricatorMailReceiver` classtree. This is similar to `PhabricatorMailReplyHandler` but a bit cleaner and more general. I plan to heavily reduce the responsibilities of `PhabricatorMailReplyHandler` or possibly eliminate it entirely.
For now, the new classtree doesn't do much of interest. The only behavioral change this diff causes is that Phabricator will now reject mail to an application when that application is uninstalled.
I also moved all the `ReplyHandler` classes into `mail/` directories in their respective applications.
Test Plan: Unit tests, used receive test to route mail to various objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: Afaque_Hussain, edward, aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5922
Summary:
Fixes T3151. Javelin treats a behavior without parameters as a global behavior and invokes it only once no matter how many times it is initialized (this is necessarily correct for any reasonable behavior, as the inputs do not vary). A recent patch changed `differential-dropdown-menus` from a zero-argument global behavior to an implicitly nonzero-argument behavior by adding `pht`.
Currently, we initialize the behavior next to dropdown menu creation, so this resulted in `O(N^2)` initializations of the menus. For large diffs, this locks browsers. Instead, initialize outside of the dropdown loop so we ginitialize each menu just once.
Test Plan: Viewed a 2,000 file diff without browser lock.
Reviewers: wez, vrana, btrahan
Reviewed By: wez
CC: aran
Maniphest Tasks: T3151
Differential Revision: https://secure.phabricator.com/D5885
Summary: Fixes the button spacing issue (doesn't seem related to forms?) and moves fonts and sizes over to Helvetica.
Test Plan: Submit many inline comments.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5882
Summary:
This creates a common form look and feel across the site. I spent a bit of time working out a number of kinks in our various renderings. Some things:
- Font Styles are correctly applied for form elements now.
- Everything lines up!
- Selects are larger, easier to read, interact.
- Inputs have been squared.
- Consistant CSS applied glow (try it!)
- Improved Mobile Responsiveness
- CSS applied to all form elements, not just Aphront
- Many other minor tweaks.
I tried to hit as many high profile forms as possible in an effort to increase consistency. Stopped for now and will follow up after this lands. I know Evan is not a super fan of the glow, but after working with it for a week, it's way cleaner and responsive than the OS controls. Give it a try.
Test Plan: Tested many applications, forms, mobile and tablet.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5860
Summary:
Ref T1751. This still doesn't do anything very interesting, but loads the acutal Commit objects that a commit message claims to revert.
The only tricky thing here is that we need to interpret "reverts rnnn" or "reverts nnn" in an SVN repository as "reverts rXnnn", where "X" is the current repository. This adds a method to do allow `DiffusionCommitQuery` to do that.
Test Plan:
Used `reparse.php --message` to reparse several commits with revert language and verify they loaded the correct affected commits.
In an SVN repository, created a commit with ambiguous revert language ("reverts n", "reverts rn", "reverts n, n") and verified it identified the affected commits correctly despite ambiguity.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1751
Differential Revision: https://secure.phabricator.com/D5842
Summary:
Ref T1751. When commit messages include language like "reverts X", parse it. This change doesn't do anything with the commits yet.
I attempted to cover all "natural" VCS messages and all reasonable human variations of these messages.
Test Plan: Added unit tests. Added `var_dump()` and used `scripts/repository/reparse.php --message X` to reparse some commit messages, with expected results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1751
Differential Revision: https://secure.phabricator.com/D5840
Summary:
getDiffDict method returns the JSON for the changesets in a diff
The JSON descriping a changeset didn't containt the changeset ID
in some scenarios, knowing the changeset ID is really useful for the
clients.(i.e. when you want to open a standalone view of the changeset,
you need to know its ID)
Test Plan: existing
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5816
Summary: These should be in (true, false) order, not (false, true) order. This was a transcritpion error in porting the config over.
Test Plan: Edited config from the web, verified the labels matched the underlying values.
Reviewers: tberman, btrahan
Reviewed By: tberman
CC: aran
Differential Revision: https://secure.phabricator.com/D5786
Summary:
We now have the app menu top-right, so we don't need the side navbar anymore
Refs T2014
Test Plan: Visited list view in narrow / wide browser. Could access everything either directly (tables :P) or the top right menus
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D5728
Summary:
Messy POC. I hope I did this one right :P
Refs T2014
Test Plan: Switched browser forcefully into `device-phone`, saw filters in the menu top right. Browsed around Differential in desktop mode, nothing was broken, I think.
Reviewers: epriestley, chad, btrahan
CC: aran, Korvin
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D5693
Summary: Tightens up spacing, remove some of the borders, add alpha channel, make them all blue (sorry, red green and yellow are for 'status'). If we want to do more colors just for hovercards, I have a brown and a black in the mock, but would like to try just blue for now.
Test Plan: UIExamples, Tasks, People, Diffs, and Pastes.
Reviewers: epriestley, AnhNhan, btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5609
Summary:
Also join concepts of installed and enabled applications.
Also respect uninstalled Maniphest where disabled Maniphest was checked.
Test Plan:
Visited T1, D1.
Uninstalled Maniphest then visited T1, D1.
Disabled Maniphest then visited T1.
Visited /config/edit/maniphest.enabled/.
Reviewers: epriestley, Afaque_Hussain, edward
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5602
Summary:
Refs T1048
Adding Differential Hovercard EventListener
Adding People Hovercard EventListener
Adding basic Diffusion hovercard
Adding Conpherence Hovercard EventListener
Test Plan:
Used in a combo with working hovercards. So beautiful.
Also visited test page. Works alright.
awesometown
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1048
Differential Revision: https://secure.phabricator.com/D5576
Summary:
So I don't have to copy/paste everything again.
Used them at places I could find with my limited `grep` skills.
Test Plan: Visited hovercards, revision and tasks. No crashes.
Reviewers: epriestley, btrahan, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5592
Summary:
This sets more reasonable values for the object handle fields imo. It's not like I ever want to find out what letter to use and then do `substr($handle->getType(), 0, 1).$handle->getID()` to get `D1` each time I use handles.
Name:
- D1
- T1
- M1
- P1
- etc.
Fullname:
- D1: Something
- T1: Something
- etc.
In addition, this helps me to reasonable prefill Hovercards in case there is no application-specific event listener.
Also deletes `title` and `alternateID` completely. They deserved that.
Test Plan:
Visited places, nothing broke (We only ever used `$handle->getName()` for users and commits).
Tested mail reply handler. Did not test the other way around, but should be fine.
Hovercards broken until D5572 (would love to induce a cyclic dependency)
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5571
Summary:
Well, I'm just putting it into the DAO classes, or am I doing something wrong?
Will be used by future event listeners
Test Plan: Visited some tasks and revisions. Look fine.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5542
Summary:
This differential implements Phrequent's time tracking
functionality for users and hooks it up to Maniphest. It
also includes a basic "Time Tracked" list for the Phrequent
application, where users can review what they've spent time
working on.
Test Plan:
Apply the patch and track some things in Maniphest. They
should appear in the "Time Tracked" view of Phrequent.
There is also a `phrequent.show-prompt` option which toggles
whether to display a prompt when tracking time. I'm unsure
of whether the prompt is useful or is more likely to cause
people to click "Track Time", go off and do the task and then
come back to the prompt still waiting for them to confirm. A
potential solution to the "accidentally clicking the button
and recording 2 seconds of time" might be to show a prompt
on stop if the total time is under 10 seconds, asking whether
the user wants to keep or discard the tracked time.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2857
Differential Revision: https://secure.phabricator.com/D5479
Summary:
Fixes a weird string in the flag dialog
Migrated to ObjectItemView (Cards)
Removed filters from side nav (unnecessary)
Go away, go away, panel. Nobody will miss you.
Adding sorting/ordering to Flag (separation like in Maniphest ordering still remaining)
Hacking our way in DifferentialRevisionListController thanks to panels (who added `->setFlush()` btw? It's awesome!)
Test Plan: I would not dare to stand here if it did not work.
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5449
Summary:
Ref T2825. Line stats are misleading when multiple commits are associated with a revision. It's better to have no data than misleading/bad data.
We can compute them accurately against commits via Fact/ETL at some point far in the future (see T1562 / T1135).
Test Plan: {F37519}
Reviewers: ptarjan
Reviewed By: ptarjan
CC: aran
Maniphest Tasks: T2825
Differential Revision: https://secure.phabricator.com/D5432
Summary: Not for full review. This makes crumbs appear consistently in mobile. It helps give a quick link to the apps home, the page title currently on, and action icons for the object. It will take additional clean-up to make this consistent across apps. Passing for early review from a UEX perspective. I actually really like it and think onces it's everywhere, helps mobile feel complete.
Test Plan: Testing in iOS and Simulator.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2796
Differential Revision: https://secure.phabricator.com/D5446
Summary: Randomly displaying tips about different features of arc.
Test Plan: Will test by running arc diff and seeing if the tips appear in text-based UI of arc
Reviewers: epriestley, AnhNhan
Reviewed By: AnhNhan
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5351
Summary:
Fixes T2765
Wrong brace placement D:
Test Plan: `/api/differential.getcommitmessage` with some set field
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2765
Differential Revision: https://secure.phabricator.com/D5376
Summary: Especially when doing 'arc diff' containing multiple commits which have pre-filled template fields. Names would pop up multiple times
Test Plan:
{F36314}
Local `arc diff` with some uber-branch also agrees with me
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5371
Summary: Same as the title
Test Plan: Will test locally by running arc diff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan, starruler
Maniphest Tasks: T2663
Differential Revision: https://secure.phabricator.com/D5297
Summary: Fixes T2698. When applications are installed, their Conduit calls should drop out. This will also let us land Releeph without exposing Conduit calls.
Test Plan:
- Viewed Conduit console; uninstalled some applications and verified their calls dropped out.
- Tried to make an uninstalled call; got an appropriate error.
Reviewers: edward, btrahan
Reviewed By: edward
CC: aran
Maniphest Tasks: T2698
Differential Revision: https://secure.phabricator.com/D5302
Summary:
A few things
- pht Maniphest where I could
- implement dust background
- optimize pages for mobile
- adds aphront-two-column-layout
- reworks maniphest page with two column layout
- tweaks task table for mobile, though we should move to object-list-view
Stopping here as I want to get feedback in. Super excited about mobile and the new tasks views. Only sort of excited about the sidebar filters, they need more UI work, but we should talk about that.
Test Plan: Test Maniphest, Differential, and Homepage views. Sort tasks, make reports
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5314
Summary:
I considered also parsing "Depends On:" field in the commit message but it's more involved and I also prefer having this information in the summary where it's more visible.
I also didn't want the field displayed by default so user would have to write "Depends On:" anyway.
Test Plan: Used "Depends on D1" in summary, saw it in dependencies.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5300
Summary: After D5305, this method does nothing since we automatically figure out what we need to do.
Test Plan:
- Viewed a page with the main menu on it (MainMenuView).
- Viewed a revision with transactions on it (TransactionView).
- Viewed timeline UIExample (TimelineView, TimelineEventView).
- Viewed a revision (PropertyListView).
- Viewed a profile (ProfileHeaderView).
- Viewed Pholio list (PinboardView, PinboardItemView).
- Viewed Config (ObjectItemView, ObjectItemListView).
- Viewed Home (MenuView).
- Viewed a revision (HeaderView, CrumbsView, ActionListView).
- Viewed a revision with an inline comment (anchorview).
- Viewed a Phriction diff page (AphrontCrumbsView).
- Filed T2721 to get rid of this.
- Looked at Pholio and made inlines and comments (mockimages, pholioinlinecomment/save/edit).
- Looked at conpherences.
- Browsed around.
Reviewers: chad, vrana
Reviewed By: chad
CC: edward, aran
Differential Revision: https://secure.phabricator.com/D5307
Summary: Allows views to work like tags.
Test Plan: Implemented a few completely arbitrary render() / singleView simplifications. I just picked some that were easy to test. I'll do a more thorough pass on this in a followup; these calls don't really hurt anything.
Reviewers: chad, vrana
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5306
Summary: These actions are dumb, and not smart, and no one likes them.
Test Plan: Looked at a revision and saw fewer actions; higher average action quality.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5304
Summary: Implmented conduit methods from base class in DifferentialDependsOnFieldSpecification
Test Plan: Created two test diffs, made one of them dependent on another. Then checked the dependency over conduit interface for differential.query. Dependency is shown but in terms of PHIDs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5238
Summary: Ported the the loadAuxiliaryfields method from differential.getrevision method to modern conduit differential.query method
Test Plan: Created a test diff in differential to see if nothing has been broken
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5230
Summary:
Provide a viewer to all remarkup engines.
This fixes commit summaries in Diffusion, which were failing to link because they didn't have a user and thus couldn't see/load `D123`, e.g.
Test Plan: Grepped for engine creation.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5152
Summary: I know that this code would be replaced by something else but until then...
Test Plan: Used it in our renderer.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5165
Summary:
Unmuck almost all of the we-sort-of-have-viewers-some-of-the-time mess.
There are a few notable cases here:
- I used Omnipotent users when indexing objects for search. I think this is correct; we do policy filtering when showing results.
- I cheated in a bad way in the Remarkup object rule, but fixing this requires fixing all the PhabricatorRemarkupEngine callsites (there are 85). I'll do that in the next diff.
- I cheated in a few random places, like when sending mail about package edits. These aren't a big deal.
Test Plan:
- Grepped for all PhabricatorObjectHandleData references.
- Gave them viewers.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5151
Summary: This triggers mainly for SVN branch copy.
Test Plan: Ran this code separately.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5126
Summary:
- Unify all the reference/embed Remarkup rules for Differential, Maniphest, Paste and Ponder.
- Add rules for Pholio.
- Does not yet unify Diffusion or Files (both are a bit more involved).
- Prepare for hovercards.
Test Plan: {F33894}
Reviewers: chad, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5120
Summary:
D5120 and followups refactor and generalize object references in Remarkup -- notably, they move remarkup rules from a central location to the implementing applications.
Preserve blame by doing moves/renames only first. This change moves application remarkup rules into those applications, and renames the ones D5120 modifies.
Test Plan: Typed some preview text into a textarea, got a valid Remarkup render.
Reviewers: vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5123
Summary:
We had some null bytes appearing in strings from unit test results, which
caused the PhutilRemarkupEngine to fail at properly generating html for it.
Specifically, the string would get cut off at the null byte, and the closing
</p> tag would never get added. The lack of this tag caused the dom for the
rest of the page to end up inside a hidden td in the unit test results table.
This is a horrible hack of a solution for this - it would be better to fix
PhutilRemarkupEngine (and in the future, strip out null bytes in input in
strings).
Test Plan: load a differential revision and see content after the unittest results
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, rm
Differential Revision: https://secure.phabricator.com/D5065
Summary: This includes assigned tasks in the Maniphest number.
Test Plan: Looked at it.
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D5067
Summary:
Also splits blocking and active revisions.
This could display 0 with non-empty tip over it.
It's intentional meaning that 0 objects need your attention but there is still some work to do.
Test Plan: Hovered over number.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5049
Summary: It's displayed right above it in the breadcrumbs including a link.
Test Plan: Looked at the pages.
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: aran, epriestley, s.o.butler
Differential Revision: https://secure.phabricator.com/D5045
Summary:
If the revision doesn't have reviewers then it's really not waiting on someone else and the author must take an action.
An improvement would be to check if the reviewers are not disabled but that would require loading their handles.
Test Plan:
/
/differential/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, s.o.butler
Differential Revision: https://secure.phabricator.com/D5046
Summary: Mostly applies a new call spacing rule; also a few things that have slipped through via pull requests and such
Test Plan: `find src/ -type f -name '*.php' | xargs -n16 arc lint --output summary --apply-patches`
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5002
Summary: Is this correct for Mercurial?
Test Plan: Saw it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4957
Summary: We are running out of horizontal space, this should help a bit.
Test Plan: Homepage, revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4518
Summary:
- Publish feed/notification.
- I think this is too lightweight for an email?
- We don't tell them which token right now. Laziness? Or intentional aura of mystery?!
- For tasks, notify both author and current owner.
- Fixes T2562.
Test Plan: {F33187}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2562
Differential Revision: https://secure.phabricator.com/D5007
Summary:
Some transactions (like editing configuration values, task descriptions, or Conpherence images) can't be simply explained and need an additional larger element to show them fully (like a text diff).
Support change details like this in ApplicationTransactions. Implements the element in Config, so you can see changes.
Test Plan: {F32974}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2213
Differential Revision: https://secure.phabricator.com/D4984
Summary: These are full of PhutilSafeHTML objects now, which are destroyed by JSON serialization.
Test Plan: Dropped cache, then reloaded pages.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4942
Summary: Assuming this is right?
Test Plan: No more exception error when viewing a revision.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4937
Summary: Sgrepped for `"=~/</"` and manually changed every HTML.
Test Plan: This doesn't work yet but it is hopefully one of the last diffs before Phabricator will be undoubtedly HTML safe.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4927
Summary: I'm too lazy to attaching them for diffs where they were introduced.
Test Plan:
/
/D1, wrote comment with code snippet
DarkConsole
commit detail, wrote comment
task detail, wrote comment
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4911
Summary: `renderChildren()` now returns array which isn't ideal but I prefer it to having two methods.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4910
Summary: They are same because render() returns safe HTML and raw strings are automatically escaped.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4909
Summary:
This is pretty brutal and it adds some `phutil_safe_html()`.
But it is a big step in the right direction.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4905
Summary:
I like this abstraction better.
Result of `phutil_implode_html()` may be also used as a param of `hsprintf()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4904
Summary:
This resolves lots of double escaping.
We changed most of `phutil_render_tag(, , $s)` to `phutil_tag(, , $s)` which means that `$s` is now auto-escaped.
Also `pht()` auto escapes if it gets `PhutilSafeHTML`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4889
Summary:
Lots of killed `phutil_escape_html()`.
Done by searching for `AphrontTableView` and then `$rows` (usually) backwards.
Test Plan:
Looked at homepage.
echo id(new AphrontTableView(array(array('<'))))->render();
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4884
Summary:
Done by searching for `AphrontDialogView` and then `appendChild()`.
Also added some `pht()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4882
Summary: Searched for `AphrontFormView` and then for `appendChild()`.
Test Plan: /login/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4855
Summary: Done by searching for `AphrontErrorView` and then `appendChild()`.
Test Plan:
Looked at Commit Detail.
Looked at Revision Detail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4843
Summary:
This masks possible configuration issues and slightly degrades functionality
with the tradeoff of having differential work when phabricator isn't quite
configured correctly.
Test Plan:
remove directory for a repository, load differential revision from that repo,
and see differential load.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2512
Differential Revision: https://secure.phabricator.com/D4859
Summary:
After D4825, this information is often available to us in a safe way. Provide it explictly.
This removes or reduces functionality in some cases, but I think we can plug those holes with Conpherence addresses and/or explicit user acknowledgement/config.
Test Plan: Patched a commit with `arc patch` and got the original address out.
Reviewers: btrahan, edward, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4828
Summary: do so via event engine. note different order now...
Test Plan: toggled "show beta applications" to off and noted that Conpherence disappeared. Otherwise noted that links showed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2424
Differential Revision: https://secure.phabricator.com/D4708
Summary:
I attempted to test every interface. I probably missed some stuff, but I at least have some level of confidence that the `phutil_tag` branch is fairly stable.
Fixed these issues:
[1] Fixed a Herald issue with object links in transcripts. Some actions return
links; this was previously hard-coded.
[2] DarkConsole refactoring created an issue where the "`" event handler registered too many times.
[3] Fixed a bug where `strlen($value)` was being checked, but fields may now return array(). Possibly we should implement phutil_is_empty_html() or similar.
[4] Fixed a undefined variable issue for image edit transactions.
[5] Fixed an issue with rendering participant transactions. This required phutil_safe_html() because `pht()` can't handle `array()` for `%s`.
[6] Fixed an issue where feed was entirely overescaped by reverting an overly ambitious render_tag -> tag.
[7] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[8] Fixed an issue where • was shown escaped.
[9] Fixed an issue where "no data" was overescaped.
[10] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[11] Fixed an issue with strict tables and inserting `''`.
[12] Fixed an issue with missing space after ":" for mini panels.
Encountered (but did not fix) these issues:
[X1] "e" works incorrectly on comments you are not allowed to edit. Did not fix.
[X2] Coverage currently interacts incorrectly with "--everything" for Phutil tests.
Test Plan:
- Viewed Differential.
- Created a diff via copy/paste.
- Viewed standalone diff.
- Jumped to diff via changeset table.
- Created a revision.
- Updated revision.
- Added a comment.
- Edited revision dependencies.
- Edited revision tasks.
- Viewed MetaMTA transcripts.
- Viewed Herald transcripts [1].
- Downloaded raw diff.
- Flagged / unflagged revision.
- Added/edited/deleted inline comment.
- Collapsed/expanded file.
- Did show raw left.
- Did show raw right.
- Checked previews for available actions.
- Clicked remarkup buttons
- Used filetree view.
- Used keyboard: F, j, k, J, K, n, p, t, h, "?" [2] [X1].
- Created a meme.
- Uploaded a file via drag and drop.
- Viewed a revision with no reviewers.
- Viewed a revision with >100 files.
- Viewed various other revisions [3].
- Viewed an image diff.
- Added image diff inline comments.
- Viewed Maniphest.
- Ran various queries.
- Created task.
- Created similar task.
- Added comments to tasks.
- Ran custom query.
- Saved custom query.
- Edited custom queries.
- Drag-reordered tasks.
- Batch edited tasks.
- Exported tasks to excel.
- Looked at reports (issue in T2311 notwithstanding).
- Viewed Diffusion.
- Browsed Git, SVN, HG repositories.
- Looked at history, browse, change, commit views.
- Viewed audit.
- Performed various audit searches.
- Viewed Paste.
- Performed paste searches.
- Created, edited, forked paste.
- Viewed Phriction.
- Edited a page.
- Viewed edit history.
- Used search typeahead to search for user / application.
- Used search to search for text.
- Viewed Phame.
- Viewed Blog, Post.
- Viewed live post.
- Published/unpublished post.
- Previewed post.
- Viewed Pholio.
- Edited/commented mock.
- Viewed ponder.
- Viewed question.
- Added answer/comment.
- Viewed Diviner.
- Viewed Conpherence [4] [5].
- Made Conpherence updates.
- Viewed calendar.
- Created status.
- Viewed status.
- Viewed Feed [6].
- Viewed Projects.
- Viewed project detail.
- Edited project.
- Viewed Owners.
- Viewed package detail.
- Edited package [7].
- Viewed flags.
- Edited flag.
- Deleted flag.
- Viewed Herald.
- Viewed rules.
- Created rule.
- Edited rule.
- Viewed edit log.
- Viewed transcripts.
- Inspected a transcript.
- Viewed People.
- Viewed list.
- Administrated user.
- Checked username/delete stuff.
- Looked at create/import LDAP/activity logs.
- Looked at a user profile.
- Looked at user about page.
- Looked at Repositories.
- Edited repository.
- Edited arcanist project.
- Looked at daemons.
- Looked at all daemons [8].
- Viewed combined log.
- Looked at configuration.
- Edited configuration.
- Looked at setup issues [9].
- Looked at current settings.
- Looked at application list.
- Installed / uninstalled applications [10].
- Looked at mailing lists.
- Created a mailing list.
- Edited a mailing list.
- Looked at sent mail.
- Looked at received mail.
- Looked at send/receive tests.
- Looked at settings.
- Clicked through all the panels.
- Looked at slowvote.
- Created a slowvote [11].
- Voted in a slowvote.
- Looked at Macro.
- Created a macro.
- Edited a macro.
- Commented on a macro.
- Looked at Countdown.
- Created a Countdown.
- Looked at it.
- Looked at Drydock.
- Poked around a bit.
- Looked at Fact.
- Poked around a bit.
- Looked at files.
- Looked at a file.
- Uploaded a file.
- Looked at Conduit.
- Made a Conduit call.
- Looked at UIExamples.
- Looked at PHPAST.
- Looked at PHIDs.
- Looked at notification menu.
- Looked at notification detail.
- Logged out.
- Logged in.
- Looked at homepage [12].
- Ran `arc unit --everything --no-coverage` [X2].
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4807
Summary:
See D4812.
- This preference disables the file tree completely.
- It defaults off, so users who want it will have to go turn it on.
- Maybe slightly cleaner would be doing this if the tree was hidden and then ajaxing it in if you press "F", but that's complicated and I don't want to bother.
- Generally, I think this element is useful to something like 5% of users and not useful to 95%.
Test Plan: Enabled and disabled file tree. Looked at commits and revisions; verified they reflected the setting correctly.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4813
Summary: This is the most requested feature in FB by far.
Test Plan:
Toggled, verified that data are saved.
Reloaded, toggled, toggled, toggled, verified that data are saved.
Reloaded.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4812
Summary:
We are doing a better job in 1) of D3069#3 and 2) is just dumb.
Let's see if someone notices this change.
Test Plan: /settings/panel/display/
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4789
Test Plan: Ran the migration on a single revision, verified DB, called `loadUnsubscribedPHIDs()`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4786
Test Plan:
Looked at file with lint errors in Diffusion.
I've also tried inline comments in Differential but it failed.
I'll try it again after you land all your diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4755
Summary: Regression to original behavior.
Test Plan: Clicked on it twice, didn't see confirmation dialog.
Reviewers: epriestley, codeblock
Reviewed By: codeblock
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4788
Summary: Also convert to `phutil_tag()`.
Test Plan: Displayed revision with hidden comments.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4772
Summary: It's a little bit confusing that you couldn't use %d but kind of expected taken that the number will be formatted so it's not a number anymore.
Test Plan: /paste/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4764
Summary: Added date created and date modified to diff
Test Plan: Creat a new diff. Check to see that dateCreated and dateModified appear
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4756
Summary: Convert most phabricator_render_form callsites. In the case of the "headsup view", it converts it by deleting the element entirely (this is the very old Maniphest/Differential header which we no longer use).
Test Plan: Poked around a bit.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4726
Summary: Pretty straightforward.
Test Plan: Viewed inline edit on left / right and new /edit.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4724
Summary: I cheated in a couple of places here, but this is in the process of getting refactored anyway, and there's a pretty clear boundary.
Test Plan: Viewed changesets in Differential, viewed standalone. Viewed context elements.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4723
Summary: see title. Thanks for the report @poop
Test Plan: loaded up FF, left a comment but did not submit and instead cancelled - observed sane UI
Reviewers: epriestley, vrana, poop
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2460
Differential Revision: https://secure.phabricator.com/D4728
Summary: Neither author nor reviewer can be a mailing list.
Test Plan: /differential/filter/revisions/, saw "Type a user name".
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4722
Summary: Converts various callsites from render_tag variants to tag variants.
Test Plan: See inlines.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4689
Summary: Added phts, tested forms on mobile.
Test Plan: Review each page in Chrome and iOS.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4702
Summary: Fixes various array_combine() warnings for PHP < 5.4
Test Plan: lint/unit/grep
Reviewers: btrahan, vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4660
Summary:
- Implements `javelin_tag()`, which is `javelin_render_tag()` on top of `phutil_tag()` instead of `phutil_render_tag()`.
- Manually converts all or almost all of the trivial callsites.
Test Plan:
- Site does not seem any more broken than before.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4639
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, pht('...'))
The searched for `<` and `&` by sgrep.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4504
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, '...')
Then searched for `&` and `<` in the output and replaced them.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4503
