Last Comment Bug 616684 - Remove support for DOM Views
: Remove support for DOM Views
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on: 652580
Blocks: 310343 657252
  Show dependency treegraph
 
Reported: 2010-12-04 04:58 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-05-16 03:24 PDT (History)
8 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (113.71 KB, patch)
2011-03-09 08:56 PST, :Ms2ger (⌚ UTC+1/+2)
jonas: review+
Details | Diff | Splinter Review
Patch v2 (125.24 KB, patch)
2011-04-27 13:22 PDT, :Ms2ger (⌚ UTC+1/+2)
ehsan: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2010-12-04 04:58:37 PST
One question is what we should do about document.implementation.hasFeature("views", "2.0").
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2011-03-09 08:56:21 PST
Created attachment 518083 [details] [diff] [review]
Patch v1
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2011-03-13 23:08:43 PDT
Comment on attachment 518083 [details] [diff] [review]
Patch v1

Wow, very nice!

There were a couple of places where I would have included an empty line before a return statement as it tends to read cleaner and be consistent with other code. But didn't feel that it was important enough, as I'd rather just see this landed. But something to keep in mind for future patches.
Comment 3 Boris Zbarsky [:bz] 2011-03-30 10:20:38 PDT
This needs to change nsIDOMHTMLDocument's IID and the like as well...
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-04-24 05:14:13 PDT
http://hg.mozilla.org/mozilla-central/rev/13f6847dd840
Comment 5 Eric Shepherd [:sheppy] 2011-04-25 08:05:00 PDT
Why was this removed, so I can mention that when I write the note explaining its removal? Thanks.
Comment 6 Eric Shepherd [:sheppy] 2011-04-25 11:40:27 PDT
We had no docs for this to begin with, so I've simply added a note that they were removed to Firefox 6 for developers:

https://developer.mozilla.org/en/Firefox_6_for_developers#DOM
Comment 7 :Ehsan Akhgari (away Aug 1-5) 2011-04-25 14:03:24 PDT
This patch has caused the crash in bug 652580.  This is because this patch changes the semantics of the code in several places, which can (and does) break assumptions all over the code base.

Particularly, for bug 652580, the faulty change is this hunk: <http://hg.mozilla.org/mozilla-central/rev/13f6847dd840#l61.30>.  Before this change, this code: |mCSSView = do_QueryInterface(abstractView, &rv);| will set rv to NS_ERROR_NULL_POINTER <http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMPtr.cpp#70> since |abstractView| is null.  After this change, that check has been removed, which causes the mozInlineSpellWordUtil to remain in an inconsistent state.  A similar bug may exist here as well: <http://hg.mozilla.org/mozilla-central/rev/13f6847dd840#l60.47>

I'm going to back out this patch, and I think the next time around that this wants to land, it should go through a more detailed review to make sure that the silent semantics changes are really fine.
Comment 8 :Ehsan Akhgari (away Aug 1-5) 2011-04-25 14:06:56 PDT
Backed out: <http://hg.mozilla.org/mozilla-central/rev/00d644aeaae8>
Comment 9 Eric Shepherd [:sheppy] 2011-04-25 14:38:56 PDT
Removed the text about this from Firefox 6 for developers.
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2011-04-25 15:42:56 PDT
That first mxr link should be
http://hg.mozilla.org/mozilla-central/rev/13f6847dd840#l61.30
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-04-27 13:22:00 PDT
Created attachment 528696 [details] [diff] [review]
Patch v2
Comment 12 :Ehsan Akhgari (away Aug 1-5) 2011-04-28 11:10:56 PDT
Comment on attachment 528696 [details] [diff] [review]
Patch v2

Can you please test to see whether this version of the patch fixes bug 652580?

In order to do this, you should apply the previous version, verify that you can reproduce bug 652580 using the steps to reproduce that I posted there, and then apply the new version of the patch, and verify that the bug doesn't happen any more.

Code-wise, this looks fine, but I want to make sure that it doesn't regress bug 652580.

Thanks!
Comment 13 :Ehsan Akhgari (away Aug 1-5) 2011-04-28 11:46:42 PDT
Comment on attachment 528696 [details] [diff] [review]
Patch v2

Review of attachment 528696 [details] [diff] [review]:

r=me with the below nits addressed.

::: content/events/src/nsDOMScrollAreaEvent.cpp
@@ +155,5 @@
                          nsPresContext *aPresContext,
                          nsScrollAreaEvent *aEvent)
 {
+  nsDOMScrollAreaEvent* it = new nsDOMScrollAreaEvent(aPresContext, aEvent);
+  return CallQueryInterface(it, aInstancePtrResult);

You've taken a bad variable name here, and made it worse.  ;-)  How about |event|?

::: content/events/src/nsDOMUIEvent.cpp
@@ +425,1 @@
   return CallQueryInterface(it, aInstancePtrResult);

I'd probably address the above nit here too.

::: content/events/src/nsDOMXULCommandEvent.cpp
@@ +147,1 @@
   return CallQueryInterface(it, aInstancePtrResult);

...And here!

::: content/html/document/src/nsHTMLDocument.cpp
@@ +1391,5 @@
+NS_IMETHODIMP
+nsHTMLDocument::GetDefaultView(nsIDOMWindow** aWindow)
+{
+  return nsDocument::GetDefaultView(aWindow);
+}

I don't really get the purpose behind this function.  Can't we just remove it?

::: editor/libeditor/html/nsHTMLCSSUtils.cpp
@@ +595,2 @@
 {
+  *aViewCSS = nsnull;

As I talked to Ms2ger, this changes the semantics if GetElementContainerOrSelf fails.  Please move this to after the below NS_ENSURE_SUCCESS line.
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2011-04-29 11:14:41 PDT
(In reply to comment #13)
> Comment on attachment 528696 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 528696 [details] [diff] [review]:
> 
> r=me with the below nits addressed.
> 
> ::: content/events/src/nsDOMScrollAreaEvent.cpp
> @@ +155,5 @@
>                           nsPresContext *aPresContext,
>                           nsScrollAreaEvent *aEvent)
>  {
> +  nsDOMScrollAreaEvent* it = new nsDOMScrollAreaEvent(aPresContext, aEvent);
> +  return CallQueryInterface(it, aInstancePtrResult);
> 
> You've taken a bad variable name here, and made it worse.  ;-)  How about
> |event|?
> 

I reverted that name change. This patch is more than big enough already.

> ::: content/events/src/nsDOMUIEvent.cpp
> @@ +425,1 @@
>    return CallQueryInterface(it, aInstancePtrResult);
> 
> I'd probably address the above nit here too.
> 
> ::: content/events/src/nsDOMXULCommandEvent.cpp
> @@ +147,1 @@
>    return CallQueryInterface(it, aInstancePtrResult);
> 
> ...And here!
> 

Left those alone as well. Note that you missed some above nsDOMScrollAreaEvent.

> ::: content/html/document/src/nsHTMLDocument.cpp
> @@ +1391,5 @@
> +NS_IMETHODIMP
> +nsHTMLDocument::GetDefaultView(nsIDOMWindow** aWindow)
> +{
> +  return nsDocument::GetDefaultView(aWindow);
> +}
> 
> I don't really get the purpose behind this function.  Can't we just remove it?
> 

No. Blame the NS_DECL_NSIDOMDOCUMENT here: <http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.h#124>

> ::: editor/libeditor/html/nsHTMLCSSUtils.cpp
> @@ +595,2 @@
>  {
> +  *aViewCSS = nsnull;
> 
> As I talked to Ms2ger, this changes the semantics if GetElementContainerOrSelf
> fails.  Please move this to after the below NS_ENSURE_SUCCESS line.

Done.
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2011-05-01 01:35:12 PDT
http://hg.mozilla.org/mozilla-central/rev/0fb6deab9b72
Comment 16 Eric Shepherd [:sheppy] 2011-05-02 12:07:43 PDT
Note restored to Firefox 6 for developers.
Comment 17 TheRave 2011-05-06 13:28:16 PDT
Missing a link to Bug 616684 in Firefox 6 for developers/remove dom view.
Comment 18 Eric Shepherd [:sheppy] 2011-05-06 13:31:45 PDT
We only link to bugs in specific cases (it clutters the documentation). Is there a reason to do so here?
Comment 19 TheRave 2011-05-06 13:49:42 PDT
Yes i have searched the reason why dom view no longer works. I know what i search but can't find it in bugzilla. So i searching in mxr the old idl file and can't find it. At next searching for changes related to the idl directory the last four weeks and find in the showed pushlog this bug. Very complicated. Is this really necessary?

The history of changes is already a good idea. My personal opinion is, to make a link to every point showed on tis site.

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