Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove cached owner info #6028

Merged
merged 2 commits into from Nov 22, 2016
Merged

remove cached owner info #6028

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce53899
owner
zhouyx Nov 4, 2016
4790ab2
use getElementsByClassName
zhouyx Nov 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/service/resource.js
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ import {
} from '../layout-rect';
import {dev} from '../log';
import {toggle} from '../style';
import {childElements} from '../dom';

const TAG = 'Resource';
const RESOURCE_PROP_ = '__AMP__RESOURCE';
Expand Down Expand Up @@ -103,6 +104,15 @@ export class Resource {
Resource.forElementOptional(element).updateOwner(owner);
}
element[OWNER_PROP_] = owner;
// Need to clear owner cache for all child elements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely want this to be recursive, as it could be <owner> <child> <div> <grandchild>. Using Resources#discoverResourcesForElement_ or #discoverResourcesForArray_ would do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it needs to be recursive. However I don't think Resources#discoverResourcesForElement_ or #discoverResourcesForArray_ will do the work here. In the case of <owner> <child> <div> <grandchild> #discoverResourcesForElement_ will stop searching when it reaches <child>. Thus will never clear the cached value for <grandchild>. Also what if <owner><child><div><grandchild><grandgrandchild>?
A correct approach I can think of would be finding all the children, grandchildren .... (sorry I'm too lazy find the opposite word for ancestor 😄), but I assume it would be time consuming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to getElementByClassName('-amp-element') which finds me all the amp children.
It's same concept as discoverResourcesForElement_ except discoverResourcesForElement doesn't work in case <owner><child><grandchild><greatgrandchild> because it will not clear cache for greatgrandchild.
PTAL

const cachedElements = childElements(element, ele => {
return !ele[OWNER_PROP_];
});
cachedElements.forEach(element => {
if (Resource.forElementOptional(element)) {
Resource.forElementOptional(element).updateOwner(undefined);
}
});
}

/**
Expand Down Expand Up @@ -190,7 +200,7 @@ export class Resource {

/**
* Update owner element
* @param {!AmpElement} owner
* @param {AmpElement|undefined} owner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when I want to clear the cached value and reset it to undefine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't see that.

*/
updateOwner(owner) {
this.owner_ = owner;
Expand Down
33 changes: 31 additions & 2 deletions test/functional/test-resource.js
View file
Open in desktop
Expand Up @@ -517,19 +517,30 @@ describe('Resource', () => {
let child;
let parentResource;
let resources;
let grandChild;
beforeEach(() => {
const parent = {
ownerDocument: {defaultView: window},
tagName: 'AMP-STICKY-AD',
tagName: 'PARENT',
isBuilt: () => false,
contains: () => true,
};
child = {
ownerDocument: {defaultView: window},
tagName: 'AMP-AD',
tagName: 'CHILD',
isBuilt: () => false,
contains: () => true,
parentElement: parent,
};
grandChild = {
ownerDocument: {defaultView: window},
tagName: 'GRANDCHILD',
isBuilt: () => false,
contains: () => true,
parentElement: child,
};
parent.firstElementChild = child;
child.firstElementChild = grandChild;
resources = new Resources(new AmpDocSingle(window));
parentResource = new Resource(1, parent, resources);
});
Expand All @@ -547,6 +558,24 @@ describe('Resource', () => {
expect(childResource.owner_).to.equal(parentResource.element);
expect(childResource.getOwner()).to.equal(parentResource.element);
});

it('should remove cached value for grandchild', () => {
const childResource = new Resource(1, child, resources);
const grandChildResouce = new Resource(1, grandChild, resources);
expect(grandChildResouce.getOwner()).to.be.null;
resources.setOwner(childResource.element, parentResource.element);
expect(childResource.getOwner()).to.equal(parentResource.element);
expect(grandChildResouce.getOwner()).to.equal(parentResource.element);
});

it('should not remove change owner if it is set via setOwner', () => {
const childResource = new Resource(1, child, resources);
const grandChildResouce = new Resource(1, grandChild, resources);
resources.setOwner(grandChildResouce.element, parentResource.element);
expect(grandChildResouce.getOwner()).to.equal(parentResource.element);
resources.setOwner(childResource.element, parentResource.element);
expect(grandChildResouce.getOwner()).to.equal(parentResource.element);
});
});

describe('unlayoutCallback', () => {
Expand Down