Adding ChromeVox as a component extensions (enabled only for ChromeOS, for no...

Adding ChromeVox as a component extensions (enabled only for ChromeOS, for now)

BUG=none
TEST=none


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

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

[Chaitanya, 2011-01-19 02:57:53]

Hi,
Just getting this out so that you can start looking at it.
We test the ChromeVox extension internally using js test driver.

[Chaitanya, 2011-01-19 04:47:12]

On 2011/01/19 02:57:53, Chaitanya wrote:
> Hi,
> Just getting this out so that you can start looking at it.
> We test the ChromeVox extension internally using js test driver.

I just got an alternate approach working -- I install my .crx on the file system
as a part of ChromeOS. I then unzip this crx to the user's home dir
(/home/chronos/user/Accessibility-Extensions/ on ChromeOS) inside
ProfileImpl::RegisterComponentExtensions().
Personally, I think this approach is better than what this CL has.
If you feel so too, then I will go ahead and update the CL.

[raman_google.com, 2011-01-19 17:49:03]

My primary metric at this point, ship by the 24th deadline,
    whichever approach works -- wins. 

chaitanyag@chromium.org writes:
 > On 2011/01/19 02:57:53, Chaitanya wrote:
 > > Hi,
 > > Just getting this out so that you can start looking at it.
 > > We test the ChromeVox extension internally using js test driver.
 > 
 > I just got an alternate approach working -- I install my .crx on the file  
 > system
 > as a part of ChromeOS. I then unzip this crx to the user's home dir
 > (/home/chronos/user/Accessibility-Extensions/ on ChromeOS) inside
 > ProfileImpl::RegisterComponentExtensions().
 > Personally, I think this approach is better than what this CL has.
 > If you feel so too, then I will go ahead and update the CL.
 > 
 > http://codereview.chromium.org/6254007/
 > 
 > -- 
 > You received this message because you are subscribed to the Google Groups
"access-reviews" group.
 > To post to this group, send email to access-reviews@google.com.
 > To unsubscribe from this group, send email to
access-reviews+unsubscribe@google.com.
 > For more options, visit this group at
http://groups.google.com/a/google.com/group/access-reviews/?hl=en.

-- 

[asargent_no_longer_on_chrome, 2011-01-19 19:14:15]

> I just got an alternate approach working -- I install my .crx on the file
system
> as a part of ChromeOS. I then unzip this crx to the user's home dir
> (/home/chronos/user/Accessibility-Extensions/ on ChromeOS) inside
> ProfileImpl::RegisterComponentExtensions().
> Personally, I think this approach is better than what this CL has.
> If you feel so too, then I will go ahead and update the CL.

This seems resonable - I assume this /home/chronos/user directory is shared
between all users that log in to a ChromeOS device, and the profile data is kept
segregated?

http://codereview.chromium.org/6254007/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6254007/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:420:
ExtractAccessibilityExtensionResources(iter->first, path.DirName());
Doesn't this mean you'll be doing this unpacking every time the system starts
up? I can see two approaches to fixing this problem:

-Store a pref once you've done the unpacking, and add a check for the presence
of the pref.
-Put the code that unpacks the resources somewhere close to 
where the profile directory gets initially created (not sure if the creation of
the profile directory works different for ChromeOS, but for Chrome I think it's
created on first run). 

Also, you'll need to think about how to handle updates - when a new version of
Chrome/ChromeOS is installed with an updated version of the extension, how will
you make sure you do the unpacking again?

[Chaitanya, 2011-01-19 19:47:23]

Ok great! Now that we know this approach will be Ok, I will work on fixing the
things you mentioned. Will get back to you once I have the fixes.

One question -- Is there a way to bundle the crx in the Chrome binary for
ChromeOS. This might be preferable to installing the crx as a part of ChromeOS.

On 2011/01/19 19:14:15, Antony Sargent wrote:
> > I just got an alternate approach working -- I install my .crx on the file
> system
> > as a part of ChromeOS. I then unzip this crx to the user's home dir
> > (/home/chronos/user/Accessibility-Extensions/ on ChromeOS) inside
> > ProfileImpl::RegisterComponentExtensions().
> > Personally, I think this approach is better than what this CL has.
> > If you feel so too, then I will go ahead and update the CL.
> 
> This seems resonable - I assume this /home/chronos/user directory is shared
> between all users that log in to a ChromeOS device, and the profile data is
kept
> segregated?
> 
>
http://codereview.chromium.org/6254007/diff/6001/chrome/browser/profiles/prof...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
http://codereview.chromium.org/6254007/diff/6001/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_impl.cc:420:
> ExtractAccessibilityExtensionResources(iter->first, path.DirName());
> Doesn't this mean you'll be doing this unpacking every time the system starts
> up? I can see two approaches to fixing this problem:
> 
> -Store a pref once you've done the unpacking, and add a check for the presence
> of the pref.
> -Put the code that unpacks the resources somewhere close to 
> where the profile directory gets initially created (not sure if the creation
of
> the profile directory works different for ChromeOS, but for Chrome I think
it's
> created on first run). 
> 
> Also, you'll need to think about how to handle updates - when a new version of
> Chrome/ChromeOS is installed with an updated version of the extension, how
will
> you make sure you do the unpacking again?

[asargent_no_longer_on_chrome, 2011-01-19 21:56:56]

> Is there a way to bundle the crx in the Chrome binary for
> ChromeOS. This might be preferable to installing the crx as a part of
ChromeOS.

I don't know.

[Chaitanya, 2011-01-20 00:46:47]

I have updated the CL. Please take a look.

On 2011/01/19 21:56:56, Antony Sargent wrote:
> > Is there a way to bundle the crx in the Chrome binary for
> > ChromeOS. This might be preferable to installing the crx as a part of
> ChromeOS.
> 
> I don't know.

[asargent_no_longer_on_chrome, 2011-01-20 05:55:59]

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:392: if
(ExtensionAccessibilityEventRouter::GetInstance()->
It seems kind of weird to ask the event router if accessibility is enabled - can
you just look at the pref value here?

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:426:
UnzipAccessibilityExtensionResources(iter->first, path, manifest);
nit: it might read more clearly if you factor out the "are we up to date" check,
and then make this something like:

if (!AccessibilityExtensionIsUpToDate(...))
  UnzipAccessibilityExtensionResources(...)

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:465:
.AppendASCII(ext_name).ReplaceExtension(FILE_PATH_LITERAL(".zip")),
This is a little hard to read. Can you factor this out into something like

FilePath zipfile_path = ....
bool success = Unzip(zipfile_path, dir_path);
if (success)
   ...

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:176: void
UnzipAccessibilityExtensionResources(const std::string ext_name,
Does this method actually need to be a member function of ProfileImpl, or can it
just be defined in the .cc file as either static or in an anonymous namespace?

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/resources/co...
File chrome/browser/resources/component_extension_resources.grd (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/resources/co...
chrome/browser/resources/component_extension_resources.grd:25: <include
name="IDR_CHROMEVOX_MANIFEST_JSON" file="access_chromevox/manifest.json"
type="BINDATA" />
Wait, do we need an entry in both this .grd file and in browser_resources.grd?
They look pretty similar.

http://codereview.chromium.org/6254007/diff/10002/chrome/common/extensions/ex...
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/common/extensions/ex...
chrome/common/extensions/extension_constants.cc:322:
"/usr/share/chromeos-assets/accessibility/extensions";
I think we discussed this offline, but you'll need to get someone from ChromeOS
team to give the ok for putting the files in this location.

http://codereview.chromium.org/6254007/diff/10002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:442: // A string pref that stored the version of the
currently installed ChromeVox
nit: consider rewording "that stored" to something like "which stores" or "which
records" or "which represents"

[Chaitanya, 2011-01-20 20:04:51]

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:392: if
(ExtensionAccessibilityEventRouter::GetInstance()->
using prefs now

On 2011/01/20 05:55:59, Antony Sargent wrote:
> It seems kind of weird to ask the event router if accessibility is enabled -
can
> you just look at the pref value here?
>

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:426:
UnzipAccessibilityExtensionResources(iter->first, path, manifest);
On 2011/01/20 05:55:59, Antony Sargent wrote:
> nit: it might read more clearly if you factor out the "are we up to date"
check,
> and then make this something like:
> 
> if (!AccessibilityExtensionIsUpToDate(...))
>   UnzipAccessibilityExtensionResources(...)

Done.

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:465:
.AppendASCII(ext_name).ReplaceExtension(FILE_PATH_LITERAL(".zip")),
Removed UnzipAccessibilityExtensionResources and added
IsAccessibilityExtensionUpToDate

On 2011/01/20 05:55:59, Antony Sargent wrote:
> This is a little hard to read. Can you factor this out into something like
> 
> FilePath zipfile_path = ....
> bool success = Unzip(zipfile_path, dir_path);
> if (success)
>    ...
> 
>

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:176: void
UnzipAccessibilityExtensionResources(const std::string ext_name,
On 2011/01/20 05:55:59, Antony Sargent wrote:
> Does this method actually need to be a member function of ProfileImpl, or can
it
> just be defined in the .cc file as either static or in an anonymous namespace?


removed.

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/resources/co...
File chrome/browser/resources/component_extension_resources.grd (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/browser/resources/co...
chrome/browser/resources/component_extension_resources.grd:25: <include
name="IDR_CHROMEVOX_MANIFEST_JSON" file="access_chromevox/manifest.json"
type="BINDATA" />
On 2011/01/20 05:55:59, Antony Sargent wrote:
> Wait, do we need an entry in both this .grd file and in browser_resources.grd?
> They look pretty similar. 

removed.

http://codereview.chromium.org/6254007/diff/10002/chrome/common/extensions/ex...
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/common/extensions/ex...
chrome/common/extensions/extension_constants.cc:322:
"/usr/share/chromeos-assets/accessibility/extensions";
On 2011/01/20 05:55:59, Antony Sargent wrote:
> I think we discussed this offline, but you'll need to get someone from
ChromeOS
> team to give the ok for putting the files in this location. 

We have a green signal on that. See Issues 6317007 & 6357008

http://codereview.chromium.org/6254007/diff/10002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6254007/diff/10002/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:442: // A string pref that stored the version of the
currently installed ChromeVox
On 2011/01/20 05:55:59, Antony Sargent wrote:
> nit: consider rewording "that stored" to something like "which stores" or
"which
> records" or "which represents"

Done.

[asargent_no_longer_on_chrome, 2011-01-20 22:05:23]

I've thought about this some more and discussed with some other extensions team
members, and I think unpacking the crx separately like this is the wrong
approach. 

I'd like to see us instead either fix any bugs that prevent the extension from
running properly out of a resources.pak file, or have the files installed on
disk as part of the  ChromeOS installation, similar to the help application.

[Chaitanya, 2011-01-21 04:34:00]

Ok, one more try :)
as per our conversation, I have removed the runtime unpacking logic.
The unpacking will happen as a part of ChromeOS package install.
I am now just pointing the component extension to the correct path.

On 2011/01/20 22:05:23, Antony Sargent wrote:
> I've thought about this some more and discussed with some other extensions
team
> members, and I think unpacking the crx separately like this is the wrong
> approach. 
> 
> I'd like to see us instead either fix any bugs that prevent the extension from
> running properly out of a resources.pak file, or have the files installed on
> disk as part of the  ChromeOS installation, similar to the help application.

[asargent_no_longer_on_chrome, 2011-01-21 05:53:44]

looking better - just a couple minor issues

http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:386: if
(g_browser_process->local_state()->
nit: I think you can just call GetPrefs() instead of
g_browser_process->local_state()

http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:387:
GetBoolean(prefs::kAccessibilityEnabled)) {
Looks like this current patch set is missing the change to define and register
the kAccessibilityEnabled pref (I think you had this in one of the earlier patch
sets - did you accidentally remove it?).

http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:412: if (StartsWithASCII(iter->first,
"access_", true)) {
Here's an idea - instead of pushing the ("access_chromevox",
IDR_CHROMEVOX_MANIFEST) pair into component_extensions and having to do this
special case inside the body of the for loop, instead move the test for whether
accessibility is enabled to be just after this for loop. Then, if it is enabled,
you construct the proper path and then call 

extensions_service_->register_component_extension(...)

[Chaitanya, 2011-01-21 07:03:06]

On Thu, Jan 20, 2011 at 9:53 PM, <asargent@chromium.org> wrote:

> looking better - just a couple minor issues
>
>
>
>
>
http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
>
> File chrome/browser/profiles/profile_impl.cc (right):
>
>
>
http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_impl.cc:386: if
> (g_browser_process->local_state()->
> nit: I think you can just call GetPrefs() instead of
> g_browser_process->local_state()
>

Seems like GetPrefs() is different than g_browser_process->local_state()
Accessibility gets enabled at the login screen of ChromeOS when the pref is
set in g_browser_process->local_state().


>
>
>
http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_impl.cc:387:
> GetBoolean(prefs::kAccessibilityEnabled)) {
> Looks like this current patch set is missing the change to define and
> register the kAccessibilityEnabled pref (I think you had this in one of
> the earlier patch sets - did you accidentally remove it?).
>

No, kAccessibilityEnabled existed before this change. Its is declared in
chrome/common/pref_names.h<https://cs.corp.google.com/#chrome/trunk/src/chrome/common/pref_names.h&q=kaccessibilityenabled&exact_package=chrome&ct=rc&cd=1>


>
>
>
http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_impl.cc:412: if
> (StartsWithASCII(iter->first, "access_", true)) {
> Here's an idea - instead of pushing the ("access_chromevox",
> IDR_CHROMEVOX_MANIFEST) pair into component_extensions and having to do
> this special case inside the body of the for loop, instead move the test
> for whether accessibility is enabled to be just after this for loop.
> Then, if it is enabled, you construct the proper path and then call
>
> extensions_service_->register_component_extension(...)
>
>
Good idea. I will fix it tomorrow morning.


>
> http://codereview.chromium.org/6254007/
>

[Chaitanya, 2011-01-21 20:09:32]

http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6254007/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:412: if (StartsWithASCII(iter->first,
"access_", true)) {
On 2011/01/21 05:53:44, Antony Sargent wrote:
> Here's an idea - instead of pushing the ("access_chromevox",
> IDR_CHROMEVOX_MANIFEST) pair into component_extensions and having to do this
> special case inside the body of the for loop, instead move the test for
whether
> accessibility is enabled to be just after this for loop. Then, if it is
enabled,
> you construct the proper path and then call 
> 
> extensions_service_->register_component_extension(...)
> 

Done.

[asargent_no_longer_on_chrome, 2011-01-21 21:18:40]

lgtm

http://codereview.chromium.org/6254007/diff/9002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6254007/diff/9002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:414: FilePath path =
FilePath().AppendASCII(extension_misc::kAccessExtensionPath)
nit: this should be:

FilePath path = FilePath(extension_misc::kAccessExtensionPath)
    .AppendASCII("access_chromevox");

eg you can use the FilePath constructor that takes a string argument, and the
overflow on the next line should be 4-spaced indented

http://codereview.chromium.org/6254007/diff/9002/chrome/browser/resources/acc...
File chrome/browser/resources/access_chromevox/manifest.json (right):

http://codereview.chromium.org/6254007/diff/9002/chrome/browser/resources/acc...
chrome/browser/resources/access_chromevox/manifest.json:5: "description":
"ChromeVox - Giving Voice to Chrome (uncompiled).",
I assume you guys will switch to bundling the compiled version at some point?

[Chaitanya, 2011-01-21 21:38:49]

Will submit once try bots cycle green.
thanks!

http://codereview.chromium.org/6254007/diff/9002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6254007/diff/9002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:414: FilePath path =
FilePath().AppendASCII(extension_misc::kAccessExtensionPath)
On 2011/01/21 21:18:40, Antony Sargent wrote:
> nit: this should be:
> 
> FilePath path = FilePath(extension_misc::kAccessExtensionPath)
>     .AppendASCII("access_chromevox");
> 
> eg you can use the FilePath constructor that takes a string argument, and the
> overflow on the next line should be 4-spaced indented

Done.

http://codereview.chromium.org/6254007/diff/9002/chrome/browser/resources/acc...
File chrome/browser/resources/access_chromevox/manifest.json (right):

http://codereview.chromium.org/6254007/diff/9002/chrome/browser/resources/acc...
chrome/browser/resources/access_chromevox/manifest.json:5: "description":
"ChromeVox - Giving Voice to Chrome (uncompiled).",
On 2011/01/21 21:18:40, Antony Sargent wrote:
> I assume you guys will switch to bundling the compiled version at some point?

Oh it is compiled...
Fixed the description.

[rniwa-cr, 2011-01-22 03:30:59]

Reverted this CL in
http://src.chromium.org/viewvc/chrome?view=rev&revision=72264 because the CL
seems to have caused perm check on Linux builder to fail.

See:
http://build.chromium.org/p/chromium/builders/Linux/builds/2696
http://build.chromium.org/p/chromium/builders/Linux/builds/2696/steps/check%2...

[Chaitanya, 2011-01-22 06:08:38]

Looks like this failed because manifest.json had the svn:executable property
set.
Maybe I need to do svn propdel svn:executable manifest.json before checking
in?
Btw, why is this check needed?

On Fri, Jan 21, 2011 at 7:30 PM, <rniwa@chromium.org> wrote:

> Reverted this CL in
> http://src.chromium.org/viewvc/chrome?view=rev&revision=72264 because the
> CL
> seems to have caused perm check on Linux builder to fail.
>
> See:
> http://build.chromium.org/p/chromium/builders/Linux/builds/2696
>
>
http://build.chromium.org/p/chromium/builders/Linux/builds/2696/steps/check%2...
>
>
>
> http://codereview.chromium.org/6254007/
>

[Aaron Boodman, 2011-01-22 10:58:05]

Is this going to run on all Chrome OSes? I ask because it is going to add yet
another background process there and I know that is something they are trying to
minimize

[Chaitanya, 2011-01-22 20:12:45]

This will run only on those ChromeOS devices where accessibility is enabled.
Btw, do I need to check this in before Monday, or can I do it Monday
morning? Just want to make sure we make the M10 branch.

On Sat, Jan 22, 2011 at 2:58 AM, <aa@chromium.org> wrote:

> Is this going to run on all Chrome OSes? I ask because it is going to add
> yet
> another background process there and I know that is something they are
> trying to
> minimize
>
>
> http://codereview.chromium.org/6254007/
>

[Chaitanya, 2011-01-23 03:05:26]

I think I have fixed the svn:executable property on manifest.json, and running
trybots again...
Will resubmit Monday morning.

On 2011/01/22 20:12:45, Chaitanya wrote:
> This will run only on those ChromeOS devices where accessibility is enabled.
> Btw, do I need to check this in before Monday, or can I do it Monday
> morning? Just want to make sure we make the M10 branch.
> 
> On Sat, Jan 22, 2011 at 2:58 AM, <mailto:aa@chromium.org> wrote:
> 
> > Is this going to run on all Chrome OSes? I ask because it is going to add
> > yet
> > another background process there and I know that is something they are
> > trying to
> > minimize
> >
> >
> > http://codereview.chromium.org/6254007/
> >