Move Cid service to doc-scope and fixes to elements to work in shadow #7113
Conversation
|
You'll need to rebase. |
mkhatib
force-pushed
the
debug-analytics-shadow
branch
from
492d2f0
to
66b0f1a
Compare
mkhatib
force-pushed
the
debug-analytics-shadow
branch
from
66b0f1a
to
4aba37b
Compare
| */ | ||
| export function installCidService(window) { | ||
| return fromClass(window, 'cid', Cid); | ||
| export function installCidServiceForDoc(ampdoc) { |
There was a problem hiding this comment.
So, is this now only useful for testing? If so, could you please rename this method to installCidServiceForDocForTesting?
There was a problem hiding this comment.
We don't currently have such naming convention. Isn't cid service installed in amp-analytics? Could we use installCidServiceForDoc in amp-analytics instead of renaming?
There was a problem hiding this comment.
OK, I now understand we don't need this method after compilation. Either change it to ForTesting or dropping this method is fine.
| @@ -100,7 +100,7 @@ export class AmpForm { | |||
| this.timer_ = timerFor(this.win_); | |||
|
|
|||
| /** @const @private {!../../../src/service/url-replacements-impl.UrlReplacements} */ | |||
| this.urlReplacement_ = urlReplacementsForDoc(this.win_.document); | |||
| this.urlReplacement_ = urlReplacementsForDoc(element); | |||
There was a problem hiding this comment.
oops. thanks for fixing.
| this.playerReadyPromise_ = new Promise(resolve => { | ||
| this.playerReadyResolver_ = resolve; | ||
| }); | ||
|
|
||
| this.win.addEventListener( | ||
| const ampdoc = this.getAmpDoc(); | ||
| ampdoc.win.addEventListener( |
There was a problem hiding this comment.
We should be using our iframe messaging helpers here. /cc @jridgewell
There was a problem hiding this comment.
cc/ @aghassemi as he has another PR that touches this path - maybe can update the listener to use the helpers
Fixes #7064