Move allocate instance id to chrome/.

Move allocate instance id to chrome/.

BUG=364141
Test=No visible change.

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

[lazyboy, 2014-05-27 02:26:53]

Hi Fady, can you take a look at the patch at its current state? There are some
printf/debug code and some nits remaining, but other than that should be OK.
WebViewTest and WebViewInteractive tests pass.
BrowserPluginHostTest is probably screwed, I haven't checked.

[Fady Samuel, 2014-05-27 14:19:06]

A few initial comments. I'm really excited about this change! A lot of stuff is
moving out of content! I think a prerequisite to landing this change is to
remove all BrowserPluginHostTests.

I think we need to also discuss autonavigate at this point.

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:101:
browser_context())->GetNextInstanceID();
Let's make this a "GuestView" API, as this will apply for appview and others as
well.

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (left):

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:95: content::WebContents*
GuestViewManager::CreateGuest(
I'd like to chat about this method. I want to make it sufficiently general so
that it also works for <appview>. Ideally, what we would have is one method that
can create any type of guest.

Maybe we can determine what to do based on the extra params?

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:523: // TODO(lazyboy): This
codepath doesn't seem to tested by our tests.
This code comes from middle click or ctrl+click. I believe it is tested. I just
can't recall where :P

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (left):

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:23: var
WEB_VIEW_ATTRIBUTE_MINWIDTH = 'minwidth';
I think we might start needing to think about refactoring web_view.js. It's
getting larger and larger and harder to read.

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:265: var ALL_ATTRIBUTES =
WEB_VIEW_ATTRIBUTES;
No point in having this at all.

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:774:
WebView.allocateInstanceId(
Let's make it GuestView.allocateInstanceId

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1265: params.src = opt_src;
For consistency, ca we simply make this and userAgentOverride:

var params = {
 'api': 'webview',
 'instanceId': this.viewInstanceId,
 'name': this.name || '',
 'src': opt_src || '',
 'userAgentOverride': this.userAgentOverride || '',
 'storagePartitionId': opt_partitionId || '',
;

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1336:
proto.customElementDetached = false;
What is the purpose of this flag? Debugging? Can we get rid of it?

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1348:
LOG('this.customElementDetached was: ' + this.customElementDetached);
Let's remove all the listeners here.

https://codereview.chromium.org/299753011/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/299753011/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:283: has_render_view_ =
true;
This flag only makes sense for a new window with a referrer (via the new window
API).

[lazyboy, 2014-05-27 20:42:59]

Addressed comment in patchset #6

Once you think this looks reasonable, I'll remove all debug code/printfs and
re-up, thanks.

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:101:
browser_context())->GetNextInstanceID();
On 2014/05/27 14:19:06, Fady Samuel wrote:
> Let's make this a "GuestView" API, as this will apply for appview and others
as
> well.

Done.

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (left):

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:95: content::WebContents*
GuestViewManager::CreateGuest(
On 2014/05/27 14:19:06, Fady Samuel wrote:
> I'd like to chat about this method. I want to make it sufficiently general so
> that it also works for <appview>. Ideally, what we would have is one method
that
> can create any type of guest.
> 
> Maybe we can determine what to do based on the extra params?

OK.
We can do this in future CLs.

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/299753011/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:523: // TODO(lazyboy): This
codepath doesn't seem to tested by our tests.
On 2014/05/27 14:19:06, Fady Samuel wrote:
> This code comes from middle click or ctrl+click. I believe it is tested. I
just
> can't recall where :P

Ya you're right, WebViewInteractiveTest.NewWindow_OpenInNewTab does that.

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (left):

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:23: var
WEB_VIEW_ATTRIBUTE_MINWIDTH = 'minwidth';
On 2014/05/27 14:19:06, Fady Samuel wrote:
> I think we might start needing to think about refactoring web_view.js. It's
> getting larger and larger and harder to read.

Right, I was going in that direction by starting to use classes (like Partition)
so that it gets easier to refactor.

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:265: var ALL_ATTRIBUTES =
WEB_VIEW_ATTRIBUTES;
On 2014/05/27 14:19:06, Fady Samuel wrote:
> No point in having this at all.

Done.

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:774:
WebView.allocateInstanceId(
On 2014/05/27 14:19:06, Fady Samuel wrote:
> Let's make it GuestView.allocateInstanceId

Done.

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1265: params.src = opt_src;
On 2014/05/27 14:19:06, Fady Samuel wrote:
> For consistency, ca we simply make this and userAgentOverride:
> 
> var params = {
>  'api': 'webview',
>  'instanceId': this.viewInstanceId,
>  'name': this.name || '',
>  'src': opt_src || '',
>  'userAgentOverride': this.userAgentOverride || '',
>  'storagePartitionId': opt_partitionId || '',
> ;

There's a bit of difference between this and what we have currently,
DictionaryValue.GetString(.., &src) would return false when opt_ is undefined,
while ||ing with '' would return true.
However for our cases this is fine, and src can simply be opt_src.

Done.

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1336:
proto.customElementDetached = false;
On 2014/05/27 14:19:06, Fady Samuel wrote:
> What is the purpose of this flag? Debugging? Can we get rid of it?

This is for detecting an attach after detach.

https://codereview.chromium.org/299753011/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1348:
LOG('this.customElementDetached was: ' + this.customElementDetached);
On 2014/05/27 14:19:06, Fady Samuel wrote:
> Let's remove all the listeners here.

I need this to allocate new instanceId on attaching a node after detaching it.

https://codereview.chromium.org/299753011/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/299753011/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:283: has_render_view_ =
true;
On 2014/05/27 14:19:06, Fady Samuel wrote:
> This flag only makes sense for a new window with a referrer (via the new
window
> API).

So what change is required to get that info here to set the flag?

[Fady Samuel, 2014-05-29 20:35:22]

Some more comments! I may have more as we iterate.

https://codereview.chromium.org/299753011/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/299753011/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:1398:
request_info.Set(guestview::kStoragePartitionId,
Comment explaining why we need to send the partition ID.

https://codereview.chromium.org/299753011/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/299753011/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:237:
this.beforeFirstNavigation_ = true;
We haven't been using _ for privates now. Let's drop that for consistency. I'm
not sure what the style should be for privates... Maybe we should restore the
underscores?

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (left):

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:232:
OnNavigateGuest(instance_id_, params.src);
We probably get rid of the IPC message and the method right? This probably also
means we can probably get rid of BrowserPluginGuestDelegate::NavigateGuest.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:574:
SendQueuedMessages();
I don't think we need a separate BrowserPluginGuest::Attach method anymore.

Perhaps we can move this all into BrowserPluginGuest::Initialize or
BrowserPluginGuest::Attach?

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:244: // Inform the
embedder of the guest's information.
We can get rid of the parameters on Attach_ACK.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:257: // TODO(lazyboy):
Should not set this if src is empty.
In theory it should always be the case that we've navigated at this point. I
think

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:550:
BrowserPluginHostMsg_Attach_Params params,
If we're not modifying the attach params now, this can be a const reference.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:402:
std::queue<IPC::Message*> pending_messages_;
Can we convert this to:

std::deque<linked_ptr<IPC::Message> > pending_messages_ like we do in
GuestViewBase? This will make cleanup easier and more readable.

https://codereview.chromium.org/299753011/diff/80001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://codereview.chromium.org/299753011/diff/80001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:348:
TriggerEvent(browser_plugin::kEventInternalInstanceIDAllocated, &props);
We can probably delete TriggerEvent too!

[lazyboy, 2014-05-30 05:48:20]

Comments addressed in patchset #7

https://codereview.chromium.org/299753011/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/299753011/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:1398:
request_info.Set(guestview::kStoragePartitionId,
On 2014/05/29 20:35:23, Fady Samuel wrote:
> Comment explaining why we need to send the partition ID.

Done.

https://codereview.chromium.org/299753011/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/299753011/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:237:
this.beforeFirstNavigation_ = true;
On 2014/05/29 20:35:23, Fady Samuel wrote:
> We haven't been using _ for privates now. Let's drop that for consistency. I'm
> not sure what the style should be for privates... Maybe we should restore the
> underscores?

Ya, underscores are supposed to be for privates according to google js style
guide.
We can do that if we feel like, moved _'s for consistency.
Done.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (left):

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:232:
OnNavigateGuest(instance_id_, params.src);
On 2014/05/29 20:35:23, Fady Samuel wrote:
> We probably get rid of the IPC message and the method right? This probably
also
> means we can probably get rid of BrowserPluginGuestDelegate::NavigateGuest.

Done.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:574:
SendQueuedMessages();
On 2014/05/29 20:35:23, Fady Samuel wrote:
> I don't think we need a separate BrowserPluginGuest::Attach method anymore.
> 
> Perhaps we can move this all into BrowserPluginGuest::Initialize or
> BrowserPluginGuest::Attach?

I've made BPEmbedder always call Attach, and made Initialize private.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:244: // Inform the
embedder of the guest's information.
On 2014/05/29 20:35:23, Fady Samuel wrote:
> We can get rid of the parameters on Attach_ACK.

Done.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:257: // TODO(lazyboy):
Should not set this if src is empty.
On 2014/05/29 20:35:23, Fady Samuel wrote:
> In theory it should always be the case that we've navigated at this point. I
> think

Removed TODO.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:550:
BrowserPluginHostMsg_Attach_Params params,
On 2014/05/29 20:35:23, Fady Samuel wrote:
> If we're not modifying the attach params now, this can be a const reference.

Done.

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/299753011/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:402:
std::queue<IPC::Message*> pending_messages_;
On 2014/05/29 20:35:23, Fady Samuel wrote:
> Can we convert this to:
> 
> std::deque<linked_ptr<IPC::Message> > pending_messages_ like we do in
> GuestViewBase? This will make cleanup easier and more readable.

Done.

https://codereview.chromium.org/299753011/diff/80001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://codereview.chromium.org/299753011/diff/80001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:348:
TriggerEvent(browser_plugin::kEventInternalInstanceIDAllocated, &props);
On 2014/05/29 20:35:23, Fady Samuel wrote:
> We can probably delete TriggerEvent too!

Done.

[lazyboy, 2014-06-03 16:19:11]

Ping.

[lazyboy, 2014-06-03 16:19:11]

Ping.

[Fady Samuel, 2014-06-03 16:36:34]

https://codereview.chromium.org/299753011/diff/90001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/299753011/diff/90001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:28: void
ParsePartitionParam(const std::string& partition_str,
This seems like a <webview> feature. Can we move this WebViewGuest?

https://codereview.chromium.org/299753011/diff/90001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/299753011/diff/90001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:398: printf("DidAttach, no
src\n");
Is this even possible? I want to say this is invalid input. Maybe we should be
CHECK'ing out here? Perhaps log a UMA stat too to see if anyone is running into
this case in the wild.

https://codereview.chromium.org/299753011/diff/90001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/299753011/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:604: if (name != 'src' && name
!= 'partition') {
Early exit preferred?

https://codereview.chromium.org/299753011/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:726: LOG('parseSrcAttribute,
srcAttribute: ' + this.src);
Can we get rid of all these LOGs?

https://codereview.chromium.org/299753011/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:775: 'webview',
Are we doing anything with this at this point? If not, let's get rid of it and
introduce it in a separate CL.

https://codereview.chromium.org/299753011/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:776: this.viewInstanceId,
Do we need this?

https://codereview.chromium.org/299753011/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1332: };
Empty line between these two functions please.

https://codereview.chromium.org/299753011/diff/90001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1339: otherThis.instanceId =
undefined;
Maybe move this into a helper that makes it clear that we are resetting state?

https://codereview.chromium.org/299753011/diff/90001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/299753011/diff/90001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.cc:28: 
Remove extra space.

[lazyboy, 2014-06-03 19:17:58]

Removed all the debug logs. Patchset #9

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.cc:28: void
ParsePartitionParam(const std::string& partition_str,
On 2014/06/03 16:36:34, Fady Samuel wrote:
> This seems like a <webview> feature. Can we move this WebViewGuest?

I've moved the utility function to web_view_guest.cc, not sure if that's what
you meant.
Done.

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/browser/gu...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/browser/gu...
chrome/browser/guest_view/web_view/web_view_guest.cc:398: printf("DidAttach, no
src\n");
On 2014/06/03 16:36:34, Fady Samuel wrote:
> Is this even possible? I want to say this is invalid input. Maybe we should be
> CHECK'ing out here? Perhaps log a UMA stat too to see if anyone is running
into
> this case in the wild.

I see this on newwindow API test, e.window.attach() call.

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/renderer/r...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view.js:604: if (name != 'src' && name
!= 'partition') {
On 2014/06/03 16:36:34, Fady Samuel wrote:
> Early exit preferred?

Done.

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view.js:726: LOG('parseSrcAttribute,
srcAttribute: ' + this.src);
On 2014/06/03 16:36:34, Fady Samuel wrote:
> Can we get rid of all these LOGs?

Done.

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view.js:775: 'webview',
On 2014/06/03 16:36:34, Fady Samuel wrote:
> Are we doing anything with this at this point? If not, let's get rid of it and
> introduce it in a separate CL.

Done.

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view.js:776: this.viewInstanceId,
On 2014/06/03 16:36:34, Fady Samuel wrote:
> Do we need this?

Nope, removed.

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view.js:1332: };
On 2014/06/03 16:36:34, Fady Samuel wrote:
> Empty line between these two functions please.

Done.

https://chromiumcodereview.appspot.com/299753011/diff/90001/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view.js:1339: otherThis.instanceId =
undefined;
On 2014/06/03 16:36:34, Fady Samuel wrote:
> Maybe move this into a helper that makes it clear that we are resetting state?

Done.

https://chromiumcodereview.appspot.com/299753011/diff/90001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/299753011/diff/90001/content/browser/b...
content/browser/browser_plugin/browser_plugin_embedder.cc:28: 
On 2014/06/03 16:36:34, Fady Samuel wrote:
> Remove extra space.

Done.

[Fady Samuel, 2014-06-03 19:40:39]

Another round of comments. I think we're getting a lot closer :D

https://codereview.chromium.org/299753011/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/guestview/guestview_api.cc (right):

https://codereview.chromium.org/299753011/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/guestview/guestview_api.cc:19:
EXTENSION_FUNCTION_VALIDATE(!args_->GetSize());
Verify that we have webview permission here.

https://codereview.chromium.org/299753011/diff/130001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/299753011/diff/130001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_manager.cc:67: if
(extra_params->GetString(guestview::kStoragePartitionId,
The partition ID is really a <webview> concept. Can we move this GetString check
into ParsePartitionParam? Perhaps it can do the check there? kStoragePartitionId
should probably also be in web_view_constants.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:239:
//this.allocateInstanceId();
Remove?

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:278:
WebViewInternal.prototype.resetUponReAttachment = function() {
Call it resetUponReattachment. Reattachment is one word:
http://www.thefreedictionary.com/reattachment

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:471:
window.console.log('getter.partition');
Remove

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:472: var ret =
self.partition.toAttribute();
Return this directly?

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:473:
window.console.log('returning: ' + ret);
Remove

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:477:
window.console.log('setter.partition, value: ' + value);
Remove.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:484: // TODO(lazyboy):
CanRemovePartitionAttribute() when removeAttribute().
Fix this?

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:742: 
Unnecessary new line?

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:756: function(info) {
Give it a more specific name? instanceId?

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:1352: var otherThis =
privates(this).internal;
otherThis is a confusing name. This is grabbing the webViewInternal. Let's call
it that?

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:1356:
this.customElementDetached = false;
Can't we move this to resetUponReattachment?

https://codereview.chromium.org/299753011/diff/130001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/299753011/diff/130001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin.h:311:
base::WeakPtrFactory<BrowserPlugin> weak_ptr_factory_;
You can probably remove this.

[lazyboy, 2014-06-03 22:30:11]

Addressed in patchset #10

https://codereview.chromium.org/299753011/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/guestview/guestview_api.cc (right):

https://codereview.chromium.org/299753011/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/guestview/guestview_api.cc:19:
EXTENSION_FUNCTION_VALIDATE(!args_->GetSize());
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Verify that we have webview permission here.

Added CHECK.

https://codereview.chromium.org/299753011/diff/130001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/299753011/diff/130001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_manager.cc:67: if
(extra_params->GetString(guestview::kStoragePartitionId,
On 2014/06/03 19:40:40, Fady Samuel wrote:
> The partition ID is really a <webview> concept. Can we move this GetString
check
> into ParsePartitionParam? Perhaps it can do the check there?
kStoragePartitionId
> should probably also be in web_view_constants.

Done.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:239:
//this.allocateInstanceId();
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Remove?

Done.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:278:
WebViewInternal.prototype.resetUponReAttachment = function() {
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Call it resetUponReattachment. Reattachment is one word:
> http://www.thefreedictionary.com/reattachment

Done.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:471:
window.console.log('getter.partition');
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Remove

Wrong grep, fixed.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:472: var ret =
self.partition.toAttribute();
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Return this directly?

Done.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:473:
window.console.log('returning: ' + ret);
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Remove

Done.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:477:
window.console.log('setter.partition, value: ' + value);
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Remove.

Done.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:484: // TODO(lazyboy):
CanRemovePartitionAttribute() when removeAttribute().
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Fix this?

This should be taken care of.
I was thinking webview.removeAttribute('partition') would throw synchronous
exception (w/o my change), but it doesn't.
So, there's nothing to do here, removed the TODO and added a test for this
(WebViewTest.Shim_TestPartitionRemovalAfterNavigationFails).

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:616: if (name == 'src') {
FYI,
I'm removing this part as this won't help with auto navigate.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:742: 
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Unnecessary new line?

Removed.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:756: function(info) {
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Give it a more specific name? instanceId?

Done.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:1352: var otherThis =
privates(this).internal;
On 2014/06/03 19:40:40, Fady Samuel wrote:
> otherThis is a confusing name. This is grabbing the webViewInternal. Let's
call
> it that?

Done.

https://codereview.chromium.org/299753011/diff/130001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:1356:
this.customElementDetached = false;
On 2014/06/03 19:40:40, Fady Samuel wrote:
> Can't we move this to resetUponReattachment? 

Doesn't seem like, resetUponReattachment is under WebViewInternal, while this
member is under proto

https://codereview.chromium.org/299753011/diff/130001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/299753011/diff/130001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin.h:311:
base::WeakPtrFactory<BrowserPlugin> weak_ptr_factory_;
On 2014/06/03 19:40:40, Fady Samuel wrote:
> You can probably remove this.

This is still used in couple of places: ChildCompositingHelper and
ShowSadGraphic.

[Fady Samuel, 2014-06-04 04:54:19]

lgtm with nit.

https://codereview.chromium.org/299753011/diff/150001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/299753011/diff/150001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:239:
//this.allocateInstanceId();
Remove this?

[lazyboy, 2014-06-04 20:29:55]

+kalman for review.
This CL is adding two extension functions for <webview>
webview.navigate()
guestview.allocateInstanceId()

Specific files need to reviewed are:

A +	 chrome/browser/extensions/api/guestview/OWNERS	
A	 chrome/browser/extensions/api/guestview/guestview_api.h
A	 chrome/browser/extensions/api/guestview/guestview_api.cc
M	 chrome/browser/extensions/api/webview/webview_api.h	
M	 chrome/browser/extensions/api/webview/webview_api.cc

M	 chrome/common/extensions/api/_api_features.json
M	 chrome/common/extensions/api/api.gyp
A	 chrome/common/extensions/api/guestview.json
M	 chrome/common/extensions/api/webview.json

Thanks.

[not at google - send to devlin, 2014-06-04 20:56:47]

lg but I'd like all internal APIs to be explicitly named as such to avoid
confusion with public APIs.

https://chromiumcodereview.appspot.com/299753011/diff/190001/chrome/common/ex...
File chrome/common/extensions/api/_api_features.json (right):

https://chromiumcodereview.appspot.com/299753011/diff/190001/chrome/common/ex...
chrome/common/extensions/api/_api_features.json:440: "guestview": {
can we call this guestviewInternal? (and then reconcile that webview isn't
called webviewInternal).

(btw the directory appears to be guest_view, implying to me that this should be
guestView, same comment re webview...)

[lazyboy, 2014-06-05 02:19:52]

Patchset #14

https://chromiumcodereview.appspot.com/299753011/diff/190001/chrome/common/ex...
File chrome/common/extensions/api/_api_features.json (right):

https://chromiumcodereview.appspot.com/299753011/diff/190001/chrome/common/ex...
chrome/common/extensions/api/_api_features.json:440: "guestview": {
On 2014/06/04 20:56:48, kalman wrote:
> can we call this guestviewInternal? (and then reconcile that webview isn't
> called webviewInternal).
> 
> (btw the directory appears to be guest_view, implying to me that this should
be
> guestView, same comment re webview...)

I've renamed this to guestViewInternal.
Changed directory name under chrome/browser/extensions/api/guestview to
guest_view

For webview, can I do it in a separate patch?since we already have internal
functions there that are incorrectly named?

[not at google - send to devlin, 2014-06-05 16:51:57]

thanks, lgtm.

I don't think renaming the webview API to webViewInternal should be done in this
CL :)

[lazyboy, 2014-06-05 16:55:07]

+John for reviewing:
M	 content/browser/web_contents/web_contents_impl.cc

M	 content/public/browser/browser_plugin_guest_delegate.h	
M	 content/public/browser/browser_plugin_guest_manager.h
M	 content/public/browser/browser_plugin_guest_manager.cc

[lazyboy, 2014-06-05 16:55:28]

+John for reviewing:
M	 content/browser/web_contents/web_contents_impl.cc

M	 content/public/browser/browser_plugin_guest_delegate.h	
M	 content/public/browser/browser_plugin_guest_manager.h
M	 content/public/browser/browser_plugin_guest_manager.cc

[jam, 2014-06-05 20:47:49]

lgtm

[lazyboy, 2014-06-05 20:58:45]

+Nasko for IPC changes:
content/common/browser_plugin/browser_plugin_messages.h

+Alexei for histograms:
extension_function_histogram_value.h and
histograms.xml

[Alexei Svitkine (slow), 2014-06-05 21:01:29]

lgtm

[nasko, 2014-06-06 16:19:46]

Removing code almost always LGTM!

[lazyboy, 2014-06-06 22:56:39]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-06-06 22:57:15]

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

[lazyboy, 2014-06-07 06:29:02]

The CQ bit was unchecked by lazyboy@chromium.org

[lazyboy, 2014-06-07 06:30:01]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-06-07 06:31:46]

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

[commit-bot: I haz the power, 2014-06-07 09:54:38]

Change committed as 275674