alexander :surkov (:asurkov) Assignee
	Description • 10 years ago

      No description provided.

	alexander :surkov (:asurkov) Assignee
	Comment 1 • 10 years ago

fixed bunch of stuffs

roc review for layout and widget code

	Trevor Saunders (:tbsaunde)
	Comment 2 • 10 years ago

Comment on attachment 8498894 [details] [diff] [review]
patch

Please break this up some, 300k is seriouslly big, and looking at the first file or two I see at least a hunk or two that could be there own precurser patches.

	alexander :surkov (:asurkov) Assignee
	Comment 3 • 10 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 8498894 [details] [diff] [review]
> patch
> 
> Please break this up some, 300k is seriouslly big, and looking at the first
> file or two I see at least a hunk or two that could be there own precurser
> patches.

that be nearly equivalent to write new patch and possibly even harder because I need to make sure all pieces are independent and built successfully, tests pass. I see that I could spend some time on separate path to replace XPCOM calls into internal methods that we missed in previous cycles but that'd a relatively small patch and you won't likely benefit reviewing the primary one. So can I convince you to review the existing patch? I realize it may take longer but after all you need to make sure that these all same-kind changes are valid.

	Trevor Saunders (:tbsaunde)
	Comment 4 • 10 years ago

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Comment on attachment 8498894 [details] [diff] [review]
> > patch
> > 
> > Please break this up some, 300k is seriouslly big, and looking at the first
> > file or two I see at least a hunk or two that could be there own precurser
> > patches.
> 
> that be nearly equivalent to write new patch and possibly even harder

shouldn't be, some chunks are pretty clearly independant of everything else so you can just put them in there own patch.

> because I need to make sure all pieces are independent and built
> successfully, tests pass. I see that I could spend some time on separate
> path to replace XPCOM calls into internal methods that we missed in previous
> cycles but that'd a relatively small patch and you won't likely benefit
> reviewing the primary one. So can I convince you to review the existing
> patch? I realize it may take longer but after all you need to make sure that
> these all same-kind changes are valid.

no, its just too big

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 5 • 10 years ago

Big patches are a royal pain. I can take this review.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 6 • 10 years ago

Hey Alex to help reduce reviewer pain can you please describe what this patch is for, and pros cons, and whether it is part of a larger plan and what that plan is etc?

	alexander :surkov (:asurkov) Assignee
	Comment 7 • 10 years ago

(In reply to David Bolter [:davidb] from comment #6)
> Hey Alex to help reduce reviewer pain can you please describe what this
> patch is for, and pros cons, and whether it is part of a larger plan and
> what that plan is etc?

that's complete segregation XPCOM tree from internal tree. So that's just big reorg to achieve the following hierarchy:

class Accessible; // internal class, implements all logic

class xpcAccessible : public nsIAccessible // XPCOM wrapper around internal class
{
  Accessible* mIntl;
}

That's final destination, XPCOM lives separately from internal stuff so for example if you run platform screen reader then we don't spend extra memory to allocate XPCOM stuff nobody needs. There are couple things left but I'll finish them separately.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 8 • 10 years ago

Comment on attachment 8498894 [details] [diff] [review]
patch

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

There is some good random cleanup here and this patch is way too big. I don't think I can sanely review a patch this size with confidence and my eyes glazed a bit at times. With that r=me (confidence 75%) with nits/comments. I'd review an interdiff if asked ;)

::: accessible/atk/AccessibleWrap.cpp
@@ +319,5 @@
> +    atk_object_initialize(mAtkObject, this);
> +    mAtkObject->role = ATK_ROLE_INVALID;
> +    mAtkObject->layer = ATK_LAYER_INVALID;
> +  }
> +

In general I'd definitely prefer formatting changes to be a separate changeset.

@@ +1015,5 @@
>          break;
>        }
> +
> +  case nsIAccessibleEvent::EVENT_VALUE_CHANGE:
> +    if (accessible->HasNumericValue()) {

nit: formatting.

::: accessible/atk/nsStateMap.h
@@ +27,5 @@
> +                         No ATK equivalent. The accessible state is not
> +                         currently supported.
> +  STATE_PINNED:          The object is pinned, usually indicating it is fixed in
> +                         place and has permanence. No ATK equivalent. The
> +                        accessible state is not currently supported.

nit: format.

::: accessible/base/ARIAMap.cpp
@@ -31,5 @@
>   *  Definition of nsRoleMapEntry contains comments explaining this table.
>   *
> - *  When no nsIAccessibleRole enum mapping exists for an ARIA role, the
> - *  role will be exposed via the object attribute "xml-roles".
> - *  In addition, in MSAA, the unmapped role will also be exposed as a BSTR string role.

Any particular reason you removed this?

::: accessible/generic/Accessible.h
@@ +557,5 @@
>     */
>    void ScrollToPoint(uint32_t aCoordinateType, int32_t aX, int32_t aY);
>  
> +  /**
> +   * Get a pointer to accessibility interface for this node, which is specific 

nit: unecessary whitespace at end of comment.

::: accessible/generic/BaseAccessibles.h
@@ +117,5 @@
>   */
>  class DummyAccessible : public AccessibleWrap
>  {
>  public:
> +  DummyAccessible(DocAccessible* aDocument = nullptr) :

what is this?

::: accessible/generic/DocAccessible.cpp
@@ +658,5 @@
>                                bool aIsFromUserInput)
>  {
> +  nsRefPtr<AccEvent> event =
> +    new AccVCChangeEvent(
> +      this, (aOldAccessible ? aOldAccessible->ToInternalAccessible() : nullptr),

ToInternalAccessible ...

::: accessible/mac/mozAccessible.mm
@@ +400,1 @@
>  

While we're here and you remind me why don't we check for scaleFactor == 0?

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +613,5 @@
>    }
>    STDMETHODIMP Clone(IEnumVARIANT FAR* FAR* ppenum);
>  
>  private:
> +  nsTArray<Accessible*> mArray;

nsTArray...

@@ -657,4 @@
>    for (uint32_t i = 0; i < celt; ++i, ++mCurIndex) {
> -    // Copy the elements of the array into rgvar
> -    nsCOMPtr<nsIAccessible> accel(do_QueryElementAt(mArray, mCurIndex));
> -    NS_ASSERTION(accel, "Invalid pointer in mArray");

What is the history of this assertion?

::: accessible/xpcom/xpcAccessible.cpp
@@ +262,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  nsAutoString help;
> +  Intl()->Help(help);
> +  aHelp.Assign(help);

This GetHelp stuff seems like it should be a separate bug and patch.

::: accessible/xpcom/xpcAccessible.h
@@ +17,5 @@
> +class Accessible;
> +
> +/**
> + * XPCOM nsIAccessible interface implementation, used by xpcAccessilbeGeneric
> + * class.

nit: "xpcAccessibleGeneric"

::: accessible/xpcom/xpcAccessibleDocument.cpp
@@ +26,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(xpcAccessibleDocument,
> +                                                xpcAccessibleGeneric)
> +  tmp->mCache.Clear();
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END

I'd prefer someone else double check the cycle collection macros.

@@ +198,5 @@
> +void
> +xpcAccessibleDocument::Shutdown()
> +{
> +  mCache.Clear();
> +  xpcAccessibleGeneric::Shutdown();

I'd probably call shutdown first.

::: accessible/xpcom/xpcAccessibleDocument.h
@@ +61,5 @@
> +  {
> +    xpcAccessibleGeneric* xpcAcc = mCache.GetWeak(aAccessible);
> +    if (xpcAcc)
> +      xpcAcc->Shutdown();
> +    mCache.Remove(aAccessible);

If no {}'s I will beg for a spacer line.

@@ +95,5 @@
> +    return nullptr;
> +
> +  xpcAccessibleDocument* xpcDoc =
> +    GetAccService()->GetXPCDocument(aAccessible->Document());
> +  return static_cast<xpcAccessibleHyperText*>(xpcDoc->GetAccessible(aAccessible));

If aAccessible->Document() can fail (can mDoc be null?) then we need to nullcheck xpcDoc before calling anything on it.

::: accessible/xpcom/xpcAccessibleGeneric.h
@@ +42,5 @@
> +
> +  // nsIAccessible
> +  virtual Accessible* ToInternalAccessible() const MOZ_FINAL;
> +
> +  // xpcAccessilbeGeneric

nit: *Accessible*

	alexander :surkov (:asurkov) Assignee
	Comment 9 • 10 years ago

(In reply to David Bolter [:davidb] from comment #8)

> > +  case nsIAccessibleEvent::EVENT_VALUE_CHANGE:
> > +    if (accessible->HasNumericValue()) {
> 
> nit: formatting.

sorry what's wrong with it?

> > - *  When no nsIAccessibleRole enum mapping exists for an ARIA role, the
> > - *  role will be exposed via the object attribute "xml-roles".
> > - *  In addition, in MSAA, the unmapped role will also be exposed as a BSTR string role.
> 
> Any particular reason you removed this?

it's sort of senseless comment, crossplatform shouldn't say much about platfrom stuffs, that xml-roles comment is not complete and should be part of documentation. I can get it back in some form if you like.

> ::: accessible/generic/BaseAccessibles.h
> @@ +117,5 @@
> >   */
> >  class DummyAccessible : public AccessibleWrap
> >  {
> >  public:
> > +  DummyAccessible(DocAccessible* aDocument = nullptr) :
> 
> what is this?

if it doesn't have a document then it fails to have XPCOM wrapper object, just some tweaks to keep XPCOM behavior unchanged

> ::: accessible/mac/mozAccessible.mm
> @@ +400,1 @@
> >  
> 
> While we're here and you remind me why don't we check for scaleFactor == 0?

honestly I don't know, but no crashes should mean it's true :)

> @@ -657,4 @@
> >    for (uint32_t i = 0; i < celt; ++i, ++mCurIndex) {
> > -    // Copy the elements of the array into rgvar
> > -    nsCOMPtr<nsIAccessible> accel(do_QueryElementAt(mArray, mCurIndex));
> > -    NS_ASSERTION(accel, "Invalid pointer in mArray");
> 
> What is the history of this assertion?

I think that is just in case assertion

> @@ +95,5 @@
> > +    return nullptr;
> > +
> > +  xpcAccessibleDocument* xpcDoc =
> > +    GetAccService()->GetXPCDocument(aAccessible->Document());
> > +  return static_cast<xpcAccessibleHyperText*>(xpcDoc->GetAccessible(aAccessible));
> 
> If aAccessible->Document() can fail (can mDoc be null?)

not really, until accessible is defunct but we should be protected against this

> then we need to
> nullcheck xpcDoc before calling anything on it.

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/ef5d07a500fd
https://hg.mozilla.org/mozilla-central/rev/5a296abf61a9