|
|
DescriptionChange from_surface default value to true
BUG=714058
Review-Url: https://codereview.chromium.org/2871113002
Cr-Commit-Position: refs/heads/master@{#474207}
Committed: https://chromium.googlesource.com/chromium/src/+/b7b00695911984b3be8f9e7894ac84123136e38e
Patch Set 1 #Patch Set 2 : default fromSurface to true (remove from experimental?) #Patch Set 3 : remove headless fromSurface #
Total comments: 3
Patch Set 4 : added experimental flag #Patch Set 5 : DO_NOT_SUBMIT, set alway surface false #Patch Set 6 : revert to defaulting to true #
Messages
Total messages: 72 (32 generated)
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
dvallet@chromium.org changed reviewers: + eseckler@chromium.org, skyostil@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eseckler@chromium.org changed reviewers: + dgozman@chromium.org
Let's update the comment in browser_protocol.json, too. Otherwise lgtm :) +dgozman for owners
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with Eric's comment.
The plan is to change it to true by default, and eventually remove it. We can try that right now and see whether any bots fail.
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL, webkit layout tests are failing, but I suspect it is because the webkit tree is closed atm
lgtm % nit https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (left): https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name": "fromSurface", "type": "boolean", "optional": true, "description": "Capture the screenshot from the surface, rather than the view. Defaults to false.", "experimental": true } I'd keep "experimental": true, if we want to get rid of this soon.
lgtm https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (left): https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name": "fromSurface", "type": "boolean", "optional": true, "description": "Capture the screenshot from the surface, rather than the view. Defaults to false.", "experimental": true } On 2017/05/11 07:34:18, Eric Seckler wrote: > I'd keep "experimental": true, if we want to get rid of this soon. +1
On 2017/05/11 20:48:05, dgozman wrote: > lgtm > > https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/browser_protocol.json (left): > > https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name": > "fromSurface", "type": "boolean", "optional": true, "description": "Capture the > screenshot from the surface, rather than the view. Defaults to false.", > "experimental": true } > On 2017/05/11 07:34:18, Eric Seckler wrote: > > I'd keep "experimental": true, if we want to get rid of this soon. > > +1 Please also update the description.
Description was changed from ========== Change from_surface default when running in headless mode to true BUG=714058 ========== to ========== Change from_surface default value to true BUG=714058 ==========
The CQ bit was checked by dvallet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, dgozman@chromium.org, eseckler@chromium.org Link to the patchset: https://codereview.chromium.org/2871113002/#ps60001 (title: "added experimental flag")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dvallet@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dvallet@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
So changing the default brakes gpu_tests.pixel_integration_test.PixelIntegrationTest.Pixel_Video_VP9 on Mac. It looks like it taking a screenshot from this code: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... I'm not sure how to change this (I think it might be related to a selenium python library) So changing the default behavior might actually brake a bunch of stuff. My recommendation is going back to having from_surface == true for headless mode and leave as is for non-headless WDYT? https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (left): https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name": "fromSurface", "type": "boolean", "optional": true, "description": "Capture the screenshot from the surface, rather than the view. Defaults to false.", "experimental": true } On 2017/05/11 at 20:48:05, dgozman wrote: > On 2017/05/11 07:34:18, Eric Seckler wrote: > > I'd keep "experimental": true, if we want to get rid of this soon. > > +1 Done
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/12 05:45:21, dvallet wrote: > So changing the default brakes > gpu_tests.pixel_integration_test.PixelIntegrationTest.Pixel_Video_VP9 > on Mac. > It looks like it taking a screenshot from this code: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... I looked at the test failures - perhaps we could just rebaseline? I'll add someone from gpu in the loop. > > I'm not sure how to change this (I think it might be related to a selenium > python library) So changing the default behavior might actually brake a bunch of > stuff. > > My recommendation is going back to having from_surface == true for headless mode > and leave as is for non-headless I don't really like checking for headless in page_handler.cc. Can you make headless always send true as a parameter? > > WDYT? > > https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/browser_protocol.json (left): > > https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name": > "fromSurface", "type": "boolean", "optional": true, "description": "Capture the > screenshot from the surface, rather than the view. Defaults to false.", > "experimental": true } > On 2017/05/11 at 20:48:05, dgozman wrote: > > On 2017/05/11 07:34:18, Eric Seckler wrote: > > > I'd keep "experimental": true, if we want to get rid of this soon. > > > > +1 > > Done
dgozman@chromium.org changed reviewers: + kbr@chromium.org
Ken, could you please take a look at gpu test failures? For more context: it would be really nice to switch screenshots to be taken from surface rather than doing readback (as I understand it). Both DevTools and headless already use this mode, with only telemetry left on the old path. This will allow us to clean up a lot of code and reduce maintenance, but I'm not sure how comfortable you are with this, since it's a semantic change. WDYT? Tests log here: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2... Sample diff here: http://chromium-browser-gpu-tests.commondatastorage.googleapis.com/view_test_...
Description was changed from ========== Change from_surface default value to true BUG=714058 ========== to ========== Change from_surface default value to true BUG=714058 ==========
On 2017/05/12 18:13:49, dgozman wrote: > Ken, could you please take a look at gpu test failures? > > For more context: it would be really nice to switch screenshots to be taken from > surface rather than doing readback (as I understand it). Both DevTools and > headless already use this mode, with only telemetry left on the old path. This > will allow us to clean up a lot of code and reduce maintenance, but I'm not sure > how comfortable you are with this, since it's a semantic change. WDYT? > > Tests log here: > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2... > Sample diff here: > http://chromium-browser-gpu-tests.commondatastorage.googleapis.com/view_test_... The pixel differences seem fine but the semantic change is concerning. The main reason the GPU tests try to read back the on-screen view is that there is a significant amount of platform-specific code which presents to the screen, and it's possible to break this code so that all the browser's tests pass but the browser window is actually rendering black. I think we need to preserve the ability to screenshot the on-screen view, and expand it. jbauman@ found for example that on Windows with Aura it's not actually reading the on-screen view, and he's been working to get that code path to work with DirectComposition. Could we please discuss this more offline?
Looks like we might take a while to decide. Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for headless for now?
On 2017/05/16 23:33:52, dvallet wrote: > Looks like we might take a while to decide. > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for > headless for now? Patch set #2 breaks the Telemetry based tests on the commit queue, so no, it can't be submitted as is. I suggest you change the implementation of this method to ignore the fromSurface argument and treat it as always true if running in headless mode, which you should be able to easily detect from inside Chrome's code. This will avoid changes to the DevTools API.
On 2017/05/16 23:46:55, Ken Russell wrote: > On 2017/05/16 23:33:52, dvallet wrote: > > Looks like we might take a while to decide. > > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for > > headless for now? > > Patch set #2 breaks the Telemetry based tests on the commit queue, so no, it > can't be submitted as is. > > I suggest you change the implementation of this method to ignore the fromSurface > argument and treat it as always true if running in headless mode, which you > should be able to easily detect from inside Chrome's code. This will avoid > changes to the DevTools API. I'd prefer to send |fromSurface=false| from telemetry, and make it true by default. Special-casing headless is not the right way to move forward.
On 2017/05/16 23:57:31, dgozman wrote: > On 2017/05/16 23:46:55, Ken Russell wrote: > > On 2017/05/16 23:33:52, dvallet wrote: > > > Looks like we might take a while to decide. > > > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for > > > headless for now? > > > > Patch set #2 breaks the Telemetry based tests on the commit queue, so no, it > > can't be submitted as is. > > > > I suggest you change the implementation of this method to ignore the > fromSurface > > argument and treat it as always true if running in headless mode, which you > > should be able to easily detect from inside Chrome's code. This will avoid > > changes to the DevTools API. > > I'd prefer to send |fromSurface=false| from telemetry, and make it true by > default. Special-casing headless is not the right way to move forward. OK. https://codereview.chromium.org/2887023002/ uploaded. I'm not sure it's correct.
On 2017/05/17 at 00:18:45, kbr wrote: > On 2017/05/16 23:57:31, dgozman wrote: > > On 2017/05/16 23:46:55, Ken Russell wrote: > > > On 2017/05/16 23:33:52, dvallet wrote: > > > > Looks like we might take a while to decide. > > > > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for > > > > headless for now? > > > > > > Patch set #2 breaks the Telemetry based tests on the commit queue, so no, it > > > can't be submitted as is. > > > > > > I suggest you change the implementation of this method to ignore the > > fromSurface > > > argument and treat it as always true if running in headless mode, which you > > > should be able to easily detect from inside Chrome's code. This will avoid > > > changes to the DevTools API. > > > > I'd prefer to send |fromSurface=false| from telemetry, and make it true by > > default. Special-casing headless is not the right way to move forward. > > OK. https://codereview.chromium.org/2887023002/ uploaded. I'm not sure it's correct. Sorry, I meant Patch #1. But yea happy to wait until crrev/2887023002 is submitted
On 2017/05/17 07:03:50, dvallet wrote: > On 2017/05/17 at 00:18:45, kbr wrote: > > On 2017/05/16 23:57:31, dgozman wrote: > > > On 2017/05/16 23:46:55, Ken Russell wrote: > > > > On 2017/05/16 23:33:52, dvallet wrote: > > > > > Looks like we might take a while to decide. > > > > > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs > for > > > > > headless for now? > > > > > > > > Patch set #2 breaks the Telemetry based tests on the commit queue, so no, > it > > > > can't be submitted as is. > > > > > > > > I suggest you change the implementation of this method to ignore the > > > fromSurface > > > > argument and treat it as always true if running in headless mode, which > you > > > > should be able to easily detect from inside Chrome's code. This will avoid > > > > changes to the DevTools API. > > > > > > I'd prefer to send |fromSurface=false| from telemetry, and make it true by > > > default. Special-casing headless is not the right way to move forward. > > > > OK. https://codereview.chromium.org/2887023002/ uploaded. I'm not sure it's > correct. > > Sorry, I meant Patch #1. But yea happy to wait until crrev/2887023002 is > submitted https://codereview.chromium.org/2887023002/ has rolled in in https://chromium-review.googlesource.com/c/508260 . OK to proceed with this CL (whichever patch set you had in mind). Thanks for waiting.
On 2017/05/18 at 16:39:04, kbr wrote: > On 2017/05/17 07:03:50, dvallet wrote: > > On 2017/05/17 at 00:18:45, kbr wrote: > > > On 2017/05/16 23:57:31, dgozman wrote: > > > > On 2017/05/16 23:46:55, Ken Russell wrote: > > > > > On 2017/05/16 23:33:52, dvallet wrote: > > > > > > Looks like we might take a while to decide. > > > > > > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs > > for > > > > > > headless for now? > > > > > > > > > > Patch set #2 breaks the Telemetry based tests on the commit queue, so no, > > it > > > > > can't be submitted as is. > > > > > > > > > > I suggest you change the implementation of this method to ignore the > > > > fromSurface > > > > > argument and treat it as always true if running in headless mode, which > > you > > > > > should be able to easily detect from inside Chrome's code. This will avoid > > > > > changes to the DevTools API. > > > > > > > > I'd prefer to send |fromSurface=false| from telemetry, and make it true by > > > > default. Special-casing headless is not the right way to move forward. > > > > > > OK. https://codereview.chromium.org/2887023002/ uploaded. I'm not sure it's > > correct. > > > > Sorry, I meant Patch #1. But yea happy to wait until crrev/2887023002 is > > submitted > > https://codereview.chromium.org/2887023002/ has rolled in in https://chromium-review.googlesource.com/c/508260 . OK to proceed with this CL (whichever patch set you had in mind). Thanks for waiting. Thanks!, I'll try to commit the patch
The CQ bit was checked by dvallet@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dvallet@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/19 02:04:35, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) This is http://crbug.com/722246 , but there's even worse flakiness on this tryserver now filed as http://crbug.com/724350 .
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/19 05:48:37, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences. Doesn't this imply that Telemetry's fromSurface=false isn't being respected?
On 2017/05/19 at 17:26:23, kbr wrote: > On 2017/05/19 05:48:37, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences. Doesn't this imply that Telemetry's fromSurface=false isn't being respected? Are you certain that those tests make use of the code changed in https://codereview.chromium.org/2887023002/? My guess is that part of those tests are still not setting the fromSurface command, and thus is defaulting to true.
On 2017/05/22 02:58:47, dvallet wrote: > On 2017/05/19 at 17:26:23, kbr wrote: > > On 2017/05/19 05:48:37, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences. > Doesn't this imply that Telemetry's fromSurface=false isn't being respected? > > Are you certain that those tests make use of the code changed in > https://codereview.chromium.org/2887023002/ > My guess is that part of those tests are still not setting the fromSurface > command, and thus is defaulting to true. Yes, I'm positive these tests are going through that code path in Telemetry. Do you have a Mac and can you reproduce this behavior locally? You can undo all your changes, build Release, delete the following directories: src/content/test/data/gpu/gpu_reference/ src/content/test/data/gpu/generated/ and run: ./content/test/gpu/run_gpu_integration_test.py pixel --browser=release --test-filter=Pixel_Video Then apply your changes, rebuild, and re-run. The tests should hopefully fail the second time. They will be using the files written into gpu_reference/ and write the error images into generated/ .
On 2017/05/22 at 18:45:06, kbr wrote: > On 2017/05/22 02:58:47, dvallet wrote: > > On 2017/05/19 at 17:26:23, kbr wrote: > > > On 2017/05/19 05:48:37, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > > > Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences. > > Doesn't this imply that Telemetry's fromSurface=false isn't being respected? > > > > Are you certain that those tests make use of the code changed in > > https://codereview.chromium.org/2887023002/ > > My guess is that part of those tests are still not setting the fromSurface > > command, and thus is defaulting to true. > > Yes, I'm positive these tests are going through that code path in Telemetry. > > Do you have a Mac and can you reproduce this behavior locally? You can undo all your changes, build Release, delete the following directories: > > src/content/test/data/gpu/gpu_reference/ > src/content/test/data/gpu/generated/ > > and run: > > ./content/test/gpu/run_gpu_integration_test.py pixel --browser=release --test-filter=Pixel_Video > > Then apply your changes, rebuild, and re-run. The tests should hopefully fail the second time. They will be using the files written into gpu_reference/ and write the error images into generated/ . OK, I debugged this locally and I found the problem: The change in https://codereview.chromium.org/2887023002/ is incorrect, instead of request = { 'method': 'Page.captureScreenshot', 'fromSurface': False } It should be request = { 'method': 'Page.captureScreenshot', 'params': { 'fromSurface': False } } I'd sent a change myself but I don't really know how send cls to catapult
On 2017/05/22 at 18:45:06, kbr wrote: > On 2017/05/22 02:58:47, dvallet wrote: > > On 2017/05/19 at 17:26:23, kbr wrote: > > > On 2017/05/19 05:48:37, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > > > Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences. > > Doesn't this imply that Telemetry's fromSurface=false isn't being respected? > > > > Are you certain that those tests make use of the code changed in > > https://codereview.chromium.org/2887023002/ > > My guess is that part of those tests are still not setting the fromSurface > > command, and thus is defaulting to true. > > Yes, I'm positive these tests are going through that code path in Telemetry. > > Do you have a Mac and can you reproduce this behavior locally? You can undo all your changes, build Release, delete the following directories: > > src/content/test/data/gpu/gpu_reference/ > src/content/test/data/gpu/generated/ > > and run: > > ./content/test/gpu/run_gpu_integration_test.py pixel --browser=release --test-filter=Pixel_Video > > Then apply your changes, rebuild, and re-run. The tests should hopefully fail the second time. They will be using the files written into gpu_reference/ and write the error images into generated/ . OK, I debugged this locally and I found the problem: The change in https://codereview.chromium.org/2887023002/ is incorrect, instead of request = { 'method': 'Page.captureScreenshot', 'fromSurface': False } It should be request = { 'method': 'Page.captureScreenshot', 'params': { 'fromSurface': False } } I'd sent a change myself but I don't really know how send cls to catapult
On 2017/05/23 05:03:34, dvallet wrote: > I'd sent a change myself but I don't really know how send cls to catapult It's pretty similar to Chrome actually -- instructions here: https://github.com/catapult-project/catapult/blob/master/CONTRIBUTING.md
On 2017/05/23 16:43:14, Sami wrote: > On 2017/05/23 05:03:34, dvallet wrote: > > I'd sent a change myself but I don't really know how send cls to catapult > > It's pretty similar to Chrome actually -- instructions here: > https://github.com/catapult-project/catapult/blob/master/CONTRIBUTING.md Oops. Thanks dvallet@ for finding this. Fixing in https://codereview.chromium.org/2901153002/ .
On 2017/05/23 at 22:31:06, kbr wrote: > On 2017/05/23 16:43:14, Sami wrote: > > On 2017/05/23 05:03:34, dvallet wrote: > > > I'd sent a change myself but I don't really know how send cls to catapult > > > > It's pretty similar to Chrome actually -- instructions here: > > https://github.com/catapult-project/catapult/blob/master/CONTRIBUTING.md > > Oops. Thanks dvallet@ for finding this. Fixing in https://codereview.chromium.org/2901153002/ . Thanks! Let me know when it's rolled in
The CQ bit was checked by dvallet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, eseckler@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2871113002/#ps100001 (title: "revert to defaulting to true")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495602655877680, "parent_rev": "e296b2b02fe6997bd09faca1b4efb86f3158c90e", "commit_rev": "b7b00695911984b3be8f9e7894ac84123136e38e"}
Message was sent while issue was closed.
Description was changed from ========== Change from_surface default value to true BUG=714058 ========== to ========== Change from_surface default value to true BUG=714058 Review-Url: https://codereview.chromium.org/2871113002 Cr-Commit-Position: refs/heads/master@{#474207} Committed: https://chromium.googlesource.com/chromium/src/+/b7b00695911984b3be8f9e7894ac... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/b7b00695911984b3be8f9e7894ac... |