Enable runtime switching between ChromeVox and ChromeVox next via command line.

Enable runtime switching between ChromeVox and ChromeVox next via command line.

Committed: https://crrev.com/3e9fff3446b07f68d1a4f669b3e5a494ebd6e03c
Cr-Commit-Position: refs/heads/master@{#296417}

[David Tseng, 2014-09-20 01:24:35]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2014-09-22 06:36:29]

lgtm

This will make it much more convenient for development and testing, thanks!

https://codereview.chromium.org/584313003/diff/1/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/584313003/diff/1/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:357: manifest_filename = is_guest
nit: could you indent this the same as the "if" block, with a newline before
"is_guest ?"?

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:22:
'use_chromevox_next': 1,
Can we just get rid of this from gyp rather than defining it to be 1 by default?
It's okay if you think there'd ever be a reason to compile it out.

[Peter Lundblad, 2014-09-22 09:00:45]

plundblad@chromium.org changed reviewers:
+ plundblad@chromium.org

[Peter Lundblad, 2014-09-22 09:00:46]

As-is, this break chromevox classic.  Please look at my comments below.

https://codereview.chromium.org/584313003/diff/1/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/584313003/diff/1/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:347: CommandLine* command_line =
CommandLine::ForCurrentProcess();
Why was the const qualifier removed here?  Note that it is the const in
'pointer to const' that was removed, not 'const pitner'.

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:22:
'use_chromevox_next': 1,
this means that all manifests generated will be for chromevox next (I patched
the CL and confirmed).  You need to add the % prefix to pick up the
value from the including context if set.  I also suggest defaulting to
0 for consistency.  So:
  'use_chromevox_next%': 0,

In addition, I'd rename this is_chromevox_next to be consistent with
is_guest_manifest because this is now not a build flag for whether to build
chromevox next at all, but rather a parameter to the manifest generation.

Also, please mention this variable in the file header as an optional
variable to be set before including this file.

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox2/chromevox.gyp (right):

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox2/chromevox.gyp:11: 'variables': {
Variables put here will only apply to the chromevox2 target.  I don't think
use_chromevox_next is used by anything inside this target.  I think you meant
for this section to be outside the 'targets' list to apply to everything in
this file (inside the outer conditions).

https://codereview.chromium.org/584313003/diff/1/chrome/chrome_resources.gyp
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/584313003/diff/1/chrome/chrome_resources.gyp#...
chrome/chrome_resources.gyp:116:
'browser/resources/chromeos/chromevox2/chromevox.gyp:chromevox2',
OK, so the intention is to not have a build flag and always include chromevox
next on chromeos builds.  If that's the case, then please also remove
use_chromevox_next from build/common.gypi and make sure that chromevox_tests
runs all tests in the appropriate environment.

[David Tseng, 2014-09-22 15:51:47]

https://codereview.chromium.org/584313003/diff/1/chrome/chrome_resources.gyp
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/584313003/diff/1/chrome/chrome_resources.gyp#...
chrome/chrome_resources.gyp:116:
'browser/resources/chromeos/chromevox2/chromevox.gyp:chromevox2',
On 2014/09/22 09:00:46, Peter Lundblad wrote:
> OK, so the intention is to not have a build flag and always include chromevox
> next on chromeos builds.  If that's the case, then please also remove
> use_chromevox_next from build/common.gypi and make sure that chromevox_tests
> runs all tests in the appropriate environment.

This comment will probably answer the other questions you had.

At first, I did indeed try to remove the use_chromevox_next defined in
build/common.gypi. The issue is the next tests need to pass through the
enable-chromevox-next flag.  Currently, js test generation doesn't make this
easy. Can do it in this cl, or my preference is to do it in a separate cl.

[Peter Lundblad, 2014-09-22 16:09:58]

dtseng@chromium.org writes:
> 
> https://codereview.chromium.org/584313003/diff/1/chrome/chrome_resources.gyp
> File chrome/chrome_resources.gyp (right):
> 
>
https://codereview.chromium.org/584313003/diff/1/chrome/chrome_resources.gyp#...
> chrome/chrome_resources.gyp:116:
> 'browser/resources/chromeos/chromevox2/chromevox.gyp:chromevox2',
> On 2014/09/22 09:00:46, Peter Lundblad wrote:
> > OK, so the intention is to not have a build flag and always include
> chromevox
> > next on chromeos builds.  If that's the case, then please also remove
> > use_chromevox_next from build/common.gypi and make sure that
> chromevox_tests
> > runs all tests in the appropriate environment.
> 
> This comment will probably answer the other questions you had.
> 
> At first, I did indeed try to remove the use_chromevox_next defined in
> build/common.gypi. The issue is the next tests need to pass through the
> enable-chromevox-next flag.  Currently, js test generation doesn't make
> this easy. Can do it in this cl, or my preference is to do it in a
> separate cl.
> 
> https://codereview.chromium.org/584313003/

-- 

I think you should do that, separately if you like, first then, because as
currently written, this cl doesn't work.  You're using the same variable to
mean two different things which is very confusing.

Thanks,
//Peter

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-09-22 17:22:40]

Sorry for the confusion. I was in hte midst of removing the
use_chromevox_next gyp variable used as a build GYP_DEFINES. Uploaded a new
patchset that also addresses the ChromeVox next tests. PTAL.

On Mon, Sep 22, 2014 at 9:09 AM, <plundblad@chromium.org> wrote:

> dtseng@chromium.org writes:
> >
> >
> https://codereview.chromium.org/584313003/diff/1/chrome/chrome_resources.gyp
> > File chrome/chrome_resources.gyp (right):
> >
> >
>
https://codereview.chromium.org/584313003/diff/1/chrome/chrome_resources.gyp#...
> > chrome/chrome_resources.gyp:116:
> > 'browser/resources/chromeos/chromevox2/chromevox.gyp:chromevox2',
> > On 2014/09/22 09:00:46, Peter Lundblad wrote:
> > > OK, so the intention is to not have a build flag and always include
> > chromevox
> > > next on chromeos builds.  If that's the case, then please also remove
> > > use_chromevox_next from build/common.gypi and make sure that
> > chromevox_tests
> > > runs all tests in the appropriate environment.
> >
> > This comment will probably answer the other questions you had.
> >
> > At first, I did indeed try to remove the use_chromevox_next defined in
> > build/common.gypi. The issue is the next tests need to pass through the
> > enable-chromevox-next flag.  Currently, js test generation doesn't make
> > this easy. Can do it in this cl, or my preference is to do it in a
> > separate cl.
> >
> > https://codereview.chromium.org/584313003/
>
> --
>
> I think you should do that, separately if you like, first then, because as
> currently written, this cl doesn't work.  You're using the same variable to
> mean two different things which is very confusing.
>
> Thanks,
> //Peter
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-09-22 17:23:12]

https://codereview.chromium.org/584313003/diff/1/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/584313003/diff/1/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:347: CommandLine* command_line =
CommandLine::ForCurrentProcess();
On 2014/09/22 09:00:46, Peter Lundblad wrote:
> Why was the const qualifier removed here?  Note that it is the const in
> 'pointer to const' that was removed, not 'const pitner'.

Done; inadvertently removed.

https://codereview.chromium.org/584313003/diff/1/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:357: manifest_filename = is_guest
On 2014/09/22 06:36:28, dmazzoni wrote:
> nit: could you indent this the same as the "if" block, with a newline before
> "is_guest ?"?

Done.

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:22:
'use_chromevox_next': 1,
On 2014/09/22 09:00:46, Peter Lundblad wrote:
> this means that all manifests generated will be for chromevox next (I patched
> the CL and confirmed).  You need to add the % prefix to pick up the
> value from the including context if set.  I also suggest defaulting to
> 0 for consistency.  So:
>   'use_chromevox_next%': 0,
> 
> In addition, I'd rename this is_chromevox_next to be consistent with
> is_guest_manifest because this is now not a build flag for whether to build
> chromevox next at all, but rather a parameter to the manifest generation.
> 
> Also, please mention this variable in the file header as an optional
> variable to be set before including this file.


Added comment and made this optional. Was still working on getting
use_chromevox_next to be entirely out of the build/common.gypi when messing
around with this.

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox2/chromevox.gyp (right):

https://codereview.chromium.org/584313003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox2/chromevox.gyp:11: 'variables': {
On 2014/09/22 09:00:46, Peter Lundblad wrote:
> Variables put here will only apply to the chromevox2 target.  I don't think
> use_chromevox_next is used by anything inside this target.  I think you meant
> for this section to be outside the 'targets' list to apply to everything in
> this file (inside the outer conditions).

Done.

[David Tseng, 2014-09-22 17:34:36]

Patchset #3 (id:40001) has been deleted

[David Tseng, 2014-09-22 17:34:48]

Patchset #2 (id:20001) has been deleted

[Peter Lundblad, 2014-09-23 09:06:20]

lgtm

Now I like this!  Minor suggestions below.

https://codereview.chromium.org/584313003/diff/60001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs
(right):

https://codereview.chromium.org/584313003/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:34:
},
This looks like boilerplate that should go into a base class, perhaps with a
flag that you can enable in tests that are intended to run in cvox next if
feasable.

https://codereview.chromium.org/584313003/diff/60001/chrome/chrome_resources.gyp
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/584313003/diff/60001/chrome/chrome_resources....
chrome/chrome_resources.gyp:116:
'browser/resources/chromeos/chromevox2/chromevox.gyp:chromevox2',
Maybe unify to only have one dependency here to clutter this file even less.

[David Tseng, 2014-09-23 17:16:30]

dtseng@chromium.org changed reviewers:
+ finnur@chromium.org, jochen@chromium.org

[David Tseng, 2014-09-23 17:16:32]

+ finnur for *extensions* files, jochen for chrome_resources OWNERS.

https://codereview.chromium.org/584313003/diff/60001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs
(right):

https://codereview.chromium.org/584313003/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:34:
},
On 2014/09/23 09:06:20, Peter Lundblad wrote:
> This looks like boilerplate that should go into a base class, perhaps with a
> flag that you can enable in tests that are intended to run in cvox next if
> feasable.

Once we have a second consumer class of this, I'll bubble up this to a shared
base class.

https://codereview.chromium.org/584313003/diff/60001/chrome/chrome_resources.gyp
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/584313003/diff/60001/chrome/chrome_resources....
chrome/chrome_resources.gyp:116:
'browser/resources/chromeos/chromevox2/chromevox.gyp:chromevox2',
On 2014/09/23 09:06:20, Peter Lundblad wrote:
> Maybe unify to only have one dependency here to clutter this file even less.

I'd like to keep both separated since (at some time in the future), we'll merge
chromevox2  into chromevox, but I don't think we're quite there yet.

[David Tseng, 2014-09-23 23:37:38]

dtseng@chromium.org changed reviewers:
+ sky@chromium.org

[David Tseng, 2014-09-23 23:37:38]

+ sky; trying you for chrome_resources change.

[sky, 2014-09-23 23:49:47]

LGTM

[Finnur, 2014-09-24 10:55:31]

LGTM

https://codereview.chromium.org/584313003/diff/100001/chrome/common/extension...
File chrome/common/extensions/extension_constants.cc (right):

https://codereview.chromium.org/584313003/diff/100001/chrome/common/extension...
chrome/common/extensions/extension_constants.cc:70: const char
kChromeVoxNextGuestManifestFilename[] = "manifest_next_guest.json";
Out of curiosity, where do these files come from?

[Peter Lundblad, 2014-09-24 11:47:03]

finnur@chromium.org writes:
> LGTM
> 
> 
>
https://codereview.chromium.org/584313003/diff/100001/chrome/common/extension...
> File chrome/common/extensions/extension_constants.cc (right):
> 
>
https://codereview.chromium.org/584313003/diff/100001/chrome/common/extension...
> chrome/common/extensions/extension_constants.cc:70: const char
> kChromeVoxNextGuestManifestFilename[] = "manifest_next_guest.json";
> Out of curiosity, where do these files come from?

They are generated from
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2
at compile time.

> https://codereview.chromium.org/584313003/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-09-24 14:24:40]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-09-24 14:25:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584313003/100001

[commit-bot: I haz the power, 2014-09-24 15:24:08]

Committed patchset #4 (id:100001) as dcafbc1c492666c15de908bd6f680c9f7b54131c

[commit-bot: I haz the power, 2014-09-24 15:24:47]

Patchset 4 (id:??) landed as
https://crrev.com/3e9fff3446b07f68d1a4f669b3e5a494ebd6e03c
Cr-Commit-Position: refs/heads/master@{#296417}