scrapy / scurl

Performance-focused replacement for Python urllib
Apache License 2.0
21 stars 6 forks source link

Update chromium source #34

Closed malloxpb closed 6 years ago

malloxpb commented 6 years ago

This PR aims to update the chromium source that SCURL is using

lopuhin commented 6 years ago

@nctl144 I'm getting the same error locally as here on travis: https://travis-ci.org/nctl144/scurl/jobs/405492728 - probably you didn't push some commits to https://github.com/nctl144/url-chromium

malloxpb commented 6 years ago

The build is green now @lopuhin!

codecov[bot] commented 6 years ago

Codecov Report

Merging #34 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #34   +/-   ##
=======================================
  Coverage   88.46%   88.46%           
=======================================
  Files           2        2           
  Lines         312      312           
=======================================
  Hits          276      276           
  Misses         36       36
lopuhin commented 6 years ago

Hey @nctl144 nice job getting it to work! Now that you know about all issues and which bits are required, will it be possible to create another branch in the subrepos that avoids commits like https://github.com/nctl144/url-chromium/commit/df10da12940bb7816a1e15593983f86877bedc43 and https://github.com/nctl144/base-chromium/commit/5c86b03dc04aca309db681682b8d4a8ec317b1f0, and keeps changes to the minimum? The reason is that if upstream changes files that you removed, then you'll get a conflict (it not too hard to resolve, but it's better if there are no such conflicts).

Also I wonder where did https://github.com/nctl144/url-chromium/commit/e07c4bc73baca713fb1508e62016ca23678ded00 come from, is it copied from somewhere? If yes, would be nice to put the source in the commit message and/or the file itself.

Also I'm curious why https://github.com/nctl144/url-chromium/commit/b6abe2b1cbfcc1fa85d7e2e304281f21e1f4f992 was needed? Also I'm a bit worried about https://github.com/nctl144/base-chromium/commit/a61a1f53241b7bc9dc2368bf97c68a8794a3a575

If you prefer, it's possible to defer solving this issues, although if they don't take too much time to solve, it would be better to solve them now IMO :)

I'll review the rest of the PR in a minute.

malloxpb commented 6 years ago

Also I'm a bit worried about nctl144/base-chromium@a61a1f5

Hey @lopuhin , I believe lock functionality is different depends on the OS platform that the users have, and it is not used in any of the functions that GURL components have..

Also I'm curious why nctl144/url-chromium@b6abe2b was needed?

This is not needed but I commented it out to make it easier to get which source .cc files I needed to include (it help narrow down the scope a little bit), but I will put it back now!

I think I'm going to make a new branch which has all the original files and only make changes to the files that we need, if that's okay :) The submodules are kinda messy right now indeed...

lopuhin commented 6 years ago

I think I'm going to make a new branch which has all the original files and only make changes to the files that we need, if that's okay :)

Yeah, that would be perfect! 👍

malloxpb commented 6 years ago

Hey @lopuhin , I just updated the submodules so they have the same structure with the upstream. I just modified the files that we need :) I will work on updating the doc now.

In addition, the scurl branch of those 2 submodules are the old branches, while the new branches that we officially use are named scurl_clean...

lopuhin commented 6 years ago

Thanks @nctl144 , that's massively cleaner and better now! Here are the links I used to check the changes: https://github.com/nctl144/url-chromium/compare/upstream...scurl_clean and https://github.com/nctl144/base-chromium/compare/master...scurl_clean

malloxpb commented 6 years ago

I just changed the branch names to make it less confusing... I changed the offical branches that we are using to 'master' and the upstream branch to 'upstream'. So the links are going to be: nctl144/url-chromium@upstream...master and nctl144/base-chromium@upstream...master

malloxpb commented 6 years ago

Hey @lopuhin , I will merge this PR now and I will create an issue for the tox problem on Mac OS that I found :)