Closed
Bug 828463
Opened 12 years ago
Closed 12 years ago
Remove accessibility from mozilla-b2g18
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-basecamp:-)
RESOLVED
WONTFIX
blocking-basecamp | - |
People
(Reporter: hub, Unassigned)
Details
Attachments
(2 files)
1.22 KB,
patch
|
tbsaunde
:
review-
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
davidb
:
feedback+
|
Details | Diff | Splinter Review |
Remove AccessFu from the current release train (ie gecko-18)
AccessFu is used for Accessibility touch discovery and given the feature is not part of that build, we don't need it.
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
For consideration. Not urgent but I'd like this code to not mistakenly instanciate Accessibility.
blocking-basecamp: --- → ?
Comment 3•12 years ago
|
||
Comment on attachment 699857 [details] [diff] [review]
Bug 828463 - Remove AccessFu.
>-Cu.import('resource://gre/modules/accessibility/AccessFu.jsm');
>+// XXX reenable for accessibility. Bug 828463
>+//Cu.import('resource://gre/modules/accessibility/AccessFu.jsm');
why don't you consider to build b2g with --disable-accessibility? (and btw why isn't that code under #ifdef ACCESSIBILITY guard?)
Updated•12 years ago
|
Summary: Remove AccessFu from the current release train → Remove accessibility from mozilla-b2g18
Reporter | ||
Comment 4•12 years ago
|
||
I am opposed to --disable-accessibility at this stage.
I don't know why it isn't surrounded by a #ifdef ACCESSIBILITY, but in that case it is irrelevant since we want to remove it while not removing base accessibility support.
Summary: Remove accessibility from mozilla-b2g18 → Remove AccessFu from the current release train
Comment 5•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #4)
> I am opposed to --disable-accessibility at this stage.
why? I think not disabling it requires some substantial justification since it probably wins you 100 k les of code at very low risk. I'm only guessing I've never measured the effect on size.
Comment 6•12 years ago
|
||
Okay, bb- but please request uplift for a patch if you'd like.
blocking-basecamp: ? → -
Reporter | ||
Comment 7•12 years ago
|
||
For some reason, the title got reverted after dougt changes :-/
(In reply to Andrew Overholt [:overholt] from comment #6)
> Okay, bb- but please request uplift for a patch if you'd like.
The problem is that I want this only to happen in b2g18. But since we don't seem have noticed any issue, I'm possibly becoming over paranoid.
Summary: Remove AccessFu from the current release train → Remove accessibility from mozilla-b2g18
Comment 8•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #4)
> I am opposed to --disable-accessibility at this stage.
>
I agree. Shaving off a few KB, or getting some minor performance gains by removing a11y is a bad idea in an app where a11y is in the roadmap. Once we enable it again people are going to wonder why there are regressions, and we are going to get last minute push back. If there are any unacceptable drawbacks for building with a11y support the underlying issues should be addressed now, and not kicked down the road for that day when we drop a11y back in unannounced.
This is all hypothetical of course. I don't think a11y introduces any noticeable issues.
Comment 9•12 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #8)
> (In reply to Hub Figuiere [:hub] from comment #4)
> > I am opposed to --disable-accessibility at this stage.
> >
>
> I agree. Shaving off a few KB, or getting some minor performance gains by
> removing a11y is a bad idea in an app where a11y is in the roadmap. Once we
its removing dead code not a11y since that code never runs and never does anything.
> enable it again people are going to wonder why there are regressions, and we
> are going to get last minute push back. If there are any unacceptable
> drawbacks for building with a11y support the underlying issues should be
> addressed now, and not kicked down the road for that day when we drop a11y
> back in unannounced.
wait who said it should be last minute or unanounced???
what's unacceptable and should be fixed is large amounts of dead code. Which isn't a problem if that code isn't dead.
> This is all hypothetical of course. I don't think a11y introduces any
> noticeable issues.
other than if we put a bunch of dead code in libxul there is almost certainly no effect of just changing between --disable-accessibility and --enable-accessibility which is exactly why we should make it --disable-accessibility on branches where we know the code won't be used.
Comment 10•12 years ago
|
||
I'm going to stop spamming this bug after this post.
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> (In reply to Eitan Isaacson [:eeejay] from comment #8)
> > (In reply to Hub Figuiere [:hub] from comment #4)
> > > I am opposed to --disable-accessibility at this stage.
> > >
> >
> > I agree. Shaving off a few KB, or getting some minor performance gains by
> > removing a11y is a bad idea in an app where a11y is in the roadmap. Once we
>
> its removing dead code not a11y since that code never runs and never does
> anything.
>
> > enable it again people are going to wonder why there are regressions, and we
> > are going to get last minute push back. If there are any unacceptable
> > drawbacks for building with a11y support the underlying issues should be
> > addressed now, and not kicked down the road for that day when we drop a11y
> > back in unannounced.
>
> wait who said it should be last minute or unanounced???
>
I said we would get push back in the last minute. Because people would be living under the false assumption that libxul is a couple hundred KBs thinner and that our memory footprint is smaller, this could lead to all kinds of decisions that could bite us once we relink it. Of course I am projecting here a bit.
Tangent: During the Android native rewrite, when everyone was perf conscious, it was a tough choice to build with accessibility because previous XUL versions didn't have it. Of course, the XUL version was bloated *despite* not having a11y in it, but when everyone is focused on optimizations and you want to reintroduce a whole chunk of code, it is a bitter pill for everyone to swallow.
> what's unacceptable and should be fixed is large amounts of dead code.
> Which isn't a problem if that code isn't dead.
>
> > This is all hypothetical of course. I don't think a11y introduces any
> > noticeable issues.
>
> other than if we put a bunch of dead code in libxul there is almost
> certainly no effect of just changing between --disable-accessibility and
> --enable-accessibility which is exactly why we should make it
> --disable-accessibility on branches where we know the code won't be used.
The fact that there is no effect makes me reach the exact opposite conclusion. Disabling accessibility is not an optimization, at best it is a temporary measure. It will need to be relinked soon, and if relinking pushes us over some proverbial edge we will need to find something else to cut.
Comment 11•12 years ago
|
||
So, I have some rough numbers for linux64 opt builds. Which are about 450k of code and 210 of data. Presumably a significant bit of the data is duplicated, and so --disable-accessibility won't actually make libxul more than .5mb smaller, but still that's a substantial amount of dead code :)
tbsaunde@iceball:/src/firefox-opt3$ find accessible/ -name *.o | xargs size | awk '{ sum += $1 } END { print sum }'
457393
tbsaunde@iceball:/src/firefox-opt3$ find accessible/ -name *.o | xargs size | awk '{ sum += $2 } END { print sum }'
208940
Comment 12•12 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #10)
> I'm going to stop spamming this bug after this post.
ok, I'm perfectly happy to stop worrying about convincing you that you are wrong and write a patch ;)
Comment 13•12 years ago
|
||
Dougt I assume updating the in tree mozconfigs is the sane way to do this? (note adding the ifdefs is something we need to do anyway)
Attachment #701151 -
Flags: review?(doug.turner)
Comment 14•12 years ago
|
||
I like the idea of us not unnecessarily making b2g fatter. However...
There are a few things I'd like to know:
1. Has our a11y engine shown up in any b2g profiling? (accidentally instantiated)
2. Would reducing footprint bring us a significant win?
3. Could a b2g extension make use of our a11y engine and are we potentially breaking that by fixing this bug?
Finally, there is non-zero risk in removing a big chunk of code late in the game even if it is dormant.
Comment 15•12 years ago
|
||
Comment on attachment 701151 [details] [diff] [review]
bug 828463 - disable a11y on b2g
Review of attachment 701151 [details] [diff] [review]:
-----------------------------------------------------------------
I think the only way we should approve this is if accessibility is completely busted on b2g -- that is, with it on it hurts the experience for all users.
I am speaking from a bit of experience -- I proposed and committed similar patches for mobile many times. It was short sighted and unimaginative. Lets do the hard work to understand why a11y is this big and use that data to improve a11y for everyone.
::: b2g/config/mozconfigs/common
@@ +6,5 @@
>
> . "$topsrcdir/build/mozconfig.common"
> +
> +# disable accessibility until its used (bug 828463)
> +ac_add_options --disable-accessibility
This change goes against what Mozilla should stand for -- building products for everyone.
Attachment #701151 -
Flags: review?(doug.turner) → review-
Updated•12 years ago
|
Attachment #701151 -
Flags: review?(dbolter)
Comment 16•12 years ago
|
||
> I think the only way we should approve this is if accessibility is
> completely busted on b2g -- that is, with it on it hurts the experience for
> all users.
Well, afaik there is no way to enable accessibility on b2g so there is nothing to be busted.
> I am speaking from a bit of experience -- I proposed and committed similar
> patches for mobile many times. It was short sighted and unimaginative.
I'm not sure what you mean here.
> Lets do the hard work to understand why a11y is this big and use that data
> to improve a11y for everyone.
I only think we should do this because this code can never be used, so any size substantially larger than 0 is something we should just not build.
> ::: b2g/config/mozconfigs/common
> @@ +6,5 @@
> >
> > . "$topsrcdir/build/mozconfig.common"
> > +
> > +# disable accessibility until its used (bug 828463)
> > +ac_add_options --disable-accessibility
>
> This change goes against what Mozilla should stand for -- building products
> for everyone.
I'd agree if reverting this would help anyone but it won't because there's no way to use the code enabling it would cause to be built.
> 1. Has our a11y engine shown up in any b2g profiling? (accidentally
> instantiated)
that might be interesting but I don't think its particularly relevent.
> 2. Would reducing footprint bring us a significant win?
I suspect we've spent more energy on smaller reductions, but that's not really an answer. Of course you have to define "significant"...
> 3. Could a b2g extension make use of our a11y engine and are we potentially
> breaking that by fixing this bug?
I have no idea if we allow extensions on b2g that can use random xpcom. If people an write extensions that use random xpcom that would be a good reason to want to build a11y, but I'm not sure how much we care keep in mind we didn't support a11y xpcom accessibility on mac for a long time.
> Finally, there is non-zero risk in removing a big chunk of code late in the
> game even if it is dormant.
true, we certainly should have considered this earlier when we decided that we weren't going to have working a11y in b2g v1. However I'd think the only risk greater than epsilon is js that we can't prove at build time doesn't rely on stuff that's only available when we build a11y.
Comment 17•12 years ago
|
||
Comment on attachment 701151 [details] [diff] [review]
bug 828463 - disable a11y on b2g
Heyo. Did you find the try build binary size?
Please re-request if the value/cost ratio clearly flips (e.g. huge size cost or accidental instantiation) but right now I can't endorse removing the a11y engine if there is any potential for back-porting some usage.
Attachment #701151 -
Flags: review?(dbolter) → review-
Comment 18•12 years ago
|
||
Doug can you help with Trevor's questions in comment 16?
Flags: needinfo?(doug.turner)
Comment 19•12 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #16)
> > 3. Could a b2g extension make use of our a11y engine and are we potentially
> > breaking that by fixing this bug?
>
> I have no idea if we allow extensions on b2g that can use random xpcom. If
> people an write extensions that use random xpcom that would be a good reason
> to want to build a11y, but I'm not sure how much we care keep in mind we
> didn't support a11y xpcom accessibility on mac for a long time.
This would be good to nail down.
Comment 20•12 years ago
|
||
CC+ Fabrice, Philikon.
Comment 21•12 years ago
|
||
(In reply to David Bolter [:davidb] from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
>
> > > 3. Could a b2g extension make use of our a11y engine and are we potentially
> > > breaking that by fixing this bug?
> >
> > I have no idea if we allow extensions on b2g that can use random xpcom. If
> > people an write extensions that use random xpcom that would be a good reason
> > to want to build a11y, but I'm not sure how much we care keep in mind we
> > didn't support a11y xpcom accessibility on mac for a long time.
>
>
> This would be good to nail down.
I have no idea what you mean here
(In reply to David Bolter [:davidb] from comment #17)
> Comment on attachment 701151 [details] [diff] [review]
> bug 828463 - disable a11y on b2g
>
> Heyo. Did you find the try build binary size?
no, mostly because of infr issues when I tried, but I don't really care much its significant and this code is dead.
> Please re-request if the value/cost ratio clearly flips (e.g. huge size cost
> or accidental instantiation) but right now I can't endorse removing the a11y
> engine if there is any potential for back-porting some usage.
all dead code could theoretically be useful someday, so should we never stop building any of it?
Comment 22•12 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #21)
> (In reply to David Bolter [:davidb] from comment #19)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> >
> > > > 3. Could a b2g extension make use of our a11y engine and are we potentially
> > > > breaking that by fixing this bug?
> > >
> > > I have no idea if we allow extensions on b2g that can use random xpcom. If
> > > people an write extensions that use random xpcom that would be a good reason
> > > to want to build a11y, but I'm not sure how much we care keep in mind we
> > > didn't support a11y xpcom accessibility on mac for a long time.
> >
> >
> > This would be good to nail down.
>
> I have no idea what you mean here
It is an expression. Turns out that partners lock down extensions (bundles I think they are called). Assuming partners can also flip the build config for building a11y or not... this would be moot as per IRC.
>
> (In reply to David Bolter [:davidb] from comment #17)
> > Comment on attachment 701151 [details] [diff] [review]
> > bug 828463 - disable a11y on b2g
> >
> > Heyo. Did you find the try build binary size?
>
> no, mostly because of infr issues when I tried, but I don't really care much
> its significant and this code is dead.
>
> > Please re-request if the value/cost ratio clearly flips (e.g. huge size cost
> > or accidental instantiation) but right now I can't endorse removing the a11y
> > engine if there is any potential for back-porting some usage.
>
> all dead code could theoretically be useful someday, so should we never
> stop building any of it?
No.
Comment 23•12 years ago
|
||
Comment on attachment 699857 [details] [diff] [review]
Bug 828463 - Remove AccessFu.
ftr: incase it wasn't obvious, we shouldn't remove the only possible usage of code and not remove the code.
Attachment #699857 -
Flags: review-
Comment 24•12 years ago
|
||
(In reply to David Bolter [:davidb] from comment #17)
> Comment on attachment 701151 [details] [diff] [review]
> bug 828463 - disable a11y on b2g
>
> Heyo. Did you find the try build binary size?
>
> Please re-request if the value/cost ratio clearly flips (e.g. huge size cost
> or accidental instantiation) but right now I can't endorse removing the a11y
> engine if there is any potential for back-porting some usage.
I don't really have much more to say given
15:03 <@davidb> so it sounds like the usage of the a11y engine is under control of the partner
and
15:05 <@tbsaunde> davidb: well, they control how gecko is built I would assume, so if they want to use a11y then they can just build with it enabled
wasn't enough to convince you this code is completely and totally useless I don't see any reason to believe you can be convinced it is.
In general I hate these situations where we both think the other option is completely unacceptable, and the only deciding factor is what happens to be true.
Comment 25•12 years ago
|
||
Aside: Was a bit baffled by that last comment. I talked with Trevor in person to help clarify that I do not find the patch completely unacceptable. My r- was based on concerns about shutting the door to potential a11y backport by extension. My comment 22 was intended to show I now don't think this is an option.
I woke up this morning thinking that removing a11y from the build config *given there is no way to use it* is far more palatable.
Given all that, I think it is good project citizen citizenship to land this. A significant win may make it harder to add a11y back in but I'm willing to fight that fight.
That's my position.
Updated•12 years ago
|
Attachment #701151 -
Flags: review- → feedback+
Comment 26•12 years ago
|
||
Comment on attachment 701151 [details] [diff] [review]
bug 828463 - disable a11y on b2g
Doug I think you should reconsider your position here understanding that no users will be helped by a11y in b2g as things stand, and per David's last comment I'm not the only a11y peer who thinks we should do this.
Attachment #701151 -
Flags: review- → review?
Reporter | ||
Comment 28•12 years ago
|
||
That was a silly request. WONTFIX.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Updated•11 years ago
|
Attachment #701151 -
Flags: review?
You need to log in
before you can comment on or make changes to this bug.
Description
•