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

Move out removeForChildWindow() from resources-impl.js #23344

Merged
merged 5 commits into from Sep 9, 2019
Merged

Move out removeForChildWindow() from resources-impl.js #23344

merged 5 commits into from Sep 9, 2019

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Jul 16, 2019

For #23311

@lannka lannka requested a review from dvoytenko July 16, 2019 03:57
dvoytenko
dvoytenko reviewed Jul 16, 2019
View reviewed changes
src/friendly-iframe-embed.js
* Removes all resources belonging to the FIE window.
* @private
*/
removeResources_() {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, a much better option is when a FIE ampdoc gets its own Resources instance that can be completely thrown out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. This PR still brings value though, as once this childwindow concept is moved out of resources service, we don't need to worry about providing a dummy impl for inabox runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dvoytenko this is a pure dependency clean up, I think we should get it in. No?

@mrjoro
Copy link
Member

mrjoro commented Aug 8, 2019

Is this PR still active?

@lannka
Copy link
Contributor Author

lannka commented Sep 9, 2019 •
edited

The bundle size (gzipped compressed size of v0.js) of this pull request could not be determined because the base size (that is, the bundle size of the master commit that this pull request was compared against) was not found in the https://github.com/ampproject/amphtml-build-artifacts repository. This can happen due to failed or delayed Travis builds on said master commit.

I retried the whole travis build, it failed with same msg. @danielrozenberg have you seen this before?

@danielrozenberg
Copy link
Member

Yes, this is unfortunately a side-effect of master being red. Since this PR changes src/ files, I can't rubberstamp it for bundle-size. You can wait until master is green again (you can help hasten this by approving #24415) and rebase your PR on that master

@lannka
Copy link
Contributor Author

lannka commented Sep 9, 2019

@danielrozenberg that worked! thanks

@lannka lannka merged commit 9a9c55b into ampproject:master Sep 9, 2019
@lannka lannka deleted the move-removeResourcesInChileWindow branch September 9, 2019 17:59
@ampprojectbot ampprojectbot mentioned this pull request Aug 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvoytenko dvoytenko dvoytenko approved these changes

@dreamofabear dreamofabear Awaiting requested review from dreamofabear

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lannka @mrjoro @danielrozenberg @dvoytenko @googlebot