1454460 - Simplify BOM handling in StreamLoader

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

See bug 1454030 comment 6.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0f2599a8ed0b5e091fb6befc870bad51639b2e0

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7df5f56fdc336064ecd71a35dbcfb60be6326e69

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4938d68edb7e4686dfb9d98dd4b3302aecf6c417

Comment 4

[Bobby Holley (:bholley)]

Attachment: Buffer the potentially-BOM-related bytes separately and handle them on the fly. v1

MozReview-Commit-ID: 5zrKyadBppO

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Pass a bonafide nsACString to Servo. v2

This is the rebased patch from the other bug.

MozReview-Commit-ID: HCop9h2abZU

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968328 [details] [diff] [review]
Buffer the potentially-BOM-related bytes separately and handle them on the fly. v1

>+   * For uninteresting reasons, the caller may have the data split into two

Why not just have CSS use the "streaming mode" SRICheckDataVerifier instead?  That's designed to handle multiple chunks, etc...

>+++ b/layout/style/SheetLoadData.h
>+                                   const nsACString& aPrefix,
>+                                   const nsACString& aRest,

This needs at least documentation about how aPrefix differs from aRest.

>+    if (encoding == UTF_8_ENCODING && Encoding::UTF8ValidUpTo(bytes) == bytes.Length()) {

This is losing an optimization the old code used to have: when we had a UTF8 encoding (at least via the BOM) but not all the data was valid, it would pass in the valid length to DecodeWithoutBOMHandling.  It seems like it should be possible to preserve that optimization.  Maybe something like:

  size_t validated = 0;
  if (encoding == UTF_8_ENCODING) {
    validated = Encoding::UTF8ValidUpTo(bytes);
  }

  if (validated == bytes.Length()) {
    // Either this is UTF-8 and all valid, or it's not UTF-8 but is an
    // empty string.  This assumes that an empty string in any encoding
    // decodes to empty string, which seems like a plausible assumption.
    utf8String.Assign(bytes);
  } else {
    rv = encoding->DecodeWithoutBOMHandling(bytes, utf8String, validated);
  }

or something.

>+++ b/layout/style/StreamLoader.h
>   nsCString mBytes;

Please add a comment about how this never contains the BOM when there is a BOM involved, just the actual text bytes.

>+  nsAutoCStringN<3> mFirstThreeBytes;

I'm torn between that and mBOM or something.  By the time we get to the end it has the latter meaning.  During load it can be the first 3, or 2, or 1 or 0 bytes...

Either way, we should document what's stored in this string at what stage of the load.

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968345 [details] [diff] [review]
Part 2 - Pass a bonafide nsACString to Servo. v2

>+  if (aUTF8.IsEmpty()) {

Worth testing && !aUTF16.IsEmpty()?  Or should we just not worry too much about the empty-data case and the extra "copy" we end up doing then?

r=me

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c92f6b8e882b
Buffer the potentially-BOM-related bytes separately and handle them on the fly. r=bz
https://hg.mozilla.org/integration/autoland/rev/985679bb1928
Pass a bonafide nsACString to Servo. r=bz
https://hg.mozilla.org/integration/autoland/rev/a368c8d6676b
Hoist SRI helper into Loader.cpp. r=bz

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> Comment on attachment 8968328 [details] [diff] [review]
> Buffer the potentially-BOM-related bytes separately and handle them on the
> fly. v1
> 
> >+   * For uninteresting reasons, the caller may have the data split into two
> 
> Why not just have CSS use the "streaming mode" SRICheckDataVerifier instead?
> That's designed to handle multiple chunks, etc...

Per discussion on IRC, I'll add a third patch to move the helper to Loader.cpp.

> 
> >+++ b/layout/style/SheetLoadData.h
> >+                                   const nsACString& aPrefix,
> >+                                   const nsACString& aRest,
> 
> This needs at least documentation about how aPrefix differs from aRest.

Done. I also renamed to aBytes1 and aBytes2, which I've decided is clearer.

> 
> >+    if (encoding == UTF_8_ENCODING && Encoding::UTF8ValidUpTo(bytes) == bytes.Length()) {
> 
> This is losing an optimization the old code used to have: when we had a UTF8
> encoding (at least via the BOM) but not all the data was valid, it would
> pass in the valid length to DecodeWithoutBOMHandling.  It seems like it
> should be possible to preserve that optimization.  Maybe something like:

ok.

> 
> >+++ b/layout/style/StreamLoader.h
> >   nsCString mBytes;
> 
> Please add a comment about how this never contains the BOM when there is a
> BOM involved, just the actual text bytes.
> 
> >+  nsAutoCStringN<3> mFirstThreeBytes;
> 
> I'm torn between that and mBOM or something.  By the time we get to the end
> it has the latter meaning.  During load it can be the first 3, or 2, or 1 or
> 0 bytes...

I'll make it mBOMBytes.

> 
> Either way, we should document what's stored in this string at what stage of
> the load.

Done.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> Comment on attachment 8968345 [details] [diff] [review]
> Part 2 - Pass a bonafide nsACString to Servo. v2
> 
> >+  if (aUTF8.IsEmpty()) {
> 
> Worth testing && !aUTF16.IsEmpty()?  Or should we just not worry too much
> about the empty-data case and the extra "copy" we end up doing then?

I'll just switch it to !aUTF16.IsEmpty() and MOZ_ASSERT(aUTF16.IsEmpty() || aUTF8.IsEmpty());

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c92f6b8e882b
https://hg.mozilla.org/mozilla-central/rev/985679bb1928
https://hg.mozilla.org/mozilla-central/rev/a368c8d6676b