Remove GuestWebContentsCreated

Remove GuestWebContentsCreated

This CL introduces GuestViewInternal.createGuest which replaces GuestViewInternal.allocateInstanceId.

This new internal API does the following:

1. Allocates an instance ID.
2. Creates a new guest delegate of the specified and guest WebContents, without navigating the new WebContents.
3. On attachment, the new WebContents is navigated.

In this CL, the particular GuestView type decides how to create the WebContents. Thus <webview> can create WebContents with SiteInstances of the form chrome-guest:// whereas <appview> can create guests with the SiteInstances matching the app they are hosting.

BrowserPluginGuestDelegate (e.g. WebViewGuest) is now passed in to the constructor of BrowserPluginGuest. Now, we can assume a delegate always exists. I added a DCHECK to the constructor. With this patch, BrowserPluginGuestDelegate outlives BrowserPluginGuest too so we can always assume delegate_ is safe to access.

BUG=364141
NOTRY=true

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

[lazyboy, 2014-06-17 23:46:46]

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/ap...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/ap...
chrome/browser/apps/web_view_browsertest.cc:6: #include
"base/memory/singleton.h"
Revert.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/ch...
File chrome/browser/chrome_content_browser_client.cc (left):

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/ch...
chrome/browser/chrome_content_browser_client.cc:828: void
ChromeContentBrowserClient::GuestWebContentsCreated(
After GuestWebContentsAttached() gets/got removed, you can clean up lots of
#includes now.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc
(right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:22:
std::string view_type;
Since this function has params now, use
extensions::api::..Function::Params to deserialize the params.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.cc:100: if (!guest_web_contents)
Should this be DCHECK instead?

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.cc:190: // GuestViewBase tracks the
lifetime of its embedder render process until it
This comment should also be class level comment for GuestViewBase.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.cc:216: // After attachment, this
GuestViewBase's lifetime is restricted to the
Same as above, lifetime should also be documented class level.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.cc:245: int
GuestViewBase::GetGuestInstanceID() const {
Why do you add this method? to make it virtual? In that case we can get rid of
guest_instance_id()

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_base.h (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.h:93: // This method can be overriden
by subclasses. This gives the derived class
"This method can be overriden by subclasses." -> I think the function being
virtual pretty much says that. So get rid of these.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.h:111: typedef
base::Callback<void(content::WebContents*)> WebContentsCallback;
WebContentsCreatedCallback

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.h:120: void Init(const std::string&
embedder_extension_id,
I found this function very confusing with the virtual Init().
One of them needs to be renamed.

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

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.cc:7: #include
"base/debug/stack_trace.h"
Revert.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.cc:228: // We might get some late
arriving messages at tear down. Let's let the
Would be cool if we had a test for this? Or is this pretty common?

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.h:57: content::RenderProcessHost*
embedder_render_process_host,
(Here and below) Why would this be non const?

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:120: if (!delegate_)
This would pretty much result in null pointer exception, we need to CHECK/DCHECK

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.h:115: WebContentsImpl*
CreateNewGuestWindow(WebContents::CreateParams* params);
I'd add a note that |params| could be modified.
Also add function comment.

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1428: &create_params);
|create_params| being a (mutable) pointer to CreateNewGuestWindow() does not
seem necessary, why can't it be const ref?
I see you change create_params.guest_delegate at some point but that can be a
copy when used, otherwise I was chasing down if create_params being modified has
any side effects.

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1430:
new_contents->GetController().SetSessionStorageNamespace(
DCHECK(new_contents)

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/public/br...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/public/br...
content/public/browser/web_contents.h:113: BrowserPluginGuestDelegate*
guest_delegate;
Add comment.

[Fady Samuel, 2014-06-18 21:08:34]

PTAL

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/apps/web_...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/apps/web_...
chrome/browser/apps/web_view_browsertest.cc:6: #include
"base/memory/singleton.h"
On 2014/06/17 23:46:45, lazyboy wrote:
> Revert.

Done.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:828: void
ChromeContentBrowserClient::GuestWebContentsCreated(
On 2014/06/17 23:46:45, lazyboy wrote:
> After GuestWebContentsAttached() gets/got removed, you can clean up lots of
> #includes now.

We still refer to GuestView* and WebViewGuest in this class, so i don't think I
can remove anything.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc
(right):

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:22:
std::string view_type;
On 2014/06/17 23:46:45, lazyboy wrote:
> Since this function has params now, use
> extensions::api::..Function::Params to deserialize the params.

No, can't because I'd like to keep the dictionary of CreateParams as a
dictionary instead of a struct.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:100: if (!guest_web_contents)
On 2014/06/17 23:46:45, lazyboy wrote:
> Should this be DCHECK instead?

Done.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:190: // GuestViewBase tracks the
lifetime of its embedder render process until it
On 2014/06/17 23:46:45, lazyboy wrote:
> This comment should also be class level comment for GuestViewBase.

Done.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:216: // After attachment, this
GuestViewBase's lifetime is restricted to the
On 2014/06/17 23:46:45, lazyboy wrote:
> Same as above, lifetime should also be documented class level.

Done.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:245: int
GuestViewBase::GetGuestInstanceID() const {
On 2014/06/17 23:46:45, lazyboy wrote:
> Why do you add this method? to make it virtual? In that case we can get rid of
> guest_instance_id()

Done.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.h:93: // This method can be overriden
by subclasses. This gives the derived class
On 2014/06/17 23:46:45, lazyboy wrote:
> "This method can be overriden by subclasses." -> I think the function being
> virtual pretty much says that. So get rid of these.

Done.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.h:111: typedef
base::Callback<void(content::WebContents*)> WebContentsCallback;
On 2014/06/17 23:46:45, lazyboy wrote:
> WebContentsCreatedCallback

Done.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.h:120: void Init(const std::string&
embedder_extension_id,
On 2014/06/17 23:46:45, lazyboy wrote:
> I found this function very confusing with the virtual Init().
> One of them needs to be renamed.

This pattern is used elsewhere: See DidStopLoading. The idea is some work needs
to be done in the base class and other work needs to be done in the derived
class. It should not be possible for the derived class to circumvent the work
being done in the base class. I have nothing against another name but I can't
think of a good one :P I'm open to suggestions :-)

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

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:7: #include
"base/debug/stack_trace.h"
On 2014/06/17 23:46:45, lazyboy wrote:
> Revert.

Done.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:228: // We might get some late
arriving messages at tear down. Let's let the
On 2014/06/17 23:46:46, lazyboy wrote:
> Would be cool if we had a test for this? Or is this pretty common?

This is really common. Basically all the tests start randomly failing if I get
rid of this relaxing check.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.h:57: content::RenderProcessHost*
embedder_render_process_host,
On 2014/06/17 23:46:46, lazyboy wrote:
> (Here and below) Why would this be non const?

This needs to be non-const because we call AddObserver.

With that said, I switched it over to embedder_render_process_id so that we
eliminate the fear of accessing an invalid pointer.

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

https://codereview.chromium.org/336283002/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:120: if (!delegate_)
On 2014/06/17 23:46:46, lazyboy wrote:
> This would pretty much result in null pointer exception, we need to
CHECK/DCHECK

I've made delegate a part of the constructor. Now we can assume delegates always
exist. I added a DCHECK to the constructor. With this patch
BrowserPluginGuestDelegate outlives BrowserPluginGuest too so we can always
assume delegate_ is safe to access.

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

https://codereview.chromium.org/336283002/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:115: WebContentsImpl*
CreateNewGuestWindow(WebContents::CreateParams* params);
On 2014/06/17 23:46:46, lazyboy wrote:
> I'd add a note that |params| could be modified.
> Also add function comment.

Done.

https://codereview.chromium.org/336283002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/336283002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1428: &create_params);
On 2014/06/17 23:46:46, lazyboy wrote:
> |create_params| being a (mutable) pointer to CreateNewGuestWindow() does not
> seem necessary, why can't it be const ref?
> I see you change create_params.guest_delegate at some point but that can be a
> copy when used, otherwise I was chasing down if create_params being modified
has
> any side effects.

Done.

https://codereview.chromium.org/336283002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1430:
new_contents->GetController().SetSessionStorageNamespace(
On 2014/06/17 23:46:46, lazyboy wrote:
> DCHECK(new_contents)

Unnecessary. We will always segfault here.

https://codereview.chromium.org/336283002/diff/40001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/336283002/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:113: BrowserPluginGuestDelegate*
guest_delegate;
On 2014/06/17 23:46:46, lazyboy wrote:
> Add comment.

Done.

[lazyboy, 2014-06-19 16:17:32]

lgtm

One renaming suggestions, looks good.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc
(right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/guest_view/guest_view_internal_api.cc:22:
std::string view_type;
On 2014/06/18 21:08:33, Fady Samuel wrote:
> On 2014/06/17 23:46:45, lazyboy wrote:
> > Since this function has params now, use
> > extensions::api::..Function::Params to deserialize the params.
> 
> No, can't because I'd like to keep the dictionary of CreateParams as a
> dictionary instead of a struct.

Ok.

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_base.h (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.h:120: void Init(const std::string&
embedder_extension_id,
On 2014/06/18 21:08:33, Fady Samuel wrote:
> On 2014/06/17 23:46:45, lazyboy wrote:
> > I found this function very confusing with the virtual Init().
> > One of them needs to be renamed.
> 
> This pattern is used elsewhere: See DidStopLoading. The idea is some work
needs
> to be done in the base class and other work needs to be done in the derived
> class. It should not be possible for the derived class to circumvent the work
> being done in the base class. I have nothing against another name but I can't
> think of a good one :P I'm open to suggestions :-)

Since the parameterless Init() is called after initialization, how about
DidInitialize() for the other one?

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/336283002/diff/40001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:120: if (!delegate_)
On 2014/06/18 21:08:34, Fady Samuel wrote:
> On 2014/06/17 23:46:46, lazyboy wrote:
> > This would pretty much result in null pointer exception, we need to
> CHECK/DCHECK
> 
> I've made delegate a part of the constructor. Now we can assume delegates
always
> exist. I added a DCHECK to the constructor. With this patch
> BrowserPluginGuestDelegate outlives BrowserPluginGuest too so we can always
> assume delegate_ is safe to access.

That's great.
Can you also update your CL description to reflect this?

https://chromiumcodereview.appspot.com/336283002/diff/60001/extensions/browse...
File extensions/browser/extension_function_histogram_value.h (right):

https://chromiumcodereview.appspot.com/336283002/diff/60001/extensions/browse...
extensions/browser/extension_function_histogram_value.h:853:
GUESTVIEWINTERNAL_CREATEGUEST,
I'm not entirely sure it is a good idea to remove elements from this enum,
Alexei or someone familiar probably knows.

[Fady Samuel, 2014-06-20 15:16:35]

+asvitkine@ for extensions/browser/extension_function_histogram_value.h
+jam for content/public changes.

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/336283002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.h:120: void Init(const std::string&
embedder_extension_id,
On 2014/06/19 16:17:31, lazyboy wrote:
> On 2014/06/18 21:08:33, Fady Samuel wrote:
> > On 2014/06/17 23:46:45, lazyboy wrote:
> > > I found this function very confusing with the virtual Init().
> > > One of them needs to be renamed.
> > 
> > This pattern is used elsewhere: See DidStopLoading. The idea is some work
> needs
> > to be done in the base class and other work needs to be done in the derived
> > class. It should not be possible for the derived class to circumvent the
work
> > being done in the base class. I have nothing against another name but I
can't
> > think of a good one :P I'm open to suggestions :-)
> 
> Since the parameterless Init() is called after initialization, how about
> DidInitialize() for the other one?

Good idea! Done!

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

https://codereview.chromium.org/336283002/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:120: if (!delegate_)
On 2014/06/19 16:17:31, lazyboy wrote:
> On 2014/06/18 21:08:34, Fady Samuel wrote:
> > On 2014/06/17 23:46:46, lazyboy wrote:
> > > This would pretty much result in null pointer exception, we need to
> > CHECK/DCHECK
> > 
> > I've made delegate a part of the constructor. Now we can assume delegates
> always
> > exist. I added a DCHECK to the constructor. With this patch
> > BrowserPluginGuestDelegate outlives BrowserPluginGuest too so we can always
> > assume delegate_ is safe to access.
> 
> That's great.
> Can you also update your CL description to reflect this?

Done.

https://codereview.chromium.org/336283002/diff/60001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/336283002/diff/60001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:853:
GUESTVIEWINTERNAL_CREATEGUEST,
On 2014/06/19 16:17:31, lazyboy wrote:
> I'm not entirely sure it is a good idea to remove elements from this enum,
> Alexei or someone familiar probably knows.

Will ask.

[Alexei Svitkine (slow), 2014-06-20 15:18:27]

https://codereview.chromium.org/336283002/diff/80001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (left):

https://codereview.chromium.org/336283002/diff/80001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:847:
GUESTVIEWINTERNAL_ALLOCATEINSTANCEID,
It's incorrect to remove elements here, since this will renumber other entries.
If necessary, simply rename this one indicating that it's obsolete/deprecated.

https://codereview.chromium.org/336283002/diff/80001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/336283002/diff/80001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:855: //
tools/metrics/histograms/histograms/histograms.xml.
Please also update histograms.xml per the comment in this cl.

[jam, 2014-06-20 15:58:27]

https://codereview.chromium.org/336283002/diff/80001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/336283002/diff/80001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:41: virtual int
GetGuestInstanceID() const;
i thought the instance ids in content were hidden from chrome, is that not done
yet?

[Fady Samuel, 2014-06-20 16:09:01]

https://codereview.chromium.org/336283002/diff/80001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/336283002/diff/80001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:41: virtual int
GetGuestInstanceID() const;
On 2014/06/20 15:58:27, jam wrote:
> i thought the instance ids in content were hidden from chrome, is that not
done
> yet?

No not yet. BrowserPlugin has an '-internal-attach' method that takes in an
instance ID. Once we figure out how to get rid of that, this will go away. I've
been chatting with Nasko on and off about that. We haven't had a chance to get
to that yet though.

[Fady Samuel, 2014-06-20 16:12:28]

PTAL Alexei and John! Thanks!

[Alexei Svitkine (slow), 2014-06-20 17:15:11]

https://codereview.chromium.org/336283002/diff/140001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (left):

https://codereview.chromium.org/336283002/diff/140001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:847:
GUESTVIEWINTERNAL_ALLOCATEINSTANCEID,
Re-using this value for another API is still no good.

Just mark the old one as deprecated and add a new constant at the end.

If you re-use the same value for different things, then things will be confusing
when people look at UMA dashboards and aggregate between versions.

Once you do the above, please also update histograms.xml in the same cl.

[Fady Samuel, 2014-06-20 17:43:51]

PTAL Alexei

https://codereview.chromium.org/336283002/diff/140001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (left):

https://codereview.chromium.org/336283002/diff/140001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:847:
GUESTVIEWINTERNAL_ALLOCATEINSTANCEID,
On 2014/06/20 17:15:11, Alexei Svitkine wrote:
> Re-using this value for another API is still no good.
> 
> Just mark the old one as deprecated and add a new constant at the end.
> 
> If you re-use the same value for different things, then things will be
confusing
> when people look at UMA dashboards and aggregate between versions.
> 
> Once you do the above, please also update histograms.xml in the same cl.

Done.

[jam, 2014-06-20 17:44:56]

lgtm

[Alexei Svitkine (slow), 2014-06-20 17:52:40]

lgtm

[Fady Samuel, 2014-06-20 22:54:49]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-06-20 23:00:08]

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

[Fady Samuel, 2014-06-21 00:08:51]

The CQ bit was unchecked by fsamuel@chromium.org

[Fady Samuel, 2014-06-21 00:08:53]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-06-21 00:09:36]

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

[Fady Samuel, 2014-06-21 04:16:04]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-06-21 04:17:33]

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

[commit-bot: I haz the power, 2014-06-21 06:18:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-21 06:21:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-21 06:21:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[lazyboy, 2014-06-21 12:09:29]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-06-21 12:10:29]

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

[commit-bot: I haz the power, 2014-06-21 12:16:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-21 12:20:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-21 12:20:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Fady Samuel, 2014-06-23 15:24:18]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-06-23 15:25:29]

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

[commit-bot: I haz the power, 2014-06-23 17:39:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-23 17:42:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-23 17:42:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Fady Samuel, 2014-06-23 18:00:36]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-06-23 18:01:38]

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

[commit-bot: I haz the power, 2014-06-23 18:14:26]

Change committed as 279140