I'm prepared to have it reverted like it was on the 2.8.x branch. I'll revert the revert in a branch and try to fix them. I'm working with the app harness currently and it depends on the DataResource work, but I don't think it depends crucially on it. Even if it is vital, I could still fix it up in a branch and use that for our app harness work.
It's not hard to revert it and put it back later, so I think we should go with that. I'll push it again when it's actually fixed. I agree with you that the unit tests are a good idea, as well. Joe, can you give me some test cases where it's failing? Is there a bug? I don't have a clear sense of the cases where it crashes or does the wrong thing. Braden On Thu, Jun 6, 2013 at 1:45 PM, Joe Bowser <bows...@gmail.com> wrote: > I'm OK with it staying in there if: > > 1. There's unit tests in the test directory to make sure it works. This > should be unit tested > 2. The unit tests in that directory pass > 3. All the existing MobileSpec tests pass. > > Now, I think that this is going to be tricky, which is why I want it out. > However, I don't know what the final decision on the plugins going in on > 2.9.0 or 3.0.0 is, so I don't know whether we should just suck it up and > fix it or rip it out yet. This code seems like it'd work fine for files, > but not for remote URIs, since that's where we're having the problems. > > Is that cool? > > On Jun 6, 2013 8:00 AM, "Braden Shepherdson" <bra...@chromium.org> wrote: > > > Its intention is to provide a single place for URL handling by plugins. > > Then plugins can capture new schemes, and handle URLs that have been > > modified by other plugins into URLs they know how to handle. It unifies > > that various bits of code in different places that knew how to handle > > gallery URLs and so on. > > > > If we want to revert it on master and only push it when we're sure it's > > working, I can take on the task of rehabilitating it eventually, or it > can > > wait until Andrew is back. It isn't vital to the app harness, though it > > prevents some things from working like app harness-hosted apps accessing > > gallery URLs. > > > > Braden > > > > > > On Wed, Jun 5, 2013 at 11:37 PM, Joe Bowser <bows...@gmail.com> wrote: > > > > > The reverts were only on the 2.8.0 branch, not on master. It's > > > currently totally broken right now. > > > > > > On Wed, Jun 5, 2013 at 8:09 PM, Michal Mocny <mmo...@chromium.org> > > wrote: > > > > 100 yard summary: our intern Shravan from last term was adding this > as > > > part > > > > of his app-harness work. This specific change landed a too hastily > as > > > > there were some issues in corner cases (perhaps over-eagerness due to > > > time > > > > pressure as he approach term end), but all actual uses of > DataResource > > > > should have been reverted before 2.8 branch (right?), and so just > idle > > > code > > > > remains in the codebase. The plan is to fix the remaining issues > > before > > > > re-adding its usage.. but Andrew was working on that, hence the > delay. > > > > > > > > The specifics details of why it has been added / what its used for, I > > > will > > > > defer to some others (Max/Braden?) who would know the answer. > > > > > > > > As far as I am aware, leaving it in isn't harmful, but perhaps > leaving > > it > > > > in unfixed in isn't helpful either. Lets see what Max/Braden say. > > > > > > > > > > > > On Wed, Jun 5, 2013 at 4:54 PM, Joe Bowser <bows...@gmail.com> > wrote: > > > > > > > >> Hey > > > >> > > > >> Why is DataResouce still in master? I don't want this code to go > into > > > >> 2.9.0 or 3.0.0, since I have no idea what this is trying to > > > >> accomplish. I'm going to start ripping it out of master tomorrow if > > > >> someone doesn't tell me why it should still be here. > > > >> > > > >> Seriously, WTF? > > > >> > > > > > >
