✨ add ads: baidu #19489

PengXing · 2018-11-28T08:12:44Z

Ad an ads: baidu.

Baidu is an advertising provider in China, and it plays an important role.

PengXing · 2018-11-28T09:41:38Z

The limitation of bundlesize of v0.js is so strict. I don't know what I can do now. Maybe I should @choumx and @jridgewell.

[08:59:33] FAIL  ./dist/v0.js: 83.23KB > maxSize 83.2KB (gzip)
[08:59:33] ERROR: bundlesize found that ./dist/v0.js has exceeded its size cap of 83.2KB (Δ +0.03KB)
[08:59:33] This is part of a new effort to reduce AMP's binary size (#14392).
[08:59:33] Please contact @choumx or @jridgewell for assistance.

dreamofabear · 2018-11-28T15:41:43Z

This PR shouldn't affect the size of v0.js. This may be related:

[08:59:33] Branch point from master is fa99cee
[08:59:33] WARNING: Failed to retrieve bundle size of fa99cee
[08:59:33] Falling back to comparing to the max bundle size only

I restarted the build for you. @danielrozenberg FYI.

dreamofabear · 2018-11-28T15:41:57Z

/to @lannka

danielrozenberg · 2018-11-28T16:06:27Z

Re bundle-size - the base commit on master failed to store the new bundle size because, ironically, it increased the bundle size and failed. This isn't how gulp bundle-size should behave when executed on a master commit (it shouldn't reject the build, it should still store it) but since this is a rare case and I'm already working on changing the behaviour of bundle size checks (#19146,) we'll have to skip this one.

To unblock you we're increasing the max bundle size (ampproject/amphtml-build-artifacts#7), and will manually re-trigger Travis builds on the master branch to create files to store those base sizes for each of the commits that failed so far. Once that's done, we can re-trigger Travis here and it should pass. This should take about ~1-2 hours. Apologies for the hassle!

danielrozenberg · 2018-11-28T19:23:38Z

Update: bundle-size now passes, but your PR fails on validator tests. PTAL

lannka · 2018-11-28T22:33:40Z

ads/_config.js

@@ -202,6 +202,10 @@ export const adConfig = {
    ],
  },

+  'baidu': {
+    prefetch: 'https://dup.baidustatic.com/js/dm.js',
+  },


could you please implement the render-start API?

full guideline

PengXing · 2018-11-29T02:47:34Z

@lannka The renderStart and noContentAvailable API have been implemented in the latest commit. You may check it.
@danielrozenberg The ads.out file has been regenerated by running the gulp validator --update-tests.

lannka · 2018-11-30T00:33:50Z

ads/baidu.js

+        global.context.renderStart();
+      },
+      () => {
+        global.context.noContentAvailable();


this only gets called when the script is failed to load.

does dm.js script fire global events when ad response is back? that will be the time to call renderStart or noContentAvailable, depending on if the ad is filled or not.

PengXing · 2018-11-30T03:25:20Z

It would be better, but for now, dm.js doesn't fire global events when ad response is back. I have adjusted the timing of invoking renderStart instead of global events, see ef71a8b. The inner ad iframe will post a message to parent iframe once the inner page loaded.

I made this change 85fd9ca with the reference of the source code of directadvert.js.

lannka · 2018-11-30T04:38:06Z

does the message tell parent if the ad request returns an ad or not? noContentAvailable should be called when the response is empty.

PengXing · 2018-11-30T05:26:45Z

Yes, the ad content is in the page. It exists as a JSON, and will be written in the page by JS immediately. The message is a sign of successful loading, no message, no ad. But I don't know how to prevent the disappearance of default AMP ad loading mask until message received.

var ad = [
  {
    "surl" : "gongyi.baidu.com",
    "isHb" : "true",
    "bid" : "",
    "res" : "http://ubmcmm.baidustatic.com/media/v1/0f000F_LSb3bh3jmLIlBS6.png",
    "width" : 728,
    "height" : 90,
    "type" : "image",
  }
];

lannka · 2018-11-30T18:19:28Z

Got you, seems tricky then. Would you add a comment to explain that there is no good way to do noContentAvailable call.

Otherwise the PR looks good to me. Thanks!

PengXing · 2018-12-01T06:45:23Z

noContentAvailable should be called if parent iframe receives no message. I can use setTimeout, but it's not that reliable, due to complicated and unstable network conditions. So, there is no good way to do perfect noContentAvailable call.

lannka · 2018-12-03T18:14:33Z

Thanks @PengXing . I meant to have that as JS comment in the code to inform other developers.

PengXing · 2018-12-04T07:08:25Z

@lannka Please see da772ce.

lannka

You will want to merge with "master head" to resolve the failing travis test.

lannka · 2018-12-04T18:59:55Z

examples/ads-legacy.amp.html

@@ -492,5 +492,11 @@ <h2>PulsePoint 300x250</h2>
      data-size="300X250">
  </amp-ad>

+  <h2>Baidu</h2>


not needed. this is a legacy file that is deprecated

lannka · 2018-12-04T19:00:10Z

3p/integration.js

@@ -250,6 +250,7 @@ import {zedo} from '../ads/zedo';
 import {zen} from '../ads/zen';
 import {zergnet} from '../ads/zergnet';
 import {zucks} from '../ads/zucks';
+import {baidu} from '../ads/baidu';


alphabetical order pls

lannka · 2018-12-04T19:00:25Z

3p/integration.js

@@ -479,6 +480,7 @@ register('zedo', zedo);
 register('zen', zen);
 register('zergnet', zergnet);
 register('zucks', zucks);
+register('baidu', baidu);


alphabetical order

lannka · 2018-12-04T19:00:55Z

validator/testdata/feature_tests/ads.html

@@ -1336,5 +1337,11 @@ <h2>Zucks</h2>
      type="zucks"
      data-frame-id="_bda46cf5ac">
  </amp-ad>
+
+  <h2>Baidu</h2>


alphabetical order pls

lannka · 2018-12-05T17:32:31Z

examples/ads.amp.html

@@ -235,6 +235,7 @@
        <option>zen</option>
        <option>zergnet</option>
        <option>zucks</option>
+        <option>badiu</option>


one more here

lannka · 2018-12-05T17:32:36Z

examples/ads.amp.html

@@ -1911,5 +1912,11 @@ <h2>Zucks</h2>
      data-frame-id="_bda46cf5ac">
  </amp-ad>

+  <h2>Baidu</h2>


delete preconnect, as it has the same domain with prefetch url

The new `ads.out` is regenerated by running `gulp validator --update-tests`.

Following the guidance of this documentation, [ads README.md](https://github.com/ampproject/amphtml/blob/master/ads/README.md)

Add JS comment to explain why there is no good way to do `noContentAvailable` call.

1. rearrage the code 2. regenerate ads.out

PengXing · 2018-12-06T02:36:25Z

I have completed the modification of all alphabetical order problems. Please check it.

The Travis CI build failed caused by failure of dead link check. And the link of baidu ad union website is not dead. Maybe the Travis CI build should be re-run.

> [ LinkCheckResult {
    link: 'http://union.baidu.com/product/prod-cpro.html',
    statusCode: 200,
    err: null,
    status: 'alive' } ]

lannka · 2018-12-07T22:35:42Z

Tests passed. Merged.

PengXing · 2018-12-08T00:16:16Z

Thanks a lot.

* ✨ add ads: baidu * fix lint errors * delete preconnect delete preconnect, as it has the same domain with prefetch url * Update validator test: ads.out The new `ads.out` is regenerated by running `gulp validator --update-tests`. * Implelmentation of renderStart and noContentAvaliable API Following the guidance of this documentation, [ads README.md](https://github.com/ampproject/amphtml/blob/master/ads/README.md) * fix lint errors. * Adjust the timing of invoking renderStart * Add JS comment Add JS comment to explain why there is no good way to do `noContentAvailable` call. * Rearrage the code in alphabetical order 1. rearrage the code 2. regenerate ads.out * Modify a missing alphabetical order problem * Modify a missing alphabetical order problem

googlebot added the cla: yes label Nov 28, 2018

dreamofabear requested a review from lannka November 28, 2018 15:39

lannka requested changes Nov 28, 2018

View reviewed changes

lannka reviewed Nov 30, 2018

View reviewed changes

lannka approved these changes Dec 4, 2018

View reviewed changes

PengXing force-pushed the master branch from cab8d75 to 694cb38 Compare December 5, 2018 11:41

lannka reviewed Dec 5, 2018

View reviewed changes

PengXing added 9 commits December 6, 2018 09:36

✨ add ads: baidu

6e80caa

fix lint errors

0f4af0b

delete preconnect

0e4af45

delete preconnect, as it has the same domain with prefetch url

Update validator test: ads.out

316ba20

The new `ads.out` is regenerated by running `gulp validator --update-tests`.

Implelmentation of renderStart and noContentAvaliable API

9a74596

Following the guidance of this documentation, [ads README.md](https://github.com/ampproject/amphtml/blob/master/ads/README.md)

fix lint errors.

16feb9e

Adjust the timing of invoking renderStart

34096f1

Add JS comment

5f66bb6

Add JS comment to explain why there is no good way to do `noContentAvailable` call.

Rearrage the code in alphabetical order

2306a7c

1. rearrage the code 2. regenerate ads.out

PengXing added 2 commits December 6, 2018 09:36

Modify a missing alphabetical order problem

0e11a03

Modify a missing alphabetical order problem

153faad

PengXing force-pushed the master branch from 4649f6a to 153faad Compare December 6, 2018 01:37

lannka merged commit 384cf61 into ampproject:master Dec 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ add ads: baidu #19489

✨ add ads: baidu #19489

PengXing commented Nov 28, 2018

PengXing commented Nov 28, 2018

dreamofabear commented Nov 28, 2018

dreamofabear commented Nov 28, 2018

danielrozenberg commented Nov 28, 2018

danielrozenberg commented Nov 28, 2018

lannka Nov 28, 2018

PengXing commented Nov 29, 2018 •

edited

Loading

lannka Nov 30, 2018

PengXing commented Nov 30, 2018 •

edited

Loading

lannka commented Nov 30, 2018

PengXing commented Nov 30, 2018 •

edited

Loading

lannka commented Nov 30, 2018

PengXing commented Dec 1, 2018 •

edited

Loading

lannka commented Dec 3, 2018

PengXing commented Dec 4, 2018

lannka left a comment

lannka Dec 4, 2018

lannka Dec 4, 2018

lannka Dec 4, 2018

lannka Dec 4, 2018

lannka Dec 5, 2018

lannka Dec 5, 2018

PengXing commented Dec 6, 2018

lannka commented Dec 7, 2018

PengXing commented Dec 8, 2018

✨ add ads: baidu #19489

✨ add ads: baidu #19489

Conversation

PengXing commented Nov 28, 2018

PengXing commented Nov 28, 2018

dreamofabear commented Nov 28, 2018

dreamofabear commented Nov 28, 2018

danielrozenberg commented Nov 28, 2018

danielrozenberg commented Nov 28, 2018

Choose a reason for hiding this comment

PengXing commented Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

PengXing commented Nov 30, 2018 • edited Loading

lannka commented Nov 30, 2018

PengXing commented Nov 30, 2018 • edited Loading

lannka commented Nov 30, 2018

PengXing commented Dec 1, 2018 • edited Loading

lannka commented Dec 3, 2018

PengXing commented Dec 4, 2018

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PengXing commented Dec 6, 2018

lannka commented Dec 7, 2018

PengXing commented Dec 8, 2018

PengXing commented Nov 29, 2018 •

edited

Loading

PengXing commented Nov 30, 2018 •

edited

Loading

PengXing commented Nov 30, 2018 •

edited

Loading

PengXing commented Dec 1, 2018 •

edited

Loading