Remove several getServiceForDoc() in extensions/* #22511
Conversation
| LiveListManager.forDoc(this.element).then(manager => { | ||
| this.manager_ = manager; | ||
| this.manager_.register(this.liveListId_, this); | ||
| }); |
There was a problem hiding this comment.
@erwinmombay PTAL at amp-live-list code.
There was a problem hiding this comment.
As discussed, changes are needed to make sure that this delayed register calls triggers polling.
| 'height': '1px', | ||
| 'overflow': 'hidden', | ||
| 'visibility': 'hidden', | ||
| }); |
There was a problem hiding this comment.
@torch2424 PTAL at amp-recaptcha-input code.
There was a problem hiding this comment.
LGTM @choumx Thank you for this! 😄
There was a problem hiding this comment.
Also, just to be double-sure, I tested it, and it also works in manual testing:
| element, | ||
| 'position-observer' | ||
| )); | ||
| } |
There was a problem hiding this comment.
@zhouyx PTAL at PositionObserver usages above.
There was a problem hiding this comment.
sorry, missed the comment here. LGTM
| /** | ||
| * Returns a service to register callbacks we wish to execute when an | ||
| * amp-form is submitted. | ||
| * @param {!Element|!./service/ampdoc-impl.AmpDoc} elementOrAmpDoc | ||
| * @return {!Promise<../extensions/amp-form/0.1/form-submit-service.FormSubmitService>} | ||
| */ | ||
| static formSubmitPromiseForDoc(elementOrAmpDoc) { | ||
| static formSubmitForDoc(elementOrAmpDoc) { |
There was a problem hiding this comment.
@cvializ PTAL at FormSubmitService usage above.
There was a problem hiding this comment.
It should be OK as it is above, since the amp-form extension registers the service. But to make it safe from future changes or races I think it would be better the save a reference to the Promise and then it when a service reference is needed, like how the amp-analytics linker-manager.js does it.
|
|
||
| webPushServiceForDoc(this.element).then(service => { | ||
| service.start(config).catch(() => {}); | ||
| }); |
There was a problem hiding this comment.
@jasonpang PTAL at amp-web-push code.
dreamofabear
requested review from
torch2424,
zhouyx,
cvializ,
erwinmombay and
jasonpang
| /** | ||
| * Returns a service to register callbacks we wish to execute when an | ||
| * amp-form is submitted. | ||
| * @param {!Element|!./service/ampdoc-impl.AmpDoc} elementOrAmpDoc | ||
| * @return {!Promise<../extensions/amp-form/0.1/form-submit-service.FormSubmitService>} | ||
| */ | ||
| static formSubmitPromiseForDoc(elementOrAmpDoc) { | ||
| static formSubmitForDoc(elementOrAmpDoc) { |
There was a problem hiding this comment.
It should be OK as it is above, since the amp-form extension registers the service. But to make it safe from future changes or races I think it would be better the save a reference to the Promise and then it when a service reference is needed, like how the amp-analytics linker-manager.js does it.
* Fix fx-collection, next-page, position-observer, video-docking and web-push. * Fix test-live-list-manager.js. * Fix test-amp-user-notification. * Fix amp-recaptcha-service. * Remove sync formSubmitForDoc. * Fix types. * Fix types. * Fix test-amp-live-list.js. * Fix test-amp-recaptcha-input.js. * Add issue # to presubmit message. * Start live list polling on register(), if not already started. * Check this.poller_ instead of doc ready.
Partial for #22414. Removes synchronous service getter
getServiceForDocusage in extensions code.