<webview>: Refactor Permission API to chrome

<webview>: Refactor Permission API to chrome

BUG=166165
Test=WebViewTest.*, WebViewInteractiveTest.*
TBR=cdn@chromium.org for removing IPCs in browser_plugin_messages.h
NOTRY=true, previous patch worked, just merging.

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

[lazyboy, 2013-08-05 09:26:58]

https://chromiumcodereview.appspot.com/21297005/diff/6001/chrome/browser/gues...
File chrome/browser/guestview/webview/webview_guest.h (right):

https://chromiumcodereview.appspot.com/21297005/diff/6001/chrome/browser/gues...
chrome/browser/guestview/webview/webview_guest.h:71: // |user_input|.
Also say what the return value is

https://chromiumcodereview.appspot.com/21297005/diff/6001/chrome/renderer/res...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/21297005/diff/6001/chrome/renderer/res...
chrome/renderer/resources/extensions/web_view.js:39: 'dialog': {
mixing this up with stable here seems a bit unclear to the reader, can we keep
this separate in web_view_experimental.js, probably in a
WEB_VIEW_EXPERIMENTAL_EXT_EVENTS object there?

https://chromiumcodereview.appspot.com/21297005/diff/6001/chrome/renderer/res...
chrome/renderer/resources/extensions/web_view.js:86: fields: ['initialHeight',
'initialWidth', 'targetUrl',
nit: here and below, since these don't fit in one line, better put each of them
in separate lines.

https://chromiumcodereview.appspot.com/21297005/diff/6001/chrome/renderer/res...
chrome/renderer/resources/extensions/web_view.js:405: if (event[field] !=
undefined) {
use !== undefined

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:418: callback.Run(false,
"");
We should also not keep the request stored in permission_requests_map_, better
call RespondToPermissionRequest(.., false, ..);

Also, delegate.RequestPermission's bool return value seems confusing, can't this
be just void? The notion would be if there's a delegate, it must process
"RequestPermission" (even if that's no-op).

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.h:286: int request_id,
nit: can fit in previous line.

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/public/brow...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/public/brow...
content/public/browser/browser_plugin_guest_delegate.h:23: typedef
base::Callback<void(bool /* allow */,
nit: indentation is off

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/public/brow...
content/public/browser/browser_plugin_guest_delegate.h:52: virtual bool
RequestPermission(
Add comment about the method.

[Fady Samuel, 2013-08-06 07:14:35]

PTAL addressed comments

https://codereview.chromium.org/21297005/diff/6001/chrome/browser/guestview/w...
File chrome/browser/guestview/webview/webview_guest.h (right):

https://codereview.chromium.org/21297005/diff/6001/chrome/browser/guestview/w...
chrome/browser/guestview/webview/webview_guest.h:71: // |user_input|.
On 2013/08/05 09:26:58, lazyboy wrote:
> Also say what the return value is

Done.

https://codereview.chromium.org/21297005/diff/6001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/21297005/diff/6001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:39: 'dialog': {
On 2013/08/05 09:26:58, lazyboy wrote:
> mixing this up with stable here seems a bit unclear to the reader, can we keep
> this separate in web_view_experimental.js, probably in a
> WEB_VIEW_EXPERIMENTAL_EXT_EVENTS object there?

Done.

https://codereview.chromium.org/21297005/diff/6001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:86: fields: ['initialHeight',
'initialWidth', 'targetUrl',
On 2013/08/05 09:26:58, lazyboy wrote:
> nit: here and below, since these don't fit in one line, better put each of
them
> in separate lines.

Done.

https://codereview.chromium.org/21297005/diff/6001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:405: if (event[field] !=
undefined) {
On 2013/08/05 09:26:58, lazyboy wrote:
> use !== undefined

Done.

https://codereview.chromium.org/21297005/diff/6001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/21297005/diff/6001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:418: callback.Run(false,
"");
On 2013/08/05 09:26:58, lazyboy wrote:
> We should also not keep the request stored in permission_requests_map_, better
> call RespondToPermissionRequest(.., false, ..);
> 

That's exactly what callback.Run(false, "") does.

> Also, delegate.RequestPermission's bool return value seems confusing, can't
this
> be just void? The notion would be if there's a delegate, it must process
> "RequestPermission" (even if that's no-op).

The default behavior should be to reject permissions for non-WebView Guestviews,
and for BrowserPlugin without a content embedder.

https://codereview.chromium.org/21297005/diff/6001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/21297005/diff/6001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:286: int request_id,
On 2013/08/05 09:26:58, lazyboy wrote:
> nit: can fit in previous line.

Done.

https://codereview.chromium.org/21297005/diff/6001/content/public/browser/bro...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/21297005/diff/6001/content/public/browser/bro...
content/public/browser/browser_plugin_guest_delegate.h:23: typedef
base::Callback<void(bool /* allow */,
On 2013/08/05 09:26:58, lazyboy wrote:
> nit: indentation is off

I'm not actually sure what the correct indentation is. Fixed.

https://codereview.chromium.org/21297005/diff/6001/content/public/browser/bro...
content/public/browser/browser_plugin_guest_delegate.h:52: virtual bool
RequestPermission(
On 2013/08/05 09:26:58, lazyboy wrote:
> Add comment about the method.

Done.

[lazyboy, 2013-08-08 19:58:01]

lgtm

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/21297005/diff/6001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_guest.cc:418: callback.Run(false,
"");
On 2013/08/06 07:14:40, Fady Samuel wrote:
> On 2013/08/05 09:26:58, lazyboy wrote:
> > We should also not keep the request stored in permission_requests_map_,
better
> > call RespondToPermissionRequest(.., false, ..);
> > 
> 
> That's exactly what callback.Run(false, "") does.
Ah, I misread this part, for some reason I was thinking |callback| is something
else. Thanks.
> 
> > Also, delegate.RequestPermission's bool return value seems confusing, can't
> this
> > be just void? The notion would be if there's a delegate, it must process
> > "RequestPermission" (even if that's no-op).
> 
> The default behavior should be to reject permissions for non-WebView
Guestviews,
> and for BrowserPlugin without a content embedder.

https://chromiumcodereview.appspot.com/21297005/diff/22001/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://chromiumcodereview.appspot.com/21297005/diff/22001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view_experimental.js:18: var
createEvent = require('webView').CreateEvent;
nit: can sort these.

[Fady Samuel, 2013-08-08 20:13:45]

+jam@ for browser_plugin_guest_delegate.*

https://codereview.chromium.org/21297005/diff/22001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://codereview.chromium.org/21297005/diff/22001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_experimental.js:18: var
createEvent = require('webView').CreateEvent;
On 2013/08/08 19:58:01, lazyboy wrote:
> nit: can sort these.

Done.

[jam, 2013-08-08 21:22:11]

https://codereview.chromium.org/21297005/diff/27001/content/public/browser/br...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/21297005/diff/27001/content/public/browser/br...
content/public/browser/browser_plugin_guest_delegate.h:8: #include
"base/callback.h"
nit: callback_forward is enough?

https://codereview.chromium.org/21297005/diff/27001/content/public/browser/br...
content/public/browser/browser_plugin_guest_delegate.h:24: const std::string& /*
user_input */)>
nit: the const should line up with the bool

also, convention for content api is to put the callback right before the method
which uses it

https://codereview.chromium.org/21297005/diff/27001/content/public/common/bro...
File content/public/common/browser_plugin/browser_plugin_message_enums.h
(right):

https://codereview.chromium.org/21297005/diff/27001/content/public/common/bro...
content/public/common/browser_plugin/browser_plugin_message_enums.h:6: #define
CONTENT_COMMON_BROWSER_PLUGIN_BROWSER_PLUGIN_MESSAGE_ENUMS_H_
1) no browser_plugin directory in content public
2) update header guard

https://codereview.chromium.org/21297005/diff/27001/content/public/common/bro...
content/public/common/browser_plugin/browser_plugin_message_enums.h:10:
BrowserPluginPermissionTypeUnknown,
chrome style guide (and which we stick closely to in content api) is
BROWSER_PLUGIN_PERMISSION_TYPE_UNKNOWN

[Fady Samuel, 2013-08-08 21:42:49]

PTAL

https://codereview.chromium.org/21297005/diff/27001/content/public/browser/br...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/21297005/diff/27001/content/public/browser/br...
content/public/browser/browser_plugin_guest_delegate.h:8: #include
"base/callback.h"
On 2013/08/08 21:22:11, jam wrote:
> nit: callback_forward is enough?

Done.

https://codereview.chromium.org/21297005/diff/27001/content/public/browser/br...
content/public/browser/browser_plugin_guest_delegate.h:24: const std::string& /*
user_input */)>
On 2013/08/08 21:22:11, jam wrote:
> nit: the const should line up with the bool
> 
> also, convention for content api is to put the callback right before the
method
> which uses it

Done.

https://codereview.chromium.org/21297005/diff/27001/content/public/common/bro...
File content/public/common/browser_plugin/browser_plugin_message_enums.h
(right):

https://codereview.chromium.org/21297005/diff/27001/content/public/common/bro...
content/public/common/browser_plugin/browser_plugin_message_enums.h:6: #define
CONTENT_COMMON_BROWSER_PLUGIN_BROWSER_PLUGIN_MESSAGE_ENUMS_H_
On 2013/08/08 21:22:11, jam wrote:
> 1) no browser_plugin directory in content public
> 2) update header guard

Done.

https://codereview.chromium.org/21297005/diff/27001/content/public/common/bro...
content/public/common/browser_plugin/browser_plugin_message_enums.h:10:
BrowserPluginPermissionTypeUnknown,
On 2013/08/08 21:22:11, jam wrote:
> chrome style guide (and which we stick closely to in content api) is
> BROWSER_PLUGIN_PERMISSION_TYPE_UNKNOWN

Done.

[jam, 2013-08-09 00:07:27]

https://codereview.chromium.org/21297005/diff/27001/content/public/browser/br...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/21297005/diff/27001/content/public/browser/br...
content/public/browser/browser_plugin_guest_delegate.h:24: const std::string& /*
user_input */)>
On 2013/08/08 21:42:49, Fady Samuel wrote:
> On 2013/08/08 21:22:11, jam wrote:
> > nit: the const should line up with the bool
> > 
> > also, convention for content api is to put the callback right before the
> method
> > which uses it

where?

> 
> Done.

https://codereview.chromium.org/21297005/diff/33001/content/public/common/bro...
File content/public/common/browser_plugin_message_enums.h (right):

https://codereview.chromium.org/21297005/diff/33001/content/public/common/bro...
content/public/common/browser_plugin_message_enums.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
nit: rename this file to browser_plugin_permission_type.h

https://codereview.chromium.org/21297005/diff/33001/content/public/common/bro...
content/public/common/browser_plugin_message_enums.h:12: // Download.
nit: this, line 15, and line 21 should be taken out as they don't serve a
purpose

https://codereview.chromium.org/21297005/diff/33001/content/public/common/bro...
content/public/common/browser_plugin_message_enums.h:24: 
nit: extra line

[Fady Samuel, 2013-08-09 15:52:58]

PTAL

https://codereview.chromium.org/21297005/diff/33001/content/public/common/bro...
File content/public/common/browser_plugin_message_enums.h (right):

https://codereview.chromium.org/21297005/diff/33001/content/public/common/bro...
content/public/common/browser_plugin_message_enums.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/08/09 00:07:27, jam wrote:
> nit: rename this file to browser_plugin_permission_type.h

Done.

https://codereview.chromium.org/21297005/diff/33001/content/public/common/bro...
content/public/common/browser_plugin_message_enums.h:12: // Download.
On 2013/08/09 00:07:27, jam wrote:
> nit: this, line 15, and line 21 should be taken out as they don't serve a
> purpose

Done.

https://codereview.chromium.org/21297005/diff/33001/content/public/common/bro...
content/public/common/browser_plugin_message_enums.h:24: 
On 2013/08/09 00:07:27, jam wrote:
> nit: extra line

Done.

[jam, 2013-08-09 19:29:17]

lgtm

[commit-bot: I haz the power, 2013-08-09 19:32:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/21297005/50003

[commit-bot: I haz the power, 2013-08-09 20:31:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-09 20:36:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/21297005/50003

[commit-bot: I haz the power, 2013-08-10 02:40:07]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 02:55:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/21297005/50003

[commit-bot: I haz the power, 2013-08-10 02:56:25]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 588.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
d9450ab274a6ba6bc803aaacb901062932382cb3..50c6ab75b8bb8bea2af0649982b018efdcfd2242
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -588,6 +588,7 @@ enum HistogramValue {
   RECOVERYPRIVATE_DESTROYPARTITIONS,
   FEEDBACKPRIVATE_GETSTRINGS,
   LOGPRIVATE_GETHISTORICAL,
+  WEBVIEW_SETPERMISSION,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-08-12 14:41:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/21297005/80001

[commit-bot: I haz the power, 2013-08-12 19:54:27]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-08-12 19:56:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/21297005/80001

[commit-bot: I haz the power, 2013-08-12 19:59:25]

Change committed as 217057