From ce5389910c47df3886e71fa4bc57f65f646a8055 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Thu, 3 Nov 2016 17:42:00 -0700 Subject: [PATCH 1/2] owner --- src/service/resource.js | 12 +++++++++++- test/functional/test-resource.js | 33 ++++++++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/service/resource.js b/src/service/resource.js index 9e41e29a8fcd..cf47e9c73d7b 100644 --- a/src/service/resource.js +++ b/src/service/resource.js @@ -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'; @@ -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 + const cachedElements = childElements(element, ele => { + return !ele[OWNER_PROP_]; + }); + cachedElements.forEach(element => { + if (Resource.forElementOptional(element)) { + Resource.forElementOptional(element).updateOwner(undefined); + } + }); } /** @@ -190,7 +200,7 @@ export class Resource { /** * Update owner element - * @param {!AmpElement} owner + * @param {AmpElement|undefined} owner */ updateOwner(owner) { this.owner_ = owner; diff --git a/test/functional/test-resource.js b/test/functional/test-resource.js index b2c11b1ad1d5..5c29698b00aa 100644 --- a/test/functional/test-resource.js +++ b/test/functional/test-resource.js @@ -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); }); @@ -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', () => { From 4790ab217ca5c27583629a2f8c385d86d012208f Mon Sep 17 00:00:00 2001 From: zhouyx Date: Mon, 21 Nov 2016 18:51:43 -0800 Subject: [PATCH 2/2] use getElementsByClassName --- src/service/resource.js | 15 +++++++-------- test/functional/test-resource.js | 21 +++++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/service/resource.js b/src/service/resource.js index cf47e9c73d7b..48456c01b048 100644 --- a/src/service/resource.js +++ b/src/service/resource.js @@ -21,7 +21,6 @@ import { } from '../layout-rect'; import {dev} from '../log'; import {toggle} from '../style'; -import {childElements} from '../dom'; const TAG = 'Resource'; const RESOURCE_PROP_ = '__AMP__RESOURCE'; @@ -104,15 +103,15 @@ export class Resource { Resource.forElementOptional(element).updateOwner(owner); } element[OWNER_PROP_] = owner; + // Need to clear owner cache for all child elements - const cachedElements = childElements(element, ele => { - return !ele[OWNER_PROP_]; - }); - cachedElements.forEach(element => { - if (Resource.forElementOptional(element)) { - Resource.forElementOptional(element).updateOwner(undefined); + const cachedElements = element.getElementsByClassName('-amp-element'); + for (let i = 0; i < cachedElements.length; i++) { + const ele = cachedElements[i]; + if (Resource.forElementOptional(ele)) { + Resource.forElementOptional(ele).updateOwner(undefined); } - }); + } } /** diff --git a/test/functional/test-resource.js b/test/functional/test-resource.js index 5c29698b00aa..08f6e265bc4b 100644 --- a/test/functional/test-resource.js +++ b/test/functional/test-resource.js @@ -537,10 +537,11 @@ describe('Resource', () => { tagName: 'GRANDCHILD', isBuilt: () => false, contains: () => true, + getElementsByClassName: () => {return [];}, parentElement: child, }; - parent.firstElementChild = child; - child.firstElementChild = grandChild; + parent.getElementsByClassName = () => {return [child, grandChild];}; + child.getElementsByClassName = () => {return [grandChild];}; resources = new Resources(new AmpDocSingle(window)); parentResource = new Resource(1, parent, resources); }); @@ -561,20 +562,20 @@ describe('Resource', () => { 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; + const grandChildResource = new Resource(1, grandChild, resources); + expect(grandChildResource.getOwner()).to.be.null; resources.setOwner(childResource.element, parentResource.element); expect(childResource.getOwner()).to.equal(parentResource.element); - expect(grandChildResouce.getOwner()).to.equal(parentResource.element); + expect(grandChildResource.getOwner()).to.equal(parentResource.element); }); - it('should not remove change owner if it is set via setOwner', () => { + it('should not 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); + const grandChildResource = new Resource(1, grandChild, resources); + resources.setOwner(grandChildResource.element, parentResource.element); + expect(grandChildResource.getOwner()).to.equal(parentResource.element); resources.setOwner(childResource.element, parentResource.element); - expect(grandChildResouce.getOwner()).to.equal(parentResource.element); + expect(grandChildResource.getOwner()).to.equal(parentResource.element); }); });