Update all commands to rely solely on callbacks #55

nealrosen · 2023-01-30T21:12:19Z

Resolves issues #23, #51, and #52

Changes in this PR include

Removed direct return values from all commands
Added callback support for all commands
Updated addEventHandler command to return initial EventListener response through callback instead of return value
Added success parameter to all callbacks
Fixed 2 typos in the sample stub code
Removed getGPPData command

fixed 2 typos in sample stub code

janwinkler

see comments :-)

Core/CMP API Specification.md

janwinkler · 2023-01-31T10:53:58Z

Core/CMP API Specification.md

@@ -234,32 +228,25 @@ The `addEventListener` command can be used to define a callback function (or a p
  <tr>
    <td><code>callback</code></td>
    <td>function</td>
-    <td>function (data: EventListener)</td>
+    <td>function (data: <a href="https://github.com/InteractiveAdvertisingBureau/Global-Privacy-Platform/blob/main/Core/CMP%20API%20Specification.md#eventlistener-">EventListener</a>, success: boolean)</td>


what do i get there? the callback for the event or the callback for the eventlistener object? what doess sucess do? (why give success = false if there is still an eventListener Object in the callback)?

proposal:

use callback as a callback (always). in this case, the callback would return immediatly after registering the eventListerner and gives the eventListener Object (so i can unregister it)

use parameter as the function that should be called when the event happens. the function will (also) get the eventLister Object (but filled with context data about the event)

Not sure I fully understand your comment here...
The callback returns the EventListener object as defined just below this section. I'm not sure what you mean by the callback fro the event vs the callback for the eventlistener object.

I've added success to the callbacks to align with both the TCF spec and the sample stub code in this GPP spec. For this command, success would be false if the CMP failed to register the listener for any reason.

After re-reading your comment, I think we're on the same page. The only change here is that instead of directly returning immediately, the callback would be used for that initial response. The callback would, of course, also continue to be used when future events are triggered. I have added more descriptive text to the text above this table

Core/CMP API Specification.md

…Data command

patmmccann · 2023-02-02T16:31:45Z

Core/CMP API Specification.md

@@ -166,6 +160,12 @@ supportedAPIs : Array of string, // list of supported APIs (prefix strings), e.g

 cmpId : Number, // IAB assigned CMP ID, may be 0 during stub/loading

+sectionList : Array of Number, // may be empty during loading of the CMP


should this say array of integer?

since we are using "Number" elsewere, it makes sence to keep it

I'm hoping to avoid inflating this PR any bigger than it already is. Jan mentioned that Number is used in other parts of this document. I'd prefer to use Number here to be consistent. If we want to update all "Number"s that should be "Integer"s, I think that should be handled in a separate issue/pull request

makes sense, the string specification documents say integer though

patmmccann · 2023-02-02T16:32:01Z

Core/CMP API Specification.md

@@ -166,6 +160,12 @@ supportedAPIs : Array of string, // list of supported APIs (prefix strings), e.g

 cmpId : Number, // IAB assigned CMP ID, may be 0 during stub/loading

+sectionList : Array of Number, // may be empty during loading of the CMP
+
+applicableSections: Array of Number, // Section ID considered to be in force for this transaction. In most cases, this field should have a single section ID. In rare occasions where such a single section ID can not be determined, the field may contain up to 2 values. The value can be 0 or a Section ID specified by the Publisher / Advertiser, during stub / load. When no section is applicable, the value will be -1.


should this be array of integer

Jan and I would prefer to leave it as Number for this PR to be consistent with the rest of the documentation. If it is decided that this and other parts of the document should be changed to Integer, I feel that should be addressed in a separate issue/PR

…ple places that were showing integer

…mands

janwinkler

Two tiny changes please ;-)

janwinkler · 2023-02-07T17:14:44Z

Core/CMP API Specification.md

@@ -871,7 +773,7 @@ window.__gpp_stub = function ()
 if (cmd === 'ping')
 {
  return {
-  gppVersion      : '1.0', // must be “Version.Subversion”, current: “1.0”
+  gppVersion      : '1.1', // must be “Version.Subversion”, current: “1.1”


the 3 "new" properties (sectionList, applicableSections, gppString) need to be added to ping, addEventListener and removeEventlisterner:

sectionList:[],
applicableSections:[-1], //or 0 or ID set by publisher
gppString:''

janwinkler · 2023-02-07T17:16:00Z

Core/CMP API Specification.md

@@ -1042,7 +926,7 @@ The sent message shall follow the form outlined below. The command, parameter an
  __gppCall: {
    command: "command",
    parameter: “parameter”,


can you correct the " ? we have three different quotes here (normal, cursive and single quotes) - lets stick to one ;-)

…tListener, and removeEventListener callbacks in example stub code

janwinkler

:-)

lamrowena · 2023-04-13T01:26:22Z

@nealrosen can you please also update the description of applicableSections to the following --
applicableSections: Array of Number, // Section ID considered to be in force for this transaction. In most cases, this field should have a single section ID. In rare occasions where such a single section ID can not be determined, the field may contain up to 2 values. During the transition period which ends on July 1, 2023, the legacy USPrivacy section may be determined as applicable along with another US section. In this case, the field may contain up to 3 values where one of the values is 6, representing the legacy USPrivacy section. The value can be 0 or a Section ID specified by the Publisher / Advertiser, during stub / load. When no section is applicable, the value will be -1.

This addresses Issue #21.

… Sectionlist unclear

nealrosen · 2023-04-13T13:34:52Z

@lamrowena I have updated the applicableSections description. I see there are now conflict between the PR and the develop branch. At least part of the conflict is due to a recent commit made by HeinzBaumann. I think we should review the conflicts on today's call to determine how to resolve them.

nealrosen04 added 3 commits January 30, 2023 14:50

Update all GPP commands to rely solely on callbacks

59f5ffe

Adding success parameters to all command callbacks (except ping)

6dd6693

Fixed indentation of sample stub code

f7bf21a

fixed 2 typos in sample stub code

nealrosen force-pushed the main branch from 3aae366 to f7bf21a Compare January 30, 2023 21:14

nealrosen marked this pull request as ready for review January 30, 2023 21:18

janwinkler suggested changes Jan 31, 2023

View reviewed changes

Updates based on review feedback, including the removal of the getGpp…

9725039

…Data command

nealrosen requested a review from janwinkler January 31, 2023 15:56

brondsem mentioned this pull request Feb 1, 2023

GPP module: update to support 1.1 including callback compatability, ready status, deprecation getGPPDATA prebid/Prebid.js#9483

Closed

patmmccann reviewed Feb 2, 2023

View reviewed changes

Updated version to 1.1 and updated version type to be string in a cou…

a112c28

…ple places that were showing integer

nealrosen requested review from patmmccann and janwinkler and removed request for janwinkler and patmmccann February 2, 2023 16:59

nealrosen04 added 2 commits February 2, 2023 12:08

Updated references of current version from 1.0 to 1.1

29589ea

Added 2nd parameter (success) to the ping and removeEventListener com…

e956dc7

…mands

nealrosen requested review from patmmccann and removed request for janwinkler February 7, 2023 16:34

janwinkler reviewed Feb 7, 2023

View reviewed changes

Added sectionList, applicableSections, and gppString to ping, addEven…

cd1e984

…tListener, and removeEventListener callbacks in example stub code

nealrosen requested review from janwinkler and removed request for patmmccann February 7, 2023 17:26

janwinkler approved these changes Feb 7, 2023

View reviewed changes

patmmccann approved these changes Feb 8, 2023

View reviewed changes

nealrosen mentioned this pull request Feb 9, 2023

JS API callback definition missing #23

Closed

patmmccann mentioned this pull request Feb 23, 2023

Clarification on event status #37

Closed

ajsf mentioned this pull request Mar 14, 2023

Updates needed to better support vendor scripts within iframes calling the CMP #51

Closed

lamrowena changed the base branch from main to develop April 13, 2023 01:15

Issue InteractiveAdvertisingBureau#21 ApplicableSection length issue;…

29fa300

… Sectionlist unclear

Merge branch 'develop' into main

54a5d6c

lamrowena mentioned this pull request Apr 14, 2023

Added missing parenthesis in the if statement #59

Closed

lamrowena merged commit 315ced5 into InteractiveAdvertisingBureau:develop Apr 14, 2023

christceder approved these changes May 16, 2024

View reviewed changes

		@@ -166,6 +160,12 @@ supportedAPIs : Array of string, // list of supported APIs (prefix strings), e.g

		cmpId : Number, // IAB assigned CMP ID, may be 0 during stub/loading

		sectionList : Array of Number, // may be empty during loading of the CMP

Update all commands to rely solely on callbacks #55

Update all commands to rely solely on callbacks #55

Uh oh!

Conversation

nealrosen commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janwinkler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nealrosen Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janwinkler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janwinkler left a comment

Choose a reason for hiding this comment

Uh oh!

lamrowena commented Apr 13, 2023

Uh oh!

nealrosen commented Apr 13, 2023

Uh oh!

Uh oh!

nealrosen commented Jan 30, 2023 •

edited

Loading

nealrosen Jan 31, 2023 •

edited

Loading