979835 - Port BoxObject to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Jon Morton [:jmorton]]

Attachment: 0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8437386 [details] [diff] [review]
0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

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

This looks pretty good, but one part of the patch is screwed up.  Please do the file renaming (e.g. nsTreeBoxObject.h/cpp -> TreeBoxObject.h/cpp) as moves (git mv) rather than deleting the originals and creating new ones.  That'll preserve blame and make those files significantly easier to review.

::: toolkit/content/widgets/datetimepicker.xml
@@ +1258,4 @@
>        <property name="open" onget="return this.hasAttribute('open');">
>          <setter>
>            <![CDATA[
> +            if (this.boxObject.openMenu)

Why not just instanceof MenuBoxObject?

::: toolkit/content/widgets/popup.xml
@@ +107,5 @@
> +          else {
> +            try {
> +              this.boxObject.hidePopup();
> +            } catch(e) {}
> +          }

I would prefer you kept this closer to the original and did

else if (popupBox instanceof PopupBoxObject) {
  this.boxObject.hidePopup();
}

Comment 3

[Jon Morton [:jmorton]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> Comment on attachment 8437386 [details] [diff] [review]
> 0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch
> 
> Review of attachment 8437386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good, but one part of the patch is screwed up.  Please do
> the file renaming (e.g. nsTreeBoxObject.h/cpp -> TreeBoxObject.h/cpp) as
> moves (git mv) rather than deleting the originals and creating new ones. 
> That'll preserve blame and make those files significantly easier to review.
> 

I think git doesn't really actually track renames, but just detects a certain level of similarity between added/removed files. In this case, I've changed too much (part of the reason being that my editor fixes whitespace errors) for git to consider them the same. What'd you suggest? I can upload explicit diffs of the two files if that would help.

> ::: toolkit/content/widgets/datetimepicker.xml
> @@ +1258,4 @@
> >        <property name="open" onget="return this.hasAttribute('open');">
> >          <setter>
> >            <![CDATA[
> > +            if (this.boxObject.openMenu)
> 
> Why not just instanceof MenuBoxObject?
> 
> ::: toolkit/content/widgets/popup.xml
> @@ +107,5 @@
> > +          else {
> > +            try {
> > +              this.boxObject.hidePopup();
> > +            } catch(e) {}
> > +          }
> 
> I would prefer you kept this closer to the original and did
> 
> else if (popupBox instanceof PopupBoxObject) {
>   this.boxObject.hidePopup();
> }

I was trying to avoid exposing the PopupBoxObject interface object (minimizing how much the interface test complained), but if you think it should be exposed for this change I'll go ahead and change that.

Comment 4

[Jon Morton [:jmorton]]

(In reply to Jon Morton from comment #3)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> > Comment on attachment 8437386 [details] [diff] [review]
> > 0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch
> > 
> > Review of attachment 8437386 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks pretty good, but one part of the patch is screwed up.  Please do
> > the file renaming (e.g. nsTreeBoxObject.h/cpp -> TreeBoxObject.h/cpp) as
> > moves (git mv) rather than deleting the originals and creating new ones. 
> > That'll preserve blame and make those files significantly easier to review.
> > 
> 
> I think git doesn't really actually track renames, but just detects a
> certain level of similarity between added/removed files. In this case, I've
> changed too much (part of the reason being that my editor fixes whitespace
> errors) for git to consider them the same. What'd you suggest? I can upload
> explicit diffs of the two files if that would help.
> 

Nevermind! I figured out how to make git find the reneames.

Comment 5

[Jon Morton [:jmorton]]

Attachment: 0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

Address comments and fix last outstanding test failure.

SR on bz for WebIDL changes.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8446908 [details] [diff] [review]
0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

> What'd you suggest?

Generally, I'd suggest doing renames in one changeset and substantive changes in another.  And if you plan to mass-modify whitespace, whether it's because your editor is messing with it or for some other reason, you wantto do that in yet a separate changeset.  That would sure make all three changesets way easier to review, bisect against, etc, than a huge combined thing....

I'm not sure it's worth going back and splitting this up now that it's all been lumped together, but that's just because I'm not the reviewer for this patch. ;)

Substantive comments, but keep in mind that I mostly looked at the webidl and idl and glanced at the implementations; this is NOT a thorough review of the implementations:

BoxObject:

1) BoxObject::GetElement() should just return Element* so you can just do:

  return mContent && mContent->IsElement() ? mContent->AsElement() : nullptr;

That said, this is a behavior change: non-element non-null mContent (can that happen?) used to throw.  Purposeful change?

2) Using BeginReading() as you do in GetPropertyAsSupports/SetPropertyAsSupports()/SetProperty()/RemoveProperty() is not ok: nothing guarantees that propertyName is null-terminated.  You want PromiseFlatString(propertyName).get() and similar.

3) "+using namespace dom;" should presumably be "using namespace mozilla::dom"?

ListBoxObject:

4) In ListBoxObject::GetItemAtIndex, no need to init the nsCOMPtr to nullptr.

PopupBoxObject:

5)  The indent on the args of the ShowPopupWithAnchorAlign call in
    PopupBoxObject::ShowPopup is off.

6)  GetTriggerNode could just return nsINode*.

7)  GetAnchorNode could return Element*.

8)  MoveToAnchor didn't use to throw on null anchorElement arg, but now it
    will.  Why? 

ScrollBoxObject:

9)  You lost the scrollTo documentation.

10) The API change to getPosition looks like it'll break various addons.  Probably better to leave the old API for now, even if we add the new one, add deprecation warnings, etc.  Similar for getScrolledSize.

TreeBoxObject:

11) selectionRegion seems to have gone AWOL.

12) ensureCellIsVisible() didn't use to throw on null TreeColumn arg, but now will?

13) Likewise for scrollToCell, scrollToColumn, invalidateColumn, invalidateCell, invalidateColumnRange.

14) The getCellAt API change will also break addons.

15) So will the getCoordsForCellItem one.

16) isCellCropped used to allow a null second arg but will now throw.

The rest of the idl bits look ok but the API change issues really need addressing.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and please post interdiffs as you make changes!

Comment 8

[Jon Morton [:jmorton]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8446908 [details] [diff] [review]
> 0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch
> 
> > What'd you suggest?
> 
> Generally, I'd suggest doing renames in one changeset and substantive
> changes in another.  And if you plan to mass-modify whitespace, whether it's
> because your editor is messing with it or for some other reason, you wantto
> do that in yet a separate changeset.  That would sure make all three
> changesets way easier to review, bisect against, etc, than a huge combined
> thing....

That makes sense, I'll try to split things up in the future! As for the renames/whitespace,
I undid all my whitespace changes and lowered git's similarity threshold, so these patches
should at least be reviewable.

> BoxObject:
> 
> 1) BoxObject::GetElement() should just return Element* so you can just do:
> 
>   return mContent && mContent->IsElement() ? mContent->AsElement() : nullptr;
> 
> That said, this is a behavior change: non-element non-null mContent (can
> that happen?) used to throw.  Purposeful change?
> 

mContent is only ever initialized with an Element:

http://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsBoxObject%3A%3AInit%28class+nsIContent+*%29%22
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#6823

So I guess the one liner you suggested should work.

> The rest of the idl bits look ok but the API change issues really need
> addressing.

Right, I should have thought about addons. I've added backwards compatible versions of the changed methods.

Comment 9

[Jon Morton [:jmorton]]

Attachment: Bug-979835-Port-BoxObject-to-WebIDL.patch

Updated with review comments

Comment 10

[Jon Morton [:jmorton]]

Attachment: Bug-979835-WIP-Review-changes.patch

Interdiff of review changes

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8448404 [details] [diff] [review]
Bug-979835-Port-BoxObject-to-WebIDL.patch

>+   * DEPRECTATED: please use above version

"DEPRECATED".

Might be worth filing a followup to add actual deprecation messages.

>+  JS_DefineProperty(cx, x, "value", pt.x, 0);

  if (!JS_DefineProperty(cx, x, "value", pt.x, 0)) {
    aRv.Throw(NS_ERROR_XPC_CANT_SET_OUT_VAL);
    return;
  }

or so.  And similar in the other places where you do this.  And maybe just use JS_SetProperty instead of JS_DefineProperty, since that's what XPConnect dos right now?

>+  if (!dom::WrapObject(cx, col, &colVal)) {

Then you need to throw and return immediately.

>+  JS::Rooted<JSString*> childEltVal(cx,
>+    JS_NewUCStringCopyN(cx, childElt.get(), childElt.Length()));

Please use xpc::StringToJsval.  And again, if things fail you need to bail out.

sr=me with those fixed.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8448404 [details] [diff] [review]
Bug-979835-Port-BoxObject-to-WebIDL.patch

This diff appears to be a concatenation of two separate diffs, which makes it a real PITA to review.

Comment 13

[Jon Morton [:jmorton]]

Attachment: 0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

Address review comments and fix patch file.

Comment 14

[Jon Morton [:jmorton]]

Attachment: 0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

Welp, forgot to lower git's similarity threshold. Hopefully I've exhausted all the possible ways of screwing these patches up. Sorry.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8450679 [details] [diff] [review]
0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

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

Not bad, but I want to see it again.

We need to add shims to xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp for:

nsIBrowserBoxObject
nsIContainerBoxObject
nsIListBoxObject
nsIMenuBoxObject
nsIScrollBoxObject
nsITreeBoxObject

because they are all by addons.  That also means we need to undelete those IDL files which were deleted :(  We can leave the actual interfaces empty though, we just need Components.interfaces.nsIContainerBoxObject/etc to work.  We also need to expose QueryInterface on BoxObject, which can be done in Bindings.conf.

::: content/base/src/nsDocument.h
@@ +69,4 @@
>  #include "nsIDOMXPathEvaluator.h"
>  #include "jsfriendapi.h"
>  #include "ImportManager.h"
> +#include "mozilla/dom/BoxObject.h"

Why do we need to include the header in nsDocument.h?

@@ +99,4 @@
>  namespace mozilla {
>  class EventChainPreVisitor;
>  namespace dom {
> +class BoxObject;

If it's really necessary to include the header, forward declaring the type isn't needed.

@@ +997,4 @@
>    virtual void ForgetLink(mozilla::dom::Link* aLink);
>  
>    void ClearBoxObjectFor(nsIContent* aContent);
> +  already_AddRefed<mozilla::dom::BoxObject> GetBoxObjectFor(mozilla::dom::Element* aElement,

returnType
FunctionName(args)

Mark this virtual while you're here too.

::: dom/events/test/test_bug602962.xul
@@ +47,4 @@
>  }
>  
>  function resize() {
> +  scrollX = sbo.positionX, scrollY = sbo.positionY;

put it on 2 lines

::: layout/xul/BoxObject.cpp
@@ +3,4 @@
>   * 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/. */
>  
> +#include "BoxObject.h"

mozilla/dom/BoxObject.h

@@ +335,1 @@
>      return NS_ERROR_FAILURE;

Since you're here, braces for the if.

@@ +437,1 @@
>                                  nsIDOMElement** aResult)

line these up

@@ +537,5 @@
> +BoxObject::GetProperty(const nsAString& propertyName, nsString& aRetVal, ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsISupports> data(GetPropertyAsSupports(propertyName));
> +  if (!data)
> +    return;

single line ifs are always braced

@@ +616,4 @@
>  nsresult
>  NS_NewBoxObject(nsIBoxObject** aResult)
>  {
> +  *aResult = new BoxObject;

BoxObject() please.

@@ +620,2 @@
>    if (!*aResult)
>      return NS_ERROR_OUT_OF_MEMORY;

Go ahead and remove this if while you are here.  new Foo is infallible in Gecko; it cannot return null.

::: layout/xul/BoxObject.h
@@ +2,5 @@
>  /* 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/. */
> +#ifndef MOZILLA_BOXOBJECT_H_
> +#define MOZILLA_BOXOBJECT_H_

mozilla_dom_BoxObject_h__

@@ +26,5 @@
> +namespace dom {
> +
> +class Element;
> +
> +class BoxObject : public nsPIBoxObject, public nsWrapperCache

\n after ,

::: layout/xul/ContainerBoxObject.cpp
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsCOMPtr.h"
> +#include "mozilla/dom/ContainerBoxObject.h"
> +#include "BoxObject.h"

This is included by ContainerBoxObject.h.

@@ +18,5 @@
>  
> +NS_IMPL_ADDREF_INHERITED(ContainerBoxObject, BoxObject)
> +NS_IMPL_RELEASE_INHERITED(ContainerBoxObject, BoxObject)
> +NS_INTERFACE_MAP_BEGIN(ContainerBoxObject)
> +NS_INTERFACE_MAP_END_INHERITING(BoxObject)

You don't need to do this if your subclass doesn't add any interfaces.  You can just skip the NS_DECL_ISUPPORTS_INHERITED and all these macros and use the base class's stuff.

@@ +25,3 @@
>  {
> +  SetIsDOMBinding();
> +}

This happens in the subclass ctor, so no need to do it here.

@@ +75,5 @@
>    }
>  
> +  nsIDocShell* result = sub_doc->GetDocShell();
> +  NS_IF_ADDREF(result);
> +  return result;

You could just write:

nsCOMPtr<nsIDocShell> result = sub_doc->GetDocShell();
return result.forget();

@@ +89,2 @@
>    if (!*aResult)
>      return NS_ERROR_OUT_OF_MEMORY;

And this if

::: layout/xul/ContainerBoxObject.h
@@ +10,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "mozilla/dom/BoxObject.h"

I think BoxObject.h includes most of these other headers.  Pare the includes down to the minimum, please.

@@ +14,5 @@
> +#include "mozilla/dom/BoxObject.h"
> +
> +struct JSContext;
> +class nsIDocShell;
> +class nsIContent;

Same with these.  Most or all of these types are used in BoxObject.h so you shouldn't need to redeclare them.

@@ +22,5 @@
> +
> +class ContainerBoxObject MOZ_FINAL : public BoxObject
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED

See the comments in the .cpp.

@@ +26,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +public:
> +  ContainerBoxObject();
> +  ~ContainerBoxObject();

private dtors for MOZ_FINAL classes that are refcounted.  Make this 'virtual' explicitly too.

@@ +28,5 @@
> +public:
> +  ContainerBoxObject();
> +  ~ContainerBoxObject();
> +
> +  nsIContent* GetParentObject() const;

You shouldn't need to declare your own GetParentObject, I think.  The base class one should suffice.

::: layout/xul/ListBoxObject.cpp
@@ +25,3 @@
>    : mListBoxBody(nullptr)
>  {
> +  SetIsDOMBinding();

Same here.

@@ +54,4 @@
>  {
>    nsListBoxBodyFrame* body = GetListBoxBody(true);
>    if (body)
> +    return body->GetItemAtIndex(index, _retval);

Add braces to these ifs while you are here.

@@ +66,3 @@
>    nsListBoxBodyFrame* body = GetListBoxBody(true);
>    if (body)
> +    return body->GetIndexOfItem(aElement, aResult);

and here

@@ +76,4 @@
>  {
>    nsListBoxBodyFrame* body = GetListBoxBody(true);
>    if (body)
> +    return body->GetRowCount();

and here

@@ +85,4 @@
>  {
>    nsListBoxBodyFrame* body = GetListBoxBody(true);
>    if (body)
> +    return body->GetNumberOfVisibleRows();

and here

@@ +94,4 @@
>  {
>    nsListBoxBodyFrame* body = GetListBoxBody(true);
>    if (body)
> +    return body->GetIndexOfFirstVisibleRow();

...

@@ +103,4 @@
>  {
>    nsListBoxBodyFrame* body = GetListBoxBody(true);
>    if (body)
> +    body->EnsureIndexIsVisible(aRowIndex);

...

@@ +111,4 @@
>  {
> +  nsListBoxBodyFrame* body = GetListBoxBody(true);
> +  if (body)
> +    body->ScrollToIndex(aRowIndex);

...

@@ +120,3 @@
>    nsListBoxBodyFrame* body = GetListBoxBody(true);
>    if (body)
> +    body->ScrollByLines(aNumLines);

...

@@ +226,5 @@
>  
>  nsresult
>  NS_NewListBoxObject(nsIBoxObject** aResult)
>  {
> +  *aResult = new ListBoxObject;

()

@@ +231,2 @@
>    if (!*aResult)
>      return NS_ERROR_OUT_OF_MEMORY;

And remove this if too.

::: layout/xul/ListBoxObject.h
@@ +10,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "mozilla/dom/BoxObject.h"

Same here about the headers.

@@ +18,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class Element;

And the forward decls.

@@ +28,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_NSILISTBOXOBJECT
> +
> +  ListBoxObject();
> +  ~ListBoxObject();

private dtor

@@ +30,5 @@
> +
> +  ListBoxObject();
> +  ~ListBoxObject();
> +
> +  nsIContent * GetParentObject() const;

and unnecessary

::: layout/xul/MenuBoxObject.cpp
@@ +19,5 @@
> +
> +NS_IMPL_ADDREF_INHERITED(MenuBoxObject, BoxObject)
> +NS_IMPL_RELEASE_INHERITED(MenuBoxObject, BoxObject)
> +NS_INTERFACE_MAP_BEGIN(MenuBoxObject)
> +NS_INTERFACE_MAP_END_INHERITING(BoxObject)

You didn't add any interfaces, so there's no need to redeclare and redefine nsISupports.

@@ +24,4 @@
>  
> +MenuBoxObject::MenuBoxObject()
> +{
> +  SetIsDOMBinding();

The base class handles this.

@@ +90,3 @@
>    nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
>    if (!pm)
> +    return false;

add braces throughout this function.

@@ +133,5 @@
>    nsIFrame* frame = menuframe->GetParent();
>    while (frame) {
>      nsMenuBarFrame* menubar = do_QueryFrame(frame);
>      if (menubar) {
> +      return  menubar->IsActiveByKeyboard();

fix the two spaces between return and menubar

@@ +155,2 @@
>    if (!*aResult)
>      return NS_ERROR_OUT_OF_MEMORY;

Same comments here as before.

::: layout/xul/MenuBoxObject.h
@@ +10,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "mozilla/dom/BoxObject.h"

Same about these headers

@@ +18,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class Element;
> +class KeyboardEvent;

And the forward decls

@@ +26,5 @@
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  MenuBoxObject();
> +  ~MenuBoxObject();

and the dtor

@@ +28,5 @@
> +
> +  MenuBoxObject();
> +  ~MenuBoxObject();
> +
> +  nsIContent* GetParentObject() const;

and GetParentObject

::: layout/xul/PopupBoxObject.cpp
@@ +3,1 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this

Everything from the previous files applies here too.

@@ +162,2 @@
>    }
> +  return true;

\n before return

::: layout/xul/PopupBoxObject.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

and here

::: layout/xul/ScrollBoxObject.cpp
@@ +4,1 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

and here

@@ +249,2 @@
>  
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(&child);

This shouldn't be necessary.  nsIContent is a superclass of Element.

::: layout/xul/ScrollBoxObject.h
@@ +1,1 @@
> +

this file needs a license header, and all the same stuff as before.

::: layout/xul/nsMenuPopupFrame.cpp
@@ +1499,5 @@
>  bool nsMenuPopupFrame::ConsumeOutsideClicks()
>  {
>    // If the popup has explicitly set a consume mode, honor that.
> +  if (mConsumeRollupEvent != PopupBoxObject::ROLLUP_DEFAULT)
> +    return (mConsumeRollupEvent == PopupBoxObject::ROLLUP_CONSUME);

Since you're here, add braces.

::: layout/xul/tree/nsTreeSelection.cpp
@@ -265,4 @@
>  
>  DOMCI_DATA(TreeSelection, nsTreeSelection)
>  
> -// QueryInterface implementation for nsBoxObject

This comment is just wrong ... this implements QI for nsTreeSelection.  But go ahead and remove it because that's obvious.

::: security/manager/pki/resources/content/viewCertDetails.js
@@ +146,4 @@
>    var certDumpTree = Components.classes[nsASN1Tree].
>                            createInstance(nsIASN1Tree);
>    certDumpTree.loadASN1Structure(cert.ASN1Structure);
> +  document.getElementById('prettyDumpTree').view =  certDumpTree;

since you're here, one ' ' after =.

Comment 16

[Jon Morton [:jmorton]]

Attachment: 0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

review changes

Comment 17

[Jon Morton [:jmorton]]

Attachment: interdiff.patch

Interdiff of changes from kyle's comments.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8452767 [details] [diff] [review]
interdiff.patch

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

::: layout/xul/nsIBrowserBoxObject.idl
@@ +8,5 @@
> +/**
> + * @deprecated Please consider using ContainerBoxObject.
> + */
> +
> +[scriptable, uuid(db436f2f-c656-4754-b0fa-99bc353bd63f)]

I'm pretty sure this doesn't need to be scriptable.

::: xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp
@@ +351,4 @@
>    DEFINE_SHIM(AnimationEvent),
>    DEFINE_SHIM(Attr),
>    DEFINE_SHIM(BeforeUnloadEvent),
> +  DEFINE_SHIM_WITH_CUSTOM_INTERFACE(nsIBrowserBoxObject, ContainerBoxObject),  

extra whitespace at EOL.

@@ +468,4 @@
>    DEFINE_SHIM(Rect),
>    DEFINE_SHIM(Screen),
>    DEFINE_SHIM(ScrollAreaEvent),
> +  DEFINE_SHIM_WITH_CUSTOM_INTERFACE(nsIScrollBoxObject, ScrollBoxObject),  

here too

@@ +479,4 @@
>    DEFINE_SHIM(TimeEvent),
>    DEFINE_SHIM(TimeRanges),
>    DEFINE_SHIM(TransitionEvent),
> +  DEFINE_SHIM_WITH_CUSTOM_INTERFACE(nsITreeBoxObject, TreeBoxObject),  

here too.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8452765 [details] [diff] [review]
0001-Bug-979835-Port-BoxObject-and-its-subclasses-to-WebI.patch

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

r=me with those comments

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Stub out part of quickstubs

This is dead now, but we still need some other stuff in this file for another couple days.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Rebased.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8504449 [details] [diff] [review]
Stub out part of quickstubs

r=me

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/944cba921a29
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3304b7e0e0

Comment 24

[Gary [:streetwolf52]]

This patch breaks the functioning of my bookmarks and history sidebar.  Only two bookmarks that I have that I placed above my first folder work.  All the rest do nothing when I click on them.

Regression:

20141014130253
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3099024c40a  - GOOD

20141014131634
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3304b7e0e0  - BAD

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

File a bug please.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/944cba921a29
https://hg.mozilla.org/mozilla-central/rev/8d3304b7e0e0

Comment 27

[Axel Grude [:realRaven]]

Can somebody advise how to fix code that uses boxObject with QueryInterface for Add-on authors to remain compatible with the patch? Is it sufficient to remove QueryInterface?

 // if scrolling happens too soon (before filtering is done) 
 // it can scroll to invisible portion of treeview contents.
 let idx = lastLogin.idx;
 let tree = win.signonsTree;
 win.setTimeout( function() {
   let boxobject = tree.boxObject.QueryInterface(Components.interfaces.nsITreeBoxObject);
   boxobject.ensureRowIsVisible(idx); }, 100);

Comment 28

[Hartmut Arlt]

+1 for the last comment.

Our add-on needs to be compatible with FF 31 ESR up to latest release.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Axel, boxObject.QueryInterface should still work fine in system-principal code...  Certainly BoxObject.prototype.QueryInterface exists in chrome.

What, exactly, fails in your code?

Comment 30

[Axel Grude [:realRaven]]

(In reply to Boris Zbarsky [:bz] from comment #29)
> Axel, boxObject.QueryInterface should still work fine in system-principal
> code...  Certainly BoxObject.prototype.QueryInterface exists in chrome.
> 
> What, exactly, fails in your code?

It is flagged by AMO's validation process. I guess I can take the whole QueryInterface code out as it was on a dead line anyway. I was just curious if any change was necessary and apparently it is not (as long as the addon had chrome privilege).

Comment 31

[Boris Zbarsky [:bzbarsky]]

> It is flagged by AMO's validation process.

That seems wrong in this case.

Specifically, QI on objects that had all their interfaces in classinfo was and is a no-op, but BoxObject did NOT list nsITreeBoxObject in classinfo.  So on Firefox versions earlier than Firefox 36 that QI is necessary.

Once 31 ESR is out of support, we can think about flagging the QI because then it really will be a no-op in all supported versions.

Comment 32

[Jorge Villalobos [:jorgev] (he/him)]

Sorry, I thought those interfaces had been removed and that QI'ing to them would throw an error. We ran the automatic compatibility validation yesterday and that was one of the error cases.

Comment 33

[Axel Grude [:realRaven]]

(In reply to Jorge Villalobos [:jorgev] from comment #32)
> Sorry, I thought those interfaces had been removed and that QI'ing to them
> would throw an error. We ran the automatic compatibility validation
> yesterday and that was one of the error cases.

There is definitely a use in flagging this on AMO, I saw it throw an error in (and break) the Stationery Add-on on Thunderbird 36.0beta

Comment 34

[Boris Zbarsky [:bzbarsky]]

Hmm.   nsITreeBoxObject has not been removed.  QIing to it should work.  If someone has steps to reproduce it not working (ideally with a Firefox extension, though I can deal with a Thunderbird one if that's really needed), please file a bug and assign it to me to investigate ASAP.  Unfortunately, changing things for 36 is pretty much impossible at this late point.  :(

Comment 35

[Patrick Storz]

QI'ing for "nsIScrollBoxObject" with
  boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
definitely throws an "NS_NOINTERFACE" error on Firefox 36.

It happens for me with SearchWP, see [1] for the relevant code line.
To reproduce you could use the latest stable version from [2] (enter a long search term that does not fit into the search box; clicking the ellipsis button then triggers the error).

[1] https://code.google.com/p/searchwp/source/browse/trunk/src/chrome/content/searchbox.xml#210
[2] https://addons.mozilla.org/addon/searchwp/

Comment 36

[Boris Zbarsky [:bzbarsky]]

Yes, that part is definitely not going to work.  Someone screwed up and removed the QI implementation from ScrollBoxObject, and at this point that's going to ship in 36...  The best way to modify an extension to handle this is to put the QI call inside a try/catch.  :(

But nsITreeBoxObject doesn't seem to have that problem.  The QI impl for it is correct, and when I just tried this:

  var s = document.createElement("tree"); 
  document.documentElement.appendChild(s);
  alert(document.getBoxObjectFor(s).QueryInterface(Components.interfaces.nsITreeBoxObject))

in a browser window it works fine... at least in a nightly and on beta 36.

Comment 37

[Kevin]

I have the same error as comment 35 and comment 36. What is the current status of this bug? We are using FireFox 38 and the bug is still present...

Comment 38

[Boris Zbarsky [:bzbarsky]]

> What is the current status of this bug?

This bug is fixed.  If there is a problem with ScrollBoxObject QI and you can't just stop trying to QI to it (why not?) then please file a new bug blocking this one on that issue and cc me.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e0e3a50fed7c
Port BoxObject and its subclasses to WebIDL. r=khuey sr=bz