Closed
Bug 979835
Opened 11 years ago
Closed 10 years ago
Port BoxObject to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: baku, Assigned: jmorton, NeedInfo)
References
(Depends on 1 open bug)
Details
(Keywords: addon-compat)
Attachments
(2 files, 8 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee: amarchesini → jmorton
Keywords: addon-compat
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(); }
Attachment #8437386 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Jon Morton from comment #3) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Assignee | ||
Comment 5•10 years ago
|
||
Address comments and fix last outstanding test failure. SR on bz for WebIDL changes.
Attachment #8437386 -
Attachment is obsolete: true
Attachment #8446908 -
Flags: superreview?(bzbarsky)
Attachment #8446908 -
Flags: review?(khuey)
![]() |
||
Comment 6•10 years ago
|
||
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.
Attachment #8446908 -
Flags: superreview?(bzbarsky) → superreview-
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
Updated with review comments
Attachment #8446908 -
Attachment is obsolete: true
Attachment #8446908 -
Flags: review?(khuey)
Attachment #8448404 -
Flags: superreview?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8448404 -
Flags: review?(khuey)
![]() |
||
Comment 11•10 years ago
|
||
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.
Attachment #8448404 -
Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #8448404 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 13•10 years ago
|
||
Address review comments and fix patch file.
Attachment #8448404 -
Attachment is obsolete: true
Attachment #8448406 -
Attachment is obsolete: true
Attachment #8450673 -
Flags: review?(khuey)
Assignee | ||
Comment 14•10 years ago
|
||
Welp, forgot to lower git's similarity threshold. Hopefully I've exhausted all the possible ways of screwing these patches up. Sorry.
Attachment #8450673 -
Attachment is obsolete: true
Attachment #8450673 -
Flags: review?(khuey)
Attachment #8450679 -
Flags: review?(khuey)
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 =.
Attachment #8450679 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 16•10 years ago
|
||
review changes
Attachment #8450679 -
Attachment is obsolete: true
Attachment #8452765 -
Flags: review?(khuey)
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 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
Attachment #8452765 -
Flags: review?(khuey) → review+
This is dead now, but we still need some other stuff in this file for another couple days.
Attachment #8452765 -
Attachment is obsolete: true
Attachment #8452767 -
Attachment is obsolete: true
Attachment #8504449 -
Flags: review?(bzbarsky)
![]() |
||
Comment 22•10 years ago
|
||
Comment on attachment 8504449 [details] [diff] [review] Stub out part of quickstubs r=me
Attachment #8504449 -
Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/944cba921a29 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3304b7e0e0
Comment 24•10 years ago
|
||
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
File a bug please.
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/944cba921a29 https://hg.mozilla.org/mozilla-central/rev/8d3304b7e0e0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 27•10 years ago
|
||
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•10 years ago
|
||
+1 for the last comment. Our add-on needs to be compatible with FF 31 ESR up to latest release.
![]() |
||
Comment 29•10 years ago
|
||
Axel, boxObject.QueryInterface should still work fine in system-principal code... Certainly BoxObject.prototype.QueryInterface exists in chrome. What, exactly, fails in your code?
![]() |
||
Updated•10 years ago
|
Flags: needinfo?(axelg)
Comment 30•10 years ago
|
||
(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).
Flags: needinfo?(axelg)
![]() |
||
Comment 31•10 years ago
|
||
> 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.
Flags: needinfo?(jorge)
Comment 32•10 years ago
|
||
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.
Flags: needinfo?(jorge)
Comment 33•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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/
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 36•10 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Comment 37•9 years ago
|
||
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...
Flags: needinfo?(jonanin)
![]() |
||
Comment 38•9 years ago
|
||
> 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•7 years ago
|
||
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/e0e3a50fed7c Port BoxObject and its subclasses to WebIDL. r=khuey sr=bz
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•