High Barrier For Contributing Patches

87 views
Skip to first unread message

PhistucK

unread,
Dec 15, 2014, 4:00:25 AM12/15/14
to Google Chrome Developer Tools
The barrier for contributing patches is pretty high (checking out the source from git/SVN, getting the depot tools, gclient, Java and more). Since the Developer Tools feature is created with a lot of JavaScript, CSS and HTML, a lot of contributors could help fix issues or implement features easily, if an easy way to contribute would exist, in my opinion.
If you add a GitHub way of contributing code (that eventually submits a patch to the code review site, if this is more comfortable for you), with an online presubmit check (one of you stated that contributing patches without running the presubmit checks generates work and issues for you that could have been prevented) for any pull request (using Travis CI or something, like the Closure Compiler project does), I think a lot more patches would be contributed.

I do not have a high performance computer at the moment (or anything close to it), so downloading the source, depot tools and the rest is difficult for me.

I know I would have contributed (more? a lot more?) patches if I could contribute using the GitHub web interface (or even the Google Code web interface, which has an 'edit file and submit patch' option). Any web interface that does not involve downloading or running anything for the matter would be good.

Thank you for reading!


☆PhistucK

Andrey Lushnikov

unread,
Dec 15, 2014, 9:05:55 AM12/15/14
to google-chrome-...@googlegroups.com
Hi Phistuck,

Developer tools front-end could be checked-out separately from the
rest of chromium, thanks to git support of sparse-checkout [1]. In this case you would not need to have depot tools, the only dependencies here are java and python for the local presubmit checks.

This setup should cover your needs. However, it does not allow to run layout tests - you still would need to download chromium and compile content_shell.

[1] Unofficial blog post with step-by-step guidance: https://medium.com/@aslushnikov/hacking-chrome-devtools-8c8896f5cef3

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-develo...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CABc02_K%2B3ksnSYePOQyWA9EWV2T%3DcwrDJvcxoZ6UrN4ggFK%2BEg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

PhistucK

unread,
Dec 15, 2014, 9:09:51 AM12/15/14
to Google Chrome Developer Tools
​So I should install/run git, Java and Python, instead of changing the source file on GitHub and submitting a pull request.​

Regarding layout tests, try bots runs should cover them.


I am just saying. I would probably be contributing more if this were the case.


☆PhistucK

To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CAJZOyhxbsaDSmS2EO8JR0opga7f-tRA5A5iG0TdAfQzjp_iJcg%40mail.gmail.com.

Alexander Pavlov

unread,
Dec 15, 2014, 9:54:37 AM12/15/14
to Google Chrome Developer Tools
Hey Phistuck,

Your concern is fully understandable. Oftentimes it looks like your single-line patch is going to be trivial and not affect anything but rather fix an obvious bug.

Unfortunately, we have to deal with JavaScript, which is a bit different from C++, Java and other strongly-typed languages that you need to compile before trying out your change (and this gives you a certain bit of confidence in the correctness of your change.) This is not the case with JavaScript. That's why we mandate a Closure compiler presubmit step for all JS-related changes in DevTools (https://code.google.com/p/chromium/issues/detail?id=78368 tracks a similar effort for the Chromium-wide JS resources, and comment #17 there references a bug that could have been avoided, should this compilation framework have been in place at that time.) In https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/google-chrome-developer-tools/wi-K1srFXaw/PonJZ4ChSdkJ I gave a quick overview of what this step does and why it is important to have it in place.

That said, I believe having JRE and Python installed on one's machine is not too big of a burden for someone who wants to contribute to a project of the Chromium size :)


For more options, visit https://groups.google.com/d/optout.


--
-alexander

PhistucK

unread,
Dec 15, 2014, 11:27:42 AM12/15/14
to Google Chrome Developer Tools
I work with JavaScript (and Closure Compiler) all of the time, I am well aware of its intricacies. :)
Which is exactly the reason I mentioned Travis CI, which can actually test every pull request with your entire pre-submit check, including specifically Closure Compiler (I am sure Python is supported as well, though I have not checked), before you accept it as an official code review (in order to let it go through the try bots).

You would be surprised how much of a burden this is for an average web developer, who sometimes only uses Notepad++ (or only the Google Chrome Developer Tools, now that it features a workspace :)).

(Once I buy a fully equipped developer machine, I intend to get the whole Chromium source and to try fixing known issues, but until that happens...)


☆PhistucK

To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CAGg1j-CL5T9xrYPqN%3D_HuMUe8S30nPJYZ2p7VTPbM5HBcahYbw%40mail.gmail.com.

Jacob G

unread,
Dec 16, 2014, 12:21:01 PM12/16/14
to google-chrome-...@googlegroups.com
I gotta say PhistucK is totally right. Most web-developers don't have python to begin with. Maybe Java, because of Minecraft (hehehe) but that's it. On the other hand, as Github is growing and almost every single important (for web-development) open-source project is located there (bootstrap, node.JS and so on). As he mentioned already, for the community it is really easy to get patches up. You fork it, you edit it (and make sure it works), you pull-request. For the most projects that is the same step everytime. Doesn't matter if you contribute to Bootstrap now, or to jQuery (as long as the coding guidelines are met). For the dev-tools, this is completely different. "So wait, where can I get the source?" "What, the WHOLE Chromium source?" "Ah no... I can 'sparse-checkout'. So how does that work? Wait, so I need Git?" "How does this even work from the command line?" (..) That ain*t the end, but now involves a lot of learning. And all that just because you want to fix an issue, which involves a few lines? Most people won't continue researching and will skip to the "Oh well, I wait until the chromium team fixes it".
I understand that it indeed is complicated to "move" / mirror the source with all that auto checks. That doesn't mean it can't be done though. Take a look at Bootstrap, they developed their own pull-request-check-bot. Or ths Travis Cl thing.


☆PhistucK



☆PhistucK

To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.


--
-alexander

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

Alexander Pavlov

unread,
Dec 16, 2014, 1:12:21 PM12/16/14
to Google Chrome Developer Tools
Jacob, PhistucK,

Sorry for the response delay, I had some other urgent stuff on my plate. In brief, both of you are right :) The issues here are of purely technical character. The existing workflow that you are fighting with ;) is THE workflow that ensures the product quality, which includes: (a) the Closure-based JavaScript-related presubmit check, (b) the layout tests run on all relevant platforms before the patch is committed into the repository. While (a) is, I believe, doable on Travis CI without a lot of extra efforts (haven't looked into that yet,) (b) is a lot more complicated and requires LOTS of try bots to run. This is of course not doable via Travis.

Now, imagine that you have filed a pull request and it has passed the (a) step on Travis, the (b) step is still ahead, and there is no easy way to make 

To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-develo...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/b7e2f291-fa71-44ae-af66-57c5048f81cf%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.


--
-alexander

Alexander Pavlov

unread,
Dec 16, 2014, 1:14:13 PM12/16/14
to Google Chrome Developer Tools
[sorry, sent too early]

...to make it work with the official Chromium try bots, which are a prerequisite for a commit into the Chromium or Blink repos. Perhaps, the team can work with GitHub on this, but most likely it will require too much work on both sides.
--
-alexander

PhistucK

unread,
Dec 16, 2014, 1:34:16 PM12/16/14
to Google Chrome Developer Tools
After step (a) is done, you decide whether a given pull request is reasonable (after a code review on GitHub, to vet out initial obvious issues, though this could also be mirrored to Rietveld if necessary). If it is, you convert it (using some tool) into a regular Rietveld issue in order to get try bot coverage just like any change list. Failures can be re[ported back to GitHub, if necessary. The rest is just the regular process on your end.

If you think this is something that will be acceptable, perhaps the community can whip out the solution, instead of Google (though the logic is that Google should really encourage this and invest some, but let us move on from this ;)).


☆PhistucK

Alexander Pavlov

unread,
Dec 16, 2014, 2:17:21 PM12/16/14
to Google Chrome Developer Tools
I love your plan, it sounds fantastic! (in all senses :))

My concerns are if GitHub exposes APIs necessary for it to be feasible (Rietveld has awesome HTTP APIs for automated interaction with depot_tools) and if Google is willing to invest enough resources into it. In fact, everything is bound by the efforts required and resources available (taking the alternative uses of the available resources into account, naturally.)


For more options, visit https://groups.google.com/d/optout.


--
-alexander

PhistucK

unread,
Dec 16, 2014, 2:28:50 PM12/16/14
to Google Chrome Developer Tools
The question is - how integrated do you require this to be with Rietveld?
Must the GitHub pull request be mirrored to Rietveld in its entirety (from the first landed patch), or only when you approve it?
Will there be a generic Rietveld user that will be the author for those mirrored pull requests?

You have not mentioned whether this will be accepted by the Chromium project and whether the community is even in a position to contribute such a flow...


☆PhistucK

To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CAGg1j-Ae8y2sf_mJSxuwo1rWfa%2B%2BdvV5MZsptAc%2Bsqb%3DTYsjcQ%40mail.gmail.com.

Pavel Feldman

unread,
Dec 16, 2014, 3:10:47 PM12/16/14
to google-chrome-...@googlegroups.com
Sorry, I did not have a chance to reply earlier.

Unfortunately, we have too many limitations to make it a reality:

- DevTools is a part of Blink/Chromium. One needs to pass through some formal steps in order to commit into the repo
- Rietveld is the official tool for code reviews, plugging us into the repo.
- There is no established practice for pull requests in the Chromium land so far.
- LayoutTests harness is something we depend on for no good reason (WebKit legacy).

In the ideal world,
- DevTools front-end could be a third party GitHub project that Chrome pulls into the build process
- We could try managing it under Chromium authors license, but in a more GitHub-friendly manner
- We would port it to a reasonable test harness.
- We would manage DEPs rolls for two-sided patches (backend and frontend).

But this all is a very high price to pay for something that is currently being so Chrome-centric. If it was a standalone product / platform / IDE it would totally make sense - it would have many features that don't need to be validated against the backend while developing. But as long as this is a front-end for Chrome backend that needs to be developed and tested against it, it hardly qualifies. I am hopeful that we can evolve it towards this ideal model and DevTools becomes a platform, but it'll take time.

Regards
Pavel

PhistucK

unread,
Dec 16, 2014, 4:10:12 PM12/16/14
to Google Chrome Developer Tools
My proposal does not contradict the current patch submission practice or process, it just supplements it with an additional step at the beginning (a GitHub pull request with a Travis CI that runs the presubmit check that get transformed into Rietveld issue). The rest can happen in Rietveld as usual, for that matter. As far as I know, any user can create a Rietveld issue and anyone with commit rights can approve it and tick the commit checkbox. This does not change, nothing changes in the regular process, really, besides the team having to also look for GitHub pull requests (in case they do not get transformed automatically from the beginning to Rietveld issues, following a presubmit check).


☆PhistucK

To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CAEX3KhasQjv-4z9m6_eXP-a2nbqx9UBS-V53h3tVkqaDoeMVtA%40mail.gmail.com.

Paul Irish

unread,
Dec 16, 2014, 8:04:07 PM12/16/14
to Google Chrome Developer Tools
In love with the ideal world vision above, but agree it's not feasible in the short term.

But, I like where Phistuck is going with this and think there can be a lightweight integration possible.
  • devtools-frontend repo that mirrors that subtree from the chromium git repo
    • master branch is never updated from the Github side, only synced back from original.
  • Pull requests kick off Travis CI work to do our devtools presubmit tests (and maybe the Blink ones)
  • When Travis comes back green, a manual reviewer (e.g. me) applies a "ready-for-rietveld" label if the code is reasonable, it looks good, crbug created, etc.
  • Some automation transforms the PR into a Rietveld issue. 
    • User will need to login to Rietveld with their Google account.
  • Code review happens on Rietveld as normal. 
  • Subsequent commits on the PR can be sent to form new patch sets.
  • Try bots do their thing as normal. We have docs on how to deal with failures.
  • After landing in chromium git it gets synced back up to github master and the PR is closed.
Sounds doable and poking around it appears nobody has built this yet. 

I don't I see us taking this work on by ourselves in the short term, but think it could get explored more with DevTools/Blink/Chromium community help.
Would definitely love to talk more with anyone who's worked with Travis, etc and wants to get their hands dirty. 





☆PhistucK



--
-alexander


--
-alexander

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.


--
-alexander

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

PhistucK

unread,
Dec 17, 2014, 2:25:32 AM12/17/14
to Google Chrome Developer Tools
I am willing to try.


☆PhistucK

To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-develo...@googlegroups.com.
 To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CAK-PPf2dQ5HEduZqe5G2uHNdEoH-NiB%2BF2h1nEz6sWjdf7pYDg%40mail.gmail.com.

Jacob G

unread,
Dec 17, 2014, 9:25:16 AM12/17/14
to google-chrome-...@googlegroups.com

[off topic: Paul, you have a huge open source community as followers on twitter. Can you tweet about it? Maybe we get some additional attention and help-willing people :)]

You received this message because you are subscribed to a topic in the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/google-chrome-developer-tools/5iLsW7T4m90/unsubscribe.
To unsubscribe from this group and all its topics, send an email to google-chrome-develo...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CABc02_JT9YVqRYUyuPQYd5qDom5PqyLo5rBbwMtQozFgaxQcOw%40mail.gmail.com.

Sandip Chitale

unread,
Dec 29, 2014, 1:27:53 PM12/29/14
to google-chrome-...@googlegroups.com
I completely agree with this issue of "High Barrier For Contributing Patches". I am a experienced developer but I am having a lot of trouble submitting a patch for the issue: https://code.google.com/p/chromium/issues/detail?id=441374 

After a quite a bit of struggle I was able to run the 

compile_frontend.py


I had done a sparse checkout but based on the instructions in the linked page above, I need to check out the whole source tree to run the layout tests. I am going to give it a try but frankly I am dreading it.

BTW I have attached the patch to the issue. 


☆PhistucK



☆PhistucK

To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.


--
-alexander

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-developer-tools+unsub...@googlegroups.com.

PhistucK

unread,
Jun 18, 2015, 11:36:21 AM6/18/15
to Google Chrome Developer Tools, Pavel Feldman
Pavel, can you outline and detail your reservations about the plan Paul outlined?

Note that the code review process, including its patchsets, will still be conducted through Rietveld (using the Google account of the author of the patch).
GitHub will only be an easier and more familiar interface for uploading the code into Rietveld, after some preliminary automated compilation and review in GitHub.

What is so different and bad about this, considering that anyone that currently contributes does the same, only using a different and much more heavyweight interface (fetch the entire Chromium or Blink, use gclient and the like)?
It should be transparent to you (after the initial GitHub review, which Paul and others can do), after all.

Thank you.


☆PhistucK

To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-develo...@googlegroups.com.
 To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CAK-PPf2dQ5HEduZqe5G2uHNdEoH-NiB%2BF2h1nEz6sWjdf7pYDg%40mail.gmail.com.

Pavel Feldman

unread,
Jun 18, 2015, 12:34:10 PM6/18/15
to PhistucK, Google Chrome Developer Tools
tl;dr, my concerns are bug tracking, review process and regression testing.

>> When Travis comes back green, a manual reviewer (e.g. me) applies a "ready-for-rietveld" label if the code is reasonable, it looks good, crbug created, etc.

We should not fork the bug repository. It should be clear where the bug goes and it is crbug.com today. Filing a bug does not require building Chromium, so it does not contribute to the high contribution barrier.

>> Some automation transforms the PR into a Rietveld issue.
>> Subsequent commits on the PR can be sent to form new patch sets.

I don't think it is worth it: it is expensive to implement and maintain; I don't think that Rietveld contributes to the high barrier much either, it is in the cloud.

>> Pull requests kick off Travis CI work to do our devtools presubmit tests (and maybe the Blink ones)

This one is the most challenging. DevTools tests are LayoutTest-runner-based. Running them externally against some LKGR build is feasible and is on the critical path to the success of this project. I'd basically start with implementing it and if it succeeds, we can proceed further.

PhistucK

unread,
Jun 18, 2015, 1:36:13 PM6/18/15
to Pavel Feldman, Google Chrome Developer Tools
See my comments inline.


☆PhistucK

On Thu, Jun 18, 2015 at 7:34 PM, Pavel Feldman <pfel...@chromium.org> wrote:
tl;dr, my concerns are bug tracking, review process and regression testing.

>> When Travis comes back green, a manual reviewer (e.g. me) applies a "ready-for-rietveld" label if the code is reasonable, it looks good, crbug created, etc.

We should not fork the bug repository. It should be clear where the bug goes and it is crbug.com today. Filing a bug does not require building Chromium, so it does not contribute to the high contribution barrier.

But no one suggested to fork the bug repository. Where did you get that impression?​
The label is on a pull request (a GitHub patch), not on an issue (a GitHub bug).
Even more, we can explicitly disable the issue section (I just did it - https://github.com/phistuck/chromium-developer-tools-frontend).

 

>> Some automation transforms the PR into a Rietveld issue.
>> Subsequent commits on the PR can be sent to form new patch sets.

I don't think it is worth it: it is expensive to implement and maintain;

​I am willing to implement this automation and maintain it and hopefully, if I do it right (using a public Rietveld API, say), there will basically be nothing to maintain.

I don't think that Rietveld contributes to the high barrier much either, it is in the cloud.

​Of course it contributes to the high barrier (at least it does for me), it (at least, currently) requires a long git clone/pull (I tried to only clone the devtools folder and it took more than an hour instead of a minute). How can you upload patches without it?​
A subtree (as Robert suggested in the other thread) will make it much much shorter, but is it feasible?
Besides, it requires depot_tools whereas GitHub does not require anything other than a browser (you can change files and create pull requests (patches) using a web interface, which I have done many times).
You can even code using a mobile device. ;)

 

>> Pull requests kick off Travis CI work to do our devtools presubmit tests (and maybe the Blink ones)

This one is the most challenging. DevTools tests are LayoutTest-runner-based. Running them externally against some LKGR build is feasible and is on the critical path to the success of this project. I'd basically start with implementing it and if it succeeds, we can proceed further.

I never suggested to run layout tests (though it might be feasible), I only suggested to run compile_frontend.py (well, and the presubmit tests, which do not include layout tests, as far as I know)​.
What prevents anyone from currently cloning the repository and uploading a patch without ever running a single layout test?


When I fixed bugs (or implemented a feature) in the Developer Tools when it was still a part of WebKit, I did not touch layout tests, nor was I asked to touch them (that might have been wrong, of course, but this is how it went).
I fixed those bugs and implemented those features because the barrier was fairly low - I just needed SVN. I did the rest manually (I created a patch using SVN and a changelog entry by hand).

GitHub lowers the barrier even more, which is always welcome when you want to contribute.


If you absolutely require running layout tests (which I am not sure we should, the try bots can run them once the code compiles using Travis CI and is approved in GitHub by Paul and the like and uploaded to Rietveld), I can look into it.



Thank you for taking the time to respond!

Pavel Feldman

unread,
Jun 18, 2015, 2:34:33 PM6/18/15
to PhistucK, Google Chrome Developer Tools
On Thu, Jun 18, 2015 at 8:35 PM, PhistucK <phis...@gmail.com> wrote:
See my comments inline.


☆PhistucK

On Thu, Jun 18, 2015 at 7:34 PM, Pavel Feldman <pfel...@chromium.org> wrote:
tl;dr, my concerns are bug tracking, review process and regression testing.

>> When Travis comes back green, a manual reviewer (e.g. me) applies a "ready-for-rietveld" label if the code is reasonable, it looks good, crbug created, etc.

We should not fork the bug repository. It should be clear where the bug goes and it is crbug.com today. Filing a bug does not require building Chromium, so it does not contribute to the high contribution barrier.

But no one suggested to fork the bug repository. Where did you get that impression?​
The label is on a pull request (a GitHub patch), not on an issue (a GitHub bug).
Even more, we can explicitly disable the issue section (I just did it - https://github.com/phistuck/chromium-developer-tools-frontend).

Disabling it sounds good. As for impression, the manual review process implied that the work could have started with no associated crbug and I thought that it would be based on the GitHub bug.


 

>> Some automation transforms the PR into a Rietveld issue.
>> Subsequent commits on the PR can be sent to form new patch sets.

I don't think it is worth it: it is expensive to implement and maintain;

​I am willing to implement this automation and maintain it and hopefully, if I do it right (using a public Rietveld API, say), there will basically be nothing to maintain.

You might end up using parts of the API that change, but if you are willing to give it a try, who am I to say no. It is just that you'll need to sync comments and patch sets since they could be linkable, etc.


I don't think that Rietveld contributes to the high barrier much either, it is in the cloud.

​Of course it contributes to the high barrier (at least it does for me), it (at least, currently) requires a long git clone/pull (I tried to only clone the devtools folder and it took more than an hour instead of a minute). How can you upload patches without it?​

Not sure if we are talking about the same thing. I meant to reuse the Rietveld against the GitHub mirror. Yes it does require depot_tools, but that does not sound like a high barrier to me comparing to what we have today.
 
A subtree (as Robert suggested in the other thread) will make it much much shorter, but is it feasible?
Besides, it requires depot_tools whereas GitHub does not require anything other than a browser (you can change files and create pull requests (patches) using a web interface, which I have done many times).
You can even code using a mobile device. ;)

 

>> Pull requests kick off Travis CI work to do our devtools presubmit tests (and maybe the Blink ones)

This one is the most challenging. DevTools tests are LayoutTest-runner-based. Running them externally against some LKGR build is feasible and is on the critical path to the success of this project. I'd basically start with implementing it and if it succeeds, we can proceed further.

I never suggested to run layout tests (though it might be feasible), I only suggested to run compile_frontend.py (well, and the presubmit tests, which do not include layout tests, as far as I know)​.
What prevents anyone from currently cloning the repository and uploading a patch without ever running a single layout test?

Hm. So lets say I have a patch ready, but one of the tests fails or flakes with it. It can't be landed, so what do I do? I can't fall back to the heavyweight process of building Chromium - it would defeat the purpose of this project. And this happens with every patch, not mentioning that most of the changes should be adding / changing the tests.



When I fixed bugs (or implemented a feature) in the Developer Tools when it was still a part of WebKit, I did not touch layout tests, nor was I asked to touch them (that might have been wrong, of course, but this is how it went).

Yes, I introduced the testing harness for WebKit inspector when I joined the project, back in 2009. We've been writing tests since then :)
 
I fixed those bugs and implemented those features because the barrier was fairly low - I just needed SVN. I did the rest manually (I created a patch using SVN and a changelog entry by hand).

GitHub lowers the barrier even more, which is always welcome when you want to contribute.


If you absolutely require running layout tests (which I am not sure we should, the try bots can run them once the code compiles using Travis CI and is approved in GitHub by Paul and the like and uploaded to Rietveld), I can look into it.

I wonder if my example above makes you change your mind wrt this one.

PhistucK

unread,
Jun 18, 2015, 4:06:57 PM6/18/15
to Pavel Feldman, Google Chrome Developer Tools
See my comments inline.


☆PhistucK

On Thu, Jun 18, 2015 at 9:34 PM, Pavel Feldman <pfel...@chromium.org> wrote:


On Thu, Jun 18, 2015 at 8:35 PM, PhistucK <phis...@gmail.com> wrote:
See my comments inline.


☆PhistucK

On Thu, Jun 18, 2015 at 7:34 PM, Pavel Feldman <pfel...@chromium.org> wrote:
tl;dr, my concerns are bug tracking, review process and regression testing.

>> When Travis comes back green, a manual reviewer (e.g. me) applies a "ready-for-rietveld" label if the code is reasonable, it looks good, crbug created, etc.

We should not fork the bug repository. It should be clear where the bug goes and it is crbug.com today. Filing a bug does not require building Chromium, so it does not contribute to the high contribution barrier.

But no one suggested to fork the bug repository. Where did you get that impression?​
The label is on a pull request (a GitHub patch), not on an issue (a GitHub bug).
Even more, we can explicitly disable the issue section (I just did it - https://github.com/phistuck/chromium-developer-tools-frontend).

Disabling it sounds good. As for impression, the manual review process implied that the work could have started with no associated crbug and I thought that it would be based on the GitHub bug.

​No GitHub bugs. Everyone is happy. :)​

 


 

>> Some automation transforms the PR into a Rietveld issue.
>> Subsequent commits on the PR can be sent to form new patch sets.

I don't think it is worth it: it is expensive to implement and maintain;

​I am willing to implement this automation and maintain it and hopefully, if I do it right (using a public Rietveld API, say), there will basically be nothing to maintain.

You might end up using parts of the API that change, but if you are willing to give it a try, who am I to say no. It is just that you'll need to sync comments and patch sets since they could be linkable, etc.

Nothing is synchronized back - new GitHub commits get added to the Rietveld issue and that is it. GitHub is just the patch creation tool and nothing else.

Obviously, I will not start working on it before I get your green light regarding the entire plan. :)

I will coordinate my initiative with infra-dev people (or whoever else is in charge of Rietveld) in order to make sure I am using the right one. Of course, APIs can change through the years and things may suddenly break, like with any other system. Hopefully, this will keep these to a minimum.

 


I don't think that Rietveld contributes to the high barrier much either, it is in the cloud.

​Of course it contributes to the high barrier (at least it does for me), it (at least, currently) requires a long git clone/pull (I tried to only clone the devtools folder and it took more than an hour instead of a minute). How can you upload patches without it?​

Not sure if we are talking about the same thing. I meant to reuse the Rietveld against the GitHub mirror. Yes it does require depot_tools, but that does not sound like a high barrier to me comparing to what we have today.

​It is a lower barrier, but if the process I mentioned (everything ends up in Rietveld) is in place, why would we do it?
The lower the barrier is, the more likely contributions are (which is a great goal).

 
 
A subtree (as Robert suggested in the other thread) will make it much much shorter, but is it feasible?
Besides, it requires depot_tools whereas GitHub does not require anything other than a browser (you can change files and create pull requests (patches) using a web interface, which I have done many times).
You can even code using a mobile device. ;)

 

>> Pull requests kick off Travis CI work to do our devtools presubmit tests (and maybe the Blink ones)

This one is the most challenging. DevTools tests are LayoutTest-runner-based. Running them externally against some LKGR build is feasible and is on the critical path to the success of this project. I'd basically start with implementing it and if it succeeds, we can proceed further.

I never suggested to run layout tests (though it might be feasible), I only suggested to run compile_frontend.py (well, and the presubmit tests, which do not include layout tests, as far as I know)​.
What prevents anyone from currently cloning the repository and uploading a patch without ever running a single layout test?

Hm. So lets say I have a patch ready, but one of the tests fails or flakes with it. It can't be landed, so what do I do? I can't fall back to the heavyweight process of building Chromium - it would defeat the purpose of this project. And this happens with every patch, not mentioning that most of the changes should be adding / changing the tests.

​It can be landed - the repository can include layout tests (in the infra-dev thread, I mentioned them as a part of the push).
I just prefer not to mess with the Travis CI regarding running the layout tests, because I feel this is a lot of work when the try bots can do it for us.

 



When I fixed bugs (or implemented a feature) in the Developer Tools when it was still a part of WebKit, I did not touch layout tests, nor was I asked to touch them (that might have been wrong, of course, but this is how it went).

Yes, I introduced the testing harness for WebKit inspector when I joined the project, back in 2009. We've been writing tests since then :)

​It was in 2013. :P
And I support testing, of course.
​
 
 
I fixed those bugs and implemented those features because the barrier was fairly low - I just needed SVN. I did the rest manually (I created a patch using SVN and a changelog entry by hand).

GitHub lowers the barrier even more, which is always welcome when you want to contribute.


If you absolutely require running layout tests (which I am not sure we should, the try bots can run them once the code compiles using Travis CI and is approved in GitHub by Paul and the like and uploaded to Rietveld), I can look into it.

I wonder if my example above makes you change your mind wrt this one.

​I would rather make the process as lightweight as possible in terms of creating the patches...​

Pavel Feldman

unread,
Jun 18, 2015, 5:15:06 PM6/18/15
to PhistucK, Google Chrome Developer Tools
I mean third party CL can't be landed until all layout tests pass. And if there is no way for contributor to run and debug tests, nothing gets landed, ever. It seems like there is some misunderstanding here since my point is so obvious to me. Lets me try to expand on it:

Agreeing on a setup that at least runs all inspector layout tests is critical. For example, I spent half a day today debugging and fixing the test failures my change caused. Not being able to run and debug tests locally would be a show stopper.

Or consider the following change: https://codereview.chromium.org/922213004

- The change is trivial and is improving the tool a lot, see the patch sets #1-#4 for changes to semantics
- By the patch set #4 we are happy with the code, but then bots start yelling at the contributor
- He does trial and error, but inevitably gives up
- Chromium author picks the change, runs the tests, rewrites some, rebaselines other and lands it.

The high barrier of contribution is not defined by ability to fetch entire tree of Blink or Chromium - these are trivial in comparison to being able to run the tests. And that's exactly what we need to focus on. Forking repository and running lint against CLs is easy, unfortunately these do not affect the contribution bar.

PhistucK

unread,
Jun 18, 2015, 5:27:28 PM6/18/15
to Pavel Feldman, Google Chrome Developer Tools
I see. Let me think about it and see what can be done.
If you have any suggestions for running the layout tests that are workable on a Linux machine (the Travis CI environment) without cloning or compiling Chromium, that would be helpful.

Though I still disagree with the last sentence. While some may give up, the bar is still much much lower for at least creating a patch that may be accepted than the current process. Granted, layout tests can be hard or daunting to fix in such a process, but it also could be easy. It really depends on change and on the individual.


☆PhistucK

PhistucK

unread,
Jun 19, 2015, 9:10:36 AM6/19/15
to Pavel Feldman, Google Chrome Developer Tools
(Tell me if you prefer to fork the thread, or just fork it ;))
Can you share some information regarding running the layout tests for the Developer Tools?
What is currently needed for that?
content_shell?

Is there a command line flag that can make the test binary take the files of the Developer Tools from a path?


☆PhistucK

Andrey Lushnikov

unread,
Jun 19, 2015, 9:20:55 AM6/19/15
to google-chrome-...@googlegroups.com, Pavel Feldman
On Fri, Jun 19, 2015 at 4:10 PM PhistucK <phis...@gmail.com> wrote:
(Tell me if you prefer to fork the thread, or just fork it ;))
Can you share some information regarding running the layout tests for the Developer Tools?
What is currently needed for that?
content_shell?
I believe content_shell is the only thing needed.


Is there a command line flag that can make the test binary take the files of the Developer Tools from a path?
No, there's no such thing.
 
 


☆PhistucK

--
You received this message because you are subscribed to the Google Groups "Google Chrome Developer Tools" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-chrome-develo...@googlegroups.com.

PhistucK

unread,
Jun 19, 2015, 12:27:12 PM6/19/15
to Google Chrome Developer Tools, Pavel Feldman
Do you think it would be relatively easy to add such a command line flag?


☆PhistucK

To view this discussion on the web visit https://groups.google.com/d/msgid/google-chrome-developer-tools/CAJZOyhw_so-O3Bm7NWL9_FBF%3DGwttuSKy-9%2BZhNuUM%3DuKkowvg%40mail.gmail.com.

PhistucK

unread,
Jun 23, 2015, 12:39:45 PM6/23/15
to Google Chrome Developer Tools, Andrey Lushnikov, Pavel Feldman
​Do you think it would be relatively easy to add such a command line flag?​

It is currently supported in a way in Chrome, right?
--remote-debugging-port=9222 http://localhost:9222#http://localhost:8000/front_end/inspector.html
Travis CI has Python, so we can use SimpleHTTPServer as suggested in the contributor guide.


☆PhistucK
Reply all
Reply to author
Forward
0 new messages