Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1367523002:
[dom] support iterable<> NodeList (Closed)
|
Created:
5 years, 3 months ago by caitp (gmail) Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[dom] support iterable<> NodeList
Adds missing iterable<Node> support to NodeList. This results in 4
additional enumerable properties on the interface:
- values()
- entries()
- keys()
- forEach()
The WebIDL spec wants each of these methods to return an ArrayIterator,
but for the time being these are still returning IDL Iterator types.
Ported from https://codereview.chromium.org/1220883007/
BUG=229398
LOG=N
R=jsbell@chromium.org, philipj@opera.com
Committed: https://crrev.com/9004adaa987fa68b8b4da6abfdc4669d6c8a3b9b
Cr-Commit-Position: refs/heads/master@{#356744}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Fix nits / Don't ship feature by default #
Total comments: 6
Patch Set 3 : Rebase #Patch Set 4 : Remove unneeded flag code, use IterableCollections flag because NodeList isn't special, adjust tests #Patch Set 5 : Consistently put expected value on left, actual on right in tests #Patch Set 6 : Remove redundant assertion #
Total comments: 1
Patch Set 7 : Actual on the left, expected on the right #Messages
Total messages: 37 (9 generated)
Ported to chromium
On 2015/09/23 16:20:31, caitp wrote: > Ported to chromium So, this is marked as "no LGTM necessary", probably not going to get one. The way it's designed though, it's not behind a flag, so implementing === shipping, essentially, which may not be what people want.
On 2015/10/02 16:46:59, caitp wrote: > On 2015/09/23 16:20:31, caitp wrote: > > Ported to chromium > > So, this is marked as "no LGTM necessary", probably not going to get one. The > way it's designed though, it's not behind a flag, so implementing === shipping, > essentially, which may not be what people want. You should probably bump the thread with this detail, i.e. that it's really an "Intent to Implement & Ship" and does need active approval.
On 2015/10/02 17:39:14, jsbell wrote: > On 2015/10/02 16:46:59, caitp wrote: > > On 2015/09/23 16:20:31, caitp wrote: > > > Ported to chromium > > > > So, this is marked as "no LGTM necessary", probably not going to get one. The > > way it's designed though, it's not behind a flag, so implementing === > shipping, > > essentially, which may not be what people want. > > You should probably bump the thread with this detail, i.e. that it's really an > "Intent to Implement & Ship" and does need active approval. ah, I've tried to clarify that.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html (right): https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:22: test(function () { no space after function keyword https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:32: test(function () { ditto, and everywhere else in this file https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/NodeList.cpp (right): https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeList.cpp:14: explicit NodeListIterationSource(NodeList* nodeList) reference https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeList.cpp:21: if (m_index >= m_nodeList->length()) { accessing length causes us to cache the entire contents which seems unnecessary. https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeList.cpp:25: value = m_nodeList->item(m_index); accessing out of bounds always returns a nullptr, this whole function can be written as: value = m_nodeList->item(m_index); return value != nullptr; https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeList.cpp:43: return new NodeListIterationSource(this); *this https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/NodeList.idl (right): https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeList.idl:30: iterable<Node>; Why can't you write [RuntimeEnabled=NodeListIterable] here?
https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html (right): https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:32: test(function () { On 2015/10/03 05:07:45, esprehn wrote: > ditto, and everywhere else in this file Done. https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/NodeList.cpp (right): https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeList.cpp:14: explicit NodeListIterationSource(NodeList* nodeList) On 2015/10/03 05:07:45, esprehn wrote: > reference Done. https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeList.cpp:25: value = m_nodeList->item(m_index); On 2015/10/03 05:07:45, esprehn wrote: > accessing out of bounds always returns a nullptr, this whole function can be > written as: > > value = m_nodeList->item(m_index); > return value != nullptr; Done. There's a presubmit rule preventing comparisons with nullptr, so it's written as `!!value`, which is kind of weird. But I guess if people prefer that? https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/NodeList.idl (right): https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeList.idl:30: iterable<Node>; On 2015/10/03 05:07:45, esprehn wrote: > Why can't you write [RuntimeEnabled=NodeListIterable] here? I wasn't familiar with the Blink approach to hiding features behind flags. It looks like it takes a lot more work to get the feature listed in chrome://flags, and maybe it's not worth doing that for such a simple feature, or maybe it can be done separately.
On 2015/10/03 at 15:46:45, caitpotter88 wrote: > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html (right): > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:32: test(function () { > On 2015/10/03 05:07:45, esprehn wrote: > > ditto, and everywhere else in this file > > Done. > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/dom/NodeList.cpp (right): > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/NodeList.cpp:14: explicit NodeListIterationSource(NodeList* nodeList) > On 2015/10/03 05:07:45, esprehn wrote: > > reference > > Done. > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/NodeList.cpp:25: value = m_nodeList->item(m_index); > On 2015/10/03 05:07:45, esprehn wrote: > > accessing out of bounds always returns a nullptr, this whole function can be > > written as: > > > > value = m_nodeList->item(m_index); > > return value != nullptr; > > Done. > > There's a presubmit rule preventing comparisons with nullptr, so it's written as `!!value`, which is kind of weird. But I guess if people prefer that? > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/dom/NodeList.idl (right): > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/NodeList.idl:30: iterable<Node>; > On 2015/10/03 05:07:45, esprehn wrote: > > Why can't you write [RuntimeEnabled=NodeListIterable] here? > > I wasn't familiar with the Blink approach to hiding features behind flags. > > It looks like it takes a lot more work to get the feature listed in chrome://flags, and maybe it's not worth doing that for such a simple feature, or maybe it can be done separately. You don't list it separately. You mark it =experimental and then it goes behind the experimental web platform feature flag.
On 2015/10/03 17:18:44, esprehn wrote: > On 2015/10/03 at 15:46:45, caitpotter88 wrote: > > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html > (right): > > > > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:32: > test(function () { > > On 2015/10/03 05:07:45, esprehn wrote: > > > ditto, and everywhere else in this file > > > > Done. > > > > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/dom/NodeList.cpp (right): > > > > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/dom/NodeList.cpp:14: explicit > NodeListIterationSource(NodeList* nodeList) > > On 2015/10/03 05:07:45, esprehn wrote: > > > reference > > > > Done. > > > > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/dom/NodeList.cpp:25: value = > m_nodeList->item(m_index); > > On 2015/10/03 05:07:45, esprehn wrote: > > > accessing out of bounds always returns a nullptr, this whole function can be > > > written as: > > > > > > value = m_nodeList->item(m_index); > > > return value != nullptr; > > > > Done. > > > > There's a presubmit rule preventing comparisons with nullptr, so it's written > as `!!value`, which is kind of weird. But I guess if people prefer that? > > > > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/dom/NodeList.idl (right): > > > > > https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/dom/NodeList.idl:30: iterable<Node>; > > On 2015/10/03 05:07:45, esprehn wrote: > > > Why can't you write [RuntimeEnabled=NodeListIterable] here? > > > > I wasn't familiar with the Blink approach to hiding features behind flags. > > > > It looks like it takes a lot more work to get the feature listed in > chrome://flags, and maybe it's not worth doing that for such a simple feature, > or maybe it can be done separately. > > You don't list it separately. You mark it =experimental and then it goes behind > the experimental web platform feature flag. So, now that integer-indexed items have @@iterator (when experimental web features are enabled), NodeList has the equivalent of this change, but without the additional methods, and without necessarily the correct Iterator class. It's not super clear, but I don't really see a reason for NodeList's iterator to be %ArrayProto_values%. I may just be misreading Cameron, though.
On 2015/10/23 at 15:50:17, caitpotter88 wrote: > On 2015/10/03 17:18:44, esprehn wrote: > ... > > So, now that integer-indexed items have @@iterator (when experimental web features are enabled), NodeList has the equivalent of this change, but without the additional methods, and without necessarily the correct Iterator class. > > It's not super clear, but I don't really see a reason for NodeList's iterator to be %ArrayProto_values%. I may just be misreading Cameron, though. Interesting, this patch looks good now, I'm happy to land this if this agrees with the spec.
Re: "It's not super clear, but I don't really see a reason for NodeList's iterator to be %ArrayProto_values%. I may just be misreading Cameron, though." Does that apply? WebIDL says: https://heycam.github.io/webidl/#es-iterator "If the interface does not have an iterable declaration but does define an indexed property getter, then the Function object is %ArrayProto_values%" But NodeList does have an iterable declaration. https://dom.spec.whatwg.org/#interface-nodelist Is there conversation captured elsewhere? https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt (right): https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:3308: method entries A change to virtual/stable/webexposed/global-interface-listing-expected.txt implies this is shipping in stable. This doesn't make sense if the feature is tagged as experimental. https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebRuntimeFeatures.cpp (right): https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebRuntimeFeatures.cpp:152: void WebRuntimeFeatures::enableIterableNodeLists(bool enable) Why is this necessary? https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebRuntimeFeatures.h:90: BLINK_EXPORT static void enableIterableNodeLists(bool); Ditto?
https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html (right): https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:27: assert_equals(node.id, "div" + ++id, "elements should be the expected values"); The tests all look correct, but it might be more readable to collect the actual results (e.g. node.id here) in an array, then use assert_array_equals(expected, actual) where expected is a hard-coded array, e.g. ['div1', 'div2', 'div3', 'div4', 'div5'] That only works for arrays of primitive types, though.
On 2015/10/26 22:53:34, jsbell wrote: > Re: "It's not super clear, but I don't really see a reason for NodeList's > iterator to be %ArrayProto_values%. I may just be misreading Cameron, though." > > Does that apply? WebIDL says: > > https://heycam.github.io/webidl/#es-iterator > > "If the interface does not have an iterable declaration but does define an > indexed property getter, then the Function object is %ArrayProto_values%" > > But NodeList does have an iterable declaration. > > https://dom.spec.whatwg.org/#interface-nodelist > > Is there conversation captured elsewhere? > That line is the reason why I believe it's okay to not use %ArrayProto_values% for NodeList --- But for some reason I remember the opposite being true, and I think there's probably text on bugzilla.mozilla.org backing that up. Whether it's right or wrong, it's unlikely to matter unless something non-interoperable is shipped, and I'll make sure that doesn't happen. After doing some research, I think the only reason I wasn't sure about this was this line from bz a while ago "I still think the fact that using iterable<> here means that the class doesn't use the normal Array iterator is kinda busted; I think we should fix that in the spec (and in our iterable<> implementation)." --- but there's no evidence that the spec change ever happened. So presumably nothing to worry about. https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt (right): https://codereview.chromium.org/1367523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:3308: method entries On 2015/10/26 22:53:34, jsbell wrote: > A change to virtual/stable/webexposed/global-interface-listing-expected.txt > implies this is shipping in stable. This doesn't make sense if the feature is > tagged as experimental. Change predates putting it behind a flag, haven't seen this fail since fixing it, but will reset this file https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebRuntimeFeatures.cpp (right): https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebRuntimeFeatures.cpp:152: void WebRuntimeFeatures::enableIterableNodeLists(bool enable) On 2015/10/26 22:53:34, jsbell wrote: > Why is this necessary? For procedures like putting something behind a flag, I usually look for similar CLs doing the same thing, and appropriate the changes. Build worked, so I didn't question it. Per comments on the other related CL, it's not necessary
https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html (right): https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:27: assert_equals(node.id, "div" + ++id, "elements should be the expected values"); On 2015/10/26 23:02:07, jsbell wrote: > The tests all look correct, but it might be more readable to collect the actual > results (e.g. node.id here) in an array, then use assert_array_equals(expected, > actual) where expected is a hard-coded array, e.g. ['div1', 'div2', 'div3', > 'div4', 'div5'] > > That only works for arrays of primitive types, though. done https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebRuntimeFeatures.cpp (right): https://codereview.chromium.org/1367523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebRuntimeFeatures.cpp:152: void WebRuntimeFeatures::enableIterableNodeLists(bool enable) On 2015/10/26 23:11:46, caitp wrote: > On 2015/10/26 22:53:34, jsbell wrote: > > Why is this necessary? > > For procedures like putting something behind a flag, I usually look for similar > CLs doing the same thing, and appropriate the changes. Build worked, so I didn't > question it. Per comments on the other related CL, it's not necessary Anyways, got rid of this stuff
(non-OWNER) lgtm, other than the test nits. https://codereview.chromium.org/1367523002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html (right): https://codereview.chromium.org/1367523002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:29: assert_array_equals(["div1", "div2", "div3", "div4", "div5"], ids, "elements should be the expected values"); The assert functions in testharness.js take (actual, expected, ...), these are in (expected, actual, ...) order - can you flip these? https://github.com/w3c/testharness.js/blob/master/testharness.js#L856 (Obviously doesn't matter if everything is passing, but makes the errors easier to read if we break something!)
On 2015/10/27 16:47:58, jsbell wrote: > (non-OWNER) lgtm, other than the test nits. > > https://codereview.chromium.org/1367523002/diff/100001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html > (right): > > https://codereview.chromium.org/1367523002/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/dom/NodeList/nodelist-iterable.html:29: > assert_array_equals(["div1", "div2", "div3", "div4", "div5"], ids, "elements > should be the expected values"); > The assert functions in testharness.js take (actual, expected, ...), these are > in (expected, actual, ...) order - can you flip these? > > https://github.com/w3c/testharness.js/blob/master/testharness.js#L856 > > (Obviously doesn't matter if everything is passing, but makes the errors easier > to read if we break something!) done
lgtm
On 2015/10/27 19:20:33, esprehn wrote: > lgtm Thanks for the reviews, looking forward to flipping the flag
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/1367523002/#ps120001 (title: "Actual on the left, expected on the right")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1367523002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1367523002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/10/28 06:15:11, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) huh, doesn't look good
On 2015/10/28 08:30:50, caitp wrote: > On 2015/10/28 06:15:11, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > huh, doesn't look good Judging from http://build.chromium.org/p/chromium.webkit/builders/ this doesn't look caused by this CL, but I guess it's good to wait until the issues are fixed or expectations adjusted, just in case?
On 2015/10/28 12:51:00, caitp wrote: > On 2015/10/28 08:30:50, caitp wrote: > > On 2015/10/28 06:15:11, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > huh, doesn't look good > > Judging from http://build.chromium.org/p/chromium.webkit/builders/ this doesn't > look caused by this CL, but I guess it's good to wait until the issues are fixed > or expectations adjusted, just in case? It's likely that https://codereview.chromium.org/1407933008/ will have mac tests passing
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/1367523002/#ps120001 (title: "Actual on the left, expected on the right")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1367523002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1367523002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/10/28 16:54:09, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Remaining mac failure looks okay here ``` EchoBeach:src caitp$ content/test/gpu/run_gpu_test.py webgl_conformance --story-filter=WebglConformance.conformance_canvas_to_data_url_test (WARNING) 2015-10-28 20:35:43,449 browser_finder.FindBrowser:88 --browser omitted. Using most recent local build: content-shell-release /Users/caitp/google/chromium/src/tools/telemetry/telemetry/internal/results/results_options.py:143: UserWarning: Function __init__ is deprecated. It will no longer be supported on February 29, 2016. Please remove it or switch to an alternative before that time. Chart JSON is a supported alternative. See https://goo.gl/8daFav . sys.stdout, trace_tag=options.output_trace_tag)) (WARNING) 2015-10-28 20:35:43,691 browser_finder.FindBrowser:88 --browser omitted. Using most recent local build: content-shell-release [ RUN ] WebglConformance.conformance_canvas_to_data_url_test (WARNING) 2015-10-28 20:35:43,692 desktop_browser_finder.Create:55 Could not find Flash at /Users/caitp/google/chromium/src/tools/perf/chrome_telemetry_build/../../../third_party/adobe/flash/binaries/ppapi/mac/PepperFlashPlayer.plugin. Continuing without Flash. To run with Flash, check it out via http://go/read-src-internal [ OK ] WebglConformance.conformance_canvas_to_data_url_test (2404 ms) [ PASSED ] 1 test. Pages: [WebglConformance.conformance_canvas_to_data_url_test] RESULT telemetry_page_measurement_results: num_failed= 0 count RESULT telemetry_page_measurement_results: num_errored= 0 count View result at file:///Users/caitp/google/chromium/src/content/test/gpu/results.html (ERROR) 2015-10-28 20:35:46,103 ps_util._ListAllSubprocesses:61 psutil is not installed on the system. Not listing possible leaked processes. To install psutil, see: https://pypi.python.org/pypi/psutil EchoBeach:src caitp$ ``` I am going to try this again
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1367523002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1367523002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9004adaa987fa68b8b4da6abfdc4669d6c8a3b9b Cr-Commit-Position: refs/heads/master@{#356744} |
