616684 - Remove support for DOM Views

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

One question is what we should do about document.implementation.hasFeature("views", "2.0").

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 [:bzbarsky]]

This needs to change nsIDOMHTMLDocument's IID and the like as well...

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/13f6847dd840

Comment 5

[Eric Shepherd [:sheppy]]

Why was this removed, so I can mention that when I write the note explaining its removal? Thanks.

Comment 6

[Eric Shepherd [:sheppy]]

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

[(no longer active)]

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

[(no longer active)]

Backed out: <http://hg.mozilla.org/mozilla-central/rev/00d644aeaae8>

Comment 9

[Eric Shepherd [:sheppy]]

Removed the text about this from Firefox 6 for developers.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That first mxr link should be
http://hg.mozilla.org/mozilla-central/rev/13f6847dd840#l61.30

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 12

[(no longer active)]

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

[(no longer active)]

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 (he/him; ⌚ UTC+1/+2)]

(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 (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/0fb6deab9b72

Comment 16

[Eric Shepherd [:sheppy]]

Note restored to Firefox 6 for developers.

Comment 17

[TheRave]

Missing a link to Bug 616684 in Firefox 6 for developers/remove dom view.

Comment 18

[Eric Shepherd [:sheppy]]

We only link to bugs in specific cases (it clutters the documentation). Is there a reason to do so here?

Comment 19

[TheRave]

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.