On 07-03-2014 14:37, Alexis Menard wrote:
Hi,
On Fri, Mar 7, 2014 at 6:55 AM, Han, YangangX <yangangx....@intel.com
<mailto:yangangx....@intel.com>> wrote:
Hi, all,
For the before commit is too huge to review. So after talked with
Halton & Yongsheng.
Peter and I have update the Crosswalk Web Driver
PR(_https://github.com/crosswalk-project/crosswalk/pull/1616_).
Please help to review it. If you have any question, Please comment it.
We split the patch into a series of small patches. Totally commit 8
patches
1 Code base of xwalk WebDriver, copied from ../src/chrome/test/chromed
I haven't look the actual problem in details but hardcopy is a bad. My
quick look is that we're forking ~19000 LOC, this is simply a big
*no-go*. There is no maintenance plan on how to keep that updated with
upstream (for bug and security fixes) and I don't think we should waste
resources maintaining such a big directory.
Experience showed that anything we fork as small as it is is a pain (one
recent tiny example gyp_xwalk which is a fork of gyp_chromium). In some
cases we do fork because it's in our interest and because there is no
other way (see for example Tizen support) but forking an entire
directory is no-go.
This is not the approach we've been taking in Crosswalk up until now. I
myself enforced the rule (with some heated discussions granted) and I
believe it was for the good of the project. We can rebase in a
manageable way still.
I don't think we should use the code in chrome/test/chromedriver as-is.
We do have a motivation on making it live outside the chrome/ directory
then let's make it a component and land it upstream in components/ (or
whatever is more suitable). Identify what are the public interfaces,
what pieces needs to stay in chrome/ and move the rest in components/. I
haven't study the feasibility of this but we really should spend time on
that because it's a far better approach. We've been doing this with
success on StorageMonitor and the NaCl support. We did all the work
upstream and waited it to arrive in a rebase to start using it.
I myself finished what Yael started on NaCl, we could have gone the
forked way as you guys are doing, it was simply unmanageable, Chromium
moves too fast. You're delaying the feature for few months but it's
better than a feature gets delayed than the entire project because we
have a bunch of forked code that we have to deal with at each rebase
(which will most likely break).
Other fundamental mistake in that following approach is that you land in
the git history code depending on chrome/ layer (even in the meantime)
which is something we don't want in Crosswalk.
Again I'm not opposed to the feature, thus I agree with the Intent to
Implement but I disagree with the way it's done.
If you have a look of how the StorageMonitor support is implemented
right now on Crosswalk, it is a hybrid solution.
Currently we are building the StorageMonitor files inside src/chrome as
a component (check the patches on our Chrome tree), but I'm not moving
them to src/components. It is just a buildsystem trick.
Meanwhile, upstream, I did the actual move. I'm expecting an almost
seamless transition next time we do a Chromium rebase and we can get rid
of some custom patches on our tree.
Maybe you can try to do something like that.
Cheers,
_______________________________________________
Crosswalk-dev mailing list
Crosswalk-dev@lists.crosswalk-project.org
https://lists.crosswalk-project.org/mailman/listinfo/crosswalk-dev
