Last Comment Bug 739889 - densify nsOuterDocAccessible
: densify nsOuterDocAccessible
Status: RESOLVED FIXED
[good first bug][mentor=askalsky@mozi...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: densifya11y
  Show dependency treegraph
 
Reported: 2012-03-27 21:33 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-04-06 11:29 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Build Failure Screenshot (792.56 KB, image/png)
2012-04-03 02:22 PDT, Mark Capella [:capella]
no flags Details
Patch (v1) (36.85 KB, patch)
2012-04-03 02:24 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (19.24 KB, patch)
2012-04-03 04:42 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (18.15 KB, patch)
2012-04-03 05:28 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v4) (19.12 KB, patch)
2012-04-03 09:51 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v5) (19.16 KB, patch)
2012-04-03 12:20 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review-
Details | Diff | Splinter Review
Patch (v6) (18.41 KB, patch)
2012-04-03 20:25 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v7) (17.90 KB, patch)
2012-04-03 22:58 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-03-27 21:33:34 PDT
rename the class OuterDocAccessible and put in mozilla::a11y namespace.
move the files from accessible/src/base/nsOuterDocAccessible.h -> accessible/src/generic/OuterDocAccessible.h
and
base/nsOuterDocAccessible.cpp -> generic/OuterDocAccessible.cpp
adjust nsAccessibilityService.{cpp,h} accordingly.
also remove the no longer existing files from base/Makefile and add OuterDocAccessible.cpp to generic/Makefile
Comment 1 Mark Capella [:capella] 2012-04-02 14:28:10 PDT
Question: I can remove the nsOuterDocAccessible.cpp entry from accessible/src/base/makefile.in, but there doesn't seem to be a accessible/src/generic/Makefile.in file ...
Comment 2 Hubert Figuiere [:hub] 2012-04-02 14:50:54 PDT
That means you need to create a Makefile.in, based on the same model as the others.
Comment 3 Mark Capella [:capella] 2012-04-03 02:22:51 PDT
Created attachment 611749 [details]
Build Failure Screenshot
Comment 4 Mark Capella [:capella] 2012-04-03 02:24:11 PDT
Created attachment 611750 [details] [diff] [review]
Patch (v1)

Initial patch ... I get to the link step then it dies ... makefile.in problem I guess... attaching for review while I investigate.
Comment 5 alexander :surkov 2012-04-03 02:29:56 PDT
Comment on attachment 611750 [details] [diff] [review]
Patch (v1)

Review of attachment 611750 [details] [diff] [review]:
-----------------------------------------------------------------

don't remove files, but rename them

how does it happen that test_OuterDocAccessible.html is removed and added?

::: accessible/src/base/nsAccessibilityService.cpp
@@ +215,1 @@
>                               nsAccUtils::GetDocAccessibleFor(aPresShell));

wrong indentation

::: accessible/src/generic/Makefile.in
@@ +33,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

http://www.mozilla.org/MPL/headers/

::: accessible/src/generic/OuterDocAccessible.cpp
@@ +50,5 @@
> +////////////////////////////////////////////////////////////////////////////////
> +
> +OuterDocAccessible::
> +  OuterDocAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
> +  nsAccessibleWrap(aContent, aDoc)

linker errors because you should put them into a11y namespace, either a11y:: prefix them or put under namespace a11y which should be nicer
Comment 6 Mark Capella [:capella] 2012-04-03 02:41:01 PDT
Please confirm ... >don't< move the files from
accessible/src/base/nsOuterDocAccessible.h -> accessible/src/generic/OuterDocAccessible.h and
base/nsOuterDocAccessible.cpp -> generic/OuterDocAccessible.cpp
as original comment requests ... just re-name them in the same folder ?
Comment 7 alexander :surkov 2012-04-03 02:42:26 PDT
(In reply to Mark Capella [:capella] from comment #6)
> Please confirm ... >don't< move the files from
> accessible/src/base/nsOuterDocAccessible.h ->
> accessible/src/generic/OuterDocAccessible.h and
> base/nsOuterDocAccessible.cpp -> generic/OuterDocAccessible.cpp
> as original comment requests ... just re-name them in the same folder ?

no,

hg rename accessible/src/base/nsOuterDocAccesible.h accessible/src/generic/OuterDocAccessible.h
Comment 8 Mark Capella [:capella] 2012-04-03 04:42:41 PDT
Created attachment 611777 [details] [diff] [review]
Patch (v2)

Cleaner DIFF with nits addressed ... still experiencing the link problem ...
Comment 9 alexander :surkov 2012-04-03 04:46:15 PDT
Comment on attachment 611777 [details] [diff] [review]
Patch (v2)

Review of attachment 611777 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/makefile.in
@@ +32,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

please fix header as I pointed in previous comment

::: accessible/src/base/nsOuterDocAccessible.cpp
@@ +47,2 @@
>  using namespace mozilla::a11y;
>  

as I said wrap everything by namespace a11y to prevent linker errors like

namespace a11y {
Comment 10 Mark Capella [:capella] 2012-04-03 05:28:38 PDT
Created attachment 611782 [details] [diff] [review]
Patch (v3)

outerdocaccessible.h is wrapped in both mozilla and a11y, outerdocaccessible.cpp wont work with wrap just in a11y and will also not work wrapped in both mozilla and a11y ... i really think i set the new makefile up wrong or something ...

I find files outerdocaccessible.cpp.i and outerdocaccessible.h.i after the build attempt, but shouldn't there be a .obj somewhere?
Comment 11 alexander :surkov 2012-04-03 05:52:07 PDT
You have makefile.in, should be Makefile.in, also don't export OuterDocAccessible.h
Comment 12 Mark Capella [:capella] 2012-04-03 09:51:26 PDT
Created attachment 611855 [details] [diff] [review]
Patch (v4)
Comment 13 alexander :surkov 2012-04-03 10:28:45 PDT
Comment on attachment 611855 [details] [diff] [review]
Patch (v4)

Review of attachment 611855 [details] [diff] [review]:
-----------------------------------------------------------------

please uppercase 'm' in makefile.in file

thank you for figuring out the build issue

::: accessible/src/base/nsAccDocManager.cpp
@@ +54,5 @@
>  #include "nsIDOMWindow.h"
>  #include "nsIInterfaceRequestorUtils.h"
>  #include "nsIWebNavigation.h"
>  #include "nsServiceManagerUtils.h"
> +#include "OuterDocAccessible.h"

keep a11y headers grouped together, i.e move it before 'States.h'

::: accessible/src/base/nsOuterDocAccessible.cpp
@@ +79,1 @@
>                                     EWhichChildAtPoint aWhichChild)

fix indentation

@@ +95,5 @@
>    return child;
>  }
>  
>  nsresult
> +OuterDocAccessible::GetAttributesInternal(nsIPersistentProperties *aAttributes)

type* aName

@@ +181,5 @@
>    SetChildrenFlag(eChildrenUninitialized);
>  }
>  
>  bool
> +OuterDocAccessible::AppendChild(nsAccessible *aAccessible)

same

@@ +202,5 @@
>    return true;
>  }
>  
>  bool
> +OuterDocAccessible::RemoveChild(nsAccessible *aAccessible)

same

::: accessible/src/base/nsOuterDocAccessible.h
@@ +35,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +#ifndef _OuterDocAccessible_H_

_mozilla_a11y_OuterDocAccessible_h_
Comment 14 Mark Capella [:capella] 2012-04-03 12:20:30 PDT
Created attachment 611927 [details] [diff] [review]
Patch (v5)

This one was the learning experience I hoped it would be!  :-)
Comment 15 Trevor Saunders (:tbsaunde) 2012-04-03 16:55:45 PDT
Comment on attachment 611927 [details] [diff] [review]
Patch (v5)

>--- a/accessible/src/base/nsAccDocManager.cpp
>+++ b/accessible/src/base/nsAccDocManager.cpp
>@@ -36,18 +36,18 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsAccDocManager.h"
> 
> #include "nsAccessibilityService.h"
> #include "nsAccUtils.h"
> #include "nsApplicationAccessible.h"
>-#include "nsOuterDocAccessible.h"
> #include "nsRootAccessibleWrap.h"
>+#include "OuterDocAccessible.h"

I wonder if you can just not include it in the first place? I can find a decl from it being used imediately...

>+++ b/accessible/src/generic/makefile.in
>@@ -0,0 +1,43 @@
>+# -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+# vim: set ts=2 et sw=2 tw=80:

hm, c++ mode doesn't sound right, I wonder what other Makefiles use if anything?

>+LOCAL_INCLUDES = \
>+  -I$(srcdir) \

pretty sure that's never used.

>+  -I$(srcdir)/../generic \

shouldn't be needed since iirc the compiler looks in . by default.

>+  -I$(srcdir)/../xpcom \

does anything actually require this yet?

>+  -I$(srcdir)/../../../layout/generic \
>+  -I$(srcdir)/../../../layout/xul/base/src \
pretty sure these aren't used yet.

>+ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
>+LOCAL_INCLUDES += \
>+  -I$(srcdir)/../atk \
>+  $(NULL)
>+endif

why only this case? I suspect you don't need it...

>diff --git a/accessible/src/base/nsOuterDocAccessible.cpp b/accessible/src/generic/outerdocaccessible.cpp
>rename from accessible/src/base/nsOuterDocAccessible.cpp
>rename to accessible/src/generic/outerdocaccessible.cpp

OuterDocAccessible.cp please

> #include "nsAccUtils.h"


do you actually need this?

> #include "States.h"

or this one?

> 
>+using namespace mozilla;
> using namespace mozilla::a11y;
> 
>+namespace mozilla {
>+namespace a11y {



>+NS_IMPL_ISUPPORTS_INHERITED0(OuterDocAccessible,
>                              nsAccessible)

put it on one line?

>diff --git a/accessible/src/base/nsOuterDocAccessible.h b/accessible/src/generic/outerdocaccessible.h
>rename from accessible/src/base/nsOuterDocAccessible.h
>rename to accessible/src/generic/outerdocaccessible.h

OuterDocAccessible.h please

>+#ifndef _mozilla_a11y_OuterDocAccessible_h_
>+#define _mozilla_a11y_OuterDocAccessible_h_

actually, I think we should just use MOZILLA_A11Y_BLAH, since I sort of  think I remember the spec sets identifiers starting with _ aside for the compiler / system toolchain

>+  OuterDocAccessible(nsIContent* aContent, nsDocAccessible* aDoc);

add virtual destructor while your here.

>-     title="nsOuterDocAccessible chrome tests">
>+     title="OuterDocAccessible chrome tests">
>     Mozilla Bug 441519

leave bug name alone, so its easier to find if someone needs to find the bug  that changed the test.

btw this doesn't build on linux since the generic/ makefile is named makefile.in and the build system looks for Makefile.in I'll bet this is broken on every other platform that doesn't have crazy case insensitive rules too, but not sure about that.

btw I think you should add this new Makefie to toolkit/toolkit-allmakefiles.sh
Comment 16 Mark Capella [:capella] 2012-04-03 20:25:23 PDT
Created attachment 612086 [details] [diff] [review]
Patch (v6)

Well I was surprised at the list of further changes, especially some that Alex specifically asked for. 

The request to remove #include "nsAccUtils.h", and #include "States.h" failed and caused me to have to restore and rebuild. In hindsight, since that code was there originally, there wasn't a real need to re-engineer it.

The first couple lines of the license in the makefile aren't used in other makefiles, so I removed them. It now compares favorable to this production file:
http://mxr.mozilla.org/mozilla-central/source/dom/settings/Makefile.in#3 and others turned up by a quick scan.

FInally, with moving makefile to toolkit/toolkit-allmakefiles.sh ... I spent quite a bit of time getting this to work the way it was originally asked for. If there's no strong reason to change the requirements and tinker with it after the fact, I'd rather leave it as it is, or file a followup change bug.
Comment 17 alexander :surkov 2012-04-03 20:32:37 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #15)

> >+  -I$(srcdir)/../xpcom \
> 
> does anything actually require this yet?
> 
> >+  -I$(srcdir)/../../../layout/generic \
> >+  -I$(srcdir)/../../../layout/xul/base/src \
> pretty sure these aren't used yet.

I'm sort of polite on this because we're going to move things like nsAccessible or nsARIAGridAccessible, they make to get back these.


> >+ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
> >+LOCAL_INCLUDES += \
> >+  -I$(srcdir)/../atk \
> >+  $(NULL)
> >+endif
> 
> why only this case? I suspect you don't need it...
> 
> >diff --git a/accessible/src/base/nsOuterDocAccessible.cpp b/accessible/src/generic/outerdocaccessible.cpp
> >rename from accessible/src/base/nsOuterDocAccessible.cpp
> >rename to accessible/src/generic/outerdocaccessible.cpp
> 
> OuterDocAccessible.cp please

good catch, another reason why splinter diff is not always great.

> >+NS_IMPL_ISUPPORTS_INHERITED0(OuterDocAccessible,
> >                              nsAccessible)
> 
> put it on one line?

we use this style actually, so you can add/remove interfaces without lot of changes

> >+#ifndef _mozilla_a11y_OuterDocAccessible_h_
> >+#define _mozilla_a11y_OuterDocAccessible_h_
> 
> actually, I think we should just use MOZILLA_A11Y_BLAH, since I sort of 
> think I remember the spec sets identifiers starting with _ aside for the
> compiler / system toolchain

ok, I don't mind
Comment 18 alexander :surkov 2012-04-03 20:35:45 PDT
(In reply to Mark Capella [:capella] from comment #16)
> Created attachment 612086 [details] [diff] [review]
> Patch (v6)
> 
> Well I was surprised at the list of further changes, especially some that
> Alex specifically asked for. 

when you have two reviewers for the same code and when reviewers don't really read a scrollback then sometimes reviewers talk opposite things, in this case please make reviewers to talk each other to make them agree.
Comment 19 Mark Capella [:capella] 2012-04-03 22:58:34 PDT
Created attachment 612108 [details] [diff] [review]
Patch (v7)

Ok Alex, reviewers talking to each other is a good thing :)

With that, I put back the INCLUDES from the makefile as you asked, understanding that we're not using them now, but may in the future.

I put back the NS_IMPL_ISUPPORTS_INHERITED0 as a multiline, so that it's easier later to add/remove interfaces without a lot of changes.

I left in the MOZILLA_A11Y_BLAH changes as Trevor and you both asked.

Other nits requested since this was review+ in comment13 such as virtual destructor changes are now in place also.

This builds and tests ok locally and should be safe to move forward.
Comment 20 alexander :surkov 2012-04-03 23:12:34 PDT
Ok, cool, thank you.
Comment 21 alexander :surkov 2012-04-03 23:13:05 PDT
Comment on attachment 612108 [details] [diff] [review]
Patch (v7)

need r+ from Trevor before we go
Comment 22 Trevor Saunders (:tbsaunde) 2012-04-04 02:08:32 PDT
> The request to remove #include "nsAccUtils.h", and #include "States.h"
> failed and caused me to have to restore and rebuild. In hindsight, since
> that code was there originally, there wasn't a real need to re-engineer it.

ok :/ removing unneeded includes makes you rebuild less when the header changes, which is a good thing, so worth to try if it doesn't take too long.

> The first couple lines of the license in the makefile aren't used in other
> makefiles, so I removed them. It now compares favorable to this production
> file:
> http://mxr.mozilla.org/mozilla-central/source/dom/settings/Makefile.in#3 and
> others turned up by a quick scan.

ok

> FInally, with moving makefile to toolkit/toolkit-allmakefiles.sh ... I spent
> quite a bit of time getting this to work the way it was originally asked
> for. If there's no strong reason to change the requirements and tinker with
> it after the fact, I'd rather leave it as it is, or file a followup change
> bug.

Well, I'm told it speeds up Makefile generation, but probably not critically important.

(In reply to alexander :surkov from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > >+  -I$(srcdir)/../xpcom \
> > 
> > does anything actually require this yet?
> > 
> > >+  -I$(srcdir)/../../../layout/generic \
> > >+  -I$(srcdir)/../../../layout/xul/base/src \
> > pretty sure these aren't used yet.
> 
> I'm sort of polite on this because we're going to move things like
> nsAccessible or nsARIAGridAccessible, they make to get back these.

ok, I guess there's some logic to that on the other hand I don't want that to cause us to leave garbage around forever unused.

> > >+NS_IMPL_ISUPPORTS_INHERITED0(OuterDocAccessible,
> > >                              nsAccessible)
> > 
> > put it on one line?
> 
> we use this style actually, so you can add/remove interfaces without lot of
> changes

I'm not sure I think that makes a whole lot of sense but we do use it a bit so sure
Comment 23 Trevor Saunders (:tbsaunde) 2012-04-04 02:53:59 PDT
Comment on attachment 612108 [details] [diff] [review]
Patch (v7)

>+    new OuterDocAccessible(aContent, 

there's a space at the end of the line, but surkov should be able to fix that before pushing.

>+using namespace mozilla;
> using namespace mozilla::a11y;
> 
>+namespace mozilla {
>+namespace a11y {

nit, funny to have both using namespace mozilla::a1y; and namespace mozilla { namespace a11y {

since we usually don't put stuff in cpp files in namespace { } I pushed https://tbpl.mozilla.org/?tree=Try&rev=3014cf84ba6c 
to see if we could remove that and just use using, but not really a bi deal.
Comment 25 Matt Brubeck (:mbrubeck) 2012-04-06 11:29:04 PDT
https://hg.mozilla.org/mozilla-central/rev/179d4fadc70a

Note You need to log in before you can comment on or make changes to this bug.