Remove CHROME_V8 #define and related javascript_engine gyp variable.

Remove CHROME_V8 #define and related javascript_engine gyp variable.

BUG=94282
R=tfarina@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100692

[tfarina, 2011-09-09 22:01:40]

http://codereview.chromium.org/7864014/diff/1/build/common.gypi
File build/common.gypi (left):

http://codereview.chromium.org/7864014/diff/1/build/common.gypi#oldcode374
build/common.gypi:374: 'javascript_engine%': 'v8',
We doesn't override this anywhere else?

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
File chrome/browser/browser_about_handler.cc (left):

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
chrome/browser/browser_about_handler.cc:1110: std::string
js_version(v8::V8::GetVersion());
you can call v8::V8::GetVersion() directly at line 1124.

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
chrome/browser/browser_about_handler.cc:1111: std::string js_engine = "V8";
same thing here.

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
chrome/browser/browser_about_handler.cc:1141:
localized_strings->SetString("webkit_version", webkit_version);
Is this needed? We have the same code at line 1122.

[Mihai Parparita -not on Chrome, 2011-09-09 22:36:32]

http://codereview.chromium.org/7864014/diff/1/build/common.gypi
File build/common.gypi (left):

http://codereview.chromium.org/7864014/diff/1/build/common.gypi#oldcode374
build/common.gypi:374: 'javascript_engine%': 'v8',
On 2011/09/09 22:01:40, tfarina wrote:
> We doesn't override this anywhere else?

Not based on
http://www.google.com/codesearch#search&q=javascript_engine&exact_package=chr...
(the two remaining usages are outside of the Chromium repository, I can remove
those too).

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
File chrome/browser/browser_about_handler.cc (left):

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
chrome/browser/browser_about_handler.cc:1110: std::string
js_version(v8::V8::GetVersion());
On 2011/09/09 22:01:40, tfarina wrote:
> you can call v8::V8::GetVersion() directly at line 1124.

Done.

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
chrome/browser/browser_about_handler.cc:1111: std::string js_engine = "V8";
On 2011/09/09 22:01:40, tfarina wrote:
> same thing here.

Done.

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7864014/diff/1/chrome/browser/browser_about_ha...
chrome/browser/browser_about_handler.cc:1141:
localized_strings->SetString("webkit_version", webkit_version);
On 2011/09/09 22:01:40, tfarina wrote:
> Is this needed? We have the same code at line 1122.

Doesn't seem that way, removed.

[tfarina, 2011-09-09 23:32:33]

+mark for review, as he reviewed when the variable was added in
http://codereview.chromium.org/88070.

[Mark Mentovai, 2011-09-10 15:33:24]

You don’t need my review here.

If you mean to ask “is it OK to remove CHROME_V8 and javascript_engine,” the
answer is yes. We haven’t supported building with a switchable JavaScript engine
in a long time.

[tfarina, 2011-09-10 18:59:59]

On 2011/09/10 15:33:24, Mark Mentovai wrote:
> You don’t need my review here.
> 
> If you mean to ask “is it OK to remove CHROME_V8 and javascript_engine,” the
> answer is yes. We haven’t supported building with a switchable JavaScript
engine
> in a long time.

Sorry. Yes, I just wanted to check with you if were fine to remove them.

[tfarina, 2011-09-10 19:03:26]

On 2011/09/09 22:36:32, Mihai Parparita wrote:
> http://codereview.chromium.org/7864014/diff/1/build/common.gypi
> File build/common.gypi (left):
> 
> http://codereview.chromium.org/7864014/diff/1/build/common.gypi#oldcode374
> build/common.gypi:374: 'javascript_engine%': 'v8',
> On 2011/09/09 22:01:40, tfarina wrote:
> > We doesn't override this anywhere else?
> 
> Not based on
>
http://www.google.com/codesearch#search&q=javascript_engine&exact_package=chr...
> (the two remaining usages are outside of the Chromium repository, I can remove
> those too).
> 
Yes, that would be good if you can.

[tfarina, 2011-09-10 19:03:49]

With that said. LGTM.

[commit-bot: I haz the power, 2011-09-11 15:06:40]

Can't apply patch for file
chrome/browser/ui/webui/options/chromeos/about_page_handler.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/options/chromeos/about_page_handler.cc
Hunk #1 FAILED at 25.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/options/chromeos/about_page_handler.cc.rej

[commit-bot: I haz the power, 2011-09-11 15:21:08]

Presubmit check for 7864014-10 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for: net/net.gyp

[Mihai Parparita -not on Chrome, 2011-09-11 15:25:28]

Chris or Will: can you LGTM for the net/ changes?

[cbentzel, 2011-09-12 12:04:13]

net LGTM

[commit-bot: I haz the power, 2011-09-12 15:39:21]

Change committed as 100692