Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
346703002:
Small cleanups in automation_node.js. (Closed)
|
Created:
6 years, 6 months ago by David Tseng Modified:
6 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionSmall cleanups in automation_node.js.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281892
Patch Set 1 #Patch Set 2 : Nuke todo #
Total comments: 7
Patch Set 3 : TODO #Patch Set 4 : #
Total comments: 1
Patch Set 5 : setTimeout. #Patch Set 6 : Rebase. #
Messages
Total messages: 31 (0 generated)
lgtm
https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:257: throw new Error('Load is currently not supported.'); I don't know if we want to throw an Error here - can we just make it a no-op for now instead?
https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:257: throw new Error('Load is currently not supported.'); On 2014/06/30 21:57:54, aboxhall wrote: > I don't know if we want to throw an Error here - can we just make it a no-op for > now instead? I don't know if we want to fail/no-op silently; the error is mostly to let the caller know they shouldn't be calling. Would be open to some other way of indicating this to the caller.
https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:257: throw new Error('Load is currently not supported.'); On 2014/06/30 23:02:24, David Tseng wrote: > On 2014/06/30 21:57:54, aboxhall wrote: > > I don't know if we want to throw an Error here - can we just make it a no-op > for > > now instead? > > I don't know if we want to fail/no-op silently; the error is mostly to let the > caller know they shouldn't be calling. Would be open to some other way of > indicating this to the caller. I don't know if we want to train people not to call it if we eventually want them to call it. What's wrong with having it just not do anything, until such time as it does do something?
https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:257: throw new Error('Load is currently not supported.'); On 2014/06/30 23:03:36, aboxhall wrote: > On 2014/06/30 23:02:24, David Tseng wrote: > > On 2014/06/30 21:57:54, aboxhall wrote: > > > I don't know if we want to throw an Error here - can we just make it a no-op > > for > > > now instead? > > > > I don't know if we want to fail/no-op silently; the error is mostly to let the > > caller know they shouldn't be calling. Would be open to some other way of > > indicating this to the caller. > > I don't know if we want to train people not to call it if we eventually want > them to call it. What's wrong with having it just not do anything, until such > time as it does do something? If we don't want people to call it, we should probably remove it (and the loaded bool) from the API until we implement it.
https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:257: throw new Error('Load is currently not supported.'); On 2014/06/30 23:03:36, aboxhall wrote: > On 2014/06/30 23:02:24, David Tseng wrote: > > On 2014/06/30 21:57:54, aboxhall wrote: > > > I don't know if we want to throw an Error here - can we just make it a no-op > > for > > > now instead? > > > > I don't know if we want to fail/no-op silently; the error is mostly to let the > > caller know they shouldn't be calling. Would be open to some other way of > > indicating this to the caller. > > I don't know if we want to train people not to call it if we eventually want > them to call it. What's wrong with having it just not do anything, until such > time as it does do something? Who implements a non-crashing version of load()?
On 2014/06/30 23:05:02, kalman wrote: > https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... > File chrome/renderer/resources/extensions/automation/automation_node.js (right): > > https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... > chrome/renderer/resources/extensions/automation/automation_node.js:257: throw > new Error('Load is currently not supported.'); > On 2014/06/30 23:03:36, aboxhall wrote: > > On 2014/06/30 23:02:24, David Tseng wrote: > > > On 2014/06/30 21:57:54, aboxhall wrote: > > > > I don't know if we want to throw an Error here - can we just make it a > no-op > > > for > > > > now instead? > > > > > > I don't know if we want to fail/no-op silently; the error is mostly to let > the > > > caller know they shouldn't be calling. Would be open to some other way of > > > indicating this to the caller. > > > > I don't know if we want to train people not to call it if we eventually want > > them to call it. What's wrong with having it just not do anything, until such > > time as it does do something? > > Who implements a non-crashing version of load()? I don't completely understand the question, but ... we do. (David, how does jumping from the Desktop chrome to tab contents work right now?) Thinking (even) more about this, I think the habit we want people to get into is that if |loaded| is false, they need to call load() - which is a no-op (not an error) if |loaded| is true. So since |loaded| will effectively always be true as implemented now (since we have no OOP iframes), a no-op (and setting |loaded| to true) is the right behaviour.
On 2014/06/30 23:19:46, aboxhall wrote: > On 2014/06/30 23:05:02, kalman wrote: > > > https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... > > File chrome/renderer/resources/extensions/automation/automation_node.js > (right): > > > > > https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... > > chrome/renderer/resources/extensions/automation/automation_node.js:257: throw > > new Error('Load is currently not supported.'); > > On 2014/06/30 23:03:36, aboxhall wrote: > > > On 2014/06/30 23:02:24, David Tseng wrote: > > > > On 2014/06/30 21:57:54, aboxhall wrote: > > > > > I don't know if we want to throw an Error here - can we just make it a > > no-op > > > > for > > > > > now instead? > > > > > > > > I don't know if we want to fail/no-op silently; the error is mostly to let > > the > > > > caller know they shouldn't be calling. Would be open to some other way of > > > > indicating this to the caller. > > > > > > I don't know if we want to train people not to call it if we eventually want > > > them to call it. What's wrong with having it just not do anything, until > such > > > time as it does do something? > > > > Who implements a non-crashing version of load()? > > I don't completely understand the question, but ... we do. (David, how does > jumping from the Desktop chrome to tab contents work right now?) > > Thinking (even) more about this, I think the habit we want people to get into is > that if |loaded| is false, they need to call load() - which is a no-op (not an > error) if |loaded| is true. So since |loaded| will effectively always be true as > implemented now (since we have no OOP iframes), a no-op (and setting |loaded| to > true) is the right behaviour. I agree that load() should be idempotent. What I meant by "a non-crashing version of load" is that all I see is a load() function which throws an error. I had assumed this was because it was implemented in a subclass or something, so it's an assertion, but if it's not an assertion... best not to crash.
https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:257: throw new Error('Load is currently not supported.'); On 2014/06/30 23:05:01, kalman wrote: > On 2014/06/30 23:03:36, aboxhall wrote: > > On 2014/06/30 23:02:24, David Tseng wrote: > > > On 2014/06/30 21:57:54, aboxhall wrote: > > > > I don't know if we want to throw an Error here - can we just make it a > no-op > > > for > > > > now instead? > > > > > > I don't know if we want to fail/no-op silently; the error is mostly to let > the > > > caller know they shouldn't be calling. Would be open to some other way of > > > indicating this to the caller. > > > > I don't know if we want to train people not to call it if we eventually want > > them to call it. What's wrong with having it just not do anything, until such > > time as it does do something? > > Who implements a non-crashing version of load()? Ok...made this a no-op. The idea of having it in the API was to finalize the idl. Having it do nothing or crashing at this point doesn't matter all that much since we don't even have the api flipped on in dev. Wrt hooking up desktop loading of web contents, that's not done yet. I'll make that my next change for the api.
Sorry for the bikeshed here; I should have just chatted to you in person yesterday. LMK if you want to discuss offline. https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:257: throw new Error('Load is currently not supported.'); On 2014/07/01 18:48:48, David Tseng wrote: > On 2014/06/30 23:05:01, kalman wrote: > > On 2014/06/30 23:03:36, aboxhall wrote: > > > On 2014/06/30 23:02:24, David Tseng wrote: > > > > On 2014/06/30 21:57:54, aboxhall wrote: > > > > > I don't know if we want to throw an Error here - can we just make it a > > no-op > > > > for > > > > > now instead? > > > > > > > > I don't know if we want to fail/no-op silently; the error is mostly to let > > the > > > > caller know they shouldn't be calling. Would be open to some other way of > > > > indicating this to the caller. > > > > > > I don't know if we want to train people not to call it if we eventually want > > > them to call it. What's wrong with having it just not do anything, until > such > > > time as it does do something? > > > > Who implements a non-crashing version of load()? > > Ok...made this a no-op. > > The idea of having it in the API was to finalize the idl. Having it do nothing > or crashing at this point doesn't matter all that much since we don't even have > the api flipped on in dev. Right, I was just nervous that we might flip the flag without changing the behaviour, since we might not get to the point where load() is ready to implement before then. On that note, this method takes a callback in the spec and in the IDL; can we make it a proper no-op implementation by having it schedule an immediate call to the callback if |loaded| is true? And in fact if |loaded| is false, perhaps we should make this an error after all (sorry for the back and forth on that - I don't imagine it will ever be false while the error is there, but just to be on the safe side). > Wrt hooking up desktop loading of web contents, that's not done yet. I'll make > that my next change for the api. Cool - I was asking because I'm curious whether load() will need to be implemented for that. I remember talking about using it for that use case at some point, but I'm not sure what you're planning at this stage.
On 2014/07/01 19:55:11, aboxhall wrote: > Sorry for the bikeshed here; I should have just chatted to you in person > yesterday. LMK if you want to discuss offline. > > https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... > File chrome/renderer/resources/extensions/automation/automation_node.js (right): > > https://codereview.chromium.org/346703002/diff/20001/chrome/renderer/resource... > chrome/renderer/resources/extensions/automation/automation_node.js:257: throw > new Error('Load is currently not supported.'); > On 2014/07/01 18:48:48, David Tseng wrote: > > On 2014/06/30 23:05:01, kalman wrote: > > > On 2014/06/30 23:03:36, aboxhall wrote: > > > > On 2014/06/30 23:02:24, David Tseng wrote: > > > > > On 2014/06/30 21:57:54, aboxhall wrote: > > > > > > I don't know if we want to throw an Error here - can we just make it a > > > no-op > > > > > for > > > > > > now instead? > > > > > > > > > > I don't know if we want to fail/no-op silently; the error is mostly to > let > > > the > > > > > caller know they shouldn't be calling. Would be open to some other way > of > > > > > indicating this to the caller. > > > > > > > > I don't know if we want to train people not to call it if we eventually > want > > > > them to call it. What's wrong with having it just not do anything, until > > such > > > > time as it does do something? > > > > > > Who implements a non-crashing version of load()? > > > > Ok...made this a no-op. > > > > The idea of having it in the API was to finalize the idl. Having it do nothing > > or crashing at this point doesn't matter all that much since we don't even > have > > the api flipped on in dev. > > Right, I was just nervous that we might flip the flag without changing the > behaviour, since we might not get to the point where load() is ready to > implement before then. > > On that note, this method takes a callback in the spec and in the IDL; can we > make it a proper no-op implementation by having it schedule an immediate call to > the callback if |loaded| is true? Ok; added the callback. And in fact if |loaded| is false, perhaps we > should make this an error after all (sorry for the back and forth on that - I > don't imagine it will ever be false while the error is there, but just to be on > the safe side). Sounds good to me; added a check for that state. > > > Wrt hooking up desktop loading of web contents, that's not done yet. I'll make > > that my next change for the api. > > Cool - I was asking because I'm curious whether load() will need to be > implemented for that. I remember talking about using it for that use case at > some point, but I'm not sure what you're planning at this stage. It seems like it could be useful if the desktop tree creates an AutomationRootNode whenever it has a web contents node on the browser end. This might serve as a precursor to the iframe work. I don't think it's needed by ChromeVox's use case at the moment, but having access to background web contents should be useful for some features.
lgtm Mostly good, just need to fix the call to the callback. https://codereview.chromium.org/346703002/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/346703002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:287: callback(); Remember that we need to schedule the callback rather than calling it re-entrantly.
On 2014/07/07 17:24:25, aboxhall wrote: > lgtm > > Mostly good, just need to fix the call to the callback. > > https://codereview.chromium.org/346703002/diff/60001/chrome/renderer/resource... > File chrome/renderer/resources/extensions/automation/automation_node.js (right): > > https://codereview.chromium.org/346703002/diff/60001/chrome/renderer/resource... > chrome/renderer/resources/extensions/automation/automation_node.js:287: > callback(); > Remember that we need to schedule the callback rather than calling it > re-entrantly. Done. Curious; considering that this callback is being called synchronously, what are the consequences of this pattern compared with the async scheduling via setTimeout?
On 2014/07/07 17:59:26, dtseng wrote: > On 2014/07/07 17:24:25, aboxhall wrote: > > lgtm > > > > Mostly good, just need to fix the call to the callback. > > > > > https://codereview.chromium.org/346703002/diff/60001/chrome/renderer/resource... > > File chrome/renderer/resources/extensions/automation/automation_node.js > (right): > > > > > https://codereview.chromium.org/346703002/diff/60001/chrome/renderer/resource... > > chrome/renderer/resources/extensions/automation/automation_node.js:287: > > callback(); > > Remember that we need to schedule the callback rather than calling it > > re-entrantly. > > Done. > > Curious; considering that this callback is being called synchronously, what are > the consequences of this pattern compared with the async scheduling via > setTimeout? This pattern being calling callback() directly? I think it's mostly that using setTimeout() will guarantee that code in the same task as the calling context will execute before the callback does. For example, if you're doing some cleanup in a method that calls load(), and your load() callback sets some state, then you might end up cleaning up your newly set state. That's a bit of a contrived example, but that's the general idea - when we have a callback, we generally expect it to be guaranteed to be asynchronous, so if we violate that assumption we could cause problems.
(still lgtm btw)
On 2014/07/07 18:23:01, aboxhall wrote: > On 2014/07/07 17:59:26, dtseng wrote: > > On 2014/07/07 17:24:25, aboxhall wrote: > > > lgtm > > > > > > Mostly good, just need to fix the call to the callback. > > > > > > > > > https://codereview.chromium.org/346703002/diff/60001/chrome/renderer/resource... > > > File chrome/renderer/resources/extensions/automation/automation_node.js > > (right): > > > > > > > > > https://codereview.chromium.org/346703002/diff/60001/chrome/renderer/resource... > > > chrome/renderer/resources/extensions/automation/automation_node.js:287: > > > callback(); > > > Remember that we need to schedule the callback rather than calling it > > > re-entrantly. > > > > Done. > > > > Curious; considering that this callback is being called synchronously, what > are > > the consequences of this pattern compared with the async scheduling via > > setTimeout? > > This pattern being calling callback() directly? > > I think it's mostly that using setTimeout() will guarantee that code in the same > task as the calling context will execute before the callback does. For example, > if you're doing some cleanup in a method that calls load(), and your load() > callback sets some state, then you might end up cleaning up your newly set > state. That's a bit of a contrived example, but that's the general idea - when > we have a callback, we generally expect it to be guaranteed to be asynchronous, > so if we violate that assumption we could cause problems. Ok; I know I've seen both async and sync used throughout extension API's (e.g. chrome.tts.getVoices is sync). However, your example makes sense if the caller expects async behavior. If that's the convention/expectation in js, then I'll be sure to do this in the future.
The CQ bit was checked by dtseng@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/346703002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25476) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/29925)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25490)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/346703002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26029) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30475)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26040)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/346703002/100001
Message was sent while issue was closed.
Change committed as 281892 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
