XSLImportRule should not be ResourceClient.

XSLImportRule should not be ResourceClient to make XSLImportRule independent of blink.

Instead, it should use synchronous ResourceRequest to obtain imported xsl-stylesheet.

TEST=fast/xsl/transform-xhr-doc.xhtml
BUG=367689
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181536

[tasak, 2014-08-26 06:11:09]

Patchset #1 (id:1) has been deleted

[tasak, 2014-08-26 06:13:21]

tasak@google.com changed reviewers:
+ abarth@chromium.org, haraken@chromium.org

[tasak, 2014-08-26 06:13:22]

Would you review this CL?

[abarth-chromium, 2014-09-02 20:34:16]

I don't understand why we want this CL.  Don't we want the engine to have less
knowledge of XSLT, not more?

[tasak, 2014-09-03 02:29:37]

On 2014/09/02 20:34:16, abarth wrote:
> I don't understand why we want this CL.  Don't we want the engine to have less
> knowledge of XSLT, not more?

This depends on how to implement xslt in javascript:
https://docs.google.com/a/google.com/document/d/15rNBNb2-wXxuRNWNTAEFu1Nj20vg...

If we use importStylesheet, currently XSLTProcessor's importStylesheet doesn't
work well.

If xsl-stylesheet has xsl:import or xsl:include, we cannot apply XSL-transform
by using the stylesheet.

For example, in LayoutTests/fast/xsl/transform-xhr-doc.xhtml:

            var xslt = new XSLTProcessor();
            xhr.open('GET', 'resources/xhr-doc.xsl', false);
            xhr.send(null);
            xslt.importStylesheet(req.responseXML);

            xhr.open('GET', 'resources/xhr-doc.xml', false);
            xhr.send(null);
            var doc = xslt.transformToDocument(req.responseXML);


The xhr-doc.xsl imports another xsl-stylesheet, i.e. <xsl:import
href="xhr-doc-imported.xsl" />

So without this patch, the xhr-doc-imported.xsl is ignored. We will see "It's
nice that this hasn't crashed, but the XSL transformation has failed."

I think, this means, if we will ship PrivateScript-XSLT, xml-stylesheets which
has xsl:import or xsl:include will not work correctly.

[abarth-chromium, 2014-09-03 05:00:54]

not lgtm

We shouldn't add more XML features to the engine.  Maybe that means it's
impossible to remove built-in support for XSLT.

[tasak, 2014-09-03 05:11:42]

On 2014/09/03 05:00:54, abarth wrote:
> not lgtm
> 
> We shouldn't add more XML features to the engine.  Maybe that means it's
> impossible to remove built-in support for XSLT.

I would like to confirm.

This CL makes XSLStyleSheet not to use ResourceFetcher.
So I think, this patch makes XSLT (XSLTProcessor, XSLImportRule) independent of
Blink.

[abarth-chromium, 2014-09-03 05:19:48]

You're right.  I misunderstood what this CL does.  Yes, this is fine.

Would you be willing to update the description to describe what this CL does?

One comment below.

https://codereview.chromium.org/507583002/diff/20001/Source/core/xml/XSLImpor...
File Source/core/xml/XSLImportRule.cpp (right):

https://codereview.chromium.org/507583002/diff/20001/Source/core/xml/XSLImpor...
Source/core/xml/XSLImportRule.cpp:108: setXSLStyleSheet(absHref,
parentSheet->baseURL(), String(data->data(), data->size()));
String(data->data(), data->size())  <--- This seems unlikely to be correct. 
Don't you need to UTF-8 decode the bytes in order to produce a String?

[tasak, 2014-09-03 05:25:12]

On 2014/09/03 05:19:48, abarth wrote:
> You're right.  I misunderstood what this CL does.  Yes, this is fine.
> 
> Would you be willing to update the description to describe what this CL does?

Sure.
Done.

> One comment below.
> 
>
https://codereview.chromium.org/507583002/diff/20001/Source/core/xml/XSLImpor...
> File Source/core/xml/XSLImportRule.cpp (right):
> 
>
https://codereview.chromium.org/507583002/diff/20001/Source/core/xml/XSLImpor...
> Source/core/xml/XSLImportRule.cpp:108: setXSLStyleSheet(absHref,
> parentSheet->baseURL(), String(data->data(), data->size()));
> String(data->data(), data->size())  <--- This seems unlikely to be correct. 
> Don't you need to UTF-8 decode the bytes in order to produce a String?

I see. I will fix soon.

[tasak, 2014-09-04 11:14:40]

Thank you for reviewing.

https://codereview.chromium.org/507583002/diff/20001/Source/core/xml/XSLImpor...
File Source/core/xml/XSLImportRule.cpp (right):

https://codereview.chromium.org/507583002/diff/20001/Source/core/xml/XSLImpor...
Source/core/xml/XSLImportRule.cpp:108: setXSLStyleSheet(absHref,
parentSheet->baseURL(), String(data->data(), data->size()));
On 2014/09/03 05:19:47, abarth wrote:
> String(data->data(), data->size())  <--- This seems unlikely to be correct. 
> Don't you need to UTF-8 decode the bytes in order to produce a String?

Done.
I used TextResourceDecoder (XSLStyleSheetResource uses) for UTF-8 decode.

[tasak, 2014-09-04 11:15:42]

Patchset #2 (id:40001) has been deleted

[abarth-chromium, 2014-09-06 23:13:51]

https://codereview.chromium.org/507583002/diff/60001/Source/core/xml/XSLImpor...
File Source/core/xml/XSLImportRule.cpp (right):

https://codereview.chromium.org/507583002/diff/60001/Source/core/xml/XSLImpor...
Source/core/xml/XSLImportRule.cpp:111: text = text + decoder->flush();
You should just use the UTF-8 encoding directly.  TextResourceDecoder is a super
complicated class that sniffs for various encodings to be compatible with broken
web servers.  (Really, take a look at the code to see what it's doing
internally.)

[tasak, 2014-09-08 04:22:03]

Thank you for reviewing.

https://codereview.chromium.org/507583002/diff/60001/Source/core/xml/XSLImpor...
File Source/core/xml/XSLImportRule.cpp (right):

https://codereview.chromium.org/507583002/diff/60001/Source/core/xml/XSLImpor...
Source/core/xml/XSLImportRule.cpp:111: text = text + decoder->flush();
On 2014/09/06 23:13:51, abarth wrote:
> You should just use the UTF-8 encoding directly.  TextResourceDecoder is a
super
> complicated class that sniffs for various encodings to be compatible with
broken
> web servers.  (Really, take a look at the code to see what it's doing
> internally.)

I see. Done.

[abarth-chromium, 2014-09-08 05:46:52]

The CQ bit was checked by abarth@chromium.org

[abarth-chromium, 2014-09-08 05:46:52]

lgtm

[commit-bot: I haz the power, 2014-09-08 05:46:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tasak@google.com/507583002/80001

[commit-bot: I haz the power, 2014-09-08 05:50:08]

Committed patchset #3 (id:80001) as 181536