Remove support for DOM Views

RESOLVED FIXED in mozilla6

Status

()

defect
RESOLVED FIXED
9 years ago
2 months ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

({dev-doc-complete})

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 1

8 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #518083 - Flags: review?(jonas)
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.
Attachment #518083 - Flags: review?(jonas) → review+
Assignee

Updated

8 years ago
Depends on: post2.0
Whiteboard: [needs landing]
This needs to change nsIDOMHTMLDocument's IID and the like as well...
Whiteboard: [needs landing] → [needs landing][not-ready-for-cedar]
Assignee

Updated

8 years ago
No longer depends on: post2.0
Whiteboard: [needs landing][not-ready-for-cedar] → [need gk2.2 ship]
Assignee

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [need gk2.2 ship]
Target Milestone: Future → mozilla6
Assignee

Updated

8 years ago
Keywords: dev-doc-needed
Why was this removed, so I can mention that when I write the note explaining its removal? Thanks.
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

Updated

8 years ago
Depends on: 652580

Comment 7

8 years ago
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

8 years ago
Backed out: <http://hg.mozilla.org/mozilla-central/rev/00d644aeaae8>
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Removed the text about this from Firefox 6 for developers.
Posted patch Patch v2Splinter Review
Attachment #518083 - Attachment is obsolete: true
Attachment #528696 - Flags: review?(ehsan)

Comment 12

8 years ago
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!
Attachment #528696 - Flags: review?(ehsan)

Comment 13

8 years ago
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.
Attachment #528696 - Flags: review+
(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.
http://hg.mozilla.org/mozilla-central/rev/0fb6deab9b72
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Note restored to Firefox 6 for developers.

Comment 17

8 years ago
Missing a link to Bug 616684 in Firefox 6 for developers/remove dom view.
We only link to bugs in specific cases (it clutters the documentation). Is there a reason to do so here?

Comment 19

8 years ago
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.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.