CEF3 R1107 geolocation and download handler failures

[fddima]

OK, sounds like the test is flaky. I'll take a look and see if I can get it to behave better.

Thanks. But i'm not sure that problem that test is flaky. I'm doesn't find any logical error inside test (may be my miss).
I'm add logging to stdout (probably it is possible via gtest?) to OnBeforeDownload/OnDownloadUpdated methods, and really looks that OnBeforeDownload doesn't called before first OnDownloadUpdate call:

Successfull run:

Code: Select all

C:\prj.build\chromiumembedded\cef3-trunk\src\build\Debug>cef_unittests.exe --gtest_filter=DownloadTest.Download
Note: Google Test filter = DownloadTest.Download
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DownloadTest
[ RUN      ] DownloadTest.Download
[0225/223422:ERROR:renderer_main.cc(203)] Running without renderer sandbox
[0225/223422:INFO:scheme_impl.cc(257)] CefUrlRequestManager hit for http://test-download/test.html
[0225/223422:INFO:scheme_impl.cc(257)] CefUrlRequestManager hit for http://test-download/download.txt
>>>OnBeforeDownload
>>>OnDownloadUpdated
>>>OnDownloadUpdated
>>>OnDownloadUpdated
>>>OnDownloadUpdated
>>>OnDownloadUpdated
[       OK ] DownloadTest.Download (945 ms)
[----------] 1 test from DownloadTest (954 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (958 ms total)
[  PASSED  ] 1 test.


And failed run:

Code: Select all

C:\prj.build\chromiumembedded\cef3-trunk\src\build\Debug>cef_unittests.exe --gtest_filter=DownloadTest.Download
Note: Google Test filter = DownloadTest.Download
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DownloadTest
[ RUN      ] DownloadTest.Download
[0225/223627:ERROR:renderer_main.cc(203)] Running without renderer sandbox
[0225/223627:INFO:scheme_impl.cc(257)] CefUrlRequestManager hit for http://test-download/test.html
[0225/223628:INFO:scheme_impl.cc(257)] CefUrlRequestManager hit for http://test-download/download.txt
>>>OnDownloadUpdated
tests\unittests\download_unittest.cc(192): error: Value of: got_on_before_download_
  Actual: false
Expected: true
tests\unittests\download_unittest.cc(200): error: Value of: download_item->GetId()
  Actual: 1
Expected: download_id_
Which is: -842150451
>>>OnDownloadUpdated
tests\unittests\download_unittest.cc(192): error: Value of: got_on_before_download_
  Actual: false
Expected: true
tests\unittests\download_unittest.cc(200): error: Value of: download_item->GetId()
  Actual: 1
Expected: download_id_
Which is: -842150451
>>>OnDownloadUpdated
tests\unittests\download_unittest.cc(192): error: Value of: got_on_before_download_
  Actual: false
Expected: true
tests\unittests\download_unittest.cc(200): error: Value of: download_item->GetId()
  Actual: 1
Expected: download_id_
Which is: -842150451
>>>>>>>>>>>>OnBeforeDownload
tests\unittests\download_unittest.cc(154): error: Value of: got_on_download_updated_
  Actual: true
Expected: false
tests\unittests\download_unittest.cc(170): error: Value of: download_item->GetCurrentSpeed()
  Actual: 2571
Expected: 0LL
Which is: 0
tests\unittests\download_unittest.cc(171): error: Value of: download_item->GetPercentComplete()
  Actual: 100
Expected: 0
tests\unittests\download_unittest.cc(174): error: Value of: download_item->GetReceivedBytes()
  Actual: 18
Expected: 0LL
Which is: 0
>>>OnDownloadUpdated
>>>OnDownloadUpdated
[  FAILED  ] DownloadTest.Download (973 ms)
[----------] 1 test from DownloadTest (974 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (981 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DownloadTest.Download


I.e. looks like OnDownloadUpdated now called before OnBeforeDownload. But... as i'm understand - OnBeforeDownload must be called before real downloading perform? Or it is really possible that OnDownloadUpdated called before OnBeforeDownload, but once we hit OnBeforeDownload - we can cancel downloading? If it is really possible (OnDownloadUpdate before OnBeforeDownload - then this fact must be reflected in documentation, and of course test must be fixed... (how?)).

PS: I delete previous cef_unittests executable, so it is hard to try run this test to on more early revision. So it is probably related to last chrome update or not related...

[magreenblatt]

It may be possible for OnDownloadUpdated to be called before OnBeforeDownload due to the way that DownloadManager is implemented in Chromium. For example, depending on timing CefDownloadManagerDelegate::OnDownloadUpdated may be called before CefDownloadManagerDelegate::DetermineDownloadTarget. We may need to fix this in CefDownloadManagerDelegate so that it appears to CEF users that OnBeforeDownload is always called first.

[fddima]

It may be possible for OnDownloadUpdated to be called before OnBeforeDownload due to the way that DownloadManager is implemented in Chromium. For example, depending on timing CefDownloadManagerDelegate::OnDownloadUpdated may be called before CefDownloadManagerDelegate::DetermineDownloadTarget. We may need to fix this in CefDownloadManagerDelegate so that it appears to CEF users that OnBeforeDownload is always called first.

May be early OnDownloadUpdated method is also not a bad...
Chrome have relatively big delay until save dialog appears, so if early OnDownloadUpdated method allow UI react more early - it is good.
Need to be playing around this before.
But from compatibility point of view - of course you are absolutely right.

[magreenblatt]

Right, with this Chromium update Google no longer provides default API keys. People will need to register their own for local builds if they want geolocation support.

Filed as https://code.google.com/p/chromiumembed ... ail?id=905. Google has disabled their default API keys from the server end so they likely won't work with any CEF versions.