This patch is to enable the vtune profiling tool support in chrome.

This patch is to enable the vtune profiling tool support in chrome.

The V8 project provides the support of vtune profiling tool so that the
JavaScript code can be profiled. The initialization of vtune support in
the embedder of V8 is needed.

This patch has the following change:
1, enable the vtune support if "chromium_enable_vtunejit_for_v8" is
enabled before generating the project file. 

2, Add one flag "enable-vtune-support" to enable it for every render
   process. The initialization is invoked in the render process delegate.
   currently linux and windows platform is supported.

contributed by chunyang.dai@intel.com

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

[jamesr, 2013-05-07 02:03:04]

If you're just passing this flag through to V8, why isn't this passed via
--js-flags?  That mechanism allows passing things through to V8 already.

[chunyang.dai, 2013-05-07 02:15:32]

Hello, James
This flag is NOT passed to V8. We want to pass the enable-vtune-support flag to
Render process.

Our assumption is that If the use want to use this feature, He need to 
   1, compile chromium with "chromium_enable_vtunejit_for_v8" pre-definition is
defined.  
   2, pass one flag "enable-vtune-support" to render process to enable it.

Thanks.

[jamesr, 2013-05-07 02:33:37]

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
File content/renderer/renderer_main_platform_delegate_linux.cc (right):

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
content/renderer/renderer_main_platform_delegate_linux.cc:18: #include
"v8/src/third_party/vtune/v8-vtune.h"
is this not where the InitializeVtuneForV8() symbol is defined?  It looks like
v8 to me...

[Avi (use Gerrit), 2013-05-07 03:04:03]

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
File content/renderer/renderer_main_platform_delegate_linux.cc (right):

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
content/renderer/renderer_main_platform_delegate_linux.cc:34: // If
--enable-vtune-support.
pointless comment

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
content/renderer/renderer_main_platform_delegate_linux.cc:35: if
(command_line.HasSwitch(switches::kEnableVtune)) {
no {} if there is only one line

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
content/renderer/renderer_main_platform_delegate_linux.cc:36:
vTune::InitilizeVtuneForV8();
That's the real function name? Yes, it is
(https://code.google.com/p/v8/source/browse/trunk/src/third_party/vtune/v8-vtu...).
Please fix it to have a correct spelling.

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
content/renderer/renderer_main_platform_delegate_win.cc:89: // If
--enable-vtune-support flag is added.
pointless comment

https://codereview.chromium.org/14630005/diff/1/content/renderer/renderer_mai...
content/renderer/renderer_main_platform_delegate_win.cc:90: if
(ommand_line.HasSwitch(switches::kEnableVtune)) {
no {} if there is only one line

Also, this is a typo and won't compile.

[chunyang.dai, 2013-05-09 03:38:36]

hello. Avi.
I updated the code. please help to review. 
the changes are the following:
  1, change the comments.
  2, remove the redundant {}
  3, fixed the function name spelling error. This 
     "InitilizeVtuneForV8" function defined in V8 has been
     updated.

thanks.

[Avi (use Gerrit), 2013-05-10 18:20:47]

I am willing to stamp this as OWNER but please get an OK from someone who knows
about how v8 integrates into Chrome.

https://codereview.chromium.org/14630005/diff/9001/content/public/common/cont...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/14630005/diff/9001/content/public/common/cont...
content/public/common/content_switches.cc:604: const char kEnableVtune[]        
          = "enable-vtune-support";
Blank line after this.

https://codereview.chromium.org/14630005/diff/9001/content/renderer/renderer_...
File content/renderer/renderer_main_platform_delegate_linux.cc (right):

https://codereview.chromium.org/14630005/diff/9001/content/renderer/renderer_...
content/renderer/renderer_main_platform_delegate_linux.cc:35: // is invoked so
that the required call-back function is registered in V8.
This comment still doesn't explain anything that the code doesn't. You can omit
this comment entirely.

https://codereview.chromium.org/14630005/diff/9001/content/renderer/renderer_...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/14630005/diff/9001/content/renderer/renderer_...
content/renderer/renderer_main_platform_delegate_win.cc:91: if
(ommand_line.HasSwitch(switches::kEnableVtune))
This line still has a typo. Please fix it. I will not approve a patch that
obviously will not compile.

[chunyang.dai, 2013-05-13 11:58:33]

hello, Avi.
Thanks for your review comments. 
I fixed the typo error and removed the comments according to your suggestion.
One blank line is added in the content_switches.cc.

Do you know who is owner of v8 & chromium integration?

[Avi (use Gerrit), 2013-05-13 14:24:26]

https://codereview.chromium.org/14630005/diff/14002/content/renderer/renderer...
File content/renderer/renderer_main_platform_delegate_linux.cc (right):

https://codereview.chromium.org/14630005/diff/14002/content/renderer/renderer...
content/renderer/renderer_main_platform_delegate_linux.cc:33: #error "fucking.."
pardon me?

[chunyang.dai, 2013-05-14 01:14:04]

please forgive my impolite comment in the code. I just want to make sure all my
code is compiled correctly because I synchronized the code before the upload. So
I add some compilation check. But I forgot to remove it.

[haraken, 2013-05-14 01:26:29]

danno: would you take a quick look if this change is OK? I'm happy to take a
detailed look.

[danno, 2013-05-14 16:15:07]

I am fundamentally OK with this change, the VTune support in V8 is new and it
makes sense to add the build and runtime switches to Chrome to use it. Although
somebody else will have to pass final judgement whether the mechanics of how to
do this in Chromium are exactly right... it seems at first blush that the
initialization is in the right place and the gyp stuff hands off to V8 in the
right way.

[haraken, 2013-05-15 02:16:15]

The implementation looks good to me, although I'm not an OWNER.

avi: could you take a look as an OWNER?

https://codereview.chromium.org/14630005/diff/24001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/14630005/diff/24001/content/content.gyp#newcode8
content/content.gyp:8: 'chromium_enable_vtunejit_for_v8%': 0,  # enable the
vtune support for V8 engine.

Nit: chromium_enable_vtunejit_for_v8 => chromium_enable_vtune_jit_for_v8

Nit: What is the trailing '%'? (I'm not familiar with gyp syntax.)

[Avi (use Gerrit), 2013-05-15 03:53:52]

I know nothing about how v8 is supposed to tie into Blink and Chromium. As I
said before, I am willing to stamp this as OWNER but I want an ok from an expert
on how v8 and Chrome integrate. If you are not that expert, can you think of
one? If you are that expert say so.

[haraken, 2013-05-15 04:17:20]

danno is saying that it's OK to add the vtune support to Chrome. So the
remaining problem is how to add the support.

In my understanding:

- We have to somehow call vTune::InitializeVtuneForV8() for each renderer
process.

- There are two possible places where we can call vTune::InitializeVtuneForV8().
One is in the render-process-delegate layer (as proposed in the CL). The other
is in the initialization layer of Blink (initialize() in
WebKit/Source/WebKit/chromium/src/WebKit.cpp). V8 initialization is done in the
initialize(). However, I think it's OK to call vTune::InitializeVtuneForV8() in
the render-process-delegate layer because we don't want to pass command-line
options from Chrome to Blink just for this purpose.

abarth: WDYT?

[abarth-chromium, 2013-05-15 04:22:12]

Yes, makes sense to do it in Chromium.

[chunyang.dai, 2013-05-15 09:00:51]

thank you all for the review comments.

Hello, Haraken.
I changed chromium_enable_vtunejit_for_v8 to chromium_enable_vtune_jit_for_v8 in
the latest patch set. 

I can not make sure but according to my understanding, one variable
in gyp with an appendix '%' means a variable which can be overridden
by GYP_DEFINES.

[haraken, 2013-05-15 09:17:36]

OK, thanks, let's wait for L-G-T-M from avi.

[Avi (use Gerrit), 2013-05-15 14:13:20]

On 2013/05/15 09:17:36, haraken wrote:
> OK, thanks, let's wait for L-G-T-M from avi.

I think you misunderstand how owners stamps work. When I say I want an expert to
approve first, I mean I'm waiting for their LG. When I see their LG then I will
give mine.

Who is an expert here in how v8 integrates? If you are, give your LG. Step
forward.

[haraken, 2013-05-15 14:17:54]

> Who is an expert here in how v8 integrates? If you are, give your LG. Step
> forward.

abarth and me. LGTM.

[Avi (use Gerrit), 2013-05-15 14:19:08]

On 2013/05/15 14:17:54, haraken wrote:
> > Who is an expert here in how v8 integrates? If you are, give your LG. Step
> > forward.
> 
> abarth and me. LGTM.

Sounds good then. LGTM

[commit-bot: I haz the power, 2013-05-15 14:23:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chunyang.dai@intel.com/14630005/38001

[chunyang.dai, 2013-05-15 14:36:51]

Hello. All. 

Thanks a lot for your effort. 

I checked the trybot failure. It reports that " Missing LGTM from OWNERS of
directories added to DEPS:
    '+v8/src/third_party/vtune',"

It seems the owner of the renderer folder is Jamesr.

Hello, Jamesr.
Could you help to review it and give your comments? 
Thanks.

[commit-bot: I haz the power, 2013-05-15 14:39:18]

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

[jamesr, 2013-05-15 17:11:22]

On 2013/05/15 14:36:51, chunyang.dai wrote:
> Hello. All. 
> 
> Thanks a lot for your effort. 
> 
> I checked the trybot failure. It reports that " Missing LGTM from OWNERS of
> directories added to DEPS:
>     '+v8/src/third_party/vtune',"
> 
> It seems the owner of the renderer folder is Jamesr.
> 
> Hello, Jamesr.
> Could you help to review it and give your comments? 
> Thanks.

I believe you've misread what the presubmit is saying - it's asking you to get a
review from an owner of v8/src/third_party/vtune since you are adding a
dependency.  I think that #including files from v8/src/... is generally not a
great idea when v8 provides a v8/include/ directory with public API, but that's
up to the V8 folks.

Here's the list: https://code.google.com/p/v8/source/browse/trunk/OWNERS

(avi has OWNERS for all of content/, including content/renderer)

[chunyang.dai, 2013-05-16 16:25:31]

Hello, Daniel.

Could you please give a LGTM? 

Thanks.

[danno, 2013-05-16 20:20:16]

It's not the prettiest solution to directly include something from
v8/src/third_party/vtune, but vtune support in V8 is a third-party extensio and
not part of V8 directly, so I don't think it should be considered part of the
the V8 API. lgtm.

[commit-bot: I haz the power, 2013-05-16 23:39:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chunyang.dai@intel.com/14630005/38001

[commit-bot: I haz the power, 2013-05-17 03:43:16]

Change committed as 200711