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

Separate the initial creative size and the current creative size #14238

Merged
merged 9 commits into from Mar 27, 2018
Merged

Separate the initial creative size and the current creative size #14238

merged 9 commits into from Mar 27, 2018

Conversation

bradfrizzell
Copy link
Contributor

@bradfrizzell bradfrizzell commented Mar 26, 2018 •
edited

Fixes bug that I erroneously thought that iframe.height was auto-updating as the height actually changes, which it does not. iframe.height is a string also, so making new class variables that track current iframe size, original iframe size, and current slot size. This bug had the effect of breaking multiple resize requests in a row.

this.creativeSize_ was not sufficient for tracking sizes of the safeframe, need initialCreativeSize vs creativeSize which is current.

Also, send failure messages for some failure cases we were not already sending for:

  • Bad expand values
  • Expand values too large

jeffkaufman
jeffkaufman suggested changes Mar 26, 2018
View reviewed changes
extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js
@@ -476,11 +482,15 @@ export class SafeframeHostApi {
(expandWidth > this.creativeSize_.width ||
expandHeight > this.creativeSize_.height))) {
dev().error(TAG, 'Invalid expand values.');
this.sendResizeResponse(
/* SUCCESS? */ false, SERVICE.EXPAND_RESPONSE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any tests for the new sendResizeResponse behavior

(here and 2x below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js Outdated
height: creativeSize['height'],
width: creativeSize['width'],
};

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters much, but I'd expect getInitialGeometry to use initialCreativeSize_ and not creativeSize_

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, changed

extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js Outdated
this.initialCreativeSize_ = {
height: creativeSize['height'],
width: creativeSize['width'],
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like before this PR we had:

  • creativeSize_: initial creative size, what we collapse() to
  • this.iframe_.height/width: current creative size

And now we have:

  • initialCreativeSize_: initial creative size, what we collapse() to
  • creativeSize: current creative size

It's not clear to me from the PR description and the changes here what this PR fixes. Why do we need a new variable instead of just using this.iframe_.height/width for current size? Is there a bug that this fixes, or a new capability it adds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.iframe_.height/width is a string for starters, so having something that can actually be a number is nice. But I can change if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the bug was that I erroneously thought that iframe.height was auto-updating as the height actually changes, which it does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this go in the PR description then?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Including that this fixes a bug, and a little about what the bug was)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js
@@ -476,11 +482,15 @@ export class SafeframeHostApi {
(expandWidth > this.creativeSize_.width ||
expandHeight > this.creativeSize_.height))) {
dev().error(TAG, 'Invalid expand values.');
this.sendResizeResponse(
/* SUCCESS? */ false, SERVICE.EXPAND_RESPONSE);
Copy link
Contributor

Choose a reason for hiding this comment

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

PR description should include the new sendResizeResponses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

jeffkaufman
jeffkaufman reviewed Mar 26, 2018
View reviewed changes
extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js
@@ -494,10 +504,12 @@ export class SafeframeHostApi {
handleCollapseRequest_() {
// Only collapse if expanded.
if (this.isCollapsed_ || !this.isRegistered_) {
this.sendResizeResponse(
/* SUCCESS? */ false, SERVICE.COLLAPSE_RESPONSE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this one is still missing a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

keithwrightbos
keithwrightbos requested changes Mar 26, 2018
View reviewed changes
extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js Outdated
@@ -160,6 +160,12 @@ export class SafeframeHostApi {
/** @private {?({width, height}|../../../src/layout-rect.LayoutRectDef)} */
this.creativeSize_ = creativeSize;

/** @private {?({width, height}|../../../src/layout-rect.LayoutRectDef)} */
this.initialCreativeSize_ = {
height: creativeSize['height'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why access via quotes? Can this just be this.initialCreativeSize_ = Object.assign({}, this.creativeSize);

Also note that creative size can be null here... is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced, that's better. And that's fine

@keithwrightbos keithwrightbos merged commit c0f859b into master Mar 27, 2018
erwinmombay pushed a commit that referenced this pull request Mar 27, 2018
@bradfrizzell
Separate the initial creative size and the current creative size (#14238
67de708 
)

* Separate the initial creative size and the current creative size

* Respond to feedback

* Add test cases

* Respond to feedback

* Fix typing

* Fix types

* Fix lint
@bradfrizzell bradfrizzell deleted the frizz-sf-update-creative-size branch April 12, 2018 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffkaufman jeffkaufman jeffkaufman approved these changes

@keithwrightbos keithwrightbos keithwrightbos approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bradfrizzell @jeffkaufman @keithwrightbos @googlebot