Skip to content

Reland double gzip wrapping NOTICES to reduce on-disk installed space#80897

Merged
xster merged 1 commit intoflutter:masterfrom
xster:gzip
Apr 27, 2021
Merged

Reland double gzip wrapping NOTICES to reduce on-disk installed space#80897
xster merged 1 commit intoflutter:masterfrom
xster:gzip

Conversation

@xster
Copy link
Member

@xster xster commented Apr 21, 2021 •
edited
Loading

Reland the revert at #71899 to fix #71102

The difference between this and the last PR is that it now first compares uncompressed strings against the existing entry first before doing any work to avoid taking a performance hit.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Apr 21, 2021
@google-cla google-cla bot added the cla: yes label Apr 21, 2021
@xster xster force-pushed the gzip branch 2 times, most recently from dc56ff6 to b3285a3 Compare April 22, 2021 20:19
@xster xster marked this pull request as ready for review April 22, 2021 20:20
@xster xster requested a review from jonahwilliams April 22, 2021 20:20
@xster
Copy link
Member Author

xster commented Apr 22, 2021

@jonahwilliams did I get what you were telling me last time right?

jonahwilliams
jonahwilliams reviewed Apr 22, 2021
View reviewed changes
packages/flutter_tools/lib/src/asset.dart Outdated
Copy link
Contributor

@jonahwilliams jonahwilliams Apr 22, 2021

Choose a reason for hiding this comment

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

oh Gradle

jonahwilliams
jonahwilliams reviewed Apr 22, 2021
View reviewed changes
packages/flutter_tools/lib/src/asset.dart Outdated
Copy link
Contributor

@jonahwilliams jonahwilliams Apr 22, 2021

Choose a reason for hiding this comment

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

TIL

jonahwilliams
jonahwilliams reviewed Apr 22, 2021
View reviewed changes
packages/flutter_tools/lib/src/devfs.dart Outdated
Copy link
Contributor

@jonahwilliams jonahwilliams Apr 22, 2021

Choose a reason for hiding this comment

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

nit: super() isn't needed`

Copy link
Member Author

@xster xster Apr 23, 2021

Choose a reason for hiding this comment

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

ah man, I haven't written dart for so long 😔

jonahwilliams
jonahwilliams reviewed Apr 23, 2021
View reviewed changes
packages/flutter_tools/lib/src/asset.dart Outdated
Copy link
Contributor

@jonahwilliams jonahwilliams Apr 23, 2021

Choose a reason for hiding this comment

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

I would split up this code into a method with good documentation on what it is doing. Its a bit hard to follow the conditional as is

jonahwilliams
jonahwilliams reviewed Apr 23, 2021
View reviewed changes
jonahwilliams
jonahwilliams reviewed Apr 23, 2021
View reviewed changes
@jonahwilliams
Copy link
Contributor

jonahwilliams commented Apr 23, 2021

Seems to do the job, thank you @xster

xster
xster commented Apr 23, 2021
View reviewed changes
packages/flutter_tools/lib/src/asset.dart Outdated
Copy link
Member Author

@xster xster Apr 23, 2021

Choose a reason for hiding this comment

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

@chingjun I unfortunately needed to add another parameter to AssetBundle.build. I took a quick look in cs and it doesn't seem like there's any AssetBundle subclasses that implements build.

@xster
Copy link
Member Author

xster commented Apr 23, 2021

Note: I took the unzipping part out of the framework's AssetBundle and just put it into the service binding's implementation to reduce the API break surface area.

@xster xster requested a review from jonahwilliams April 24, 2021 06:28
jonahwilliams
jonahwilliams approved these changes Apr 25, 2021
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Tool changes LGTM

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Apr 26, 2021

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@xster
Copy link
Member Author

xster commented Apr 27, 2021

Not sure why my frob cls were deleted. But it's also expected to fail. I'll submit a g3fix

@xster xster merged commit ff2dde2 into flutter:master Apr 27, 2021
@xster xster deleted the gzip branch April 27, 2021 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deduplicate NOTICES

3 participants

@xster @jonahwilliams @fluttergithubbot