WebRtcBrowserTest prep-CL for IndexedDB cloning of RTCCertificate.

WebRtcBrowserTest prep-CL for IndexedDB cloning of RTCCertificate.

This is part of larger work that will allow persistent storage of
RTCCertificate in IndexedDB. The big picture can be seen here:
https://codereview.chromium.org/1949033002/

This CL implements part 3a+3b:

3. Unittests making sure RTCCertificate can be saved and loaded from IndexedDB
   a) chrome/test/data/webrtc/indexeddb.js added with functions for managing
      a database and saving and loading (cloning) certificates.
   b) WebRtcTestBase implementing C++ functions for calling the indexeddb.js
      API.

Part 3c is *NOT* implemented:
   c) WebRtcBrowserTest tests added which clone certificates and sets up calls
      using the clones.

This is because RTCCertificate cloning (pt.2) has not landed yet, so any attempt
to clone it in a unittest would fail. Part 2 + 3c will be done in a follow-up.

BUG=581354
Committed: https://crrev.com/042933a05d5d0bde0aaea8882c7d74b79810c799
Cr-Commit-Position: refs/heads/master@{#392582}

[hbos_chromium, 2016-05-09 14:15:00]

Description was changed from

==========
WebRtcBrowserTest prep-CL for IndexedDB clonging of RTCCertificate.

This is part of larger work that will allow persistent storage of
RTCCertificate in IndexedDB. The big picture can be seen here:
https://codereview.chromium.org/1949033002/

This CL implements part 3a+3b:

3. Unittests making sure RTCCertificate can be saved and loaded from IndexedDB
   a) chrome/test/data/webrtc/indexeddb.js added with functions for managing
      a database and saving and loading (cloning) certificates.
   b) WebRtcTestBase implementing C++ functions for calling the indexeddb.js
      API.

Part 3c is *NOT* implemented:
   c) WebRtcBrowserTest tests added which clone certificates and sets up calls
      using the clones.

This is because RTCCertificate cloning (pt.2) has not landed yet, so any attempt
to clone it in a unittest would fail. Part 2 + 3c will be done in a follow-up.

BUG=581354
==========

to

==========
WebRtcBrowserTest prep-CL for IndexedDB cloning of RTCCertificate.

This is part of larger work that will allow persistent storage of
RTCCertificate in IndexedDB. The big picture can be seen here:
https://codereview.chromium.org/1949033002/

This CL implements part 3a+3b:

3. Unittests making sure RTCCertificate can be saved and loaded from IndexedDB
   a) chrome/test/data/webrtc/indexeddb.js added with functions for managing
      a database and saving and loading (cloning) certificates.
   b) WebRtcTestBase implementing C++ functions for calling the indexeddb.js
      API.

Part 3c is *NOT* implemented:
   c) WebRtcBrowserTest tests added which clone certificates and sets up calls
      using the clones.

This is because RTCCertificate cloning (pt.2) has not landed yet, so any attempt
to clone it in a unittest would fail. Part 2 + 3c will be done in a follow-up.

BUG=581354
==========

[hbos_chromium, 2016-05-09 14:42:39]

hbos@chromium.org changed reviewers:
+ jsbell@chromium.org, phoglund@chromium.org

[hbos_chromium, 2016-05-09 14:42:39]

Please take a look,

jsbell: chrome/test/data/webrtc/indexeddb.js for your IndexedDB expertise (found
your name in a lot of indexeddb-related git blames).

phoglund: Owner and WebRTC dude, PTAL all.

Thanks :)

[phoglund_chromium, 2016-05-09 14:59:57]

https://codereview.chromium.org/1962673002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1962673002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.cc:471: EXPECT_EQ("ok-",
response.substr(0, 3)) << "Failed to open database: "
Maybe just match against the actual strings, like "ok-opened" rather than just
looking for the OK.

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
File chrome/test/data/webrtc/indexeddb.js (right):

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:99: // Public helper functions to test
functions. Expected to indirectly be called
You mean these are only called from the functions above? In that case they
should be internal, e.g. saveCertificate_(...). They are only meant to be called
from this file right?

If they're called by other js they're just regular javascript functions, and
there's nothing magical about them tied to ExecuteJavascript, so I'd just remove
this comment and make the functions internal.

If they're public to other javascript files they should not be internal.

[jsbell, 2016-05-09 16:42:39]

a few nits, but overall indexedb usage code lgtm

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
File chrome/test/data/webrtc/indexeddb.js (right):

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:35: certStore.createIndex('by_id', 'id');
The store is already keyed by the id property - why are you adding an index here
on id as well?

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:44: throw failTest('The database could not
be opened. Error: ' + reqOpen.error);
`throw` won't do anything here (exception thrown in error handler is ignored);
can just call failTest()

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:64: throw failTest(
Ditto

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:102: function saveCertificate(certificate,
onsuccess, onerror) {
Code might be more readable returning a Promise rather than taking callbacks,
but whatever is most consistent with surrounding code.

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:129: var reqGet = certById.get(0);
Could just use certStore.get(0) - no need for the index here.

[hbos_chromium, 2016-05-09 18:23:32]

Thanks for the quick feedback,
PTAL phoglund.

https://codereview.chromium.org/1962673002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1962673002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.cc:471: EXPECT_EQ("ok-",
response.substr(0, 3)) << "Failed to open database: "
On 2016/05/09 14:59:57, phoglund_chrome wrote:
> Maybe just match against the actual strings, like "ok-opened" rather than just
> looking for the OK.

Done.

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
File chrome/test/data/webrtc/indexeddb.js (right):

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:35: certStore.createIndex('by_id', 'id');
On 2016/05/09 16:42:39, jsbell wrote:
> The store is already keyed by the id property - why are you adding an index
here
> on id as well?

Ah, no need. Done.

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:44: throw failTest('The database could not
be opened. Error: ' + reqOpen.error);
On 2016/05/09 16:42:38, jsbell wrote:
> `throw` won't do anything here (exception thrown in error handler is ignored);
> can just call failTest()

Done.

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:64: throw failTest(
On 2016/05/09 16:42:39, jsbell wrote:
> Ditto

Done.

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:99: // Public helper functions to test
functions. Expected to indirectly be called
On 2016/05/09 14:59:57, phoglund_chrome wrote:
> You mean these are only called from the functions above? In that case they
> should be internal, e.g. saveCertificate_(...). They are only meant to be
called
> from this file right?
> 
> If they're called by other js they're just regular javascript functions, and
> there's nothing magical about them tied to ExecuteJavascript, so I'd just
remove
> this comment and make the functions internal.
> 
> If they're public to other javascript files they should not be internal.

Done.

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:102: function saveCertificate(certificate,
onsuccess, onerror) {
On 2016/05/09 16:42:39, jsbell wrote:
> Code might be more readable returning a Promise rather than taking callbacks,
> but whatever is most consistent with surrounding code.

Makes sense, changed to promises.

https://codereview.chromium.org/1962673002/diff/1/chrome/test/data/webrtc/ind...
chrome/test/data/webrtc/indexeddb.js:129: var reqGet = certById.get(0);
On 2016/05/09 16:42:38, jsbell wrote:
> Could just use certStore.get(0) - no need for the index here.

Done.

[jsbell, 2016-05-09 18:35:33]

still lgtm, but a suggestion...

https://codereview.chromium.org/1962673002/diff/20001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/indexeddb.js (right):

https://codereview.chromium.org/1962673002/diff/20001/chrome/test/data/webrtc...
chrome/test/data/webrtc/indexeddb.js:146: function
cloneCertificate_(certificate) {
Since all of the functions this calls return promises, this can be simplified
to:

function cloneCertificate_(certificate) {
  return saveCertificate_(certificate)
    .then(loadCertificate_)
    .then(function(clone) {
      // Load successful.
      if (clone === null)
        failTest('...');
      return clone;
    });
}

[hbos_chromium, 2016-05-10 06:37:59]

PTAL phoglund

https://codereview.chromium.org/1962673002/diff/20001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/indexeddb.js (right):

https://codereview.chromium.org/1962673002/diff/20001/chrome/test/data/webrtc...
chrome/test/data/webrtc/indexeddb.js:146: function
cloneCertificate_(certificate) {
On 2016/05/09 18:35:33, jsbell wrote:
> Since all of the functions this calls return promises, this can be simplified
> to:
> 
> function cloneCertificate_(certificate) {
>   return saveCertificate_(certificate)
>     .then(loadCertificate_)
>     .then(function(clone) {
>       // Load successful.
>       if (clone === null)
>         failTest('...');
>       return clone;
>     });
> }

Nice. I'll do that before landing.

[phoglund_chromium, 2016-05-10 08:10:17]

lgtm

https://codereview.chromium.org/1962673002/diff/20001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/indexeddb.js (right):

https://codereview.chromium.org/1962673002/diff/20001/chrome/test/data/webrtc...
chrome/test/data/webrtc/indexeddb.js:102: if (gDatabase === null)
Nit: indent 2?

[hbos_chromium, 2016-05-10 11:29:52]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2016-05-10 11:29:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
phoglund@chromium.org, jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1962673002/#ps60001
(title: "Addressed nits")

[commit-bot: I haz the power, 2016-05-10 11:30:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962673002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962673002/60001

[commit-bot: I haz the power, 2016-05-10 12:14:46]

Description was changed from

==========
WebRtcBrowserTest prep-CL for IndexedDB cloning of RTCCertificate.

This is part of larger work that will allow persistent storage of
RTCCertificate in IndexedDB. The big picture can be seen here:
https://codereview.chromium.org/1949033002/

This CL implements part 3a+3b:

3. Unittests making sure RTCCertificate can be saved and loaded from IndexedDB
   a) chrome/test/data/webrtc/indexeddb.js added with functions for managing
      a database and saving and loading (cloning) certificates.
   b) WebRtcTestBase implementing C++ functions for calling the indexeddb.js
      API.

Part 3c is *NOT* implemented:
   c) WebRtcBrowserTest tests added which clone certificates and sets up calls
      using the clones.

This is because RTCCertificate cloning (pt.2) has not landed yet, so any attempt
to clone it in a unittest would fail. Part 2 + 3c will be done in a follow-up.

BUG=581354
==========

to

==========
WebRtcBrowserTest prep-CL for IndexedDB cloning of RTCCertificate.

This is part of larger work that will allow persistent storage of
RTCCertificate in IndexedDB. The big picture can be seen here:
https://codereview.chromium.org/1949033002/

This CL implements part 3a+3b:

3. Unittests making sure RTCCertificate can be saved and loaded from IndexedDB
   a) chrome/test/data/webrtc/indexeddb.js added with functions for managing
      a database and saving and loading (cloning) certificates.
   b) WebRtcTestBase implementing C++ functions for calling the indexeddb.js
      API.

Part 3c is *NOT* implemented:
   c) WebRtcBrowserTest tests added which clone certificates and sets up calls
      using the clones.

This is because RTCCertificate cloning (pt.2) has not landed yet, so any attempt
to clone it in a unittest would fail. Part 2 + 3c will be done in a follow-up.

BUG=581354
==========

[commit-bot: I haz the power, 2016-05-10 12:14:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-10 12:16:19]

Description was changed from

==========
WebRtcBrowserTest prep-CL for IndexedDB cloning of RTCCertificate.

This is part of larger work that will allow persistent storage of
RTCCertificate in IndexedDB. The big picture can be seen here:
https://codereview.chromium.org/1949033002/

This CL implements part 3a+3b:

3. Unittests making sure RTCCertificate can be saved and loaded from IndexedDB
   a) chrome/test/data/webrtc/indexeddb.js added with functions for managing
      a database and saving and loading (cloning) certificates.
   b) WebRtcTestBase implementing C++ functions for calling the indexeddb.js
      API.

Part 3c is *NOT* implemented:
   c) WebRtcBrowserTest tests added which clone certificates and sets up calls
      using the clones.

This is because RTCCertificate cloning (pt.2) has not landed yet, so any attempt
to clone it in a unittest would fail. Part 2 + 3c will be done in a follow-up.

BUG=581354
==========

to

==========
WebRtcBrowserTest prep-CL for IndexedDB cloning of RTCCertificate.

This is part of larger work that will allow persistent storage of
RTCCertificate in IndexedDB. The big picture can be seen here:
https://codereview.chromium.org/1949033002/

This CL implements part 3a+3b:

3. Unittests making sure RTCCertificate can be saved and loaded from IndexedDB
   a) chrome/test/data/webrtc/indexeddb.js added with functions for managing
      a database and saving and loading (cloning) certificates.
   b) WebRtcTestBase implementing C++ functions for calling the indexeddb.js
      API.

Part 3c is *NOT* implemented:
   c) WebRtcBrowserTest tests added which clone certificates and sets up calls
      using the clones.

This is because RTCCertificate cloning (pt.2) has not landed yet, so any attempt
to clone it in a unittest would fail. Part 2 + 3c will be done in a follow-up.

BUG=581354

Committed: https://crrev.com/042933a05d5d0bde0aaea8882c7d74b79810c799
Cr-Commit-Position: refs/heads/master@{#392582}
==========

[commit-bot: I haz the power, 2016-05-10 12:16:21]

Patchset 4 (id:??) landed as
https://crrev.com/042933a05d5d0bde0aaea8882c7d74b79810c799
Cr-Commit-Position: refs/heads/master@{#392582}

[hbos_chromium, 2016-05-10 12:17:27]

https://codereview.chromium.org/1962673002/diff/20001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/indexeddb.js (right):

https://codereview.chromium.org/1962673002/diff/20001/chrome/test/data/webrtc...
chrome/test/data/webrtc/indexeddb.js:102: if (gDatabase === null)
On 2016/05/10 08:10:16, phoglund_chrome wrote:
> Nit: indent 2?

Done.