Move owners implementation from resources to owners #23668

powerivq · 2019-08-02T22:04:31Z

TODO: Move unit tests.

src/service/owners-impl.js

src/service/resources-impl.js

lannka · 2019-08-02T22:40:11Z

src/service/owners-impl.js

+import {isArray} from '../types';
+import {registerServiceBuilderForDoc} from '../service';
+
+const TAG_ = 'Owners';


nit: TAG

I know we use them interchangeably, but TAG without underscore is dominant.

lannka · 2019-08-02T22:42:01Z

src/service/owners-impl.js

+ * @param {!Element|!Array<!Element>} elements
+ * @return {!Array<!Element>}
+ */
+function elements_(elements) {


same here, no underscore

lannka · 2019-08-02T22:45:11Z

src/service/owners-impl.js

- schedulePreload(parentElement, subElements) {}
+ schedulePreload(parentElement, subElements) {
+ this.scheduleLayoutOrPreloadForSubresources_(
+ Resource.forElement(parentElement),


to reduce unnecessary dependency, use this.resources_.getResourcexxx to access this.

src/service/owners-impl.js

lannka · 2019-08-02T23:16:08Z

src/service/resources-impl.js

+ /**
+ * @param {!Resource} resource
+ * @param {boolean} layout
+ * @param {number} parentPriority


make this param optional.

also, try to refactor the param to priorityOverride, which is a more generic and meaningful param for a public method.

lannka · 2019-08-02T23:16:30Z

src/service/resources-impl.js

@@ -262,6 +260,13 @@ export class ResourcesDef extends MutatorsAndOwnersDef {
 */
 removeForChildWindow(childWin) {}

+ /**
+ * @param {!Resource} resource
+ * @param {boolean} layout


nit: layout => layoutNeeded

hmm, i think layoutNeeded is mis-leading. This boolean only controls the priority of the task.

we should refactor this boolean to isPreload (which is negate of the current boolean layout)

lannka · 2019-08-02T23:22:11Z

src/service/resources-impl.js

+ * @param {boolean} layout
+ * @param {number} parentPriority
+ */
+ measureAndTrySchedule(resource, layout, parentPriority) {}


sorry, I meant measureAndTryScheduleLayout

lannka · 2019-08-16T22:47:31Z

src/service/owners-impl.js

 * @interface
 */
-export class Owners {
+export class OwnersDef {


We decided to unify all the service interface names to XXXInterface, and move it out to its own file owners-interface.js

lannka · 2019-08-16T22:48:58Z

src/service/owners-impl.js

+/**
+ * @implements {OwnersDef}
+ */
+export class Owners {


@visibleForTesting

Owners => OwnersImpl

lannka · 2019-08-16T22:50:40Z

test/unit/test-owners.js

+import {Signals} from '../../src/utils/signals';
+import {layoutRectLtwh} from '../../src/layout-rect';
+
+/*eslint "google-camelcase/google-camelcase": 0*/


add a comment what is this for

jridgewell · 2019-08-19T18:42:11Z

src/service/resources-impl.js

+ resource.getState() === ResourceState.READY_FOR_LAYOUT &&
+ resource.isDisplayed()
+ ) {
+ this.scheduleLayoutOrPreload_(resource, !isPreload, opt_parentPriority);


Have you checked every call site to ensure that isPreload is the opposite of what it was? Why are we flipping it?

Affirm that. The only reason is that we are making this an interface, and it needs to be a bit more readable.

lannka · 2019-08-22T00:51:22Z

@powerivq any blockers to this PR?

powerivq · 2019-08-24T02:52:02Z

@jridgewell Looked into the binary. The primary cause of increase is because we made two new public interfaces, and therefore their method names are no longer obfuscated.

jridgewell

Trusting that the methods are copy-pasted with only the preload change.

* Move owners implementation from resources to owners * Refactor discoverElements * Fix tests

googlebot added the cla: yes label Aug 2, 2019

lannka reviewed Aug 2, 2019

View reviewed changes

src/service/owners-impl.js Outdated Show resolved Hide resolved

powerivq force-pushed the owner-resources branch 2 times, most recently from 549c466 to 1bcb889 Compare August 2, 2019 22:18

lannka requested changes Aug 2, 2019

View reviewed changes

src/service/owners-impl.js Outdated Show resolved Hide resolved

src/service/resources-impl.js Outdated Show resolved Hide resolved

lannka reviewed Aug 2, 2019

View reviewed changes

powerivq force-pushed the owner-resources branch from 1bcb889 to e3f9de7 Compare August 2, 2019 22:45

lannka reviewed Aug 2, 2019

View reviewed changes

src/service/owners-impl.js Show resolved Hide resolved

powerivq force-pushed the owner-resources branch 2 times, most recently from 7db76a1 to 1a01b76 Compare August 2, 2019 23:13

lannka reviewed Aug 2, 2019

View reviewed changes

powerivq force-pushed the owner-resources branch from 1a01b76 to d0e4a28 Compare August 13, 2019 21:10

amp-bundle-size bot requested a review from jridgewell August 13, 2019 21:23

powerivq force-pushed the owner-resources branch 3 times, most recently from 6788a7b to 69d93e3 Compare August 16, 2019 17:24

powerivq changed the title ~~[WIP] Move owners implementation from resources to owners~~ Move owners implementation from resources to owners Aug 16, 2019

powerivq requested a review from lannka August 16, 2019 17:56

lannka reviewed Aug 16, 2019

View reviewed changes

lannka approved these changes Aug 16, 2019

View reviewed changes

jridgewell reviewed Aug 19, 2019

View reviewed changes

powerivq force-pushed the owner-resources branch 3 times, most recently from 378b86d to 748e3b8 Compare August 23, 2019 22:52

powerivq added 3 commits August 23, 2019 18:18

Move owners implementation from resources to owners

73a6135

Refactor discoverElements

7d77f4a

Fix tests

25fb7ff

powerivq force-pushed the owner-resources branch from 748e3b8 to 25fb7ff Compare August 24, 2019 01:26

jridgewell approved these changes Aug 26, 2019

View reviewed changes

powerivq merged commit 9ec6117 into ampproject:master Aug 26, 2019

powerivq deleted the owner-resources branch August 26, 2019 20:14

thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019

Move owners implementation from resources to owners (ampproject#23668)

39abb12

* Move owners implementation from resources to owners * Refactor discoverElements * Fix tests

This was referenced Mar 4, 2021

🚨 Error: Assertion failed ampproject/error-reporting#63

Open

🚨 Error: Assertion failed ampproject/error-reporting#65

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move owners implementation from resources to owners #23668

Move owners implementation from resources to owners #23668

powerivq commented Aug 2, 2019

lannka Aug 2, 2019

lannka Aug 2, 2019

lannka Aug 2, 2019

lannka Aug 2, 2019

lannka Aug 2, 2019

lannka Aug 2, 2019 •

edited

Loading

lannka Aug 2, 2019

lannka Aug 16, 2019 •

edited

Loading

lannka Aug 16, 2019

lannka Aug 16, 2019

jridgewell Aug 19, 2019

powerivq Aug 24, 2019

lannka commented Aug 22, 2019

powerivq commented Aug 24, 2019

jridgewell left a comment

Move owners implementation from resources to owners #23668

Move owners implementation from resources to owners #23668

Conversation

powerivq commented Aug 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka commented Aug 22, 2019

powerivq commented Aug 24, 2019

jridgewell left a comment

Choose a reason for hiding this comment

lannka Aug 2, 2019 •

edited

Loading

lannka Aug 16, 2019 •

edited

Loading