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.

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

Remove wait for ini-load when layout a4a #19738

Merged
merged 4 commits into from Dec 14, 2018

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Dec 8, 2018

This is to fix the issue that Ad start layout on idle early but layout never finish because it is waiting for FIE ini-load.
This PR removes the wait for FIE ini-load, and change it render-start. Ad LayoutCallback should be completed once FIE start to render.

The change could help with the case if there are multiple ads on the page. It will also lead to loading indicator behavior change. But I think it's should be safe.

Please let me know if there's any concern. And if we want to run an experiment with it.

@zhouyx
Copy link
Contributor Author

zhouyx commented Dec 10, 2018

Talked offline. I'll add a check for meta element within the ad creative to run an experiment.

checkStillCurrent();
this.maybeTriggerAnalyticsEvent_('friendlyIframeIniLoad');
});
const isIniLoadFixExpr = !!frameDoc.querySelector(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use iframeDoc.head.querySelector, but found that Doubleclick has similar check and it doesn't require the meta tag in header, so I didn't include the head here.

.callsFake(() => {return new Promise(() => {});});
let creativeString = buildCreativeString();
// TODO: Remove after launch fie_ini_load_fix to 100%
creativeString = creativeString.replace('<body>',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't find better way to mock the creative since they need to be signed. This is a workaround that will be removed soon.

@zhouyx
Copy link
Contributor Author

zhouyx commented Dec 12, 2018

I was wrong about the placeholder behavior. The loader and placeholder will be removed at renderStart, not iniLoad. https://github.com/ampproject/amphtml/blob/master/src/friendly-iframe-embed.js#L418
So the change will only fix the renderOnIdle issue, it won't affect loading ux.

PTAL


// There's no need to wait for all resources to load.
// StartRender is enough
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// TODO: Remove after launch fie_ini_load_fix to 100%
creativeString = creativeString.replace('<body>',
'<body>' +
'<meta name="amp-experiments-opt-in" content="fie_ini_load_fix">');
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to verify query selector is contains instead of exact, you can place text around the 'fie_ini_load_fix' string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@zhouyx zhouyx merged commit 9940a5f into ampproject:master Dec 14, 2018
cathyxz pushed a commit to cathyxz/amphtml that referenced this pull request Dec 17, 2018
* remove wait for ini-load

* expr

* fix presubmit

* address comment
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Dec 19, 2018
* remove wait for ini-load

* expr

* fix presubmit

* address comment
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* remove wait for ini-load

* expr

* fix presubmit

* address comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants