Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use the right initial byte for the top-level 6-element array. #454

Merged
merged 2 commits into from
Jul 22, 2019
Merged

Use the right initial byte for the top-level 6-element array. #454

merged 2 commits into from
Jul 22, 2019

Conversation

jyasskin
Copy link
Member

Thanks irori for noticing!

@jyasskin jyasskin requested a review from irori July 10, 2019 17:56
@jyasskin jyasskin mentioned this pull request Jul 10, 2019
Merged
irori
irori approved these changes Jul 11, 2019
View reviewed changes
Copy link
Collaborator

@irori irori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % comment

draft-yasskin-wpack-bundled-exchanges.md
@@ -263,8 +263,8 @@ steps, taking the `stream` as input.
1. Seek to offset 0 in `stream`. Assert: this operation doesn't fail.

1. If reading 10 bytes from `stream` returns an error or doesn't return the
bytes with hex encoding "84 48 F0 9F 8C 90 F0 9F 93 A6" (the CBOR encoding of
the 4-item array initial byte and 8-byte bytestring initial byte, followed by
bytes with hex encoding "86 48 F0 9F 8C 90 F0 9F 93 A6" (the CBOR encoding of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number in Section 6.1 should be updated as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks.

@jyasskin jyasskin merged commit 16c93ae into WICG:master Jul 22, 2019
@jyasskin jyasskin deleted the fix-bundles-array branch July 22, 2019 14:37
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 6, 2019
@irori
BundledExchangesParser: Update the bundle format [1/2]
d8c863a 
This updates BundledExchangesParser to parse the new bundle format [1].

Changes:
- Updated the magic header bytes [2]
- Version field is added (we use implementation-specific version string
  "b1\0\0", which matches gen-bundle's output [3])
- Fallback URL (== Primary URL) field is added
- ParseMetadata() returns error type ("format error" or "version error")
  and fallback URL if available
- Updated spec ref comments

The structure change of the index section is not reflected yet. It will
be updated in the next CL.

The test bundle file (hello.wbn) is generated with gen-bundle of
github.com/WICG/webpackage revision a3cef2c, which supports the new
bundle format except for the new index section structure.

[1] WICG/webpackage#450
[2] WICG/webpackage#454
[3] WICG/webpackage#458

Bug: 969596
Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684200}
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 6, 2019
@caseq
Revert "BundledExchangesParser: Update the bundle format [1/2]"
5bd3e57 
This reverts commit d8c863a.

Reason for revert: broke build, example: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8905916412276160432/+/steps/compile__with_patch_/0/stdout

Original change's description:
> BundledExchangesParser: Update the bundle format [1/2]
> 
> This updates BundledExchangesParser to parse the new bundle format [1].
> 
> Changes:
> - Updated the magic header bytes [2]
> - Version field is added (we use implementation-specific version string
>   "b1\0\0", which matches gen-bundle's output [3])
> - Fallback URL (== Primary URL) field is added
> - ParseMetadata() returns error type ("format error" or "version error")
>   and fallback URL if available
> - Updated spec ref comments
> 
> The structure change of the index section is not reflected yet. It will
> be updated in the next CL.
> 
> The test bundle file (hello.wbn) is generated with gen-bundle of
> github.com/WICG/webpackage revision a3cef2c, which supports the new
> bundle format except for the new index section structure.
> 
> [1] WICG/webpackage#450
> [2] WICG/webpackage#454
> [3] WICG/webpackage#458
> 
> Bug: 969596
> Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#684200}

TBR=horo@chromium.org,kinuko@chromium.org,toyoshim@chromium.org,ksakamoto@chromium.org,rsesek@chromium.org

Change-Id: Ifce2b07c15a57a2030887cbfaad5853cbc5af992
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 969596
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1737883
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684207}
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 6, 2019
@irori
Reland "BundledExchangesParser: Update the bundle format [1/2]"
4f518dc 
This is a reland of d8c863a

PS1 is the original CL, PS2 is the fix for the new callsite of
BundleMetadataParseError::New() introduced by https://crrev.com/c/1732518.

Original change's description:
> BundledExchangesParser: Update the bundle format [1/2]
>
> This updates BundledExchangesParser to parse the new bundle format [1].
>
> Changes:
> - Updated the magic header bytes [2]
> - Version field is added (we use implementation-specific version string
>   "b1\0\0", which matches gen-bundle's output [3])
> - Fallback URL (== Primary URL) field is added
> - ParseMetadata() returns error type ("format error" or "version error")
>   and fallback URL if available
> - Updated spec ref comments
>
> The structure change of the index section is not reflected yet. It will
> be updated in the next CL.
>
> The test bundle file (hello.wbn) is generated with gen-bundle of
> github.com/WICG/webpackage revision a3cef2c, which supports the new
> bundle format except for the new index section structure.
>
> [1] WICG/webpackage#450
> [2] WICG/webpackage#454
> [3] WICG/webpackage#458
>
> Bug: 969596
> Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#684200}

Bug: 969596
Change-Id: I1d68a3cb2975fc7856449180517f67131d7b7817
Tbr: toyoshim@chromium.org
Tbr: kinuko@chromium.org
Tbr: rsesek@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1736907
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684216}
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 6, 2019
Revert "BundledExchangesParser: Update the bundle format [1/2]"
3a99193 
This reverts commit d8c863a.

Reason for revert: 
https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.linux/builders/Deterministic%20Linux/builds/24310


https://luci-milo.appspot.com/p/chromium/builders/ci/Deterministic%20Linux/24310

Compile failure, List of errors:

../../mojo/public/cpp/bindings/struct_ptr.h:56:18: error: no matching constructor for initialization of 'mojo::StructPtr<data_decoder::mojom::BundleMetadataParseError>::Struct' (aka 'data_decoder::mojom::BundleMetadataParseError')


Original change's description:
> BundledExchangesParser: Update the bundle format [1/2]
> 
> This updates BundledExchangesParser to parse the new bundle format [1].
> 
> Changes:
> - Updated the magic header bytes [2]
> - Version field is added (we use implementation-specific version string
>   "b1\0\0", which matches gen-bundle's output [3])
> - Fallback URL (== Primary URL) field is added
> - ParseMetadata() returns error type ("format error" or "version error")
>   and fallback URL if available
> - Updated spec ref comments
> 
> The structure change of the index section is not reflected yet. It will
> be updated in the next CL.
> 
> The test bundle file (hello.wbn) is generated with gen-bundle of
> github.com/WICG/webpackage revision a3cef2c, which supports the new
> bundle format except for the new index section structure.
> 
> [1] WICG/webpackage#450
> [2] WICG/webpackage#454
> [3] WICG/webpackage#458
> 
> Bug: 969596
> Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#684200}

TBR=horo@chromium.org,kinuko@chromium.org,toyoshim@chromium.org,ksakamoto@chromium.org,rsesek@chromium.org

Change-Id: I0e4c550179c4433022646f8ecc27cbdf4983ce87
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 969596
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1738036
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684219}
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 6, 2019
@irori
Reland "BundledExchangesParser: Update the bundle format [1/2]"
aefed1d 
This reverts commit 3a99193.

Reason for revert: 
This was already reverted at [1] and relanded with a fix at [2].
[1] https://chromium.googlesource.com/chromium/src/+/5bd3e578821d1daacdb92dd5180255a771bf6d6c
[2] https://chromium.googlesource.com/chromium/src/+/4f518dc059bb7e0167335a3bb01b47d01bd0da80

Original change's description:
> Revert "BundledExchangesParser: Update the bundle format [1/2]"
> 
> This reverts commit d8c863a.
> 
> Reason for revert: 
> https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.linux/builders/Deterministic%20Linux/builds/24310
> 
> 
> https://luci-milo.appspot.com/p/chromium/builders/ci/Deterministic%20Linux/24310
> 
> Compile failure, List of errors:
> 
> ../../mojo/public/cpp/bindings/struct_ptr.h:56:18: error: no matching constructor for initialization of 'mojo::StructPtr<data_decoder::mojom::BundleMetadataParseError>::Struct' (aka 'data_decoder::mojom::BundleMetadataParseError')
> 
> 
> Original change's description:
> > BundledExchangesParser: Update the bundle format [1/2]
> > 
> > This updates BundledExchangesParser to parse the new bundle format [1].
> > 
> > Changes:
> > - Updated the magic header bytes [2]
> > - Version field is added (we use implementation-specific version string
> >   "b1\0\0", which matches gen-bundle's output [3])
> > - Fallback URL (== Primary URL) field is added
> > - ParseMetadata() returns error type ("format error" or "version error")
> >   and fallback URL if available
> > - Updated spec ref comments
> > 
> > The structure change of the index section is not reflected yet. It will
> > be updated in the next CL.
> > 
> > The test bundle file (hello.wbn) is generated with gen-bundle of
> > github.com/WICG/webpackage revision a3cef2c, which supports the new
> > bundle format except for the new index section structure.
> > 
> > [1] WICG/webpackage#450
> > [2] WICG/webpackage#454
> > [3] WICG/webpackage#458
> > 
> > Bug: 969596
> > Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487
> > Reviewed-by: Robert Sesek <rsesek@chromium.org>
> > Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
> > Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> > Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#684200}
> 
> TBR=horo@chromium.org,kinuko@chromium.org,toyoshim@chromium.org,ksakamoto@chromium.org,rsesek@chromium.org
> 
> Change-Id: I0e4c550179c4433022646f8ecc27cbdf4983ce87
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 969596
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1738036
> Reviewed-by: Noel Gordon <noel@chromium.org>
> Commit-Queue: Noel Gordon <noel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#684219}

TBR=horo@chromium.org,kinuko@chromium.org,toyoshim@chromium.org,noel@chromium.org,ksakamoto@chromium.org,rsesek@chromium.org

Change-Id: I27423f80338576b35506bb8f7e6010fc4c2d2ec7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 969596
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1735802
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684221}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irori irori irori approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jyasskin @irori