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.

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

Change consent policy state enum #15424

Merged
merged 6 commits into from May 22, 2018
Merged

Change consent policy state enum #15424

merged 6 commits into from May 22, 2018

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented May 18, 2018

Learned my lesson: not to use 0 for enum value. Try to fix this before it's too late : )

UNKNOWN: 1,
SUFFICIENT: 2,
INSUFFICIENT: 3,
UNKNOWN_NOT_REQUIRED: 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

please document this enum will have external dependency, we should never change values anymore (after this PR).

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.

@zhouyx
Copy link
Contributor Author

zhouyx commented May 18, 2018

the amp-ad-0.1.js size didn't increase after the change. we are good with importing CONSENT_POLICY_STATE

@zhouyx
Copy link
Contributor Author

zhouyx commented May 21, 2018

Not sure about why type check fails.
is there any recent change to type check? @rsimha could you help me with this.

@rsimha
Copy link
Contributor

rsimha commented May 21, 2018

@zhouyx No recent changes to type check, so far as I know.

  1. Is the path accurate?
  2. Are the types being exported properly from the source file?
  3. Are the type names being imported properly in the file that's reporting the Type check errors?

@zhouyx
Copy link
Contributor Author

zhouyx commented May 21, 2018

Thanks @rsimha
I think I know why check type is failing. 3p/adsense.js imports from consent-state.js, which imports Services from './services.js.
I should separate the enum value to a different file to fix this.

@zhouyx
Copy link
Contributor Author

zhouyx commented May 21, 2018

@lannka The change is indeed risky. Almost forget the ima-video 😨

@zhouyx
Copy link
Contributor Author

zhouyx commented May 22, 2018

@choumx @jridgewell could you help with the v0.js size? I don't think the change will add size to it.

@jridgewell
Copy link
Contributor

Try to fix this before it's too late

What’s the bug? There’s nothing inherently wrong with using zero, as long as all checks are ===.

could you help with the v0.js size

Just bump the size.

@zhouyx
Copy link
Contributor Author

zhouyx commented May 22, 2018

It's not a bug now, but error prone. As you mention as long as all checks are ===. Since this an enum exposing to 3P code, we probably want to avoid that.

Cool! I'll bump the size!

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot
Copy link

CLAs look good, thanks!

@rsimha
Copy link
Contributor

rsimha commented May 22, 2018

Percy diff seems to be from a new video test that's failing. @aghassemi

@aghassemi
Copy link
Contributor

@rsimha Looks like the video "controls" get hidden based on a timer and depending on when Percy takes a screenshot, it can flake. #15479 to fix it. For now we can approve the existing screenshots to make this PR green.

@zhouyx zhouyx merged commit 8f003f6 into ampproject:master May 22, 2018
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.

None yet

6 participants