Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years ago by sof Modified:
7 years ago Reviewers:
tyoshino (SeeGerritForStatus), vsevik, jochen (gone - plz use gerrit), apavlov, abarth-chromium, pfeldman CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, eae+blinkwatch, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, adamk+blink_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionSupport for json media types as (non-image) mime types.
Consider mime types with a JSON media type (*) just like
"application/json" is; i.e., rendered as text content. Similarly for
devtools, have such mime types be equally previewable. Also addressed
a smaller bug which made non-XHR loaded resources with text mime types
(but no charset supplied in the response) to not be previewable.
Added inspector tests to verify that such mime types are indeed
previewed using the JSON viewer.
* - "application/<something>+json", http://tools.ietf.org/html/rfc6839#section-3.1
R=tyoshino@chromium.org,pfeldman@chromium.org,vsevik@chromium.org
BUG=278568
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164220
Patch Set 1 #
Total comments: 4
Patch Set 2 : Sync licence text + add a unit test #
Total comments: 3
Patch Set 3 : Added DOMImplementation::isJSONMIMEType() and use it to resolve default encodings #
Total comments: 14
Patch Set 4 : Style improvements + robustify media type token recognition #Patch Set 5 : Rework and simplify Inspector network tests #Patch Set 6 : Improve test descriptions a bit #
Total comments: 4
Patch Set 7 : Test via synthetic NetworkRequest #
Created: 7 years ago
Messages
Total messages: 42 (0 generated)
At your leisure, please take a look. (See https://codereview.chromium.org/97913002/ for the Chromium changes needed.)
On 2013/12/01 21:16:12, sof wrote: > At your leisure, please take a look. > > (See https://codereview.chromium.org/97913002/ for the Chromium changes needed.) And, just to be clear, that change needs to be accepted and landed before this one can potentially go anywhere.
https://codereview.chromium.org/94893003/diff/1/Source/core/dom/DOMImplementa... File Source/core/dom/DOMImplementationTest.cpp (right): https://codereview.chromium.org/94893003/diff/1/Source/core/dom/DOMImplementa... Source/core/dom/DOMImplementationTest.cpp:13: * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY APPLE COMPUTER -> Opera 3 clause BSD license. http://dev.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/94893003/diff/1/Source/core/dom/DOMImplementa... Source/core/dom/DOMImplementationTest.cpp:55: } test getTextDefaultEncodingName?
https://codereview.chromium.org/94893003/diff/1/Source/core/dom/DOMImplementa... File Source/core/dom/DOMImplementationTest.cpp (right): https://codereview.chromium.org/94893003/diff/1/Source/core/dom/DOMImplementa... Source/core/dom/DOMImplementationTest.cpp:13: * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY On 2013/12/02 07:05:13, tyoshino wrote: > APPLE COMPUTER -> Opera > > 3 clause BSD license. > http://dev.chromium.org/blink/coding-style#TOC-License Thanks, updated the header with the Opera-relative version of that. https://codereview.chromium.org/94893003/diff/1/Source/core/dom/DOMImplementa... Source/core/dom/DOMImplementationTest.cpp:55: } On 2013/12/02 07:05:13, tyoshino wrote: > test getTextDefaultEncodingName? Done.
Chromium changes to net/base/ are ready; does this CL have to be split up to get some traction?
https://codereview.chromium.org/94893003/diff/20001/Source/core/dom/DOMImplem... File Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/94893003/diff/20001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.cpp:322: if (MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType) || isJSONType(mimeType) || isTextPlainType(mimeType)) return MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType) || isJSONType(mimeType) || isTextPlainType(mimeType); https://codereview.chromium.org/94893003/diff/20001/Source/core/dom/DOMImplem... File Source/core/dom/DOMImplementation.h (right): https://codereview.chromium.org/94893003/diff/20001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.h:70: static const char* getTextDefaultEncodingName(const String& MIMEType); I'd rather expose isJSONMIMEType.
https://codereview.chromium.org/94893003/diff/20001/Source/core/dom/DOMImplem... File Source/core/dom/DOMImplementation.h (right): https://codereview.chromium.org/94893003/diff/20001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.h:70: static const char* getTextDefaultEncodingName(const String& MIMEType); On 2013/12/05 12:54:33, pfeldman wrote: > I'd rather expose isJSONMIMEType. ok; just so that i understand your suggestion: you want the symmetric use of isTextMIMEType() and getTextDefaultEncodingName() in NetworkResourcesData to be replaced by a separate case for JSON + JavaScript MIME types that uses UTF-8 as its default encoding, and then have another isTextMIMEType() case that uses ISO-8859-1?
> ok; just so that i understand your suggestion: you want the symmetric use of > isTextMIMEType() and getTextDefaultEncodingName() in NetworkResourcesData to be > replaced by a separate case for JSON + JavaScript MIME types that uses UTF-8 as > its default encoding, and then have another isTextMIMEType() case that uses > ISO-8859-1? Exactly.
On 2013/12/05 15:31:45, pfeldman wrote: > > ok; just so that i understand your suggestion: you want the symmetric use of > > isTextMIMEType() and getTextDefaultEncodingName() in NetworkResourcesData to > be > > replaced by a separate case for JSON + JavaScript MIME types that uses UTF-8 > as > > its default encoding, and then have another isTextMIMEType() case that uses > > ISO-8859-1? > > Exactly. Thanks, done.
lgtm with nits https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/in... File LayoutTests/http/tests/inspector/network/network-preview-json-type.html (right): https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/in... LayoutTests/http/tests/inspector/network/network-preview-json-type.html:1: <!doctype html> I don't think you need this test since json-mime-type is covering the same code path. https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/se... File LayoutTests/http/tests/security/mime-type-execute-as-html-16.html (right): https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/se... LayoutTests/http/tests/security/mime-type-execute-as-html-16.html:15: <iframe src="resources/send-mime-types.php?mt=application%2Fx-foo%2Bjson"></iframe> Lets leave original here as well. https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... File Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.cpp:306: return (parameterMarker == kNotFound || parameterMarker > subtype); drop () https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.cpp:314: return (mimeType.startsWith("text/", false) drop outmost ()
https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... File Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.cpp:297: bool DOMImplementation::isJSONMIMEType(const String& mimeType) doesn't this return true for "application/foo+jsonbar"?
Great review feedback, thanks - much obliged. Needs clarification on the test issues raised. https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/in... File LayoutTests/http/tests/inspector/network/network-preview-json-type.html (right): https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/in... LayoutTests/http/tests/inspector/network/network-preview-json-type.html:1: <!doctype html> On 2013/12/05 17:18:53, pfeldman wrote: > I don't think you need this test since json-mime-type is covering the same code > path. Aren't there different code paths (and internal handling) for XHR loaded content and text documents? That's why I added both, as you could preview XHR loaded content with "charset="-less Content-Type:, but not .js(on) resources loaded directly. e.g. try loading https://codereview.chromium.org/static/autocomplete/lib/jquery.js in a tab and preview its response. https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/se... File LayoutTests/http/tests/security/mime-type-execute-as-html-16.html (right): https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/se... LayoutTests/http/tests/security/mime-type-execute-as-html-16.html:15: <iframe src="resources/send-mime-types.php?mt=application%2Fx-foo%2Bjson"></iframe> On 2013/12/05 17:18:53, pfeldman wrote: > Lets leave original here as well. Hmm, not sure what you mean. You want the test removed (and not test that the +json media type handling works)? Coalesced with http/tests/security/mime-*-02.html? https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... File Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.cpp:306: return (parameterMarker == kNotFound || parameterMarker > subtype); On 2013/12/05 17:18:53, pfeldman wrote: > drop () Done. https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.cpp:314: return (mimeType.startsWith("text/", false) On 2013/12/05 17:18:53, pfeldman wrote: > drop outmost () Done.
https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... File Source/core/dom/DOMImplementation.cpp (right): https://codereview.chromium.org/94893003/diff/40001/Source/core/dom/DOMImplem... Source/core/dom/DOMImplementation.cpp:297: bool DOMImplementation::isJSONMIMEType(const String& mimeType) On 2013/12/05 18:57:42, tyoshino wrote: > doesn't this return true for "application/foo+jsonbar"? True enough. Better handle it & now done. Thanks :)
In case anyone's worried about the failing trybot run on the previous patchset against the very tests added here..I kicked it off earlier without re-remembering that the required Chromium change hasn't landed yet. D'oh. So, expected until that happens.
https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/in... File LayoutTests/http/tests/inspector/network/network-preview-json-type.html (right): https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/in... LayoutTests/http/tests/inspector/network/network-preview-json-type.html:1: <!doctype html> I am not sure I follow. It seems like this test is nearly identical to the existing one. Existing one was testing resources fetched over XHR, this one uses iframe. What code paths on the front-end / backend are different? https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/se... File LayoutTests/http/tests/security/mime-type-execute-as-html-16.html (right): https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/se... LayoutTests/http/tests/security/mime-type-execute-as-html-16.html:15: <iframe src="resources/send-mime-types.php?mt=application%2Fx-foo%2Bjson"></iframe> I just see that you are removing the "resources/send-mime-types.php?mt=application%2Fjson" case from the security test, i.e. reducing the coverage.
https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/se... File LayoutTests/http/tests/security/mime-type-execute-as-html-16.html (right): https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/se... LayoutTests/http/tests/security/mime-type-execute-as-html-16.html:15: <iframe src="resources/send-mime-types.php?mt=application%2Fx-foo%2Bjson"></iframe> On 2013/12/06 11:12:29, pfeldman wrote: > I just see that you are removing the > "resources/send-mime-types.php?mt=application%2Fjson" case from the security > test, i.e. reducing the coverage. Isn't that the diff similarity running riot?
> Isn't that the diff similarity running riot? Oh, alright, now I see "A" against it in the list.
https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/in... File LayoutTests/http/tests/inspector/network/network-preview-json-type.html (right): https://codereview.chromium.org/94893003/diff/40001/LayoutTests/http/tests/in... LayoutTests/http/tests/inspector/network/network-preview-json-type.html:1: <!doctype html> On 2013/12/06 11:12:29, pfeldman wrote: > I am not sure I follow. It seems like this test is nearly identical to the > existing one. Existing one was testing resources fetched over XHR, this one uses > iframe. What code paths on the front-end / backend are different? InspectorPageAgent.cpp:createXHRTextDecoder() vs NetworkResourcesData.cpp:createOtherResourceTextDecoder() (cf. https://bugs.webkit/show_bug.cgi?id=80684 / commit 8dc90f2bba )
> InspectorPageAgent.cpp:createXHRTextDecoder() vs > NetworkResourcesData.cpp:createOtherResourceTextDecoder() There is no need to build JSON viewers for that, most your the code your test is testing is the same. You only want to compare resource and request's mime types. Btw, I applied your change and one of the tests did not pass.
On 2013/12/06 11:54:45, pfeldman wrote: > > InspectorPageAgent.cpp:createXHRTextDecoder() vs > > NetworkResourcesData.cpp:createOtherResourceTextDecoder() > > There is no need to build JSON viewers for that, most your the code your test is > testing is the same. You only want to compare resource and request's mime types. > Btw, I applied your change and one of the tests did not pass. As mentioned above, it depends on the Chromium side change. Did you apply that also?
On 2013/12/06 12:01:09, sof wrote: > On 2013/12/06 11:54:45, pfeldman wrote: > > > InspectorPageAgent.cpp:createXHRTextDecoder() vs > > > NetworkResourcesData.cpp:createOtherResourceTextDecoder() > > > > There is no need to build JSON viewers for that, most your the code your test > is > > testing is the same. You only want to compare resource and request's mime > types. Well, we want to verify that RequestPreviewView.js matching applies in both cases (application/json and JSON media types), right? I added the testing of that to verify that the change I made did apply correctly. And did the same for the pre-existing XHR test for good measure.
> Well, we want to verify that RequestPreviewView.js matching applies in both > cases (application/json and JSON media types), right? I really don't follow what you are testing here. a) If you'd like to test InspectorPageAgent.cpp:createXHRTextDecoder() vs NetworkResourcesData.cpp:createOtherResourceTextDecoder() returning proper mime type and content, you don't have to build WebInspector.RequestPreviewView._createPreviewView, just dump these mime types. b) If you are testing WebInspector.RequestPreviewView._createPreviewView, you pass it several resources with different mime types and see how it applies (a) and (b) looks like a good coverage. The test you add only tests them in combination with unclear boundary in between and with lots of copy-pasta, hence the concern.
On 2013/12/06 12:29:41, pfeldman wrote: > > Well, we want to verify that RequestPreviewView.js matching applies in both > > cases (application/json and JSON media types), right? > > I really don't follow what you are testing here. > > a) If you'd like to test InspectorPageAgent.cpp:createXHRTextDecoder() vs > NetworkResourcesData.cpp:createOtherResourceTextDecoder() returning proper mime > type and content, you don't have to build > WebInspector.RequestPreviewView._createPreviewView, just dump these mime types. > b) If you are testing WebInspector.RequestPreviewView._createPreviewView, you > pass it several resources with different mime types and see how it applies > > (a) and (b) looks like a good coverage. The test you add only tests them in > combination with unclear boundary in between and with lots of copy-pasta, hence > the concern. If you do have suggestions for how to avoid doing that & reuse some already network/inspector test framework code, please let me know. It wasn't a fun experience having to grovel around inside the object structures here to verify that the previewer was indeed the correct one. Which was the object of the exercise.
> If you do have suggestions for how to avoid doing that & reuse some already > network/inspector test framework code, please let me know. It wasn't a fun > experience having to grovel around inside the object structures here to verify > that the previewer was indeed the correct one. Which was the object of the > exercise. I think you should do exactly what I suggested above: a) modify http/tests/inspector/network/async-xhr-json-mime-type.html to test content, type and mime type of the requests, feed it with new mime types b) introduce http/tests/inspector/network/json-preview.html with the code similar to what you made to http/tests/inspector/network/async-xhr-json-mime-type.html. That test will make sure that JSON requests are rendered with JSONViewer.
On 2013/12/06 12:51:40, pfeldman wrote: > > If you do have suggestions for how to avoid doing that & reuse some already > > network/inspector test framework code, please let me know. It wasn't a fun > > experience having to grovel around inside the object structures here to verify > > that the previewer was indeed the correct one. Which was the object of the > > exercise. > > I think you should do exactly what I suggested above: > > a) modify http/tests/inspector/network/async-xhr-json-mime-type.html to test > content, type and mime type of the requests, feed it with new mime types > b) introduce http/tests/inspector/network/json-preview.html with the code > similar to what you made to > http/tests/inspector/network/async-xhr-json-mime-type.html. That test will make > sure that JSON requests are rendered with JSONViewer. Added a pair of tests; along the lines you had in mind?
Tweaked the test description text to reflect reality better; are we looking ok test-wise and generally are ready?
Would be good to get this ready & in this week; are we looking ok here?
@yurys: do the inspector tests look ok? (pfeldman seems to have his hands full & then some.)
On 2013/12/13 08:11:17, sof wrote: > @yurys: do the inspector tests look ok? (pfeldman seems to have his hands full & > then some.) Vsevolod is more familiar with network code, I'll leave it to him.
https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... File LayoutTests/http/tests/inspector/network/async-xhr-json-mime-type-expected.txt (right): https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... LayoutTests/http/tests/inspector/network/async-xhr-json-mime-type-expected.txt:28: request.content: {"number": "42"} I am getting request.content: null in this line with the patch applied (Linux release build). Is it something that should be resolved by the chromium side patch? https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... File LayoutTests/http/tests/inspector/network/json-preview.html (right): https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... LayoutTests/http/tests/inspector/network/json-preview.html:1: <html> This is still a 99% copy-paste of another test. It think what pfeldman@ suggested you was to create a mock NetworkRequest (e.g. var request = new NetworkRequest(...); request.setMimeType(...)) and test RequestPreviewView with it.
Thanks very much for going through this, much appreciated. I hope we can sort out whatever remains, see below. https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... File LayoutTests/http/tests/inspector/network/async-xhr-json-mime-type-expected.txt (right): https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... LayoutTests/http/tests/inspector/network/async-xhr-json-mime-type-expected.txt:28: request.content: {"number": "42"} On 2013/12/16 10:01:19, vsevik wrote: > I am getting request.content: null in this line with the patch applied (Linux > release build). > Is it something that should be resolved by the chromium side patch? Yes, application/vnd.document+json won't be recognized as a supported text type without it & won't be loaded in the iframe. An unfortunate state of affairs, but I won't land that patch until this one is ready. https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... File LayoutTests/http/tests/inspector/network/json-preview.html (right): https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... LayoutTests/http/tests/inspector/network/json-preview.html:1: <html> On 2013/12/16 10:01:19, vsevik wrote: > This is still a 99% copy-paste of another test. > It think what pfeldman@ suggested you was to create a mock NetworkRequest (e.g. > var request = new NetworkRequest(...); request.setMimeType(...)) and test > RequestPreviewView with it. Hmm. We have to test that XHR over JSON types generates a JSON preview in the network panel. And that the same thing happens for iframes. Why wouldn't you want to load those resources via either XHR and iframes end-to-end to test the outcome? It was broken as-was for iframes; don't think a more focused test would have caught that. That the checking of the outcome in the network panel is similar shouldn't be surprising to anyone. Like many other tests in this directory, it two-steps to fetch content, and dumps out status information in the same manner. cf. network-xhr-sync + network-xhr-async, for instance. I don't want to waste anyone's time, including mine, so what's the issue and requested change here?
On 2013/12/16 10:35:30, sof wrote: > Thanks very much for going through this, much appreciated. I hope we can sort > out whatever remains, see below. > > https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... > File > https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... > File LayoutTests/http/tests/inspector/network/json-preview.html (right): > > https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... > LayoutTests/http/tests/inspector/network/json-preview.html:1: <html> > On 2013/12/16 10:01:19, vsevik wrote: > > This is still a 99% copy-paste of another test. > > It think what pfeldman@ suggested you was to create a mock NetworkRequest > (e.g. > > var request = new NetworkRequest(...); request.setMimeType(...)) and test > > RequestPreviewView with it. > > Hmm. We have to test that XHR over JSON types generates a JSON preview in the > network panel. And that the same thing happens for iframes. This test is supposed to test that a ui component WebInspector.RequestPreviewView uses correct way to show a certain type of WebInspector.NetworkRequest object. > Why wouldn't you want to load those resources via either XHR and iframes > end-to-end to test the outcome? It was broken as-was for iframes; don't think a > more focused test would have caught that. This goes through two entirely separate steps: 1) Creating a correct WebInspector.NetworkRequest object for a network request and 2) Displaying WebInspector.NetworkRequest object in a correct way. 1) is already tested by another test so you only need to test the second one. > That the checking of the outcome in the network panel is similar shouldn't be > surprising to anyone. Like many other tests in this directory, it two-steps to > fetch content, and dumps out status information in the same manner. cf. > network-xhr-sync + network-xhr-async, for instance. These other tests only test NetworkRequest object is created correctly, they never test UI components. > I don't want to waste anyone's time, including mine, so what's the issue and > requested change here? I am sorry this takes so much time, but I think avoiding test code duplication is worth it.
On 2013/12/18 09:13:44, vsevik wrote: > On 2013/12/16 10:35:30, sof wrote: > > Thanks very much for going through this, much appreciated. I hope we can sort > > out whatever remains, see below. > > > > > https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... > > File > > > https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... > > File LayoutTests/http/tests/inspector/network/json-preview.html (right): > > > > > https://codereview.chromium.org/94893003/diff/100001/LayoutTests/http/tests/i... > > LayoutTests/http/tests/inspector/network/json-preview.html:1: <html> > > On 2013/12/16 10:01:19, vsevik wrote: > > > This is still a 99% copy-paste of another test. > > > It think what pfeldman@ suggested you was to create a mock NetworkRequest > > (e.g. > > > var request = new NetworkRequest(...); request.setMimeType(...)) and test > > > RequestPreviewView with it. > > > > Hmm. We have to test that XHR over JSON types generates a JSON preview in the > > network panel. And that the same thing happens for iframes. > This test is supposed to test that a ui component > WebInspector.RequestPreviewView uses correct way to show a certain type of > WebInspector.NetworkRequest object. > > > Why wouldn't you want to load those resources via either XHR and iframes > > end-to-end to test the outcome? It was broken as-was for iframes; don't think > a > > more focused test would have caught that. > This goes through two entirely separate steps: > 1) Creating a correct WebInspector.NetworkRequest object for a network request > and > 2) Displaying WebInspector.NetworkRequest object in a correct way. > > 1) is already tested by another test so you only need to test the second one. > Thanks for the clarification. I've switched/reduced the test not to do a resource fetch, but synthesize a NetworkRequest() + test that it gets the desired previewer from it.
test now looking ok? :)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/94893003/120001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/94893003/120001
On 2013/12/19 20:54:54, vsevik wrote: > lgtm thanks very much. i'll have to land the chromium change first; doing it now.
On 2013/12/19 20:55:52, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/sigbjornf%40opera.com/94893003/120001 I removed commit flag since I understand you wanted to land this one simultaneously with the chromium side one
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/94893003/120001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/94893003/120001
Message was sent while issue was closed.
Change committed as 164220 |
