Make <amp-install-service-worker> a low priority element, and remove timer-based delay to installation #13409
Conversation
danielrozenberg
force-pushed
the
remove-amp-install-service-worker-delay
branch
2 times, most recently
from
ce34d8c
to
c8dfd93
Compare
|
This is awesome. Thank you @danielrozenberg! |
|
/to @cramforce, to confirm this is what he meant. |
| @@ -92,27 +92,20 @@ export class AmpInstallServiceWorker extends AMP.BaseElement { | |||
| } | |||
| } | |||
|
|
|||
| /** @override */ | |||
| getPriority() { | |||
| return 2; | |||
There was a problem hiding this comment.
We should have constants for this. At least give it a comment.
There was a problem hiding this comment.
Also, I think 3 (even lower than ads) may be more appropriate.
There was a problem hiding this comment.
Replaced with 3. Created a new issue #13555 to replace them with constants later
danielrozenberg
force-pushed
the
remove-amp-install-service-worker-delay
branch
from
c8dfd93
to
edc29c3
Compare
| /** @override */ | ||
| getPriority() { | ||
| return 3; | ||
| } |
There was a problem hiding this comment.
It seems priority only affect #layoutCallback, but we're installing the iframe/SW in #buildCallback.
There was a problem hiding this comment.
Nice catch. Perhaps we should rename getPriority to getLayoutPriority.
There was a problem hiding this comment.
@jridgewell PTAL, moved insertion of the iframe into layout phase. I also changed the description of #13555 to mention renaming the getPriority method as @choumx suggsted
|
Super good catch!!!
…On Wed, Feb 21, 2018 at 1:08 AM, Justin Ridgewell ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/amp-install-serviceworker/0.1/amp-install-serviceworker.js
<#13409 (comment)>:
> @@ -92,27 +92,20 @@ export class AmpInstallServiceWorker extends AMP.BaseElement {
}
}
+ /** @OverRide */
+ getPriority() {
+ return 3;
+ }
Sure.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13409 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFeTyNVd7UpuRKwk7_1W0iPyOhcxUgxks5tW2x3gaJpZM4SAeAp>
.
|
| @@ -49,6 +49,9 @@ export class AmpInstallServiceWorker extends AMP.BaseElement { | |||
|
|
|||
| /** @private {?UrlRewriter_} */ | |||
| this.urlRewriter_ = null; | |||
|
|
|||
| /** @private {boolean} */ | |||
| this.shouldInsertframeOnFirstLayout_ = false; | |||
| /** @private */ | ||
| insertIframe_() { | ||
| // Mutate block is not required when calling this method, since the inserted | ||
| // iframe has `display: none` and does not modify the document's layout. |
There was a problem hiding this comment.
Unfortunately not, any insertion can cause CSS style recalc.
There was a problem hiding this comment.
Is it enough to call it from inside layoutCallback() or does it need more explicit protection?
| } | ||
|
|
||
| /** @override */ | ||
| firstLayoutCompleted() { |
There was a problem hiding this comment.
Use layoutCallback. We'll need to guard against layout happening, regardless.
|
/to @jridgewell better? |
|
Ping @jridgewell |
danielrozenberg
force-pushed
the
remove-amp-install-service-worker-delay
branch
from
0e91ef6
to
ef121b9
Compare
…timer-based delay to installation (ampproject#13409) * Make <amp-install-service-worker> a low priority element, and remove timer-based delay to installation * Set priority to 3 * Move insertion of the iframe into layout phase * Address @choumx's comment * Address @jridgewell's comments * Fix missing resolved promise in layoutCallback()
… remove timer-based delay to installation (ampproject#13409)" This reverts commit 7a8be5f.
Fixed #12473