Closed Bug 502715 Opened 11 years ago Closed 10 years ago

Enable the canvas 2d context to function without an actual canvas element

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: robarnold, Assigned: robarnold)

References

Details

Attachments

(1 file, 2 obsolete files)

Windows 7 taskbar previews would like this. The surface needs to be backed by a windows surface so that we can get an HBITMAP out and the preview does not have access to the document (nor should it really be required).
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #388274 - Flags: review?(vladimir)
Comment on attachment 388274 [details] [diff] [review]
v1.0

mmmm. So I don't like just ignoring the security checks here.  If we don't have a canvas element, we should ensure at least that we have the system/chrome principal.

Also, looking at this, can we create mCSSParser lazily?  Maybe create a GetCSSParser() method that will create one if it's not created and use that instead of mCSSParser everywhere.
(In reply to comment #2)
> (From update of attachment 388274 [details] [diff] [review])
> mmmm. So I don't like just ignoring the security checks here.  If we don't have
> a canvas element, we should ensure at least that we have the system/chrome
> principal.

How can we have a context without an element in a non-system environment?

> 
> Also, looking at this, can we create mCSSParser lazily?  Maybe create a
> GetCSSParser() method that will create one if it's not created and use that
> instead of mCSSParser everywhere.

Sounds good.
Attachment #388274 - Flags: superreview?(roc)
Attachment #388274 - Flags: review?(vladimir)
Attachment #388274 - Flags: review+
Comment on attachment 388274 [details] [diff] [review]
v1.0

Ok, yeah, this is fine; I guess we can't have a no-content canvas without chrome privs.  Setting sr on roc just to make sure that's true...
Why are you asking me if it's true? I assume it's true just because InitializeWithSurface is non-scriptable.

+    if (!mCSSParser) {
+        mCSSParser = do_CreateInstance("@mozilla.org/content/css-parser;1");
+    }
+
+

Superfluous blank line

Can we have a GetPresShell helper function here to abstract over the if (mDocShell) { mDocShell->GetPresShell(); }?
Attached patch v1.1 (obsolete) — Splinter Review
(In reply to comment #5)
> Superfluous blank line

Now removed.

> Can we have a GetPresShell helper function here to abstract over the if
> (mDocShell) { mDocShell->GetPresShell(); }?

Done. Code looks much cleaner. jst suggested that we call GetOwnerDoc instead GetCurrentDoc. The document from the pres shell should be the same as before this change.
Attachment #388274 - Attachment is obsolete: true
Attachment #390102 - Flags: review?(vladimir)
Attachment #388274 - Flags: superreview?(roc)
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d41cc3134424
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reopened due to crashes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v1.1.1Splinter Review
This patch actually makes the change I talked about (not making it was the cause of the crashes).
Attachment #390102 - Attachment is obsolete: true
Pushed to mozilla-central (again):
http://hg.mozilla.org/mozilla-central/rev/fe96f2059e54
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
When compiling nsCanvasRenderingContextGL.cpp, I got the following error:

In file included from nsCanvasRenderingContextGL.h:56,
                 from nsCanvasRenderingContextGL.cpp:42:
../../../dist/include/nsICanvasRenderingContextInternal.h:65: error: 'nsIDocShell' has not been declared

I suppose to include nsIDocShell.h in nsICanvasRenderingContextInternal.h.
Can you file a follow up bug for that? That fix seems fine at a glance.
(In reply to comment #12)
Comment #11 has been filed as bug 508923.

After fixing bug 208923, I'm still in problem due to bug 507949.
(In reply to comment #13)
> After fixing bug 208923, I'm still in problem due to bug 507949.

After fixing bug 508923, I'm still in problem due to bug 507949.
Comment on attachment 392274 [details] [diff] [review]
v1.1.1


>+    /**
>+     * Gets the pres shell from either the canvas element or the doc shell
>+     */
>+    nsIPresShell *GetPresShell() {
>+      nsIPresShell *presShell = nsnull;
>+      nsCOMPtr<nsIContent> content = do_QueryInterface(mCanvasElement);
>+      if (content) {
>+          presShell = content->GetOwnerDoc()->GetPrimaryShell();
>+      } else if (mDocShell) {
>+          mDocShell->GetPresShell(&presShell);
>+      }
>+      return presShell;
>+    }
GetOwnerDoc() may return null, so this may crash, and
mDocShell->GetPresShell(...) addrefs so this leaks.
Depends on: 521608
You need to log in before you can comment on or make changes to this bug.