Summary:
Ref T9252. Long ago you sometimes manually created resources, so they had human-enterable names. However, users never make resources manually any more, so this field isn't really useful any more.
In particular, it means we write a lot of untranslatable strings like "Working Copy" to the database in the default locale. Instead, do the call at runtime so resource names are translatable.
Also clean up a few minor things I hit while kicking the tires here.
It's possible we might eventually want to introduce a human-choosable label so you can rename your favorite resources and this would just be a default name. I don't really have much of a use case for that yet, though, and I'm not sure there will ever be one.
Test Plan:
- Restarted a Harbormaster build, got a clean build.
- Released all leases/resources, restarted build, got a clean build with proper resource names.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14213
Summary:
Ref T9252. Several general changes here:
- Moves logs to use PHIDs instead of IDs. This generally improves flexibility (for example, it's a lot easier to render handles).
- Adds `blueprintPHID` to logs. Although you can usually figure this out from the leasePHID or resourcePHID, it lets us query relevant logs on Blueprint views.
- Instead of making logs a top-level object, make them strictly a sub-object of Blueprints, Resources and Leases. So you go Drydock > Lease > Logs, etc., to get to logs.
- I might restore the "everything" view eventually, but it doesn't interact well with policies and I'm not sure it's very useful. A policy-violating `bin/drydock log` might be cleaner.
- Policy-wise, we always show you that logs exist, we just don't show you log content if it's about something you can't see. This is similar to seeing restricted handles in other applications.
- Instead of just having a message, give logs "type" + "data". This will let logs be more structured and translatable. This is similar to recent changes to Herald which seem to have worked well.
Test Plan:
Added some placeholder log writes, viewed those logs in the UI.
{F855199}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14196
Summary: Updates to their new logo
Test Plan: review in photoshop
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14199
Summary: I haven't regenerated this for a while and it makes instances and unit tests a little faster.
Test Plan:
- Manually reviewed changes for sanity.
- Ran `arc unit --everything`.
- Observed runtime drop from ~15-16 seconds to ~12-13 seconds.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14192
Summary:
Fixes T6569. This implements an expiry mechanism for Drydock resources which parallels the mechanism for leases.
A few things are missing that we'll probably need in the future:
- An "EXPIRES" command to update the expiration time. This would let resources be permanent while leased, then expire after, say, 24 hours without any leases.
- A callback like `shouldActuallyExpireRightNow()` for resources and leases that lets them decide not to expire at the last second.
- A callback like `didAcquireLease()` for resource blueprints, to parallel `didReleaseLease()`, letting them clear or extend their timer.
However, this stuff would mostly just let us tune behaviors, not really open up new capabilities.
Test Plan: Changed host resources to expire after 60 seconds, leased one, saw it vanish 60 seconds later.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T6569
Differential Revision: https://secure.phabricator.com/D14176
Summary: Ref T9252. If you have a blueprint and you do not like that blueprint very much, you can disable it.
Test Plan: Disabled / enabled some blueprints.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14156
Summary:
Ref T9252. Drydock currently uses integer statuses, but there's no reason for this (they don't need to be ordered) and it makes debugging them, working with them, future APIs, etc., more cumbersome.
Switch to string instead.
Also rename `STATUS_OPEN` to `STATUS_ACTIVE` and `STATUS_CLOSED` to `STATUS_RELEASED` for consistency. This makes resources and leases have more similar states, and gives resource states more accurate names.
Test Plan: Browsed web UI, grepped for changed constants, applied patch, inspected database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14153
Summary:
Ref T9252. Leases currently have a `resourceID`, but this is a bit nonstandard and generally less flexible than giving them a `resourcePHID`.
In particular, a `resourcePHID` is easier to use when rendering interfaces, since you can get handles out of a PHID.
Add a PHID column, copy over all the PHIDs that correspond to existing IDs, then drop the ID column.
Test Plan:
- Browsed web UIs.
- Inspected database during/after migration.
- Grepped for `resourceID`.
- Allocated a new lease with `bin/drydock lease`.
Reviewers: chad, hach-que
Reviewed By: hach-que
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14151
Summary:
Ref T9464. If an ancient transaction doesn't have array values for whatever reason, we fail here.
Instead, just recover as gracefully as we can. We may get the transaction "wrong" in some sense, but this only impacts what is rendered in the transaction log.
Test Plan: This is nearly a year old and there's no real way to test it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9464
Differential Revision: https://secure.phabricator.com/D14149
Summary:
Ref T9252. This simplifies some Drydock code.
Most of this code relates to the old notion of Drydock being able to enumerate all the tasks it needs to complete in order to acquire a lease. The code has stepped back from this, since it's unnecessary, the queue is more powerful than it used to be, and it would be a lot of work to keep track of.
The ~only thing that should ever wait for leases in modern code is `bin/drydock lease`, and it's fine for it to just sit there sleeping, so this just does that.
This reduces the granularity of logging, but I'll address that separately in future logging-focused changes.
Test Plan: Used `bin/drydock lease` to acquire a lease, saw it acquire cleanly.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14147
Summary:
Ref T9252. Broadly, Drydock currently races on releasing objects from the "active" state. To reproduce this:
- Scatter some sleep()s pretty much anywhere in the release code.
- Release several times from web UI or CLI in quick succession.
Resources or leases will execute some release code twice or otherwise do inconsistent things.
(I didn't chase down a detailed reproduction scenario for this since inspection of the code makes it clear that there are no meaningful locks or mechanisms preventing this.)
Instead, add a Harbormaster-style command queue to resources and leases. When something wants to do a release, it adds a command to the queue and schedules a worker. The workers acquire a lock, then try to consume commands from the queue.
This guarantees that only one process is responsible for writes to active resource/leases.
This is the last major step to giving resources and leases a single writer during all states:
- Resource, Unsaved: AllocatorWorker
- Resource, Pending: ResourceWorker (Possible rename to "Allocated?")
- Resource, Open: This diff, ResourceUpdateWorker. (Likely rename to "Active").
- Resource, Closed/Broken: Future destruction worker. (Likely rename to "Released" / "Broken"; maybe remove "Broken").
- Resource, Destroyed: No writes.
- Lease, Unsaved: Whatever wants the lease.
- Lease, Pending: AllocatorWorker
- Lease, Acquired: LeaseWorker
- Lease, Active: This diff, LeaseUpdateWorker.
- Lease, Released/Broken: Future destruction worker (Maybe remove "Broken"?)
- Lease, Expired: No writes. (Likely rename to "Destroyed").
In most phases, we can already guarantee that there is a single writer without doing any extra work. This is more complicated in the "Active" case because the release buttons on the web UI, the release tools on the CLI, the lease requestor itself, the garbage collector, and any other release process cleaning up related objects may try to effect a release. All of these could race one another (and, in many cases, race other processes from other phases because all of these get to act immediately) as this code is currently written. Using a queue here lets us make sure there's only a single writer in this phase.
One thing which is notable is that whatever acquires a lease **can not write to it**! It is never the writer once it queues the lease for activation. It can not write to any resources, either. And, likewise, Blueprints can not write to resources while acquiring or releasing leases.
We may need to provide a mechinism so that blueprints and/or resource/lease holders get to attach some storage to resources/leases for bookkeeping. For example, a blueprint might need to keep some kind of cache on a resource to help it manage state. But I think we can cross that bridge when we come to it, and nothing else would need to write to this storage so it's technically straightforward to introduce such a mechanism if we need one.
Test Plan:
- Viewed buttons in web UI, checked enabled/disabled states.
- Clicked the buttons.
- Saw commands show up in the command queue.
- Saw some daemon stuff get scheduled.
- Ran CLI tools, saw commands get consumed and resources/leases release.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14143
Summary:
See discussion in D10304. There's a lot of context there, but the general idea is:
- Blueprints should manage locks in a granular way during the actual allocation/acquisition phase.
- Optimistic "slot locks" might a pretty good primitive to make that easy to implement and reason about in most cases.
The way these locks work is that you just pick some name for the lock (like the PHID of a resource) and say that it needs to be acquired for the allocation/acquisition to work:
```
...
->needSlotLock("mylock(PHID-XYZQ-...)")
...
```
When you fire off the acquisition or allocation, it fails unless it could acquire the slot with that name. This is really simple (no explicit lock management) and a pretty good fit for most of the locking that blueprints and leases need to do.
If you need to do limit-based locks (e.g., maximum of 3 locks) you could acquire a lock like this:
```
mylock(whatever).slot(2)
```
Blueprints generally only contend with themselves, so it's normally OK for them to pick whatever strategy works best for them in naming locks.
This may not work as well if you have a huge number of slots (e.g., 100TB you want to give out in 1MB chunks), or other complex needs for locks (like you have to synchronize access to some external resource), but slot locks don't need to be the only mechanism that blueprints use. If they run into a problem that slot locks aren't a good fit for, they can use something else instead. For now, slot locks seem like a good fit for the problems we currently face and most of the problems I anticipate facing.
(The release workflows have other race issues which I'm not addressing here. They work fine if nothing races, but aren't race-safe.)
Test Plan:
To create a race where the same binding is allocated as a resource twice:
- Add `sleep(10)` near the beginning of `allocateResource()`, after the free bindings are loaded but before resources are allocated.
- (Comment out slot lock acquisition if you have this patch.)
- Run `bin/drydock lease ...` in two windows, within 10 seconds of one another.
This will reliably double-allocate the binding because both blueprints see a view of the world where the binding is free.
To verify the lock works, un-comment it (or apply this patch) and run the same test again. Now, the lock fails in one process and only one resource is allocated.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Differential Revision: https://secure.phabricator.com/D14118
Summary: These already have a larger font, the extra height isn't needed. Make them the same size as `td`
Test Plan: Look at a bunch of tables.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14135
Summary: These currently use different yellows, and more importantly when pygmentized, is hard to read. This lightens up code embeds and pastes.
Test Plan:
Review Diviner docs, Pastes, and normal code embeds.
{F820471}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: tycho.tatitscheff, Korvin
Differential Revision: https://secure.phabricator.com/D14123
Summary: Builds a container of paste, makes it smaller on mobile.
Test Plan: View on desktop, tablet, mobile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14122
Summary: Wraps entire element in the anchor tag, gives a hover state, makes icons bounce.
Test Plan: Hover and click.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14124
Summary: Adds full ROYGBIVP color spectrum, adds basic overflow, collapse protection.
Test Plan: Review small and large panels are various breakpoints.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14120
Summary: Making these a little more fun, a little more flexible and better looking. Will have an update for rSAAS in a bit.
Test Plan:
Make lots of them. Click.
{F815658}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14115
Summary:
Ref T7785. This prepares for (but does not yet use) a pure PHP implementation of Figlet parsing and rendering.
Figlet is somewhat complex, but a parser already exists in PEAR. I'll make sure it's suitable and hook it up in the next diff.
Test Plan: N/A, code not reachable
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9408, T7785
Differential Revision: https://secure.phabricator.com/D14101
Summary:
Ref T7785. Convert the Cowsay Remarkup rule to use a PHP implementation so we don't have to execute an external `cowsay` binary.
I removed some of the default ".cow" files that come with Cowsay because they:
- include Perl code which we can not interpret; or
- are primarily in-jokes or standalone visual puns or artwork rather than usable actors on the grand stage of cowsay; or
- offended my delicate sensibilities.
Users can add new cows to `resources/cows/custom/` if they want to make new cows available.
I have included a majestic original artwork depicting the "Companion Cube" character from //Portal//.
Test Plan: {F802535}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9408, T7785
Differential Revision: https://secure.phabricator.com/D14100
Summary:
This reverts commit 1583738842.
See T8646 for discussion. This version of the feature feels terrible on real data.
Test Plan: Strict revert.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14097
Summary:
Fixes T9126. In particular:
- Add "Browse" links to all history views.
- Use icons to show "Browse" and "History" links, instead of text.
- Use FontAwesome.
- Generally standardize handling of these elements.
This might need a little design attention, but I think it's an improvement overall.
Test Plan:
- Viewed repository history.
- Viewed branch history.
- Viewed file history.
- Viewed table of contents on a commit.
- Viewed merged changes on a merge commit.
- Viewed a directory containing an external.
- Viewed a deleted file.
{F788419}
{F788420}
{F788421}
{F788422}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9126
Differential Revision: https://secure.phabricator.com/D14096
Summary:
Ref T8646. This is fairly rough:
This interface is very niche, and not really flexible enough to accommodate other result customization (but I don't think we have any plans here)?
I'm just //summarizing// the content of documents, basically showing the first paragraph of their content, summary, etc. This isn't what Google does: it shows snippets surrounding the actual search terms. However, this is more involved and might be less useful in structured data: for example, I'd imagine that the first line of most phriciton documents, maniphest tasks and Differential revisions really might be the best machine-generatable summary of them. The actual contextual snippeting in Google doesn't often seem hugely useful to me. But this might also not be very useful.
There's not much design, not sure if you had any ideas.
I only implemented this for tasks, revisions and the wiki since those seem most useful.
I'm generally on the fence about this, but it's not a ton of work to swap out for something else later. Maybe we can see how it feels? But happy to toss it or rethink the approach.
Test Plan: {F788026}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8646
Differential Revision: https://secure.phabricator.com/D14095
Summary: Fixes T9351. This is straightforward since this application is now relatively modern and doesn't have any bizarre craziness.
Test Plan:
{F787981}
{F787982}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9351
Differential Revision: https://secure.phabricator.com/D14093
Summary: Fixes T8572. Ideally we would probably just permit this, but clean up the behavior until the day arrives when inline code is actually rewritten.
Test Plan:
- Tried to launch editors in Differential and Diffusion while comments were already open.
- Verified that "Jump to inline" works in both cases.
{F788008}
{F788009}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8572
Differential Revision: https://secure.phabricator.com/D14094
Summary:
Ref T9324. When you upload a normal file, we call `willUpload` and then `didUpload`.
When you upload a chunked file, we never call `willUpload`. This can get things out of sync. Make sure we invoke this event for both chunked and non-chunked uploads.
Test Plan: Got cleaner behavior (redirect after all uploads finish) in new Phacility file upload area.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9324
Differential Revision: https://secure.phabricator.com/D14083
Summary:
- Fix missing space before "For example:".
- Fix instruction to run `bin/config set value` instead of `bin/config set key value`.
- Minor cleanup.
Test Plan: Tried to set `files.image-mime-types`, `load-libraries`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14080
Summary: This is required by Aphront now but not given a default implementation in the base class.
Test Plan: CORGI sites now work.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14079
Summary: Tables headers break all kinds of funky even though we set `word-break` to word presumably because width is not calculated. Just use normal breaks.
Test Plan: Test developer documentation table in sand box, table is readable on desktop, phone break points.
Reviewers: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14063
Summary:
One of the migrations in rPa335004a91 (`20150730.herald.5.sql`) incorrectly swapped "add" and "add blocking" Differential Herald rules.
Swap any rules last modified before this patch was applied back. This is the best we can do without possibly overwriting more recent, intentional data. I'll issue some guidance on this in the changelog.
Test Plan:
- Made a rule, ran patch, no change.
- Changed rule modified time to a few months ago, ran patch, saw swap from non-blocking to blocking.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14061
Summary:
Ref T7148. I can do most of the export stuff by only modifying the Instances codebase, but want to upload all the backups and exports as temporary files and can't currently do this via the API.
Make the necessary API changes so that the export workflow can use them when it gets built out.
Test Plan: See next diff. Uploaded files with `arc upload --temporary` and saw them upload as temporary files.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7148
Differential Revision: https://secure.phabricator.com/D14055
Summary: There are a handful of places I've been wanting to use a button here. Adds that ability and uses in app launcher.
Test Plan:
Test Applicatons->Launcher at desktop, mobile, tablet breakpoints
{F780453}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14059
Summary:
Fixes T9339.
- Don't show edit control for locked config at all.
- Don't show a "Cancel" button either.
- Change "Value" label to "Database Value" for non-custom config.
- Highlight effective value.
- Move examples under current state.
- Tweak some formatting.
Test Plan: {F777878}
Reviewers: chad, avivey
Reviewed By: chad, avivey
Subscribers: avivey
Maniphest Tasks: T9339
Differential Revision: https://secure.phabricator.com/D14054
Summary: Fixes T9314. Functionally phui-status-list should get moved off a table, but that's another day. This catches many other possible issues.
Test Plan: Review changes on a narrow browser.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9314
Differential Revision: https://secure.phabricator.com/D14036
Summary: Fixes T9264. I'm surprised this hasn't come up before, but any long string or URL in remarkup will overflow and cause remarkup areas to scroll. Prefer breaking these words.
Test Plan:
Review a timeline feed in Differential and a Ponder answer.
{F768105}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9264
Differential Revision: https://secure.phabricator.com/D14018
Summary: Makes the New Comment, See Comments more obviously placed to find.
Test Plan: Review new CSS, answer question, comment, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14043
Summary:
Ref T9309. This is a minor quality of life improvement, hopefully. We already have print CSS, just expose it more clearly.
Also, hide actions (these never seem useful?) and footers from printable versions. I opened the printable version in a new window since it now doesn't have any actions.
Test Plan:
{F777241}
{F777242}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9309
Differential Revision: https://secure.phabricator.com/D14045
Summary:
Fixes T9302. This datasource wasn't resolving package PHIDs correctly for the actual query.
Also fixes an issue with the "Affected packages that need audit" Herald rule.
Test Plan: Ran a "Needs Audit" query with only packages, and only `packages(user)`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9302
Differential Revision: https://secure.phabricator.com/D14029
Summary: Fixes T8901 by adding in additional colors used by icons. Plus fire. Fire is cool.
Test Plan: Try out new colors in maniphest priorities.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8901
Differential Revision: https://secure.phabricator.com/D14020
Summary: Adds an additional field for questions, an answer wiki, should should usually be community editable.
Test Plan: New question, edit question, no wiki, lots of wiki.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14003
Summary: Fixes T9217, adds detection for logged in users and adjusts the layout accordingly.
Test Plan: View logged in and logged out Conpherence
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9217
Differential Revision: https://secure.phabricator.com/D14002
Summary: Until we have a proper close as duplicate workflow for Ponder, remove the option with something more sensible.
Test Plan: Closed a question as invalid, saw it closed and in feed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14007
Summary: Fix T8710. I had hopes of doing something cleaver with `highlighted` (Like trying to understand `foo.bar` when clicking `bar`, but I obviously didn't do it.
Test Plan: ctrl-click.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, epriestley, gena2x, Korvin
Maniphest Tasks: T8710
Differential Revision: https://secure.phabricator.com/D13550
Summary:
Fixes T8501.
When losing focus while holding ctrl, we never get a key-up event; ctrl-f/d/tab make the browser tab lose focus.
So treat 'blur' (unfocus) as if the user released ctrl.
Test Plan: ctrl-f/ctrl-d/ctrl-tab, ctrl-click-outside-of-window, and move mouse over the content - see no help suggestions.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T8501
Differential Revision: https://secure.phabricator.com/D13260
Summary:
Ref T8783. Sort out some relationships and fields:
- Make Items 1:1 with Queues: each item is always in exactly one queue. Minor discussion on T8783. I think this is easier to understand and reason about (and implement!) and can't come up with any real cases where it isn't powerful enough.
- Remove "QueueItem", which allowed items to be in multiple queues at once.
- Remove "dateNuanced", which is equivalent to "dateCreated" in all cases.
Then add really basic routing:
- Add "Default Queue" for Sources. New items from the source route into that queue.
- (Some day there will be routing rules, but for now the rule is "always route into the default queue".)
- Show queue on items.
- Show more / more useful edit history and transactions in several UIs.
Test Plan:
{F749445}
{F749446}
{F749447}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8783
Differential Revision: https://secure.phabricator.com/D13988
Summary: There is still some general buginess with answer comments, trying to work them out. This replaces timeline rendering into one offs (less performant) but resolves many bugs. Or if there is a more performant way, let me know? Also when leaving an answer comment, you currently get redirected back to the page, but both the comment form is still populated and you dont see your answer without a reload. I feel like I'm missing some magical parameter to pass, so just redirecting back to the question itself.
Test Plan: Leave lots of answer comments.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13946
Summary: If we have lots ot `text in monospace` everywhere, the padding bleeds. Reduce 1px.
Test Plan: eyeball it
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13947
