I'm making progress on fixing the bugs it introduced. We've learned the hard way about branches that revert changes other branches depend on. The right way to branch from this is to branch /after/ the reverts you just added, and revert /them/. Then when we merge this dataresource branch, it won't introduce crazy conflicts.
I'll take care of creating that branch. Braden On Thu, Jun 6, 2013 at 5:39 PM, Joe Bowser <bows...@gmail.com> wrote: > I reverted this, now I'm going to recommend that we fork off of > e518eacbd for fixing this. That being said, things are going to get a > lot uglier post 2.9, so we should probably take a 2.9pre branch and > fix it here with the idea that the plugins have to use this. Does > that make sense? > > On Thu, Jun 6, 2013 at 11:01 AM, Joe Bowser <bows...@gmail.com> wrote: > > I created a new bug, but yeah, it's the Media AutoTests in MobileSpec. > > Debugging shows that they're being fed a URI that is missing a / > > > > https://issues.apache.org/jira/browse/CB-3628 > > > > On Thu, Jun 6, 2013 at 10:57 AM, Braden Shepherdson <bra...@chromium.org> > wrote: > >> 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? > >>> > > >> > >>> > > > >>> > > >>> >
