cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: [Android] Why is DataResource still in master?
Date Mon, 17 Jun 2013 16:24:18 GMT
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 <braden@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 <bowserj@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 <bowserj@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 <bowserj@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" <braden@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 <bowserj@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 <bowserj@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?
> > >>> > > >>
> > >>> > >
> > >>> >
> > >>>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message