Support for json media types as (non-image) mime types.

Support 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

[sof, 2013-12-01 21:16:12]

At your leisure, please take a look.

(See https://codereview.chromium.org/97913002/ for the Chromium changes needed.)

[sof, 2013-12-01 21:18:36]

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.

[tyoshino (SeeGerritForStatus), 2013-12-02 07:05:13]

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?

[sof, 2013-12-02 07:45:37]

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.

[sof, 2013-12-05 12:32:31]

Chromium changes to net/base/ are ready; does this CL have to be split up to get
some traction?

[pfeldman, 2013-12-05 12:54:32]

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.

[sof, 2013-12-05 13:42:58]

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?

[pfeldman, 2013-12-05 15:31:45]

> 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.

[sof, 2013-12-05 16:25:02]

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.

[pfeldman, 2013-12-05 17:18:52]

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 ()

[tyoshino (SeeGerritForStatus), 2013-12-05 18:57:42]

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"?

[sof, 2013-12-05 19:32:44]

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.

[sof, 2013-12-05 19:34:54]

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 :)

[sof, 2013-12-05 19:38:46]

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.

[pfeldman, 2013-12-06 11:12:28]

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.

[sof, 2013-12-06 11:16:15]

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?

[pfeldman, 2013-12-06 11:17:53]

> Isn't that the diff similarity running riot?

Oh, alright, now I see "A" against it in the list.

[sof, 2013-12-06 11:34:59]

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 )

[pfeldman, 2013-12-06 11:54:45]

> 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.

[sof, 2013-12-06 12:01:09]

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?

[sof, 2013-12-06 12:05:06]

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.

[pfeldman, 2013-12-06 12:29:41]

> 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.

[sof, 2013-12-06 12:33:38]

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.

[pfeldman, 2013-12-06 12:51:40]

> 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.

[sof, 2013-12-06 15:41:44]

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?

[sof, 2013-12-09 13:46:46]

Tweaked the test description text to reflect reality better; are we looking ok
test-wise and generally are ready?

[sof, 2013-12-11 10:05:41]

Would be good to get this ready & in this week; are we looking ok here?

[sof, 2013-12-13 08:11:17]

@yurys: do the inspector tests look ok? (pfeldman seems to have his hands full &
then some.)

[yurys, 2013-12-16 08:05:58]

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.

[vsevik, 2013-12-16 10:01:18]

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.

[sof, 2013-12-16 10:35:30]

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?

[vsevik, 2013-12-18 09:13:44]

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.

[sof, 2013-12-18 10:32:47]

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.

[sof, 2013-12-19 08:31:39]

test now looking ok? :)

[vsevik, 2013-12-19 20:54:54]

lgtm

[commit-bot: I haz the power, 2013-12-19 20:55:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/94893003/120001

[commit-bot: I haz the power, 2013-12-19 20:55:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/94893003/120001

[sof, 2013-12-19 20:57:07]

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.

[vsevik, 2013-12-19 20:57:15]

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

[commit-bot: I haz the power, 2013-12-20 04:23:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/94893003/120001

[commit-bot: I haz the power, 2013-12-20 07:26:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/94893003/120001

[commit-bot: I haz the power, 2013-12-20 08:57:31]

Change committed as 164220