Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

browser(webkit): roll to r266002 08/21/2020 #3561

Merged
merged 1 commit into from
Aug 21, 2020

Conversation

clopez
Copy link
Contributor

@clopez clopez commented Aug 21, 2020

This rebases the playwright patch on top of WebKit r266002

This is a summary of the changes done in the patch:

  • All changes in LayoutTests directory removed (this is not needed)
  • All changes to ChangeLog files removed (also not needed)
  • Changes in Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp removed (already merged upstream)
  • Changes in Tools/Scripts/libraries/webkitcorepy/webkitcorepy removed (already merged a reworked version upstream)

The rest of the changes are minor edits to make the patch apply fine.

I have tested to build it on Linux and run the tests (npm run wtest) with both WPE and GTK. Everything passed fine.
But I have not tested it with Mac or Win.
Upstream CI status for r266002 at build.webkit.org looks good

@clopez
Copy link
Contributor Author

clopez commented Aug 21, 2020

BTW; I understand the CI is testing with the pre-built previous version of webkit, so I guess the failure reported above is unrelated (likely flaky tests on webkit linux)

@yury-s
Copy link
Member

yury-s commented Aug 21, 2020

Thanks for removing all the inadvertent changes we had in changelogs etc!

To make the changes related to rolls more readable we usually attach a diff with the conflict resolution (see e.g. #3289): basically first rebase playwright patch on top of the new WebKit revision, commit all conflicts, resolve the conflicts in a separate git commit and attach a link to that commit to Playwright patch. Could you do the same and attach the diff (unless there were no conflicts)?

BTW; I understand the CI is testing with the pre-built previous version of webkit, so I guess the failure reported above is unrelated (likely flaky tests on webkit linux)

Yes, this is correct. There is no good way yet to test the roll on the CI without actually building it and updating dependency in browsers.json

@pavelfeldman
Copy link
Member

👋 @clopez

In order to review this, we need to see all the conflicts that happened and the way that you resolved them. We basically review the upstream change, not the diff to our big patch. In order to achieve that, we do the following internally:

  • configure 3-way diff via git config --global merge.conflictstyle diff3
  • create a branch based off playwright-build, say r266002
  • rebase on r266002
  • accept all the conflicts w/o resolving them. i.e. git add everything and git rebase --continue, landing all of the <<<, ===, >>>
  • fix all the conflicts and make all the necessary changes in a new commit, name it browser(webkit): roll to r266002
  • push this branch to your webkit github fork git push origin.
  • paste a link to the last commit in your r266002 branch in github into this change.

The we look at it and accept your change.

@clopez
Copy link
Contributor Author

clopez commented Aug 21, 2020

Thanks for the clear instructions!

Here is the link to my branch with the confict-resolution patch https://github.com/clopez/webkit/commits/playwright-build-rebase-r266002

@yury-s yury-s merged commit de5ecc0 into microsoft:master Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants