[Android] Why is DataResource still in master?

[Joe Bowser <bo...@gmail.com>]

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?

Re: [Android] Why is DataResource still in master?

[Andrew Grieve <ag...@chromium.org>]

Not sure where this ended up, but I can add to the motivation behind it:

On Android, there's not only file: URLs, but there are also resource:,
content:, file:///android_asset. To handle these all properly, each plugin
that accepts URLs as a parameter has to do a lot of work, and currently
pretty much no plugins do manage to handle it properly in all cases. This
API is meant to be an attempt centralize the logic of URL -> InputStream.

A side-motivation of DataResource is to allow plugins to intercept resource
loads. This ability is required by AppHarness so that it can multiplex apps.

Implementation-wise, I'd like to see the logic from FileHelpers folded into
DataResource rather than having DataResource call out to FileHelper. This
will reduce the API surface for IO. I'd also like to see the API for
getting a DataResource put into CordovaInterface rather than having to call
a static method.



On Fri, Jun 7, 2013 at 9:55 AM, Braden Shepherdson <br...@chromium.org>wrote:

> 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 <bo...@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 <bo...@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 <
> braden@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 <bo...@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" <br...@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 <bo...@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 <
> mmocny@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 <bo...@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?
> > >>> > > >>
> > >>> > >
> > >>> >
> > >>>
> >
>

Re: [Android] Why is DataResource still in master?

[Braden Shepherdson <br...@chromium.org>]

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 <bo...@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 <bo...@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 <br...@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 <bo...@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" <br...@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 <bo...@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 <mm...@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 <bo...@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?
> >>> > > >>
> >>> > >
> >>> >
> >>>
>

Re: [Android] Why is DataResource still in master?

[Joe Bowser <bo...@gmail.com>]

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 <bo...@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 <br...@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 <bo...@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" <br...@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 <bo...@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 <mm...@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 <bo...@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?
>>> > > >>
>>> > >
>>> >
>>>

Re: [Android] Why is DataResource still in master?

[Joe Bowser <bo...@gmail.com>]

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 <br...@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 <bo...@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" <br...@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 <bo...@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 <mm...@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 <bo...@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?
>> > > >>
>> > >
>> >
>>

Re: [Android] Why is DataResource still in master?

[Braden Shepherdson <br...@chromium.org>]

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 <bo...@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" <br...@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 <bo...@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 <mm...@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 <bo...@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?
> > > >>
> > >
> >
>

Re: [Android] Why is DataResource still in master?

[Joe Bowser <bo...@gmail.com>]

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" <br...@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 <bo...@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 <mm...@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 <bo...@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?
> > >>
> >
>

Re: [Android] Why is DataResource still in master?

[Braden Shepherdson <br...@chromium.org>]

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 <bo...@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 <mm...@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 <bo...@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?
> >>
>

Re: [Android] Why is DataResource still in master?

[Joe Bowser <bo...@gmail.com>]

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 <mm...@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 <bo...@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?
>>

Re: [Android] Why is DataResource still in master?

[Michal Mocny <mm...@chromium.org>]

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 <bo...@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?
>