Last Comment Bug 832158 - remove IServiceProvider from nsAccessNodeWrap and make it a tear off
: remove IServiceProvider from nsAccessNodeWrap and make it a tear off
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Trevor Saunders (:tbsaunde)
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 814569
  Show dependency treegraph
 
Reported: 2013-01-17 21:18 PST by Trevor Saunders (:tbsaunde)
Modified: 2016-07-04 12:34 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implement IServiceProvider with a taer off (17.23 KB, patch)
2013-01-17 21:20 PST, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2013-01-17 21:18:40 PST
presumably it isn't needed most of the time and when its used its for a short time, so it doesn't really make sense for lots of objects to implement it.
Comment 1 Trevor Saunders (:tbsaunde) 2013-01-17 21:20:40 PST
Created attachment 703754 [details] [diff] [review]
implement IServiceProvider with a taer off
Comment 2 alexander :surkov 2013-01-23 15:52:13 PST
Comment on attachment 703754 [details] [diff] [review]
implement IServiceProvider with a taer off

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

::: accessible/src/msaa/ServiceProvider.cpp
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + *  * License, v. 2.0. If a copy of the MPL was not distributed with this
> + *   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: please check if those whitespaces between * and * in last two lines are expected

@@ +24,5 @@
> +IMPL_IUNKNOWN_QUERY_HEAD(ServiceProvider)
> +  IMPL_IUNKNOWN_QUERY_IFACE(IServiceProvider)
> +  return mAccessible->QueryInterface(aIID, aInstancePtr);
> +A11Y_TRYBLOCK_END
> +  }

not nice, but ok, you could do
if (mAccessible)
  return mAccessible->QI;
IMPL_IUNKNOWN_QUERY_TAIL

but mAccessible shouldn't be null in practice

@@ +31,5 @@
> +// IServiceProvider
> +
> +STDMETHODIMP
> +ServiceProvider::QueryService(REFGUID aGuidService, REFIID aIID,
> +                             void** aInstancePtr)

nit: pls indent this line properly

@@ +60,5 @@
> +  if (aGuidService == SID_IAccessibleContentDocument) {
> +    if (aIID != IID_IAccessible)
> +      return E_NOINTERFACE;
> +
> +    nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem = 

nit: whitespace in the end of line

@@ +114,5 @@
> +   *   const GUID unused;
> +   *   pServProv->QueryService(unused, IID_ISimpleDOMDocument, (void**)&pAccDoc);
> +   *   pServProv->Release();
> +   * }
> +   */

nit: please use single line comment style but basically this comment is obsolete

::: accessible/src/msaa/ServiceProvider.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + *  * License, v. 2.0. If a copy of the MPL was not distributed with this
> + *   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

same here, it go with http://www.mozilla.org/MPL/headers/
Comment 3 Trevor Saunders (:tbsaunde) 2013-01-24 18:10:15 PST
> @@ +114,5 @@
> > +   *   const GUID unused;
> > +   *   pServProv->QueryService(unused, IID_ISimpleDOMDocument, (void**)&pAccDoc);
> > +   *   pServProv->Release();
> > +   * }
> > +   */
> 
> nit: please use single line comment style but basically this comment is
> obsolete

mind if I just nuke it then?
Comment 4 alexander :surkov 2013-01-27 00:23:46 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> > nit: please use single line comment style but basically this comment is
> > obsolete
> 
> mind if I just nuke it then?

it seems the best option :)
Comment 5 Trevor Saunders (:tbsaunde) 2013-02-20 13:54:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb29fef8cd2e
Comment 6 Ryan VanderMeulen [:RyanVM] 2013-02-21 06:52:16 PST
https://hg.mozilla.org/mozilla-central/rev/bb29fef8cd2e

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