Skip to content

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

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

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

Conversation

@xster

@xster xster commented Apr 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

xster commented Apr 22, 2021

Copy link
Copy Markdown
Member Author

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

jonahwilliams
jonahwilliams reviewed Apr 22, 2021
View reviewed changes
Comment thread packages/flutter_tools/lib/src/asset.dart Outdated

@jonahwilliams jonahwilliams Apr 22, 2021

Copy link
Copy Markdown
Contributor

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
Comment thread packages/flutter_tools/lib/src/asset.dart Outdated

@jonahwilliams jonahwilliams Apr 22, 2021

Copy link
Copy Markdown
Contributor

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
Comment thread packages/flutter_tools/lib/src/devfs.dart Outdated

@jonahwilliams jonahwilliams Apr 22, 2021

Copy link
Copy Markdown
Contributor

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`

@xster xster Apr 23, 2021

Copy link
Copy Markdown
Member Author

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
Comment thread packages/flutter_tools/lib/src/asset.dart Outdated

@jonahwilliams jonahwilliams Apr 23, 2021

Copy link
Copy Markdown
Contributor

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
Comment thread packages/flutter_tools/lib/src/devfs.dart Outdated
jonahwilliams
jonahwilliams reviewed Apr 23, 2021
View reviewed changes
Comment thread packages/flutter_tools/lib/src/devfs.dart Outdated
@jonahwilliams

jonahwilliams commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

Seems to do the job, thank you @xster

xster
xster commented Apr 23, 2021
View reviewed changes
Comment thread packages/flutter_tools/lib/src/asset.dart Outdated

@xster xster Apr 23, 2021

Copy link
Copy Markdown
Member Author

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

xster commented Apr 23, 2021

Copy link
Copy Markdown
Member Author

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

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tool changes LGTM

@fluttergithubbot

fluttergithubbot commented Apr 26, 2021

Copy link
Copy Markdown
Contributor

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

xster commented Apr 27, 2021

Copy link
Copy Markdown
Member Author

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