Hi Alexis, Make the chromedriver as component is beautiful solution, not sure it is possible or not, but good to try.
FYI, separating this huge PR into several commits is not what YongSheng and myself are proposing. The proposal is create a separate git repo with files under chrome/test/chromedriver and apply their patches, xwalk use that repo as third_party. The renaming “Chrome” - >”XWalk” is not agreed yet. Compared the chromedirver component on upstream, I take above proposal as short-term solution. Thanks, Halton. From: Crosswalk-dev [mailto:[email protected]] On Behalf Of Alexis Menard Sent: Friday, March 07, 2014 8:37 PM To: Han, YangangX Cc: Wang, Peter H; [email protected]; [email protected] Subject: Re: [Crosswalk-dev] Pull request for implementating xwalk WebDriver is submmited. Hi, On Fri, Mar 7, 2014 at 6:55 AM, Han, YangangX <[email protected]<mailto:[email protected]>> 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. 2 Involve build target in gyp file and make base code to pass compiling 3 To support "InitSession" function for xwalk 4 Remove commands implemented by the Chrome extension. 5 Clear code of interfaces related with Android browser. 6 Add stub class for Tizen xwalk. 7 Remove test scripts that cannot work with xwalkdriver naturally. 8 Clean the code. Thanks, Yangang _____________________________________________ From: Wang, Peter H Sent: Friday, February 21, 2014 5:56 PM To: [email protected]<mailto:[email protected]> Cc: Wu, Jackie; Han, YangangX; Wang, Jing J; Fan, Yugang; Zhang, Belem Subject: Pull request for implementating xwalk WebDriver is submmited. Hi, all, This pull request https://github.com/crosswalk-project/crosswalk/pull/1616 is about to implement WebDriver for xwalk, as approved here as “intent to implement” two months ago. Although it’s a big patch, the logic structure is pretty simple. We’ve provided an introduction in comments of pull-request. And the attached document is also a good summary of why we need xwalkddriver and what we need to do to port code from ChromeDriver. We know it’s not so easy to review a big patch, so we’ve already done a careful coding style check, function verification inside firstly. To every reviewers and developers interested with this topic, please spare some time to help to review it. Thank you very much. << File: Proposal to implement XWalk WebDriver.pdf >> Regards, Peter Wang _______________________________________________ Crosswalk-dev mailing list [email protected]<mailto:[email protected]> https://lists.crosswalk-project.org/mailman/listinfo/crosswalk-dev -- Alexis Menard
_______________________________________________ Crosswalk-dev mailing list [email protected] https://lists.crosswalk-project.org/mailman/listinfo/crosswalk-dev
