Skip to content

Delay reading 'sandbox' attribute - #10045

Merged
zhouyx merged 1 commit into
ampproject:masterfrom
zhouyx:fix-isSandbox
Jun 20, 2017
Merged

Delay reading 'sandbox' attribute#10045
zhouyx merged 1 commit into
ampproject:masterfrom
zhouyx:fix-isSandbox

Conversation

@zhouyx

@zhouyx zhouyx commented Jun 20, 2017

Copy link
Copy Markdown
Contributor

I was testing my other PR. Then I found reading element.hasAttribute('sandbox') too early (constructor) could lead to false result. Delay to buildCallback

@zhouyx zhouyx changed the title Read Delay reading 'sandbox' attribute Jun 20, 2017
@zhouyx
zhouyx requested a review from lannka June 20, 2017 18:16
@jridgewell

Copy link
Copy Markdown
Contributor

Can you do a review for other calls to #hasAttribute or #getAttribute in constructors? This could be affecting others. We'll need a presubmit.


/** @override */
buildCallback() {
this.isSandbox_ = !!this.element.hasAttribute('sandbox');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why do we need the coercion?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

@zhouyx
zhouyx requested a review from jridgewell June 20, 2017 20:33
@zhouyx

zhouyx commented Jun 20, 2017

Copy link
Copy Markdown
Contributor Author

@jridgewell Good Point. Can I merge this PR first? I can add the presubmit check in another PR. BTW How should I prevent function in constructor only?

@jridgewell

Copy link
Copy Markdown
Contributor

Sure. You'll have to write an ESLint plugin, or a closure one. /cc @erwinmombay


/** @private {!boolean} */
this.isSandbox_ = element.hasAttribute('sandbox');
this.isSandbox_ = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does sandbox actually do here?
Should we assume true until we know otherwise?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Determine if we want to scope selector inside parentElement, support all url variables and so...
I agree it's safer to initialize it to true. But it won't be used after buildCallback, so I think it does not matter so much.

@zhouyx
zhouyx merged commit 6186254 into ampproject:master Jun 20, 2017
@zhouyx
zhouyx deleted the fix-isSandbox branch June 20, 2017 23:05
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants