Linux Aura accessibility is enabled only on GNOME desktops

Linux Aura accessibility is enabled only on GNOME desktops

Linux Aura accessibility is not enabled on desktops different than GNOME2.
GNOME3 has deprecated gconf in favor of gsettings and dconf.
AT-SPI (cross-platform framework that provides communication between AT
and application) bus launcher has a property called org.a11y.Bus.IsEnabled
that could be easily read by dbus call.

In generall this patch adds an additional check to enable accessibility.
When gconf is available use gnome's accessibility key otherwise
org.a11y.Bus.IsEnabled property.

BUG=472183, 468989, 468112
TBR=stevenjb@chromium.org,jochen@chromium.org

Committed: https://crrev.com/16e2d5a386e3ddd5be818a0d27610b1e9ee9c08e
Cr-Commit-Position: refs/heads/master@{#328651}

[k.czech, 2015-03-20 17:09:43]

k.czech@samsung.com changed reviewers:
+ dmazzoni@chromium.org

[k.czech, 2015-03-20 17:15:25]

On 2015/03/20 17:09:43, k.czech wrote:

Dominic: I proposed a solution to have Aura Linux accessibility enabled on other
platform than GNOME.
Basically patch is finished, but I'd like to have your feedback whether this
approach is OK.

[dmazzoni, 2015-03-20 17:40:28]

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

[dmazzoni, 2015-03-20 17:41:07]

+plundblad

I know almost nothing about dbus. Peter, could you help me review this?

[Peter Lundblad, 2015-03-23 16:45:38]

Hi,

Have you looked at using the C++ dbus wrappers in //dbus?

Also, some of these calls are blocking. What thread are they running on?

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
File ui/accessibility/platform/atk_util_auralinux.cc (right):

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/atk_util_auralinux.cc:21: const char destination[] =
"org.a11y.Bus";
Should use the naming style for constants.

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/atk_util_auralinux.cc:22: const char
objectPathTheMessageShouldBeSentTo[] =
nit: slightly verbose name.

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/atk_util_auralinux.cc:23: "/org/a11y/bus";
Does this fit on the previous line?

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/atk_util_auralinux.cc:35: DBusConnection* connection =
dbus_bus_get(DBUS_BUS_SYSTEM, &error);
Are we leaking this connection?

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/atk_util_auralinux.cc:77: break;
Not reached.

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/atk_util_auralinux.cc:80: if (message)
When can message be null?

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/atk_util_auralinux.cc:83: if (reply)
When can reply be null?

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/atk_util_auralinux.cc:95: GConfClient* client =
gconf_client_get_default();
 Is the rest of this function redundant or is there a case
where your new code would return false but accessibility in gnome would
still be enabled?  If it is not redundant, maybe add some explanation
in a comment, but if it is, we should clean it up.

[k.czech, 2015-03-25 16:59:27]

On 2015/03/23 16:45:38, Peter Lundblad wrote:
> Hi,
> 
> Have you looked at using the C++ dbus wrappers in //dbus?
Thanks, I did not know about dbus wrappers.
> 
> Also, some of these calls are blocking. What thread are they running on?
This call (dbus_connection_send_with_reply_and_block) is synchronous, so it
blocks the current thread for a few milliseconds. I already tested it.
It's about 4 to 6 ms. API provides also a timeout param that blocks while
waiting for reply. I tested with 10ms.
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> File ui/accessibility/platform/atk_util_auralinux.cc (right):
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/atk_util_auralinux.cc:21: const char destination[] =
> "org.a11y.Bus";
> Should use the naming style for constants.
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/atk_util_auralinux.cc:22: const char
> objectPathTheMessageShouldBeSentTo[] =
> nit: slightly verbose name.
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/atk_util_auralinux.cc:23: "/org/a11y/bus";
> Does this fit on the previous line?
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/atk_util_auralinux.cc:35: DBusConnection* connection
=
> dbus_bus_get(DBUS_BUS_SYSTEM, &error);
> Are we leaking this connection?
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/atk_util_auralinux.cc:77: break;
> Not reached.
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/atk_util_auralinux.cc:80: if (message)
> When can message be null?
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/atk_util_auralinux.cc:83: if (reply)
> When can reply be null?
> 
>
https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/atk_util_auralinux.cc:95: GConfClient* client =
> gconf_client_get_default();
>  Is the rest of this function redundant or is there a case
> where your new code would return false but accessibility in gnome would
> still be enabled?  If it is not redundant, maybe add some explanation
> in a comment, but if it is, we should clean it up.
There could be a case where dbus call just failed for some reason so gconf would
be a fallback solution.

[dmazzoni, 2015-03-25 17:15:26]

On Wed, Mar 25, 2015 at 9:59 AM, <k.czech@samsung.com> wrote:

> Also, some of these calls are blocking. What thread are they running on?
>>
> This call (dbus_connection_send_with_reply_and_block) is synchronous, so
> it
> blocks the current thread for a few milliseconds. I already tested it.
> It's about 4 to 6 ms. API provides also a timeout param that blocks while
> waiting for reply. I tested with 10ms.


If it's 4 - 6 ms on your system it could easily be 100 ms on a slower or
heavily loaded machine. I think the right thing to do would be to do this
on a separate thread. My first thought is to use the file thread.

Chromium has great support for threading and calling functions on other
threads, so this shouldn't be much code. Read this doc for a start:
https://www.chromium.org/developers/design-documents/threading

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

[Peter Lundblad, 2015-03-26 14:03:58]

Dominic Mazzoni writes:
> On Wed, Mar 25, 2015 at 9:59 AM, <k.czech@samsung.com> wrote:
> 
>         Also, some of these calls are blocking. What thread are they running
>         on?
>    
>     This call (dbus_connection_send_with_reply_and_block) is synchronous, so
>     it
>     blocks the current thread for a few milliseconds. I already tested it.
>     It's about 4 to 6 ms. API provides also a timeout param that blocks while
>     waiting for reply. I tested with 10ms.
> 
> If it's 4 - 6 ms on your system it could easily be 100 ms on a slower or
> heavily loaded machine. I think the right thing to do would be to do this on
a
> separate thread. My first thought is to use the file thread.
> 
> Chromium has great support for threading and calling functions on other
> threads, so this shouldn't be much code. Read this doc for a start: https://
> www.chromium.org/developers/design-documents/threading
> 
FWIW, the dbus wrapper is async and takes a task runner to run blocking calls
on another thread.  I suggest just using the wrapper and handing it the
MessageLoopProxy of the FILE thread as the task_runner.
And you'd have to make the AtkUtilAuraLinux constructor finish asynchonously.
(Actually, ideally, the library load code should also be moved out
of the ui thread, but that's a different cl).

Thanks,
//Peter

-- 

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

[lgombos, 2015-03-26 22:05:39]

l.gombos@samsung.com changed reviewers:
+ l.gombos@samsung.com

[lgombos, 2015-03-26 22:05:40]

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/accessibil...
File ui/accessibility/accessibility.gyp (right):

https://codereview.chromium.org/1028553003/diff/1/ui/accessibility/accessibil...
ui/accessibility/accessibility.gyp:71: '../../build/linux/system.gyp:dbus',
Do we need to guard this dependency with use_dbus==1 ?

[k.czech, 2015-03-30 21:18:17]

On 2015/03/26 14:03:58, Peter Lundblad wrote:
> Dominic Mazzoni writes:
> > On Wed, Mar 25, 2015 at 9:59 AM, <mailto:k.czech@samsung.com> wrote:
> > 
> >         Also, some of these calls are blocking. What thread are they running
> >         on?
> >    
> >     This call (dbus_connection_send_with_reply_and_block) is synchronous, so
> >     it
> >     blocks the current thread for a few milliseconds. I already tested it.
> >     It's about 4 to 6 ms. API provides also a timeout param that blocks
while
> >     waiting for reply. I tested with 10ms.
> > 
> > If it's 4 - 6 ms on your system it could easily be 100 ms on a slower or
> > heavily loaded machine. I think the right thing to do would be to do this on
> a
> > separate thread. My first thought is to use the file thread.
> > 
> > Chromium has great support for threading and calling functions on other
> > threads, so this shouldn't be much code. Read this doc for a
start:&nbsp;https://
> > http://www.chromium.org/developers/design-documents/threading
> > 
> FWIW, the dbus wrapper is async and takes a task runner to run blocking calls
> on another thread.  I suggest just using the wrapper and handing it the
> MessageLoopProxy of the FILE thread as the task_runner.
> And you'd have to make the AtkUtilAuraLinux constructor finish asynchonously.
> (Actually, ideally, the library load code should also be moved out
> of the ui thread, but that's a different cl).
> 
> Thanks,
> //Peter
> 
Running it in a separate thread sounds really cool. I'm wondering on one thing ,
BrowserThread::FILE is part of the content namespace and content.gyp already
includes accessibility.gyp, so including it again in accessibility.gyp will make
cyclic target dependency, I guess. So to avoid this, maybe it might be add to
the current's task_runner ?

[Peter Lundblad, 2015-03-31 09:39:59]

k.czech@samsung.com writes:
> On 2015/03/26 14:03:58, Peter Lundblad wrote:
> > Dominic Mazzoni writes:
> > > On Wed, Mar 25, 2015 at 9:59 AM, <mailto:k.czech@samsung.com> wrote:
> > >
> > >         Also, some of these calls are blocking. What thread are they  
> > running
> > >         on?
> > >
> > >     This call (dbus_connection_send_with_reply_and_block) is  
> > synchronous, so
> > >     it
> > >     blocks the current thread for a few milliseconds. I already tested  
> > it.
> > >     It's about 4 to 6 ms. API provides also a timeout param that blocks
> while
> > >     waiting for reply. I tested with 10ms.
> > >
> > > If it's 4 - 6 ms on your system it could easily be 100 ms on a slower or
> > > heavily loaded machine. I think the right thing to do would be to do  
> > this on
> > a
> > > separate thread. My first thought is to use the file thread.
> > >
> > > Chromium has great support for threading and calling functions on other
> > > threads, so this shouldn't be much code. Read this doc for a
> start:&nbsp;https://
> > > http://www.chromium.org/developers/design-documents/threading
> > >
> > FWIW, the dbus wrapper is async and takes a task runner to run blocking  
> > calls
> > on another thread.  I suggest just using the wrapper and handing it the
> > MessageLoopProxy of the FILE thread as the task_runner.
> > And you'd have to make the AtkUtilAuraLinux constructor finish  
> > asynchonously.
> > (Actually, ideally, the library load code should also be moved out
> > of the ui thread, but that's a different cl).
> 
> > Thanks,
> > //Peter
> 
> Running it in a separate thread sounds really cool. I'm wondering on one  
> thing ,
> BrowserThread::FILE is part of the content namespace and content.gyp already
> includes accessibility.gyp, so including it again in accessibility.gyp will  
> make
> cyclic target dependency, I guess. So to avoid this, maybe it might be add  
> to
> the current's task_runner ?

Oh, didn't realize that.  The issue with using the current thread is
that the UI thread doesn't allow IO.  Unless I am mistaken you should
get a thread restrictions assertion failure (did you try it?).

@Dominic: Do you think plumbing a task runner for this purpose through the
ViewsDelegate would be a good idea or do you have any better suggestion?

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

-- 

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

[dmazzoni, 2015-03-31 16:12:02]

How about using base::WorkerPool? That seems to be a way to schedule a task on
another thread when the specific thread used doesn't matter.

[Peter Lundblad, 2015-04-01 11:03:09]

dmazzoni@chromium.org writes:
> How about using base::WorkerPool? That seems to be a way to schedule a task  
> on
> another thread when the specific thread used doesn't matter.

That doesn't seem to work well with our dbus wrapper. AFAICT, the threads
of those pools don't have message loops. We'll need to be using the dbus
wrapper
from a thread with a message loop and if it doesn't allow IO, we'll need a
task runner to pass it for proxying potentially blocking calls.

Thanks,
//Peter

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

-- 

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

[dmazzoni, 2015-04-01 15:37:38]

OK.

Rather than plumbing the message loop through, another idea would be that
somewhere in chrome/ could call the ATK initialization code during main
program initialization.

That way ui/accessibility knows that initialization needs to be called, and
knows that it can't do anything until it's been initialized, but it doesn't
need to know the details of the thread.

I don't have a strong preference one way or another, it's just a question
of if it seems cleaner to pass a thread around or abstract away
initialization.

On Wed, Apr 1, 2015 at 4:03 AM, <plundblad@chromium.org> wrote:

> dmazzoni@chromium.org writes:
> > How about using base::WorkerPool? That seems to be a way to schedule a
> task
> > on
> > another thread when the specific thread used doesn't matter.
>
> That doesn't seem to work well with our dbus wrapper. AFAICT, the threads
> of those pools don't have message loops. We'll need to be using the dbus
> wrapper
> from a thread with a message loop and if it doesn't allow IO, we'll need a
> task runner to pass it for proxying potentially blocking calls.
>
> Thanks,
> //Peter
>
> > https://codereview.chromium.org/1028553003/
>
> --
>

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

[k.czech, 2015-04-08 13:50:52]

> > Running it in a separate thread sounds really cool. I'm wondering on one  
> > thing ,
> > BrowserThread::FILE is part of the content namespace and content.gyp already
> > includes accessibility.gyp, so including it again in accessibility.gyp will 

> > make
> > cyclic target dependency, I guess. So to avoid this, maybe it might be add  
> > to
> > the current's task_runner ?
> 
> Oh, didn't realize that.  The issue with using the current thread is
> that the UI thread doesn't allow IO.  Unless I am mistaken you should
> get a thread restrictions assertion failure (did you try it?).

Yes you are right UI thread does not allow IO. I got this assertion.
This happens while doing a synchronous call on a current's task runner.
Seems like this blocking call should be performed on a dbus thread ?

[k.czech, 2015-04-15 09:40:18]

k.czech@samsung.com changed reviewers:
+ stevenjb@chromium.org

[k.czech, 2015-04-15 09:47:54]

Applied all suggestions. I followed Peter suggestions to run dbus callings in
separate thread.

Peter, Dominic I'm looking forward for your comments.

[dmazzoni, 2015-04-15 16:48:35]

Thanks, this is looking really close!

Where is the thread destroyed? It looks like the thread sticks around forever,
which seems wasteful.

I'm not sure if there's a better way to do it, but perhaps the task inside the
thread could post a task back to the main thread when it's done, and in that
task it could just reset the thread.

https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/atk_util_auralinux.cc (right):

https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/atk_util_auralinux.cc:172:
ShouldAccessibilityBeEnabled();
I'd rename this to CheckIfAccessibilityIsEnabled since it doesn't return a bool
now

https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/atk_util_auralinux.cc:180: if
(CheckGnomeAccessibilityKey()) {
I think we should run this in a separate thread too. It also slows down startup.

Would you mind changing it so that the thread is called
accessibility_init_thread_ or something like that, and we either post a task to
check dbus or to check gconf?

https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/atk_util_auralinux.cc:187: #else
Should this check USE_DBUS like the gyp file does?

https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/atk_util_auralinux.cc:207: kServiceName,
dbus::ObjectPath(kObjectPath));
nit: indent 4 spaces for line continuation

https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/atk_util_auralinux.cc:220:
objectProxy->CallMethodAndBlock(&methodCall,
nit: same (4 spaces)

https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/atk_util_auralinux.h (right):

https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/atk_util_auralinux.h:34: scoped_ptr<base::Thread>
dbusThread_;
this should be dbus_thread_

[k.czech, 2015-04-16 11:10:28]

On 2015/04/15 16:48:35, dmazzoni wrote:
> Thanks, this is looking really close!
> 
> Where is the thread destroyed? It looks like the thread sticks around forever,
> which seems wasteful.
> 
> I'm not sure if there's a better way to do it, but perhaps the task inside the
> thread could post a task back to the main thread when it's done, and in that
> task it could just reset the thread.
Thanks, that's a great idea, I used PostTaskAndReply which does exactly what you
suggested.
> 
>
https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
> File ui/accessibility/platform/atk_util_auralinux.cc (right):
> 
>
https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
> ui/accessibility/platform/atk_util_auralinux.cc:172:
> ShouldAccessibilityBeEnabled();
> I'd rename this to CheckIfAccessibilityIsEnabled since it doesn't return a
bool
> now
Done
> 
>
https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
> ui/accessibility/platform/atk_util_auralinux.cc:180: if
> (CheckGnomeAccessibilityKey()) {
> I think we should run this in a separate thread too. It also slows down
startup.
You are right, done.
> 
> Would you mind changing it so that the thread is called
> accessibility_init_thread_ or something like that, and we either post a task
to
> check dbus or to check gconf?
Done.
> 
>
https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
> ui/accessibility/platform/atk_util_auralinux.cc:187: #else
> Should this check USE_DBUS like the gyp file does?
> 
>
https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
> ui/accessibility/platform/atk_util_auralinux.cc:207: kServiceName,
> dbus::ObjectPath(kObjectPath));
> nit: indent 4 spaces for line continuation
Thanks, done
> 
>
https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
> ui/accessibility/platform/atk_util_auralinux.cc:220:
> objectProxy->CallMethodAndBlock(&methodCall,
> nit: same (4 spaces)
Thanks, done.
> 
>
https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
> File ui/accessibility/platform/atk_util_auralinux.h (right):
> 
>
https://codereview.chromium.org/1028553003/diff/20001/ui/accessibility/platfo...
> ui/accessibility/platform/atk_util_auralinux.h:34: scoped_ptr<base::Thread>
> dbusThread_;
> this should be dbus_thread_
I changed to accessibility_init_thread_, I guess this name is more descriptive.

[k.czech, 2015-04-16 11:12:54]

k.czech@samsung.com changed reviewers:
+ jochen@chromium.org

[k.czech, 2015-04-16 11:12:54]

@Dominic, applied all the suggestions, thanks.

[Peter Lundblad, 2015-04-16 14:27:48]

Hi,

[Sorry for the unusual format of the comments below. I am using a script
to upload my comments and today it doesn't work for some reason.
I hope you can make sense of the below comments anywas.]

I agree with Dominic, this is getting closer. A few questions and
comments below.

Thanks,
//peter

========================================================================
File build/linux/system.gyp
------------------------------------
Line 691: 'conditions' : [
Why are you making this change?  Something that depends on this target will
need these deps and settings.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 206: g_object_unref(client);
Move this above the previous if statement and avoid an error-prone
duplication.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 175: base::Bind(accessibility_init_callback_),
Why do you need to bind this a second time?
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 177: base::Unretained(this)));
nit: intend further to make it clear that this is part of the bind call inside
the outer function call.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 213: VLOG(1) << "Enabling ATK accessibility support.";
Just to make sure, is it safe to call this (and indirectly
gnome_accessibility_module_init) from a thread other than the UI thread? I'd
suspect not, but I don't know gnome very well. Otherwise, this could just be
moved back into the
UI thread in the reply callback and still the checking with gconf could be done
in this thread.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 188: void AtkUtilAuraLinux::CheckGnomeAccessibilityKey() {
Should we DCHECK that we're on the right thread here as well as further down?
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 223: options.connection_type = dbus::Bus::PRIVATE;
I don't think this is needed since we're on this thread already.
This option is for when you want dbus operations to run on a separate thread
(for example if you would have gone the route of making asynchronous calls
from the UI thread).  With this implementation, you should just be able to drop
this line.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 230: if (!objectProxy) {
I think you can make this a VLOG(1).
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 241: objectProxy->CallMethodAndBlock(&methodCall,
nit: indent further.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 228: kServiceName, dbus::ObjectPath(kObjectPath));
According to the documentation, GetObjectProxy never returns NULL, so change
this
to a DCHECK.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 249: dbus::MessageReader reader(response.get());
Should be isEnabled (small i).
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 262: accessibilityModuleInit();
This needs to be called even on errors (there's a DCHECK for that in the
Bus destructor). I suggest just refactoring into two methods where one
of them can keep the early returns.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 263: dbus->ShutdownAndBlock();
nit: put the condition the above preprocessor 'block' applies to
in a comment here.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 168: #if defined(USE_GCONF) || defined(USE_DBUS)
If this is false, then this function becomes totally empty, but as it is
written, that is not obvious.  I think it would be clearer if this #if would
be mvoed outside of the function. You could then put everything that requries
the condition to be true inside the same #if block and make the code easier to
read.  Write a separate version of this function in an #else that is empty.
I think it is a good idea to avoid preprocessing directives inside functions
whenever possible.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 181: #if defined(USE_GCONF) || defined(USE_DBUS)
nit: Stop isntead of Close would match te terminology in thread.h better.
(But if this function starts doing more things, it might be appropriate to
rename it to something else).
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 36: #endif
You could avoid some conditional compilation here and in the .cc file
by giving this a generic name (still telling what it does, but without referring
to
the facility used) and then have different versions of the method based on
the preprocessor defines.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 30: void CheckIfAccessibilityIsEnabled();
Why isn't this and the below method(s) private?
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 40: #if defined(USE_GCONF) || defined(USE_DBUS)
nit: suggest a blank line here.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 42: base::Callback<void(void)> accessibility_init_callback_;
Why does this need to be a member?
========================================================================
-- 

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

[Peter Lundblad, 2015-04-16 14:28:09]

Hi,

[Sorry for the unusual format of the comments below. I am using a script
to upload my comments and today it doesn't work for some reason.
I hope you can make sense of the below comments anywas.]

I agree with Dominic, this is getting closer. A few questions and
comments below.

Thanks,
//peter

========================================================================
File build/linux/system.gyp
------------------------------------
Line 691: 'conditions' : [
Why are you making this change?  Something that depends on this target will
need these deps and settings.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 206: g_object_unref(client);
Move this above the previous if statement and avoid an error-prone
duplication.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 175: base::Bind(accessibility_init_callback_),
Why do you need to bind this a second time?
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 177: base::Unretained(this)));
nit: intend further to make it clear that this is part of the bind call inside
the outer function call.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 213: VLOG(1) << "Enabling ATK accessibility support.";
Just to make sure, is it safe to call this (and indirectly
gnome_accessibility_module_init) from a thread other than the UI thread? I'd
suspect not, but I don't know gnome very well. Otherwise, this could just be
moved back into the
UI thread in the reply callback and still the checking with gconf could be done
in this thread.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 188: void AtkUtilAuraLinux::CheckGnomeAccessibilityKey() {
Should we DCHECK that we're on the right thread here as well as further down?
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 223: options.connection_type = dbus::Bus::PRIVATE;
I don't think this is needed since we're on this thread already.
This option is for when you want dbus operations to run on a separate thread
(for example if you would have gone the route of making asynchronous calls
from the UI thread).  With this implementation, you should just be able to drop
this line.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 230: if (!objectProxy) {
I think you can make this a VLOG(1).
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 241: objectProxy->CallMethodAndBlock(&methodCall,
nit: indent further.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 228: kServiceName, dbus::ObjectPath(kObjectPath));
According to the documentation, GetObjectProxy never returns NULL, so change
this
to a DCHECK.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 249: dbus::MessageReader reader(response.get());
Should be isEnabled (small i).
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 262: accessibilityModuleInit();
This needs to be called even on errors (there's a DCHECK for that in the
Bus destructor). I suggest just refactoring into two methods where one
of them can keep the early returns.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 263: dbus->ShutdownAndBlock();
nit: put the condition the above preprocessor 'block' applies to
in a comment here.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 168: #if defined(USE_GCONF) || defined(USE_DBUS)
If this is false, then this function becomes totally empty, but as it is
written, that is not obvious.  I think it would be clearer if this #if would
be mvoed outside of the function. You could then put everything that requries
the condition to be true inside the same #if block and make the code easier to
read.  Write a separate version of this function in an #else that is empty.
I think it is a good idea to avoid preprocessing directives inside functions
whenever possible.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 181: #if defined(USE_GCONF) || defined(USE_DBUS)
nit: Stop isntead of Close would match te terminology in thread.h better.
(But if this function starts doing more things, it might be appropriate to
rename it to something else).
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 36: #endif
You could avoid some conditional compilation here and in the .cc file
by giving this a generic name (still telling what it does, but without referring
to
the facility used) and then have different versions of the method based on
the preprocessor defines.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 30: void CheckIfAccessibilityIsEnabled();
Why isn't this and the below method(s) private?
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 40: #if defined(USE_GCONF) || defined(USE_DBUS)
nit: suggest a blank line here.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 42: base::Callback<void(void)> accessibility_init_callback_;
Why does this need to be a member?
========================================================================
-- 

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

[k.czech, 2015-04-16 21:58:40]

> Hi,
> 
> [Sorry for the unusual format of the comments below. I am using a script
> to upload my comments and today it doesn't work for some reason.
> I hope you can make sense of the below comments anywas.]
> 
> I agree with Dominic, this is getting closer. A few questions and
> comments below.
> 
> Thanks,
> //peter
> 
> ========================================================================
> File build/linux/system.gyp
> ------------------------------------
> Line 691: 'conditions' : [
> Why are you making this change?  Something that depends on this target will
> need these deps and settings.

"use_dbus" could be overriden by the (-D). In the case when use_dbus==0, dbus
target doesn't
need to be added to the project as it is now. You are right, this could break a
build when other
parts need a dbus and they do not use "use_dbus/USE_DBUS". This should rather go
in a separate patch.
Thanks.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 206: g_object_unref(client);
> Move this above the previous if statement and avoid an error-prone
> duplication.

Will do.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 175: base::Bind(accessibility_init_callback_),
> Why do you need to bind this a second time?
Right, I do not need it. Thanks.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 177: base::Unretained(this)));
> nit: intend further to make it clear that this is part of the bind call inside
> the outer function call.

Will do.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 213: VLOG(1) << "Enabling ATK accessibility support.";
> Just to make sure, is it safe to call this (and indirectly
> gnome_accessibility_module_init) from a thread other than the UI thread? I'd
> suspect not, but I don't know gnome very well. Otherwise, this could just be
> moved back into the
> UI thread in the reply callback and still the checking with gconf could be
done
> in this thread.

It's a good question, do you mean dynamic loading is not a thread safe ? or
there's another reason ?
That's sounds as a good idea to move it into the reply callback.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 188: void AtkUtilAuraLinux::CheckGnomeAccessibilityKey() {
> Should we DCHECK that we're on the right thread here as well as further down?
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 223: options.connection_type = dbus::Bus::PRIVATE;
> I don't think this is needed since we're on this thread already.
> This option is for when you want dbus operations to run on a separate thread
> (for example if you would have gone the route of making asynchronous calls
> from the UI thread).  With this implementation, you should just be able to
drop
> this line.

Thanks for the clarification.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 230: if (!objectProxy) {
> I think you can make this a VLOG(1).

Right, will do.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 241: objectProxy->CallMethodAndBlock(&methodCall,
> nit: indent further.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 228: kServiceName, dbus::ObjectPath(kObjectPath));
> According to the documentation, GetObjectProxy never returns NULL, so change
> this
> to a DCHECK.

Exactly, thanks.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 249: dbus::MessageReader reader(response.get());
> Should be isEnabled (small i).
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 262: accessibilityModuleInit();
> This needs to be called even on errors (there's a DCHECK for that in the
> Bus destructor). I suggest just refactoring into two methods where one
> of them can keep the early returns.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 263: dbus->ShutdownAndBlock();
> nit: put the condition the above preprocessor 'block' applies to
> in a comment here.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 168: #if defined(USE_GCONF) || defined(USE_DBUS)
> If this is false, then this function becomes totally empty, but as it is
> written, that is not obvious.  I think it would be clearer if this #if would
> be mvoed outside of the function. You could then put everything that requries
> the condition to be true inside the same #if block and make the code easier to
> read.  Write a separate version of this function in an #else that is empty.
> I think it is a good idea to avoid preprocessing directives inside functions
> whenever possible.

Thanks, will do.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 181: #if defined(USE_GCONF) || defined(USE_DBUS)
> nit: Stop isntead of Close would match te terminology in thread.h better.
> (But if this function starts doing more things, it might be appropriate to
> rename it to something else).

Will do.

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.h
> ------------------------------------
> Line 36: #endif
> You could avoid some conditional compilation here and in the .cc file
> by giving this a generic name (still telling what it does, but without
referring
> to
> the facility used) and then have different versions of the method based on
> the preprocessor defines.
Thanks, will do.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.h
> ------------------------------------
> Line 30: void CheckIfAccessibilityIsEnabled();
> Why isn't this and the below method(s) private?

Thanks, it should be private.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.h
> ------------------------------------
> Line 40: #if defined(USE_GCONF) || defined(USE_DBUS)
> nit: suggest a blank line here.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.h
> ------------------------------------
> Line 42: base::Callback<void(void)> accessibility_init_callback_;
> Why does this need to be a member?

I guess it does need to be, I will check it, thanks.
> ========================================================================

Thanks Peter for your review.

[dmazzoni, 2015-04-16 22:10:09]

On Thu, Apr 16, 2015 at 2:58 PM, <k.czech@samsung.com> wrote:

> File ui/accessibility/platform/atk_util_auralinux.cc
>>
> ------------------------------------
>> Line 213: VLOG(1) << "Enabling ATK accessibility support.";
>> Just to make sure, is it safe to call this (and indirectly
>> gnome_accessibility_module_init) from a thread other than the UI thread?
>> I'd
>> suspect not, but I don't know gnome very well. Otherwise, this could just
>> be
>> moved back into the
>> UI thread in the reply callback and still the checking with gconf could be
>>
> done
>
>> in this thread.
>>
>
> It's a good question, do you mean dynamic loading is not a thread safe ? or
> there's another reason ?
> That's sounds as a good idea to move it into the reply callback.


As soon as we call gnome_accessibility_module_init, that will trigger calls
back into Chrome from GNOME within that call stack, and we want to make
sure those calls happen on the UI thread.

Probably the call to gnome_accessibility_module_init doesn't need to be
inside of an #ifdef - we may want to add a command-line flag or check an
env var to force accessibility on or off.

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

[Peter Lundblad, 2015-04-17 14:44:23]

k.czech@samsung.com writes:
> > Hi,
> 
> > [Sorry for the unusual format of the comments below. I am using a script
> > to upload my comments and today it doesn't work for some reason.
> > I hope you can make sense of the below comments anywas.]
> 
> > I agree with Dominic, this is getting closer. A few questions and
> > comments below.
> 
> > Thanks,
> > //peter
> 
> > ========================================================================
> > File build/linux/system.gyp
> > ------------------------------------
> > Line 691: 'conditions' : [
> > Why are you making this change?  Something that depends on this target  
> > will
> > need these deps and settings.
> 
> "use_dbus" could be overriden by the (-D). In the case when use_dbus==0,  
> dbus
> target doesn't
> need to be added to the project as it is now. You are right, this could  
> break a
> build when other
> parts need a dbus and they do not use "use_dbus/USE_DBUS". This should  
> rather go
> in a separate patch.
> Thanks.
> 
> > ========================================================================
> > File ui/accessibility/platform/atk_util_auralinux.cc
> > ------------------------------------
> > Line 206: g_object_unref(client);
> > Move this above the previous if statement and avoid an error-prone
> > duplication.
> 
> Will do.
> 
> > ========================================================================
> > File ui/accessibility/platform/atk_util_auralinux.cc
> > ------------------------------------
> > Line 175: base::Bind(accessibility_init_callback_),
> > Why do you need to bind this a second time?
> Right, I do not need it. Thanks.
> > ========================================================================
> > File ui/accessibility/platform/atk_util_auralinux.cc
> > ------------------------------------
> > Line 177: base::Unretained(this)));
> > nit: intend further to make it clear that this is part of the bind call  
> > inside
> > the outer function call.
> 
> Will do.
> 
> > ========================================================================
> > File ui/accessibility/platform/atk_util_auralinux.cc
> > ------------------------------------
> > Line 213: VLOG(1) << "Enabling ATK accessibility support.";
> > Just to make sure, is it safe to call this (and indirectly
> > gnome_accessibility_module_init) from a thread other than the UI thread?  
> > I'd
> > suspect not, but I don't know gnome very well. Otherwise, this could just  
> > be
> > moved back into the
> > UI thread in the reply callback and still the checking with gconf could be
> done
> > in this thread.
> 
> It's a good question, do you mean dynamic loading is not a thread safe ? or
> there's another reason ?
I was specifically thinking of the gnome_accessibility_module_init call.
I'd expect library loading to be thread safe.


Thanks,
//Peter

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

[k.czech, 2015-04-20 10:40:27]

On 2015/04/16 22:10:09, dmazzoni wrote:
> On Thu, Apr 16, 2015 at 2:58 PM, <mailto:k.czech@samsung.com> wrote:
> 
> > File ui/accessibility/platform/atk_util_auralinux.cc
> >>
> > ------------------------------------
> >> Line 213: VLOG(1) << "Enabling ATK accessibility support.";
> >> Just to make sure, is it safe to call this (and indirectly
> >> gnome_accessibility_module_init) from a thread other than the UI thread?
> >> I'd
> >> suspect not, but I don't know gnome very well. Otherwise, this could just
> >> be
> >> moved back into the
> >> UI thread in the reply callback and still the checking with gconf could be
> >>
> > done
> >
> >> in this thread.
> >>
> >
> > It's a good question, do you mean dynamic loading is not a thread safe ? or
> > there's another reason ?
> > That's sounds as a good idea to move it into the reply callback.
> 
> 
> As soon as we call gnome_accessibility_module_init, that will trigger calls
> back into Chrome from GNOME within that call stack, and we want to make
> sure those calls happen on the UI thread.
> 
> Probably the call to gnome_accessibility_module_init doesn't need to be
> inside of an #ifdef - we may want to add a command-line flag or check an
> env var to force accessibility on or off.
> 
That's sounds as a good idea. Maybe we could use ACCESSIBILITY_ENABLED env ?.
I already used it in my patch.

[k.czech, 2015-04-20 10:42:47]

Peter, Dominic I'm looking forward for your comments. Thanks

[dmazzoni, 2015-04-20 16:33:22]

This is getting pretty close, mostly some style issues.

https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/atk_util_auralinux.cc (right):

https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:42: void
accessibilityModuleInit() {
AccessibilityModuleInit

https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:152: char* enableAccessibility =
getenv("ACCESSIBILITY_ENABLED");
Use enable_accessibility for the variable name.

Make "ACCESSIBILITY_ENABLED" a constant and put it at the top of the file.

https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:171: base::Thread::Options
threadOptions(base::MessageLoop::Type::TYPE_IO, 0);
thread_options

https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:214:
kGnomeAccessibilityEnabledKey,
indentation

https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:235: dbus::ObjectProxy*
objectProxy = dbus->GetObjectProxy(
object_proxy

https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/atk_util_auralinux.h (right):

https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.h:37: bool isEnabled;
is_enabled_

[k.czech, 2015-04-21 08:47:02]

On 2015/04/20 16:33:22, dmazzoni wrote:
> This is getting pretty close, mostly some style issues.
> 
>
https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
> File ui/accessibility/platform/atk_util_auralinux.cc (right):
> 
>
https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:42: void
> accessibilityModuleInit() {
> AccessibilityModuleInit
> 
>
https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:152: char* enableAccessibility
=
> getenv("ACCESSIBILITY_ENABLED");
> Use enable_accessibility for the variable name.
> 
> Make "ACCESSIBILITY_ENABLED" a constant and put it at the top of the file.
> 
>
https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:171: base::Thread::Options
> threadOptions(base::MessageLoop::Type::TYPE_IO, 0);
> thread_options
> 
>
https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:214:
> kGnomeAccessibilityEnabledKey,
> indentation
> 
>
https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:235: dbus::ObjectProxy*
> objectProxy = dbus->GetObjectProxy(
> object_proxy
> 
>
https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
> File ui/accessibility/platform/atk_util_auralinux.h (right):
> 
>
https://codereview.chromium.org/1028553003/diff/100001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.h:37: bool isEnabled;
> is_enabled_

Thanks Dominic, provided new patchset with all your comments.

[dmazzoni, 2015-04-21 22:32:42]

lgtm

OK with me to press "commit" after two more tiny issues and if trybots all seem
to pass (and ok to ignore obviously unrelated errors).

Please address any feedback from Peter if he posts any, but if you don't hear
anything go ahead and try to commit. We can continue to tweak and improve this
over time.

https://codereview.chromium.org/1028553003/diff/140001/ui/accessibility/platf...
File ui/accessibility/platform/atk_util_auralinux.cc (right):

https://codereview.chromium.org/1028553003/diff/140001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:150:
AtkUtilAuraLinux::AtkUtilAuraLinux() {
I would initialize is_enabled_ to false here to be safe:

AtkUtilAuraLinux::AtkUtilAuraLinux()
    : is_enabled_(false) {
  // Register our util class.
  ...
}

https://codereview.chromium.org/1028553003/diff/140001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:154: char* enableAccessibility =
getenv(kAccessibilityEnabled);
enable_accessibility

[Peter Lundblad, 2015-04-22 11:11:59]

lgtm

Minor comments, just stle

========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 28: const char kAccessibilityEnabled[] = "ACCESSIBILITY_ENABLED";
Add a brief comment explaining that this is the name of an env
var and what it does.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 184: accessibility_init_thread_.reset();
nit: DCHECK that we're on the right thread.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 34: void OnStopAccessibilityThread();
nit: ideally, method order in the .h and .cc file should match.
========================================================================

-- 

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

[k.czech, 2015-04-23 13:11:13]

On 2015/04/21 22:32:42, dmazzoni wrote:
> lgtm
> 
> OK with me to press "commit" after two more tiny issues and if trybots all
seem
> to pass (and ok to ignore obviously unrelated errors).
> 
> Please address any feedback from Peter if he posts any, but if you don't hear
> anything go ahead and try to commit. We can continue to tweak and improve this
> over time.
> 
>
https://codereview.chromium.org/1028553003/diff/140001/ui/accessibility/platf...
> File ui/accessibility/platform/atk_util_auralinux.cc (right):
> 
>
https://codereview.chromium.org/1028553003/diff/140001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:150:
> AtkUtilAuraLinux::AtkUtilAuraLinux() {
> I would initialize is_enabled_ to false here to be safe:
> 
> AtkUtilAuraLinux::AtkUtilAuraLinux()
>     : is_enabled_(false) {
>   // Register our util class.
>   ...
> }
> 
>
https://codereview.chromium.org/1028553003/diff/140001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:154: char* enableAccessibility
=
> getenv(kAccessibilityEnabled);
> enable_accessibility


There is one issue, bots are complaining about releasing accessibility thread in
UI thread.
Stopping thread is considered as a IO operation, so AssertIOAllowed is called.
What about when for a time when thread is being destructed allowing IO calls ?
and as soon as it's finished disallowing again.

Hmm releasing is done on reply callback when dbus call has just been finished
and at this moment thread could be safely destructed,
so IO should not be costly, but at the other hand, at the bottom pthread_join is
called which theoretically could consume some time, while waiting for this
thread to finish (but DBUS is finished so perhaps this should not be a
bottleneck, I guess)

Other thing comes to my mind, whether is it possible that completely other
thread could use this situation when IO is allowed and do some IO tasks ?
I proposed a patch with this change.


Dominic, Peter could you clarify those concerns ?

[dmazzoni, 2015-04-24 17:46:18]

I looked into this some more.

There's no good way to do this when creating our own thread. Sorry for leading
you astray.

I think the only good way to do it is to pass through the FILE thread so we can
use that thread.

I wrote this change to do the plumbing - if that's approved,
just rebase your change on top of that change and it should
work great.

https://codereview.chromium.org/1089003003/

[k.czech, 2015-04-27 14:10:36]

On 2015/04/24 17:46:18, dmazzoni wrote:
> I looked into this some more.
> 
> There's no good way to do this when creating our own thread. Sorry for leading
> you astray.

That's OK, I learned some new things about chromium.

> 
> I think the only good way to do it is to pass through the FILE thread so we
can
> use that thread.
> 
> I wrote this change to do the plumbing - if that's approved,
> just rebase your change on top of that change and it should
> work great.
> 
> https://codereview.chromium.org/1089003003/

Thanks Dominic as soon as it lands I will rebase my changes.

[dmazzoni, 2015-04-28 17:00:49]

Should be ready for you to rebase now

On Mon, Apr 27, 2015 at 7:10 AM, <k.czech@samsung.com> wrote:

> On 2015/04/24 17:46:18, dmazzoni wrote:
>
>> I looked into this some more.
>>
>
>  There's no good way to do this when creating our own thread. Sorry for
>> leading
>> you astray.
>>
>
> That's OK, I learned some new things about chromium.
>
>
>  I think the only good way to do it is to pass through the FILE thread so
>> we
>>
> can
>
>> use that thread.
>>
>
>  I wrote this change to do the plumbing - if that's approved,
>> just rebase your change on top of that change and it should
>> work great.
>>
>
>  https://codereview.chromium.org/1089003003/
>>
>
> Thanks Dominic as soon as it lands I will rebase my changes.
>
>
>
>
> https://codereview.chromium.org/1028553003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[k.czech, 2015-04-29 14:07:36]

On 2015/04/28 17:00:49, dmazzoni wrote:
> Should be ready for you to rebase now
> 
Thanks Dominic, it's already rebased and ready for review.

[Peter Lundblad, 2015-04-29 15:00:17]

Hi,


========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 169: scoped_refptr<base::TaskRunner> init_task_runner) {
Not part of this CL< but a previos one:
why are we not passing this smart pointer by const reference?
Unless there's a reason, perhaps just fix it here?
========================================================================
File ui/accessibility/platform/atk_util_auralinux.h
------------------------------------
Line 34: bool CheckPlatformAccessibilitySupport();
I'd suggest renaming these two above methods so they end with OnInitThread
to make it clear what thread they run on.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 165: AccessibilityModuleInit();
OK, so  we're now on the init (file) thread. This ethod indirectly calls
gnome_accessibility_module_init.  Did we convince ourselves that calling that
on a non-UI thread is safe?  (Sorry if we did and I missed that in this
thread;)
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 28: void AccessibilityModuleInit() {
Decide which thread this should be run on, give it a name reflecting that
and add an appropriate DCHECK.
========================================================================
File ui/accessibility/platform/atk_util_auralinux.cc
------------------------------------
Line 224: object_proxy->CallMethodAndBlock(&method_call,
Is there a drawback to using the nonblocking variant of this particular call
and
avoiding to block the file thread unnecessarily (the shutdown below has
to block and I think that's fine).  Nevermind if that turns out to be
annoyingly complex;)
========================================================================

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

[dmazzoni, 2015-04-29 16:02:21]

On Wed, Apr 29, 2015 at 8:00 AM, <plundblad@chromium.org> wrote:

> OK, so  we're now on the init (file) thread. This ethod indirectly calls
> gnome_accessibility_module_init.  Did we convince ourselves that calling
> that
> on a non-UI thread is safe?  (Sorry if we did and I missed that in this
> thread;)
>

Yeah, I think we should call g_module_open on the FILE thread, and check if
accessibility is enabled on the FILE thread, but if it is enabled we should
call gnome_accessibility_module_init on the main thread.

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

[k.czech, 2015-04-29 16:11:01]

On 2015/04/29 15:00:17, Peter Lundblad wrote:
> Hi,
> 
> 
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 169: scoped_refptr<base::TaskRunner> init_task_runner) {
> Not part of this CL< but a previos one:
> why are we not passing this smart pointer by const reference?
> Unless there's a reason, perhaps just fix it here?

Will check this.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.h
> ------------------------------------
> Line 34: bool CheckPlatformAccessibilitySupport();
> I'd suggest renaming these two above methods so they end with OnInitThread
> to make it clear what thread they run on.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 165: AccessibilityModuleInit();
> OK, so  we're now on the init (file) thread. This ethod indirectly calls
> gnome_accessibility_module_init.  Did we convince ourselves that calling that
> on a non-UI thread is safe?  (Sorry if we did and I missed that in this
> thread;)

Yes, that's true it should be called from main thread. Will fix this.
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 28: void AccessibilityModuleInit() {
> Decide which thread this should be run on, give it a name reflecting that
> and add an appropriate DCHECK.

Will do

> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 224: object_proxy->CallMethodAndBlock(&method_call,
> Is there a drawback to using the nonblocking variant of this particular call
> and
> avoiding to block the file thread unnecessarily (the shutdown below has
> to block and I think that's fine).  Nevermind if that turns out to be
> annoyingly complex;)
> ========================================================================

Will check this.

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

[k.czech, 2015-04-29 16:27:03]

On 2015/04/29 15:00:17, Peter Lundblad wrote:
> Hi,
> 
> 
> ========================================================================
> File ui/accessibility/platform/atk_util_auralinux.cc
> ------------------------------------
> Line 169: scoped_refptr<base::TaskRunner> init_task_runner) {
> Not part of this CL< but a previos one:
> why are we not passing this smart pointer by const reference?
> Unless there's a reason, perhaps just fix it here?

scoped_refptr here handles reference counting of init_task_runner,
so passing it by reference will disturb this process. We should let him
decide when to increase/decrease this counter.

[stevenjb, 2015-04-29 16:37:13]

stevenjb@chromium.org changed reviewers:
- stevenjb@chromium.org

[dmazzoni, 2015-04-29 16:53:50]

On Wed, Apr 29, 2015 at 9:27 AM, <k.czech@samsung.com> wrote:

> On 2015/04/29 15:00:17, Peter Lundblad wrote:
>
>> Hi,
>>
>
>
>  ========================================================================
>> File ui/accessibility/platform/atk_util_auralinux.cc
>> ------------------------------------
>> Line 169: scoped_refptr<base::TaskRunner> init_task_runner) {
>> Not part of this CL< but a previos one:
>> why are we not passing this smart pointer by const reference?
>> Unless there's a reason, perhaps just fix it here?
>>
>
> scoped_refptr here handles reference counting of init_task_runner,
> so passing it by reference will disturb this process. We should let him
> decide when to increase/decrease this counter.


Yeah, I think this is okay. It's meant to be smart about being passed this
way.

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

[Peter Lundblad, 2015-04-30 07:11:05]

Dominic Mazzoni writes:
> On Wed, Apr 29, 2015 at 9:27 AM, <k.czech@samsung.com> wrote:
> 
>     On 2015/04/29 15:00:17, Peter Lundblad wrote:
>    
>         Hi,
> 
>        
======================================================================
>         ==
>         File ui/accessibility/platform/atk_util_auralinux.cc
>         ------------------------------------
>         Line 169: scoped_refptr<base::TaskRunner> init_task_runner) {
>         Not part of this CL< but a previos one:
>         why are we not passing this smart pointer by const reference?
>         Unless there's a reason, perhaps just fix it here?
> 
>     scoped_refptr here handles reference counting of init_task_runner,
>     so passing it by reference will disturb this process. We should let him
>     decide when to increase/decrease this counter.
> 
> Yeah, I think this is okay. It's meant to be smart about being passed this
> way.
> 
Clearly, passing by value is not an error.
Passing by const reference is also valid since the caller's copy of the
object must be valid throughout the function call and the calee can take a copy
if needed.  IN my experience, const reference is usually preferred
for refcoutned pointers to avoid redundant changes to the ref counter
(which can be expensive if it is thread safe for example).
In this case, performance doesn't matter obviously, so I am fine leaving as-is.
Just wanted to explain the reasoning behind asking this question;)

Let me know when the patch is ready for another review.

Thanks,
//Peter

-- 

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

[k.czech, 2015-04-30 15:02:54]

> Line 224: object_proxy->CallMethodAndBlock(&method_call,
> Is there a drawback to using the nonblocking variant of this particular call
> and
> avoiding to block the file thread unnecessarily (the shutdown below has
> to block and I think that's fine).  Nevermind if that turns out to be
> annoyingly complex;)
> ========================================================================

Peter please correct me if I'm wrong.

Nonblocking variant will post a task on the origin thread so in our case it's
going to be file thread,
so at the end we will block it.
Having separate async post, another thread and its task runner should be used
and that thread should be created
right inside the file thread.

[dmazzoni, 2015-04-30 15:12:57]

On Thu, Apr 30, 2015 at 8:02 AM, <k.czech@samsung.com> wrote:

> Line 224: object_proxy->CallMethodAndBlock(&method_call,
>> Is there a drawback to using the nonblocking variant of this particular
>> call
>> and
>> avoiding to block the file thread unnecessarily (the shutdown below has
>> to block and I think that's fine).  Nevermind if that turns out to be
>> annoyingly complex;)
>> ========================================================================
>>
>
> Peter please correct me if I'm wrong.
>
> Nonblocking variant will post a task on the origin thread so in our case
> it's
> going to be file thread,
>

Yes.


> so at the end we will block it.
>

What, why? You'll just make that call and the function will end, then the
reply will come on the FILE thread also, and then you'll make the async
call back to the UI thread with the final result.


> Having separate async post, another thread and its task runner should be
> used
> and that thread should be created
> right inside the file thread.


No, don't create any new threads.

My vote is not to change CallMethodAndBlock in this patch. This patch is
definitely better than what we had before - before we were blocking the
main thread, now we're blocking the FILE thread. Blocking the FILE thread
briefly is a thousand times better, so let's get this change landed and
then follow up.

How about a TODO right above CallMethodAndBlock to replace this with the
asynchronous variant in a follow-up.

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

[k.czech, 2015-04-30 16:03:26]

On 2015/04/30 15:12:57, dmazzoni wrote:
> On Thu, Apr 30, 2015 at 8:02 AM, <mailto:k.czech@samsung.com> wrote:
> 
> > Line 224: object_proxy->CallMethodAndBlock(&method_call,
> >> Is there a drawback to using the nonblocking variant of this particular
> >> call
> >> and
> >> avoiding to block the file thread unnecessarily (the shutdown below has
> >> to block and I think that's fine).  Nevermind if that turns out to be
> >> annoyingly complex;)
> >> ========================================================================
> >>
> >
> > Peter please correct me if I'm wrong.
> >
> > Nonblocking variant will post a task on the origin thread so in our case
> > it's
> > going to be file thread,
> >
> 
> Yes.
> 
> 
> > so at the end we will block it.
> >
> 
> What, why? You'll just make that call and the function will end, then the
> reply will come on the FILE thread also, and then you'll make the async
> call back to the UI thread with the final result.

Yes, that is true. Sorry for misleading. I was rather meant that somewhere
inside native dbus some blocks occur
and that might have impact on file thread as well but native DBUs API supports
also non blocking calls and I guess those are used to support
async calls.

> 
> 
> > Having separate async post, another thread and its task runner should be
> > used
> > and that thread should be created
> > right inside the file thread.
> 
> 
> No, don't create any new threads.
> 
> My vote is not to change CallMethodAndBlock in this patch. This patch is
> definitely better than what we had before - before we were blocking the
> main thread, now we're blocking the FILE thread. Blocking the FILE thread
> briefly is a thousand times better, so let's get this change landed and
> then follow up.
> 
> How about a TODO right above CallMethodAndBlock to replace this with the
> asynchronous variant in a follow-up.
> 

That sounds ok with me.

[k.czech, 2015-05-04 11:31:03]

Dominic, Peter patch is ready to review.

Following Dominic's suggestion I added two TODOs (one is for
adding async call and the second is I found in the old version of at-spi2-core,
org.a11y.Bus, IsEnabled is returned as a struct with a bool but should be just
bool). As soon as it lands, I will fix those two.

Peter, regarding DCHECK to check the current thread(FILE, UI), it looks like
content_browser target should be included to check BrowserTHread::FILE/UI. This
leads to the circular dependency with accessibility.gyp that is already included
in content.

[Peter Lundblad, 2015-05-04 12:20:25]

Hi,

I think this looks good now, just move the opening of the bridge module
to the file thread and we should be ready to go.

https://codereview.chromium.org/1028553003/diff/220001/ui/accessibility/platf...
File ui/accessibility/platform/atk_util_auralinux.cc (right):

https://codereview.chromium.org/1028553003/diff/220001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:34: GModule* bridge =
g_module_open(atk_bridge_path.value().c_str(),
I agree with Dominic's earlier comment aht this, i.e. calling
g_module_open, should happen in the file thread. Opening a shared libary
must involve blocking file I/O.

[k.czech, 2015-05-04 13:52:55]

On 2015/05/04 12:20:25, Peter Lundblad wrote:
> Hi,
> 
> I think this looks good now, just move the opening of the bridge module
> to the file thread and we should be ready to go.
> 
>
https://codereview.chromium.org/1028553003/diff/220001/ui/accessibility/platf...
> File ui/accessibility/platform/atk_util_auralinux.cc (right):
> 
>
https://codereview.chromium.org/1028553003/diff/220001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:34: GModule* bridge =
> g_module_open(atk_bridge_path.value().c_str(),
> I agree with Dominic's earlier comment aht this, i.e. calling
> g_module_open, should happen in the file thread. Opening a shared libary
> must involve blocking file I/O.

Ohh, that's true, thanks Peter.
New patchset, it's ready to review.
I also made some cleanup.

[dmazzoni, 2015-05-04 15:41:05]

https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
File ui/accessibility/platform/atk_util_auralinux.cc (right):

https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:32:
gnome_accessibility_module_init accessibility_module_init = nullptr;
nit: this is a global, so by convention we prefix it with g_:

gnome_accessibility_module_init g_accessibility_module_init = nullptr;

https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:193: is_enabled_ = true;
nit: indent by only 2.

How about just this instead:

is_enabled_ = AccessibilityModuleInitOnFileThread();

https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:198:
AccessibilityModuleInitOnFileThread();
You need to set is_enabled_ here too:

is_enabled_ = AccessibilityModuleInitOnFileThread();

https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:211: is_enabled_ =
gconf_client_get_bool(client,
Nit: make this a local variable: bool is_enabled = gconf_client_get_bool(client,
... because this function returns a bool and the calling function sets
is_enabled_ to the result - so setting it twice is confusing.

https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:223: return is_enabled_;
return is_enabled (not is_enabled_)

https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:255: if
(!reader.PopVariantOfBool(&is_enabled_))
Same here, put this in a local variable and return it.

https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:260: return is_enabled_;
Return is_enabled, not is_enabled_

[k.czech, 2015-05-06 10:35:00]

On 2015/05/04 15:41:05, dmazzoni wrote:
>
https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
> File ui/accessibility/platform/atk_util_auralinux.cc (right):
> 
>
https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:32:
> gnome_accessibility_module_init accessibility_module_init = nullptr;
> nit: this is a global, so by convention we prefix it with g_:
Done.
> 
> gnome_accessibility_module_init g_accessibility_module_init = nullptr;
> 
>
https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:193: is_enabled_ = true;
> nit: indent by only 2.
> 
> How about just this instead:
> 
> is_enabled_ = AccessibilityModuleInitOnFileThread();
Sounds good, done.
> 
>
https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:198:
> AccessibilityModuleInitOnFileThread();
> You need to set is_enabled_ here too:
> 
> is_enabled_ = AccessibilityModuleInitOnFileThread();
> 
>
https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:211: is_enabled_ =
> gconf_client_get_bool(client,
> Nit: make this a local variable: bool is_enabled =
gconf_client_get_bool(client,
> ... because this function returns a bool and the calling function sets
> is_enabled_ to the result - so setting it twice is confusing.
Done.
> 
>
https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:223: return is_enabled_;
> return is_enabled (not is_enabled_)
Done.
> 
>
https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:255: if
> (!reader.PopVariantOfBool(&is_enabled_))
> Same here, put this in a local variable and return it.
Done.
> 
>
https://codereview.chromium.org/1028553003/diff/240001/ui/accessibility/platf...
> ui/accessibility/platform/atk_util_auralinux.cc:260: return is_enabled_;
> Return is_enabled, not is_enabled_
Done.

[Peter Lundblad, 2015-05-06 11:36:44]

lgtm

Now,we're down to minor things only. Please address and I am happy
with this change.

https://codereview.chromium.org/1028553003/diff/260001/ui/accessibility/platf...
File ui/accessibility/platform/atk_util_auralinux.cc (right):

https://codereview.chromium.org/1028553003/diff/260001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:196: if
(CheckPlatformAccessibilitySupportOnFileThread())
nit: this function could be simplified into one if statement since
the then parts are the same (modulo the return in the first one which would
go away).

https://codereview.chromium.org/1028553003/diff/260001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:219: return false
Missing semicolon.

https://codereview.chromium.org/1028553003/diff/260001/ui/accessibility/platf...
ui/accessibility/platform/atk_util_auralinux.cc:281: if
(g_accessibility_module_init)
g_accessibility_module_init should never be null if is_enabled_ is true, so
catch that with a DCHECK instead of this if.

[dmazzoni, 2015-05-06 14:23:04]

lgtm too with Peter's comments addressed

[k.czech, 2015-05-06 15:17:24]

On 2015/05/06 14:23:04, dmazzoni wrote:
> lgtm too with Peter's comments addressed

Done, thanks. Let's try to commit this change.

[dmazzoni, 2015-05-06 15:19:23]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-05-06 15:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1028553003/#ps280001
(title: "more cleanup")

[commit-bot: I haz the power, 2015-05-06 15:20:01]

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

[commit-bot: I haz the power, 2015-05-06 15:26:27]

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

[commit-bot: I haz the power, 2015-05-06 15:26:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[k.czech, 2015-05-06 16:48:57]

k.czech@samsung.com changed reviewers:
+ stevenjb@chromium.org

[k.czech, 2015-05-06 17:13:45]

k.czech@samsung.com changed reviewers:
+ dpranke@chromium.org

[k.czech, 2015-05-06 22:23:40]

The CQ bit was checked by k.czech@samsung.com

[commit-bot: I haz the power, 2015-05-06 22:24:45]

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

[commit-bot: I haz the power, 2015-05-06 23:21:56]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-05-06 23:22:53]

Patchset 15 (id:??) landed as
https://crrev.com/16e2d5a386e3ddd5be818a0d27610b1e9ee9c08e
Cr-Commit-Position: refs/heads/master@{#328651}

[shreeramk, 2015-05-07 11:51:06]

@K.czech: I am getting following error while I execute
./tools/accessibility/dump_accessibility_tree_auralinux.py this script.

desktop frame name="main" (0, 0) size (1024 x 768)
Traceback (most recent call last):
  File "./tools/accessibility/dump_accessibility_tree_auralinux.py", line 42, in
<module>
    Dump(desktop, 0)
  File "./tools/accessibility/dump_accessibility_tree_auralinux.py", line 39, in
Dump
    Dump(obj.get_child_at_index(i), indent + 1)
  File "./tools/accessibility/dump_accessibility_tree_auralinux.py", line 28, in
Dump
    bounds = obj.get_extents(pyatspi.DESKTOP_COORDS)
  File "/usr/lib/python2.7/dist-packages/gi/types.py", line 43, in function
    return info.invoke(*args, **kwargs)
gi._glib.GError: Method "GetExtents" with signature "u" on interface
"org.a11y.atspi.Component" doesn't exist


Could you please have a look?

[k.czech, 2015-05-07 23:39:01]

On 2015/05/07 11:51:06, shreeramk wrote:
> @K.czech: I am getting following error while I execute
> ./tools/accessibility/dump_accessibility_tree_auralinux.py this script.
> 
> desktop frame name="main" (0, 0) size (1024 x 768)
> Traceback (most recent call last):
>   File "./tools/accessibility/dump_accessibility_tree_auralinux.py", line 42,
in
> <module>
>     Dump(desktop, 0)
>   File "./tools/accessibility/dump_accessibility_tree_auralinux.py", line 39,
in
> Dump
>     Dump(obj.get_child_at_index(i), indent + 1)
>   File "./tools/accessibility/dump_accessibility_tree_auralinux.py", line 28,
in
> Dump
>     bounds = obj.get_extents(pyatspi.DESKTOP_COORDS)
>   File "/usr/lib/python2.7/dist-packages/gi/types.py", line 43, in function
>     return info.invoke(*args, **kwargs)
> gi._glib.GError: Method "GetExtents" with signature "u" on interface
> "org.a11y.atspi.Component" doesn't exist
> 
> 
> Could you please have a look?

@shreeramk 	
Could you tell the OS you tested on and pyatspi ?. Hmm, I did not have such a
problem having this change.
This CL does not really modify atspi related things.

[shatch, 2015-05-12 14:44:30]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1124173005/ by simonhatch@chromium.org.

The reason for reverting is: All linux perf bots red, failing on various
blink_perf tests. Speculatively reverting this CL since several bisects pointed
to it.

BUG=485590.

[k.czech, 2015-05-12 15:06:45]

k.czech@samsung.com changed reviewers:
- dpranke@chromium.org, jochen@chromium.org, l.gombos@samsung.com,
stevenjb@chromium.org

[k.czech, 2015-05-12 15:06:46]

Dominic, Peter, do you think posting task to the file thread could cause those
troubles with perf bots. If bots use use_gconf=0 flag in this case, we could
suspect that this is a reason of dbus.