Fix Guest geolocation API, we were using |bridge_id| and |request_id| interchangeably which is wron…

Fix Guest geolocation API, we were using |bridge_id| and |request_id| interchangeably which is wrong.
A |bridge_id| is specific to geolocation request in chrome/browser/geolocation/* where request_id is BrowserPluginGuest's local id of all types of permission requests.
Fix WebViewTest.GeolocationAPIEmbedderHasAccess* test flakiness, after frustrating hours, I found that ui_test_utils::OverrideGeolocation() is not set up to mock multiple geolocation requests reliably, made tests separate and use cached values for geolocation requests other than the first (/w maxiumAge + timeout = 0).

BUG=225819, 223877
TEST=Added tests to cover bridge_id change, using iframe to trigger second geolocation request from different frame (testMultipleBridgeIdAllow).
Fixed WebViewTest.GeolocationAPIEmbedderHasAccess flakiness due to mocking geolocation (ran them separately on each of win_rel, linux_chromeos, mac_rel and linux_rel trybots for 50/100 times).


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193230

[Fady Samuel, 2013-04-05 15:07:21]

LGTM but please add a better description of bridge ID v.s. request ID.

https://chromiumcodereview.appspot.com/13712002/diff/2001/chrome/test/data/ex...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/13712002/diff/2001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/geolocation/embedder_has_permission/embedder.js:190:
// different bridge_id-s.
Every iframe has a different bridge ID?

https://chromiumcodereview.appspot.com/13712002/diff/2001/chrome/test/data/ex...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/geolocation_access_iframe.html
(right):

https://chromiumcodereview.appspot.com/13712002/diff/2001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/geolocation/geolocation_access_iframe.html:21:
// location from <iframe> again makes Geolocation flaky and sad (b/c of
I don't really understand the request ID v.s. bridge ID relationship. Could you
please describe it in the issue description?

[lazyboy, 2013-04-05 18:30:07]

Couple updates done in the CL:
1. Added an explicit test to exercise
BPGeolocationPermissionContext::CancelGeolocationPermission() code path.
2. Removed a wrong DCHECK from BPGeolocationPermissionContext.

https://chromiumcodereview.appspot.com/13712002/diff/2001/chrome/test/data/ex...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/13712002/diff/2001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/geolocation/embedder_has_permission/embedder.js:190:
// different bridge_id-s.
On 2013/04/05 15:07:21, Fady Samuel wrote:
> Every iframe has a different bridge ID?

Yes, in my test cases.

https://chromiumcodereview.appspot.com/13712002/diff/2001/chrome/test/data/ex...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/geolocation_access_iframe.html
(right):

https://chromiumcodereview.appspot.com/13712002/diff/2001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/geolocation/geolocation_access_iframe.html:21:
// location from <iframe> again makes Geolocation flaky and sad (b/c of
On 2013/04/05 15:07:21, Fady Samuel wrote:
> I don't really understand the request ID v.s. bridge ID relationship. Could
you
> please describe it in the issue description?
Added description.

[Fady Samuel, 2013-04-05 20:03:45]

https://codereview.chromium.org/13712002/diff/7001/chrome/test/data/extension...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/embedder_has_permission/embedder.js
(right):

https://codereview.chromium.org/13712002/diff/7001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/geolocation/embedder_has_permission/embedder.js:73:
embedder.test.assertEq = function(a, b, opt) {
What's the point of opt here?

[lazyboy, 2013-04-05 20:38:32]

https://chromiumcodereview.appspot.com/13712002/diff/7001/chrome/test/data/ex...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/13712002/diff/7001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/geolocation/embedder_has_permission/embedder.js:73:
embedder.test.assertEq = function(a, b, opt) {
On 2013/04/05 20:03:46, Fady Samuel wrote:
> What's the point of opt here?

Leftover from tests, removed. Thanks.
Done.

[mthiesse, 2013-04-09 15:25:39]

Looking at the how geolocation requests work I think we should consider making
some changes to fit the rest of the permissions better (and allow them to have
the same cleanup behavior as other permission types in my CL).
Specifically, I think we should do the check to see if the embedder/app has
geolocation permission before we send the request to the permission API. 
This has two benefits that I can see:
1. Apps won't get redundant/confusing requests for geolocation permissions (i.e.
the app allows geolocation requests, but geolocation still doesn't work because
the permission isn't present).
2. We can clean up the request in OnRespondPermission like I do with all of the
other request types, because we won't have to wait on callbacks to complete
before deleting the request.

[lazyboy, 2013-04-09 18:00:51]

I've run the newly fixed and enabled tests in trybots for 50 - 100 times, the
tests didn't flake (e.g.
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/132504).
Although other issue I'm having is that similar run without my change doesn't
seem to reproduce the flake in trybots :(
I'll proceed with enabling the tests.


On 2013/04/09 15:25:39, mthiesse wrote:
> Looking at the how geolocation requests work I think we should consider making
> some changes to fit the rest of the permissions better (and allow them to have
> the same cleanup behavior as other permission types in my CL).
> Specifically, I think we should do the check to see if the embedder/app has
> geolocation permission before we send the request to the permission API. 
> This has two benefits that I can see:
> 1. Apps won't get redundant/confusing requests for geolocation permissions
(i.e.
> the app allows geolocation requests, but geolocation still doesn't work
because
> the permission isn't present).
Agreed.
> 2. We can clean up the request in OnRespondPermission like I do with all of
the
> other request types, because we won't have to wait on callbacks to complete
> before deleting the request.
This is probably harder because of the way *GeolocationPermissionContext class
is interfaced, we have a bug to investigate if that class can be removed
(crbug.com/223610).

[commit-bot: I haz the power, 2013-04-09 18:19:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/13712002/7005

[commit-bot: I haz the power, 2013-04-09 18:19:23]

Failed to apply patch for chrome/browser/extensions/web_view_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/web_view_browsertest.cc
  Hunk #1 succeeded at 352 (offset 68 lines).
  Hunk #2 FAILED at 863.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/web_view_browsertest.cc.rej

Patch:       chrome/browser/extensions/web_view_browsertest.cc
Index: chrome/browser/extensions/web_view_browsertest.cc
diff --git a/chrome/browser/extensions/web_view_browsertest.cc
b/chrome/browser/extensions/web_view_browsertest.cc
index
df713f7f8115208c4e8e114cfe8b8f6c2bceab03..58efd3f15daee05aa4b1608f0810113c01cb36b7
100644
--- a/chrome/browser/extensions/web_view_browsertest.cc
+++ b/chrome/browser/extensions/web_view_browsertest.cc
@@ -284,6 +284,30 @@ class WebViewTest : public
extensions::PlatformAppBrowserTest {
     EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
   }
 
+  void GeolocationTestHelper(const std::string& test_name) {
+    ASSERT_TRUE(StartTestServer());  // For serving guest pages.
+    ExtensionTestMessageListener launched_listener("Launched", false);
+    LoadAndLaunchPlatformApp("web_view/geolocation/embedder_has_permission");
+    ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
+
+    content::WebContents* embedder_web_contents =
+        GetFirstShellWindowWebContents();
+    ASSERT_TRUE(embedder_web_contents);
+
+    ExtensionTestMessageListener done_listener("DoneGeolocationTest", false);
+    EXPECT_TRUE(content::ExecuteScript(
+                    embedder_web_contents,
+                    base::StringPrintf("runGeolocationTest('%s')",
+                                       test_name.c_str())));
+    done_listener.WaitUntilSatisfied();
+    bool has_test_passed;
+    EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+                    embedder_web_contents,
+                    "window.domAutomationController.send(hasTestPassed());",
+                    &has_test_passed));
+    ASSERT_TRUE(has_test_passed);
+  }
+
   scoped_ptr<content::FakeSpeechRecognitionManager>
       fake_speech_recognition_manager_;
 };
@@ -839,13 +863,38 @@ IN_PROC_BROWSER_TEST_F(WebViewTest,
GeolocationAPIEmbedderHasNoAccess) {
           << message_;
 }
 
-// Embedder has geolocation permission for this test.
-// Note that the test name prefix must be "GeolocationAPI".
-IN_PROC_BROWSER_TEST_F(WebViewTest, FLAKY_GeolocationAPIEmbedderHasAccess) {
+// In following GeolocationAPIEmbedderHasAccess* tests, embedder (i.e. the
+// platform app) has geolocation permission
+//
+// Note that these test names must be "GeolocationAPI" prefixed (b/c we mock
out
+// geolocation in this case).
+//
+// Also note that these are run separately because OverrideGeolocation()
doesn't
+// mock out geolocation for multiple navigator.geolocation calls properly and
+// the tests become flaky.
+// GeolocationAPI* test 1 of 3.
+IN_PROC_BROWSER_TEST_F(WebViewTest, GeolocationAPIEmbedderHasAccessAllow) {
+  GeolocationTestHelper("testAllow");
+}
+
+// GeolocationAPI* test 2 of 3.
+IN_PROC_BROWSER_TEST_F(WebViewTest, GeolocationAPIEmbedderHasAccessDeny) {
+  GeolocationTestHelper("testDeny");
+}
+
+// GeolocationAPI* test 3 of 3.
+IN_PROC_BROWSER_TEST_F(WebViewTest,
+                       GeolocationAPIEmbedderHasAccessMultipleBridgeIdAllow) {
+  GeolocationTestHelper("testMultipleBridgeIdAllow");
+}
+
+// Tests that
+//
BrowserPluginGeolocationPermissionContext::CancelGeolocationPermissionRequest
+// is handled correctly (and does not crash).
+IN_PROC_BROWSER_TEST_F(WebViewTest, GeolocationAPICancelGeolocation) {
   ASSERT_TRUE(StartTestServer());  // For serving guest pages.
   ASSERT_TRUE(RunPlatformAppTest(
-      "platform_apps/web_view/geolocation/embedder_has_permission"))
-          << message_;
+        "platform_apps/web_view/geolocation/cancel_request")) << message_;
 }
 
 // Disabled on win debug bots due to flaky timeouts.

[commit-bot: I haz the power, 2013-04-09 19:42:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/13712002/18002

[commit-bot: I haz the power, 2013-04-09 22:11:20]

Change committed as 193230