Analytics root to separate top-level vs embed analytics for FIE #7172
Conversation
| * @param {!JSONType} config | ||
| * @private | ||
| */ | ||
| addTriggerNoInline_(config) { |
There was a problem hiding this comment.
can you add a bit of description what does noInline mean?
There was a problem hiding this comment.
Sure. Will add description. FYI, NoInline suffix ensures that closure doesn't inline the method. Since we have try/catch inlining it will cause the calling methods to loose some optimizations.
| /** | ||
| * The implementation of the analytics root for FIE. | ||
| */ | ||
| export class EmbedAnalyticsRoot extends AnalyticsRoot { |
There was a problem hiding this comment.
We will have a separate impl for ShadowDOM? In that case, should we name it explicitly as IframedAnalyticsRoot?
There was a problem hiding this comment.
No. ShadowDOM is covered by the root. If we actually go back to shadow embeds - we can worry about this then.
| /** | ||
| * Tracks custom events. | ||
| */ | ||
| export class CustomEventTracker extends EventTracker { |
There was a problem hiding this comment.
did you upload all changes? i only see this used in test.
There was a problem hiding this comment.
It's created/used here: https://github.com/ampproject/amphtml/pull/7172/files#diff-f979d4b9254b24d9616a865981a47cd6R553
Notice, since some time ago, GitHub started to arbitrarily collapse some diffs and just puts "Load diff" instead. Drives me nuts...
| this.ampdoc = ampdoc; | ||
|
|
||
| /** @const */ | ||
| this.parent = parent; |
There was a problem hiding this comment.
seems parent not used, is it needed?
There was a problem hiding this comment.
It's needed for VisibilityEventTracker. I stripped it out of this PR though - too big. I can remove until we get there if you'd like.
| */ | ||
| addListener(config, listener, analyticsElement) { | ||
| addListenerDepr_(config, listener, analyticsElement) { |
There was a problem hiding this comment.
Deprecated. Basically stuff I've removing right now.
…roject#7172) * Analytics root to separate top-level vs embed analytics for FIE * test fixes * more docs
…roject#7172) * Analytics root to separate top-level vs embed analytics for FIE * test fixes * more docs
Closes #6543.