1156 - OBJECTs without type attribute aren't rendered

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Antti Näyhä]

OBJECTs without the *optional* type attribute are not
rendered at all, even when the file format is a familiar
one.  They don't degrade correctly, either.

Example:

<object data="test.avi">
  alternative text
</object>

Either the test.avi video or the words "alternative text" should be rendered.
Currently, nothing is displayed.

Comment 1

[Alex Musil]

*** This bug has been marked as a duplicate of 678 ***

Comment 2

[glynn]

Marked verified as duplicate; reporter if you disagree please reopen.

Comment 3

[Hixie (not reading bugmail)]

This is not a dup of 678. And it's still buggy, 7 years later...

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A fix for this would likely also fix bug 96579 and bug 95549.

We should probably be kicking off resource loads in nsHTMLObjectElement and then
determining what type of frame to create (image, subdocument, or plugin). 
nsObjectFrame should be renamed nsPluginFrame and it should not handle anything
related to images or subdocuments.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... although maybe we should be thinking more of a way to do it that would allow
easier implementation of, or perhaps even use, CSS3's version of the 'content'
property.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

some testcases:

http://hixie.ch/tests/adhoc/html/flow/object/
http://biesi.damowmow.com/object/

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.5

problems with this patch:
- breaks pluginfinder. probably breaks nullplugin.
- breaks alt attributes of <embed> (if they worked.. not sure about that)

- the XXX comment in nsPluginHostImpl::InstantiatePluginForChannel

plus, I need to do some more testing.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.5, merged to trunk

Comment 9

[Boris Zbarsky [:bzbarsky]]

> - breaks pluginfinder. probably breaks nullplugin.

Frankly, I think breaking pluginfinder (for a few days or so) and fixing it in a
separate patch based on the -moz-broken CSS stuff (making it a real XBL binding
and all) may be acceptable.  Of course if -moz-broken lands before this patch we
could even fix plugin finder before you land, but....

Do embeddors (eg camino?) use nullplugin?  Because seamonkey wants to move to a
more plugin-finder-like setup, so breaking nullplugin may be ok there.

> - breaks alt attributes of <embed> (if they worked.. not sure about that)

Again, I suspect the -moz-broken patch addresses that...  Again, I think it's OK
to have this broken for a few days if you land before I do once 1.9 opens.  So I
wouldn't lose sleep over this.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.5, merged to trunk, including new files

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.51

- fix <embed> for SVG (i.e. fix the crash)
- fix linux compilation

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.52

- fix various issues with document loads (be in a document while loading, don't
create frames when a content viewer exists, but no root content (because CSSFC
screws that up))
- remove a pointless nullcheck

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

more problems with the patch:
- alternate content for <embed> doesn't work
- something seems wrong with painting of <embed> plugins

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.6

- makes nullplugin work
- removes some unused code (CreateAttributeContent from another patch + the
ifdeffed out code that uses it)
- fixes a few compile warnings
- fixes embed painting - the code in DidReflow must be called after the plugin
is instantiated (apparently, calling it from FixupWindow doesn't work (ie,
crashes) :-/)

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

note that the latest patch also makes it so that nothing is shown while waiting
for a server response. (the standby attribute should be shown, but I'll do that
in a separate bug.)

So all that's left is <embed> alternate content and pluginfinder, and I'll do
that in a separate bug, after bz's work on :moz-broken.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

oops, actually, the pluginurl check for <object> is wrong - it should look for a
<param>, not an attribute...

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.65

now also fixes the nullplugin on windows (FixupWindow must be called before
instantiating the plugin, but after creating an instanceowner, in order to
ensure that mPluginWindow is correct, whose dimensions are used when
creating/resizing the widget)

Comment 18

[Ian Macfarlane]

I see this bug is being actively developed (it's great to see the really old
bugs being fixed).

What's the chance of this getting into 1.1?

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

zero, way too much risk.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.7

fix nullplugin for <object> with a pluginurl param

I think this is ready for review now.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

bleh, the patch here breaks full-page plugins. reason: the frame is constructed
in StartLayout called from nsMediaDocumentStreamListener::OnStartRequest. That
calls nsObjectLoadingContent::SetFrame, which posts an event to call instantiate.

Now, OnDataAvailable is called, and the event hasn't been processed yet. So no
final stream listener is available, ODA reads no data, and necko cancels the load.

The event would then get processed, InstantiateFullPagePlugin gets called, and a
stream listener set.

My current workaround is to call Suspend in StartDocumentLoad and Resume in
SetSreamListener (followed by calling OnStartRequest on aListener), but I don't
like that very much... (which is why I'm not attaching it yet).

The instantiating must be async because the frame isn't fully initialized yet by
the time it calls SetFrame. Especially, it has no view.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0.8

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

patch v0.8 (attachment 189777 [details] [diff] [review]) makes it so that nsPluginDocument calls
Instantiate in the case of a fullpage plugin, and that nsObjectLoadingContent
doesn't.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Review of the content stuff:

>Index: content/base/public/nsIFrameLoader.idl

>   /**
>+   * Loads the specified URI in this frame. Behaves identical to loadFrame,

identically

>Index: content/base/public/nsIObjectLoadingContent.idl

>+/**
>+ * This interface represents a content node that loads objects.
>+ */
>+[scriptable, uuid(3e78c950-24d2-4fcc-b0b9-751686e89b8f)]

Except this can't be used from script, since nsIObjectFrame is not scriptable. 
What's the use case for this being scriptable?

>+   * Tells the content about the associated object frame.

In which presentation?  This needs to be very well documented... (and why is it
needed?  Why is GetPrimaryFrameFor not enough?)   Also, what happens with
printing here?

Per IRC discussion let's remove mFrame in favor of GetPrimaryFrameFor on the
document's 0th presshell (if this is not null).

>Index: content/base/src/nsFrameLoader.cpp
>+nsFrameLoader::LoadURI(nsIURI* aURI)
>+{
>+  NS_PRECONDITION(aURI, "Null URI?");

I know this is a perennial point of disagreement between us, but I'd really
prefer an NS_ENSURE_ARG_POINTER here.  This is not exactly perf-sensitive code,
and unlike out params it's not that hard to _accidentally_ pass in null here.

>Index: content/base/src/nsImageLoadingContent.cpp

>+nsresult
> nsImageLoadingContent::ImageURIChanged(const nsAString& aNewURI) {
>   return ImageURIChanged(aNewURI, PR_TRUE);
> }

I believe this method is now unused.  And it has no reason to exist anyway. Just
remove it?

>+nsImageLoadingContent::ImageURIChanged(nsIURI* aNewURI,

I'm torn between passing in a document here (possibly as an optional arg?) and
just swallowing the cost of doing GetOurDocument twice (it's not cheap, since it
involves QI).

Perhaps have a private method that takes a document as an arg and call it from
both ImageURIChanged protected methods?

>+        // XXXbiesi this is bad! it will reframe twice for <object>

That's OK, I plan to remove that code.  ;)

>Index: content/base/src/nsImageLoadingContent.h

Please don't make the private CancelImageRequests protected; instead use the
protected version that's been added since; that one does things "more correctly"...

>Index: content/base/src/nsObjectLoadingContent.cpp

>+struct nsAsyncInstantiateEvent : public PLEvent {
>+  // Note: This contains a content node rather than a frame to make sure
>+  // that the frame isn't going away by the time the event is handled
>+  nsObjectLoadingContent* mContent;

Why not nsRefPtr<nsObjectLoadingContent> ?  Alternately, why not just get this
from event->owner as needed (and stick with the manual refcounting we have now)?

>+  static void *PR_CALLBACK HandleEvent(PLEvent * event)

This is actually subtly wrong, as David pointed out recently.  See
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.h#113 and
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.h#757 for
the right way to do this; maybe we should move those two typedefs to
nsContentUtils.h or something?  Same for the destroy callback.

>+  static void  PR_CALLBACK CleanupEvent(PLEvent * ev)
>+  {
>+    delete (nsAsyncInstantiateEvent*) ev;

Please store that nsAsyncInstantiateEvent* in a local variable; on some
platforms delete is not happy with non-lvalues.

>+IsSupportedImage(const nsCString& aMimeType)

Can you remove this from nsObjectFrame now?  If not, file a followup on that?

>+IsSupportedDocument(nsIContent* aOurContent,

Same.  And could just make this a member method, I'd think...  No reason to pass
in |this| as aOurContent.

>+IsSupportedPlugin(const nsCString& aMIMEType)
>+  // XXX do plugins expect to work via extension too?

I'd think that we'd just use whatever type necko gave us by here.  Might want to
file a followup bug to investigate this, though...

>+// ctor/dtor

No need for that comment... ;)

>+nsObjectLoadingContent::~nsObjectLoadingContent()
>+  if (mFrameLoader)
>+    mFrameLoader->Destroy();

Unfortunately, as things stand we also need to destroy the frame loader if the
content node is removed from the tree (see what nsGenericHTMLFrameElement
does)...  Perhaps have a method subclasses of this can call when their
UnbindFromTree method is called?

>+nsObjectLoadingContent::OnStartRequest(nsIRequest *aRequest, nsISupports
*aContext)

>+  if (!IsSuccessfulRequest(aRequest)) {
>+    UnloadContent();
>+    TypeChanged(eType_Null);

Why not call Fallback() here?

>+  ObjectType newType = GetTypeOfContent(ctype);
>+  if (mType != newType) {
>+    UnloadContent();

This might cause issues in some cases; have to watch out for that...

>+  switch (newType) {

Do we want a ShouldProcess check here, or put those in places like
LoadImageWithChannel?  File a followup bug on sorting this out, please.

>+    case eType_Document: {
>+      if (!mFrameLoader) {
>+        mFrameLoader = new nsFrameLoader(thisContent);

We probably only want to do this if we're in the document...  Frame loaders
don't deal well with being hooked up to nodes that are not in a document yet.  :(

>+      if (mType != newType)
>+        TypeChanged(newType);

Why do you need to do this before calling GetDocShell?  If there's a good
reason, please document it.

>+      rv = listener->DoContent(ctype.get(), PR_TRUE, aRequest,
>+                               getter_AddRefs(mFinalListener), &abort);

Have we done any testing with mailnews or someplace else that depends on its
nsIURIContentListener impls to block loads of various sorts?  Or put another
way, should we be calling CanHandleContent here?  I guess that's handled by our
earlier check for whether this is a document type, right?  Given that, perhaps
we should have some debug-only code that calls CanHandleContent and asserts that
the answer is yes?

Also, it looks like we won't do text/plain sniffing or other content conversions
on the data we get back from our channel.  We probably should do this, and
possibly not just for documents.  Let's do a followup bug on this, though.  In
the short run this may break some things, of course....

>+      if (abort)
>+        return NS_OK; // Do nothing else XXX seems suboptimal?

I think it's safe to warn any time |abort| comes back true... I can't think of
good reasons why it would do that, basically.

>+      mInstantiating = PR_TRUE;
>+      if (mType != newType)
>+        TypeChanged(newType);
>+      if (mFrame)
>+        rv = mFrame->Instantiate(chan, getter_AddRefs(mFinalListener));

Hmm...  Why not just follow the "standard" async instantiate codepath here? 
That would work much better with reframing via content state changes (which is
async), I think.  Is the problem that we get the new frame before we've set all
our internal state up correctly?

>+nsObjectLoadingContent::SetFrame(nsIObjectFrame* aFrame)

>+    nsAsyncInstantiateEvent* ev = new nsAsyncInstantiateEvent(this, mTypeHint,
mURI);

So if we get reframed a few times in a row we'll have a bunch of events posted
and will instantiate several times in a row when they fire? Perhaps it would be
better to store aFrame in the event and only call Instantiate if that frame is
still the primary frame for the nsObjectLoadingContent in the 0th presshell? 
That way if we reframe a few times in a row only the last event will matter.

>+nsObjectLoadingContent::ObjectURIChanged(nsIURI* aURI,

>+  mTypeHint = aTypeHint;
>+
>+  const nsCString& flatType = PromiseFlatCString(aTypeHint);

Why not just use mTypeHint?  That's flat and all.

Past that, we could probably use nsCString as the type hint argument type, since
I bet every single caller of ObjectURIChanged actually passes in a flat string...

>+  // Security checks
>+  // Can't do security checks without a URI - hopefully the plugin will take
>+  // care of that

Under what conditions would we be doing anything at all if we have a null URI? 
Please document those cases here.

>+    nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
>+    NS_WARN_IF_FALSE(secMan, "No security manager!?");
>+    if (secMan) {

secMan is guaranteed non-null here; we should rename the method to
SecurityManager() to make that clearer...

>+      NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_OBJECT,

Please pass nsContentUtils::GetContentPolicy() as the last arg here, ok?

>+  if (aForceType && !aTypeHint.IsEmpty()) {
...
>+      TypeChanged(newType);

So we recreate the frame here (hence the need for the mInstantiating guards
around it...)

...
>+      case eType_Plugin:
>+        if (mFrame)
>+          mFrame->Instantiate(flatType.get(), aURI);
>+        break;

Couldn't we just do nothing in this case, do the TypeChanged() call _after_ this
whole switch, and follow the "normal" async codepath via SetFrame()?

>+  // If the URI is not supported

What does it mean for the URI to not be supported?  The only way aURI can be
null here is if NS_NewURI failed or in the "no data attribute" case for
<object>.  The former should only happen for completely mangled URIs, right? 
Should we even be getting into this code for that case?

>+  nsresult rv = thisContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::classid,
classid);

So... the current nsObjectFrame code forces particular MIME types for particular
classid's (eg java: comes to mind).  I'm not sure I see where you're doing that;
what am I missing?

>+  if (rv == NS_CONTENT_ATTR_HAS_VALUE && IsClassIDSupported(classid) ||
>+      !aURI && !aTypeHint.IsEmpty()) {

Please put in parens so people (like me) don't have to try to recall whether &&
or || binds tighter?  ;)

>+    if (mFrame)
>+      mFrame->Instantiate(flatType.get(), aURI);
>+    return NS_OK;

Shouldn't this set mInstantiating to false before returning?

>+  if (!CanHandleURI(aURI)) {

How do we handle falling back for random URI schemes that are not supported by
us or any of our plugins?  Or do we want to show the plugin finder or something?

>+  // XXX what if load starts before another one finishes?

Perhaps we should hold on to mChannel instead of mURI and cancel the previous
load when a new one starts?  Or will that asynchronously call our OnStopRequest
with the canceled load, confusing us?

>+nsObjectLoadingContent::UnloadContent()

>+  CancelImageRequests(NS_BINDING_ABORTED, PR_TRUE, nsIContentPolicy::ACCEPT);

I think we want to use the one-arg (no-arg until bug 11011 lands) version of
CancelImageRequests here.

>+  mFrameLoader = nsnull;

I'd call mFrameLoader->Destroy() here if it's non-null, to be safe.... A little
worried about cycles otherwise.

>+nsObjectLoadingContent::TypeChanged(ObjectType aType)

>+  // Set property
>+  thisContent->SetProperty(nsLayoutAtoms::frameType, NS_INT32_TO_PTR(aType));

Why?  Why not just expose this on nsIObjectLoadingContent?  You only need this
in the CSS frame constructor looks like, and it could QI just as easily as
getting the property.  Might even be faster that way.

>+  PRUint32 numShells = doc->GetNumberOfShells();
..
>+    shell->RecreateFramesFor(thisContent);

This part we might want to rethink once bug 11011 lands.  If this lands first,
file a followup bug on this, ok?

>+nsObjectLoadingContent::GetTypeOfContent(const nsCString& aMIMEType)
>+  nsCOMPtr<nsIContent> thisContent = 
>+    do_QueryInterface(NS_STATIC_CAST(nsIImageLoadingContent*, this));
>+  NS_ASSERTION(thisContent, "must be a content");

You don't need this until you're checking for IsSupportedDocument, right?  So
don't do this QI until that point, ok?  Although if IsSupportedDocument becomes
a member method, that'll happen automatically, I guess.

>+  if (IsSupportedImage(aMIMEType))
>+    return eType_Image;

Please use curly braces even around single-line if bodies in content code.  This
applies elsewhere in this patch too.

>+nsObjectLoadingContent::IsClassIDSupported(const nsString& aClassID)

I guess we just removed the "browser" classid stuff?  If so, good!  ;)

>+  if (StringBeginsWith(aClassID, NS_LITERAL_STRING("java:"))) {
>+    // Supported - is a java plugin

Only if the java plugin is installed, right?  Should we check for that here?

>+  // Otherwise, this is ActiveX content

Wouldn't ActiveX start with "clsid:"?  I know you just copied this part from
nsObjectFrame, but I'd like that change made.

>+nsObjectLoadingContent::CanHandleURI(nsIURI* aURI)

Hmm... What about protocols that have the "handle externally" pref explicitly
set?  When you go to AsyncOpen, that will trigger failure and fallback; is that
what we want in that case?

>+nsObjectLoadingContent::ShouldShowDefaultPlugin(nsIContent* aContent)

Can you assert that aContent is of type eHTML here?  If so, please do.

>+    if (child->IsContentOfType(nsIContent::eHTML | nsIContent::eELEMENT) &&

No need for eELEMENT -- eHTML means "HTML element".

jst recently checked in some actualType stuff that you probably need to merge to
-- it should most likely show the hint for cases when we overrode and the actual
network type when we know it from necko.

>Index: content/html/content/src/nsHTMLAppletElement.cpp

> class nsHTMLAppletElement : public nsGenericHTMLElement,
>-                            public nsIDOMHTMLAppletElement
>+                            public nsIDOMHTMLAppletElement,
>+                            public nsObjectLoadingContent

Put the concrete class before the interface, please.

> nsHTMLAppletElement::DoneAddingChildren()
> {
>   mIsDoneAddingChildren = PR_TRUE;
>-  RecreateFrames();  
>+  //RecreateFrames();

If this is not needed, just remove it?

But why is it not needed?  The idea was that applet shouldn't instantiate its
plugin until it's done adding children.

>+nsHTMLAppletElement::BindToTree(nsIDocument* aDocument,

>+  // Must start loading stuff after being in a document

But not if !mIsDoneAddingChildren.  In that case we want to do that when
DoneAddingChildren() is called, imo.

That leaves open the question of what our frame should do in the interim, I guess...

>+  if (aNotify &&
>+      aNameSpaceID == kNameSpaceID_None && aName == nsHTMLAtoms::code) {
>+    ObjectURIChanged(aValue, NS_LITERAL_CSTRING("application/x-java-vm"),

Again, only if mIsDoneAddingChildren.

Note that you need to override all signatures of SetAttr, otherwise you get
nasty warnings about hiding superclass methods...  See what nsHTMLImageElement
does with SetAttr, eg.  This applies to the other places you override SetAttr too.

>Index: content/html/content/src/nsHTMLObjectElement.cpp

>+nsHTMLObjectElement::BindToTree(nsIDocument* aDocument,

Don't we need to call ObjectURIChanged here if mIsDoneAddingChildren is already
true?

>+nsHTMLObjectElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,

>+    ObjectURIChanged(aValue, NS_ConvertUTF16toUTF8(type));

Again, only if mIsDoneAddingChildren.

>-  SetAttr(kNameSpaceID_None, nsHTMLAtoms::type, aType, PR_TRUE);
>+  SetAttr(kNameSpaceID_None, nsHTMLAtoms::type, nsnull, aType, PR_TRUE);

Why this change?  In any case, this code should be gone on trunk.

>Index: content/html/content/src/nsHTMLSharedElement.cpp

> nsHTMLSharedElement::nsHTMLSharedElement(nsINodeInfo *aNodeInfo)
>-  : nsGenericHTMLElement(aNodeInfo)
>+  : nsGenericHTMLElement(aNodeInfo), nsObjectLoadingContent(eType_Plugin)

Shouldn't it only be eType_Plugin if we're an embed?  Otherwise I'd think
eType_Null would be more reasonable.

>-  SetAttr(kNameSpaceID_None, nsHTMLAtoms::type, aType, PR_TRUE);
>+  SetAttr(kNameSpaceID_None, nsHTMLAtoms::type, nsnull, aType, PR_TRUE);

Why this change?  In any case, this code should be gone on trunk.

>+nsHTMLSharedElement::BindToTree(nsIDocument* aDocument,
>+    if (rv == NS_CONTENT_ATTR_HAS_VALUE) {

And if not then we leave ourselves as eType_Plugin?  Why?  Shouldn't we change
to eType_Null?

>Index: content/html/document/src/nsPluginDocument.cpp

So how are we preventing the embed from calling InstantiatePlugin as well?

I haven't read the layout stuff yet; I'll try to do that on Sunday.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> the current nsObjectFrame code forces particular MIME types for particular
> classid's

I just realized that your code just keeps doing that (and ignores the passed-in
type in those cases).  So never mind that comment.  ;)

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

>Per IRC discussion let's remove mFrame in favor of GetPrimaryFrameFor on the
>document's 0th presshell (if this is not null).

per IRC discussion I can't do that (display:none -> inline)

null uris happen when the plugin itself figures out what to load (via a <param>,
say)

unsupported uris happen for mms:// urls

more replies later...

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

>Except this can't be used from script, since nsIObjectFrame is not scriptable. 
>What's the use case for this being scriptable?

That's a good question. However, as jst added an nsIPluginElement in the
meantime, I'll move its attributes here, and that interface does need to be
scriptable.

>Per IRC discussion let's remove mFrame in favor of GetPrimaryFrameFor on the
>document's 0th presshell (if this is not null).

as per above, I can't just remove it. you wanted to think about the problem,
came to any conclusion yet? :)

> I'd really prefer an NS_ENSURE_ARG_POINTER here. 

I'll make it a NS_PRECONDITION + if (!aURI) return NS_ERROR_INVALID_POINTER;,
because I do think that this is a programming error that should not happen. the
if will ensure that we don't crash.

>Perhaps have a private method that takes a document as an arg and call it from
>both ImageURIChanged protected methods?

how about this: add an optional nsIDocument* argument to the URI-taking method,
and pass the document from the other one (changing it to only hold a weak
reference). then, the uri-taking method can do:
   if (!aDocument)
      aDocument = GetOurDocument();

Hm... if we're at that... we could make both ImageURIChanged impls take a
document and pass that from subclasses. that certainly would avoid the qi. what
do you think?

> This is actually subtly wrong, as David pointed out recently.  See

Hrm, right. that also means I can't declare them inline (I think?). although...
maybe this works, with an awful syntax. I'll test.
...testing done, doesn't work. oh well.

>Same.  And could just make this a member method, I'd think...  No reason to pass
>in |this| as aOurContent.

it passes in a qi'd this. but yeah... I can do that inside the method.

I can't remove these methods yet, plugin finder code depends on them. Will file
bugs. Although I probably broke this code anyway.

>Unfortunately, as things stand we also need to destroy the frame loader if the
>content node is removed from the tree 

urg. that sucks a lot. so the document in an iframe is torn down when the iframe
is removed from a document?
why's that necessary?

(note to self: still need to do this)

> Why not call Fallback() here?

I think this code predates Fallback() :) changed.

> This might cause issues in some cases

like what kinds of issues? :)

> We probably only want to do this if we're in the document... 

(note to self: still need to do this)

> If there's a good reason, please document it.

um... I'll try to remember the reason. I *think* I had one.

> Given that, perhaps
> we should have some debug-only code that calls CanHandleContent and asserts that
> the answer is yes?

will do (note to self: still need to do this)

> Hmm...  Why not just follow the "standard" async instantiate codepath here? 

Because I consider the async instantiating a hack... I want it to go away, and
it will once plugin loading is moved to content, which I still hope can be done.

ok, more tomorrow, stopped at:
>+nsObjectLoadingContent::SetFrame(nsIObjectFrame* aFrame)

(bleh, this middle-click paste regression is extremely annoying)

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

I remember why I didn't use nsRefPtr... because it didn't work:
nsObjectLoadingContent doesn't implement any AddRef/Release functions, and the
ones it inherits are ambiguous.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> per IRC discussion I can't do that (display:none -> inline)

That doesn't mean you can't remove mFrame.  Just keep the SetFrame() call (and
maybe rename it to "HasNewFrame" or something)...  See my comments starting with
"So if we get reframed a few times" for how I think this should work.

> null uris happen when the plugin itself figures out what to load (via a
> <param>, say)

It might be a good idea to document the behavior of ObjectURIChanged in the
header...  That would make it clear to people what it should be doing and why.

> and that interface does need to be scriptable.

OK.  Just make the one method noscript, then, with a comment as to why.

> I'll make it a NS_PRECONDITION + if (!aURI) return NS_ERROR_INVALID_POINTER;,

That seems reasonable to me.

> how about this: add an optional nsIDocument* argument to the URI-taking
> method

Could do that, sure...

> we could make both ImageURIChanged impls take a document and pass that from
> subclasses.

Also an option, I guess; I don't want to complicate the API for subclasses too
much, but we could do this too.

> so the document in an iframe is torn down when the iframe
> is removed from a document?

Yes.  We have bugs on this.  It's necessary because docshell really doesn't deal
well with being removed from the docshell tree and then reinserted...  We need
to fix that, but we haven't yet.

> like what kinds of issues? :)

Hrm.  I had something in mind, but I can't recall what.  :(  Maybe I was just on
crack; it looks fine to me now.

> I remember why I didn't use nsRefPtr... because it didn't work:

Right.  I see; hence the casting.  Just use the owner, then, ok?

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm now storing the frame in the event and only call instantiate if it's the
same frame as the one stored in the objectloadingcontent.

> follow the "normal" async codepath via SetFrame()?

(see above)

>>+  // If the URI is not supported
>What does it mean for the URI to not be supported?  The only way aURI can be
>null here is if NS_NewURI failed or in the "no data attribute" case for
><object>.

Oh. that part of the comment is in the wrong place, it seems. That, or it is
just wrong :) fixed.

>How do we handle falling back for random URI schemes that are not supported by
>us or any of our plugins?  

well, we don't, but can we detect that? I don't think plugins give us the
information that they don't support the content.

>Perhaps we should hold on to mChannel instead of mURI and cancel the previous
>load when a new one starts?  Or will that asynchronously call our OnStopRequest
>with the canceled load, confusing us?

yeah... I probably should do that; I thought of that (passing the channel as the
context and comparing it to mChannel could fix that onStopRequest problem;
comparing aRequest won't do it, since that's different for redirects). we
wouldn't cancel channels that got redirected, but maybe that's not much of a
problem, since eventually they'd finish loading by themselves...

(note to self: still need to do that)

>Why?  Why not just expose this on nsIObjectLoadingContent?  You only need this
>in the CSS frame constructor looks like, and it could QI just as easily as
>getting the property.  Might even be faster that way.

Oh. I was thinking this would be faster than the QI, which is why I did what I
did :)

> You don't need this until you're checking for IsSupportedDocument, right?  So

right, but ShouldShowDefaultPlugin needs it too

> Please use curly braces even around single-line if bodies in content code.

(still need to do this)

> I guess we just removed the "browser" classid stuff?  If so, good!  ;)

Yes, I removed it. I couldn't figure out how it's supposed to work. Also, CVS
archeology made me notice that at some point this changed from clsid:browser to
just browser, probably unintentionally. In any case, I couldn't figure out a use
case.

> Only if the java plugin is installed, right?  Should we check for that here?

Hm, probably. Should be enough to check
IsPluginEnabledForType("application/x-java-vm"), I think

(note to self: need to test this)

>Hmm... What about protocols that have the "handle externally" pref explicitly
>set? 

hmm... maybe I should get the protocol handler and QI it to
nsIExternalProtocolHandler for that check?

> Can you assert that aContent is of type eHTML here? 

Hmm... actually... I think it'd be better not to... Non-HTML languages may want
an <object> equivalent. So I'll just return false if type is not html.

>But why is it not needed?  The idea was that applet shouldn't instantiate its
>plugin until it's done adding children.

OK, I'll make sure to call ObjectURIChanged in both functions, if needed (i.e.
call it at the time when both done adding children and in a document)

(note to self: still need to do this)

>That leaves open the question of what our frame should do in the interim, I
>guess...

How about the same as if an <object> is waiting for data to arrive? (nothing
with this patch)

>Don't we need to call ObjectURIChanged here if mIsDoneAddingChildren is already
>true?

yeah. I was depending on the ordering being always the same, apparently I can't
depend on that.

(note to self: still need to do that)

> Why this change?  In any case, this code should be gone on trunk.

because I only overwrote one signature of SetAttr. the others were hidden so not
callable.

>Shouldn't it only be eType_Plugin if we're an embed?  Otherwise I'd think
>eType_Null would be more reasonable.

doesn't really matter... it's ignored in those cases...

anyway, is it a good thing that I'm bloating all HTMLSharedElements? maybe
<embed> should get its own class.

>And if not then we leave ourselves as eType_Plugin?  Why?  Shouldn't we change
>to eType_Null?

hm, yeah... we should. (note to self: still need to do this)

> So how are we preventing the embed from calling InstantiatePlugin as well?

aNotify is false in the SetAttr call.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> well, we don't, but can we detect that?

Not sure.  File a followup bug?

> passing the channel as the context and comparing it to mChannel

Let's try that?

> we wouldn't cancel channels that got redirected

Unless we listen for redirects, yeah... Could do that too, though.  ;)

> right, but ShouldShowDefaultPlugin needs it too

Sure, but you can do the image and such checks first and only QI if it's really
needed.

> maybe I should get the protocol handler and QI it to
> nsIExternalProtocolHandler

Sounds good.

> How about the same as if an <object> is waiting for data to arrive?

Makes sense.

> anyway, is it a good thing that I'm bloating all HTMLSharedElements?

They're already bloated; the whole point is that they are all very rare.

> > So how are we preventing the embed from calling InstantiatePlugin as well?
> aNotify is false in the SetAttr call.

But then it gets BindToTree called, no?

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

> But then it gets BindToTree called, no?

hmm, yeah, probably... need to look at this more (note to self)

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

>>+      if (mType != newType)
>>+        TypeChanged(newType);
>Why do you need to do this before calling GetDocShell?

Hah, I remember. It was related to the timing of the initial reflow. If I do it
later, presshell never creates any frames - bug 300540.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> It was related to the timing of the initial reflow.

Comment so most assiduously, with big XXX markers!  Wouldn't want some kind soul
to "clean this up" or anything...

Comment 35

[Boris Zbarsky [:bzbarsky]]

>Index: layout/base/nsCSSFrameConstructor.cpp
>@@ -5310,15 +5311,43 @@ nsCSSFrameConstructor::ConstructHTMLFram
>+    if (nsHTMLAtoms::applet == aTag) {
>+      type = nsObjectLoadingContent::eType_Plugin;

Won't that be the mType of the node anyway?  I seem to recall code to that
effect.  I'd really like all this to just get the type from the node and be done
with it...  and I think that would have the same behavior as now.

>+    if (NS_FAILED(rv) || type == nsObjectLoadingContent::eType_Loading) {

Why would "rv" be error here?  I don't see how that can happen.

>+      // Ideally, this should show the standby attribute

Followup bug, please.

>+    // Otherwise, it's null - doing nothing here will fallback appropriately
>+    // (i.e., render the children)

That's not the right thing to do for <embed> or <applet> is it?  Followup bug on
getting this right, please (once bug 11011 lands).

>@@ -10658,19 +10699,17 @@ nsCSSFrameConstructor::CantRenderReplace

I wouldn't bother with cleanup in this method -- bug 11011 removes it completely.

>Index: layout/base/nsLayoutAtomList.h
>+LAYOUT_ATOM(frameType, "frameType")                            // enum

This is not needed if you just get the type off the node, right?

>Index: layout/generic/nsIObjectFrame.h

>+   * Instantiate a plugin that loads the data itself.
>+   * @param aMimeType Type of the plugin to instantiate. May be null.

Document that this is ignored if a classid is present?

>Index: layout/generic/nsObjectFrame.cpp

>+nsObjectFrame::FixupWindow(const nsSize& aSize)
>+  GetOffsetFromView(origin, &parentWithView);

Make sure to merge this carefully, ok?  This might have changed recently.

>@@ -1615,15 +1300,15 @@ nsObjectFrame::CreateDefaultFrames(nsPre
>-  imageLoader->ImageURIChanged(src);
>+  // XXX imageLoader->ImageURIChanged(src);

Why?  This seems wrong.  Please leave that as it was.

>@@ -1878,38 +1563,15 @@ nsObjectFrame::Paint(nsPresContext*     
>-#if defined(XP_MAC) && !TARGET_CARBON

Could we have a Mac person glance at this removal and the next one, just in case?

>+nsObjectFrame::Instantiate(nsIChannel* aChannel, nsIStreamListener**
aStreamListener)
>+  if (!mInstanceOwner) {

Want to factor this code from here and the other Instantiate() method into an
EnsureInstanceOwner method or something?

>+nsObjectFrame::Instantiate(const char* aMimeType, nsIURI* aURI)

This method doesn't seem to use aURI.  Why bother passing it in?

>+  nsCOMPtr<nsIURI> fullURL;

I know you copied this code, but please don't declare it here; declare it as far
into each block where it's needed as you can;

>+  // if we have a clsid, we're either an internal widget, an ActiveX control,
or an applet

This duplicates some MIME types from nsObjectLoadingContent, no?  I guess this
is temporary till we move plugin loading to content?  Even so, I think I'd
prefer to see a GetTypeForClassid() function in nsContentUtils or
nsObjectLoadingContent or something that would return one of the three types we
care about; returning the empty string would mean the classid does not start
with "java:" and the two OLE types are not supported...  That could let you
replace these three separate InstantiatePlugin() calls here with a single one if
the return value is not the empty string, no?

>+    if (bJavaObject) {
...
>+        fullURL = baseURI;

Why not just pass baseURI to the InstantiatePlugin call?  No need for fullURL here.

>+    else { // otherwise, we're either an ActiveX control or an internal widget
...
>+        fullURL = baseURI;

Same.

>+    // finish up
>+    if (NS_SUCCEEDED(rv))
>+      return NS_OK;

This early return looks wrong to me.... Note that this just means we won't call
NotifyContentObjectWrapper(), which we used to call any time we reflowed after
initial instantiation.  I see no harm in calling it for java and activex stuff,
so just take out this early return?

>+    nsAutoString    src;
>+    if (!baseURI) {
>+      rv = NS_ERROR_FAILURE;

Why not just return early here?  That would make more sense, I think.

>@@ -4358,7 +4214,8 @@ void nsPluginInstanceOwner::Composite()
>+

Don't add the random whitespace?

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>@@ -2014,15 +2014,15 @@ nsPluginStreamListenerPeer::OnStartReque
>+  nsCAutoString aContentType; // XXX _again_?

Not sure what this comment means... Clarify?

>@@ -3287,14 +3287,40 @@ nsPluginHostImpl::GetPluginTempDir(nsIFi
>+#ifdef PLUGIN_LOGGING
>+  nsCAutoString urlSpec;
>+  uri->GetAsciiSpec(urlSpec);

That's not cheap; how about a LOG_ENABLED check here?

>+  // XXX stopped plugins?

Expand on this comment?

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

>>+  // XXX imageLoader->ImageURIChanged(src);
>Why?  This seems wrong.  Please leave that as it was.

I can't, I removed that method. Maybe I should've removed the entire method,
given that I broke PFS anyway.

>>+    if (NS_FAILED(rv) || type == nsObjectLoadingContent::eType_Loading) {
>Why would "rv" be error here?  I don't see how that can happen.

It could happen in the attached patch - the property was not initially set. no
issue anymore in my local tree.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> I can't, I removed that method.

Ah, indeed.  I wonder why that was even needed, since the |src| there is being
set...  File a followup bug on fixing up PFS and let's remove the code there?

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: intermediate patch

merged to trunk and many of those changes made; but more to be done. attaching
mainly for backup purposes. (this does observe redirects)

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 194240 [details] [diff] [review]
intermediate patch

hm... maybe an error return from Instantiate needs to trigger fallback. maybe
one from onStartRequest too... would need to be careful with imagelib's cache
stuff which cancels channels at some point.

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

>>+  GetOffsetFromView(origin, &parentWithView);
>Make sure to merge this carefully, ok?  This might have changed recently.

Looks like nsObjectFrame.cpp didn't actually change in that checkin...

> Could we have a Mac person glance at this removal and the next one, just in case?

(note to self: request review from a mac person)

> This method doesn't seem to use aURI.  Why bother passing it in?

Hm, you're right... I'll remove it (note to self: still need to do this).
although maybe I should instead change some of the code here to use the
passed-in URI, to avoid unnecessary newURI calls.

> declare it as far into each block where it's needed as you can;

ok, done (and changed fullURL to baseURI in some cases. although I have no idea
why the base URI is anything like the correct value to pass)

> This duplicates some MIME types from nsObjectLoadingContent, no?

yeah... hm... maybe most of this function can move to nsObjectLoadingContent,
and it can just pass the right mime type/url to the plugin host?

(note to self: still need to do this)

> This early return looks wrong to me....

it was always there... but sure, I'll take it out.

>>+    if (!baseURI) {
>>+      rv = NS_ERROR_FAILURE;
>Why not just return early here?  That would make more sense, I think.

Turns out all code paths want a nonnull baseURI, so I'll move the check up and
return early.

similarly for the pluginHost... I'll just return an error early if it's not
available.

Hm... I'm not using aMimeType either. I should. (note to self: still need to do
this)

> Not sure what this comment means... Clarify?

The function calls GetContentType twice. And I don't see any code in between the
two calls that'd modify it. Comment clarified.

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: intermediate patch 2

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

note to self: file a followup bug to investigate the baseURI business in
nsObjectFrame::Instantiate

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

as for the unused URI argument to Instantiate, I decided to use it, instead of
getting the code/data/src attributes and constructing a new uri from them.

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

nsPluginDocument works because when BindToTree is called, there is no presshell yet.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #42)
> note to self: file a followup bug to investigate the baseURI business in
> nsObjectFrame::Instantiate

(and that bug should probably include moving most of that function into
nsObjectLoadingContent)

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

I also realized that the string version of ObjectURIChanged should fallback
immediately if URI creation failed. That gives the desired behaviour that
malformed URIs always trigger fallback.

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

I realized that I wrongly handled the case where a class ID was specified, but
unsupported. we would act like no class ID was specified. fixed.

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #39)
> (From update of attachment 194240 [details] [diff] [review] [edit])
> hm... maybe an error return from Instantiate needs to trigger fallback. maybe
> one from onStartRequest too... would need to be careful with imagelib's cache
> stuff which cancels channels at some point.

note to self: need to file a followup bug on this

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

> nasty warnings about hiding superclass methods...

I don't get these warnings, actually...

Comment 50

[Boris Zbarsky [:bzbarsky]]

> nsPluginDocument works because when BindToTree is called, there is no presshell
> yet.

Please comment this clearly in the patch?

Comment 51

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: final (?) patch

fixes the things mentioned above, plus adds the flags as discussed in email
(for other interested people: See the Capabilities enum / the GetCapabilities()
function in nsObjectLoadingContent.h)

Also fixes an assertion because plugins code wants to get a weak reference to
channels' notification callbacks, by implementing nsISupportsWeakReference.

note to self: file a followup bug on nullplugin problems (033.html)

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 195045 [details] [diff] [review]
final (?) patch

smfr, can you look at the mac ifdef changes in nsObjectFrame and verify that
they are only for mac classic (i.e. unused now)? the two ones like:
-#if defined(XP_MAC) && !TARGET_CARBON

Comment 53

[Simon Fraser [no longer active]]

Comment on attachment 195045 [details] [diff] [review]
final (?) patch

Yes, it's fine to remove -#if defined(XP_MAC) && !TARGET_CARBON

Comment 54

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: merged to trunk

merged to trunk, especially (but not only) the bug 11011 changes.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196802 [details] [diff] [review]
merged to trunk

>Index: content/base/public/nsIObjectLoadingContent.idl

>+   * Gets the type of the content that's currently loaded. See
>+   * nsObjectLoadingContent::ObjectType for the list of possible values.

Is there any way we can put the constants on the interface?  And maybe define
the nsObjectLoadingContent::ObjectType enum in terms of those values (but use
the enum in the C++)?  I'm really not happy pointing from an interface to an
implementation for information...

>Index: content/base/src/nsImageLoadingContent.cpp
>+nsImageLoadingContent::ImageURIChanged(nsIURI* aNewURI,

>+  // First, get a document (needed for security checks and the like)
>+  if (!aDocument) {

Also, how about adding an assert right before this like so:

  NS_ASSERTION(!aDocument || aDocument == GetOurDocument(),
	       "Bogus document passed in");

>Index: content/base/src/nsObjectLoadingContent.cpp

>+nsAsyncInstantiateEvent::HandleEvent(PLEvent* event)
>+  // Make sure that we still have a frame

"have the right frame", rather.

>+class AutoNotifier {
>+     * Send notifications now, ignoring the value of mNotify. The new type and
>+    void Notify() {

I think we shouldn't ever call this method when mNotify is false (and I'd like
to see an assert to that effect here.

Alternately, we can make this method follow mNotify and be a no-op when it's
false (and then not have to put aNotify checks in callers).  Either way.  In
any case, this method should not be changing mNotify.

>+/**
>+ * A class that will automatically fall back if a |rv| variable has a failure
>+ * code when this class is destructed. It does not notify.

s/destructed/destroyed/

>+class AutoFallback {
>+    AutoFallback(nsObjectLoadingContent* aContent, const nsresult& rv)

Could we make that a |const nsresult* rv| and same for our member?  I think
that makes it clearer that we're basing our decision on what the value at that
location is when we go away.

>+nsObjectLoadingContent::ObjectState() const
>+      return 0; // XXX need a real state?

No need for this XXX comment, imo.  If we ever add some sort of LOADING state,
we'll deal then.

>+nsObjectLoadingContent::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
>+  AutoNotifier notifier(this, PR_TRUE);

I think you want an AutoFallback right after this... just to deal with the
various early returns in this method.

>+    case eType_Document: {
>+        if (!thisContent->GetCurrentDoc()) {

Please use IsInDoc() here instead of GetCurrentDoc().

>+        // XXX We must call this before getting the docshell to work around
>+        // bug 300540; when that's fixed, this if statement can be removed.

Make sure to file a followup bug on removing this, depending on bug 300540?

>+    case eType_Plugin:
>+        // This can go away once plugin loading moves to content (bug 90268)

Again, file a followup bug on removing this.

>+nsObjectLoadingContent::GetInterface(const nsIID & aIID, void **aResult)

Should we just call QueryInterface here?  Or do we really want to only expose
nsIChannelEventSink?

Either way is ok with me, I guess....

>+nsObjectLoadingContent::ObjectURIChanged(const nsAString& aURI,
>+  mUserDisabled = mSuppressed = PR_FALSE;

I don't think that line is needed... We'll either end up calling Fallback() or
calling ObjectURIChanged(nsIURI), both of which do this.

>+nsObjectLoadingContent::ObjectURIChanged(nsIURI* aURI,

>+  nsCOMPtr<nsIContent> thisContent = 
>+    do_QueryInterface(NS_STATIC_CAST(nsIImageLoadingContent*, this));
>+  NS_ASSERTION(thisContent, "must be a content");
>+
>+  nsIDocument* doc = GetOurDocument();

Just use thisContent->GetOwnerDoc() here.  That's all GetOurDocument does, and
if you already need thisContent, might as well use it.

>+    nsresult rv = secMan->CheckLoadURIWithPrincipal(doc->GetPrincipal(), aURI, 0);
>+    if (NS_FAILED(rv)) {
>+      mSuppressed = PR_TRUE; // XXX is this correct?

I'd not set mSuppressed here.  nsImageLoadingContent doesn't, in similar
circumstances....  mSuppressed is really for the "block XXX from server" case.

>+        if (shouldLoad == nsIContentPolicy::REJECT_REQUEST ||
>+            shouldLoad == nsIContentPolicy::REJECT_TYPE ||
>+            shouldLoad == nsIContentPolicy::REJECT_OTHER) {
>+          mUserDisabled = PR_TRUE;

mUserDisabled should really only happen for REJECT_TYPE, imo...

>+      Fallback(PR_FALSE);

That'll call UnloadContent which will clear out mUserDisabled and
mSuppressed...	That doesn't seem desirable in this case.

>+  // This fallback variable MUST be declared after the notifier variable. Do NOT
>+  // move it before it!

How about "Do NOT change the order of the declarations"?  As written now, it's
not clear which "it" is what...

>+      if (!mFrameLoader && newType == eType_Document) {
>+        if (!thisContent->GetCurrentDoc()) {

IsInDoc(), please.  And null out mURI here.

>+      // Must notify here for plugins

Only if aNotify is true, though.

>+      case eType_Image:
>+        rv = ImageURIChanged(aURI, PR_FALSE, PR_FALSE);

Document that we're passing aNotify == PR_FALSE here because we plan to notify
ourselves?  That said, shouldn't we pass aForceLoad for the relevant arg
instead of PR_FALSE?

>+  // If the class ID specifies a supported plugin, or if we have no explicit URI
>+  // but a type, immediately instantiate the plugin.
>+  nsAutoString classid;
>+  rv = thisContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::classid, classid);
This looks like an <embed> with no type set will get the classid treatment.  It
really shouldn't.  Please fix that.

>+    // No URI, but we have a type. The plugin will handle the load.
>+    // Or: supported class id, plugin will handle the load.
>+    mType = eType_Plugin;
>+    notifier.Notify();

Only notify if aNotify is true.

>+      rv = frame->Instantiate(aTypeHint.get(), aURI);

Should probably pass in typeForID if we had a useful classid here...  Doesn't
matter much given the nsObjectFrame code, I guess.  Followup bug?

>+  if (!CanHandleURI(aURI)) {
>+    // E.g. mms://
>+    mType = eType_Plugin;
>+    notifier.Notify();

Only if aNotify is true.

>+  rv = chan->AsyncOpen(this, nsnull);
>+  if (NS_SUCCEEDED(rv)) {
>+  } else {
>+    Fallback(PR_FALSE);

Why bother?  You have an AutoFallback watching rv....

>+nsObjectLoadingContent::RemovedFromDocument()
>+  if (mFrameLoader) {

Should null out mURI in this case too, so we'll redo the load when we get
reinserted.

>+nsObjectLoadingContent::NotifyTypeChanged(ObjectType aOldType,

This should really be NotifyStateChanged or something... ("state" could include
the type, but passing a state to something called NotifyTypeChanged seems odd.

>+  // Create the appropriate frame for this type

Remove that comment, please?  It doesn't make much sense here.

>+  if (newState != aOldState) {
>+    PRInt32 changedBits = aOldState ^ newState;

Could also compute changedBits first and just test that it's nonzero... either
way, I guess.

>+nsIObjectFrame*
>+nsObjectLoadingContent::GetFrame()
>+    return NULL; // No current doc -> no frame

nsnull, please.  Same elsewhere in this method.

>+nsObjectLoadingContent::ShouldShowDefaultPlugin(nsIContent* aContent)
>+  // Search for a child <param> with a pluginurl name

You also need to check for a pluginurl attr, I think.  The pluginhost code
does.

>+      nsAutoString name;
>+      nsresult rv = child->GetAttr(kNameSpaceID_None, nsHTMLAtoms::name, name);
>+      if (rv == NS_CONTENT_ATTR_HAS_VALUE &&
>+          name.LowerCaseEqualsLiteral("pluginurl")) {
>+        return PR_TRUE;

How about replacing all that with:

  if (child->AttrValueIs(kNameSpaceID_None, nsHTMLAtoms::name,
			 NS_LITERAL_STRING("pluginurl"), eIgnoreCase)) {
    return PR_TRUE;
  }

>Index: content/base/src/nsObjectLoadingContent.h

>+ * INVARIANTS OF THIS CLASS
>+ * - WRITEME

Take that WRITEME out?	Or just write them?  ;)

>+     * Object state. This is a bitmask consisting of NS_EVENT_STATE_BROKEN,
>+     * NS_EVENT_STATE_USERDISABLED and NS_EVENT_STATE_SUPPRESSED

"consisting of a some subset of", maybe?

>+    nsresult ObjectURIChanged(const nsAString& aURI,
>+                              PRBool aNotify,
>+                              const nsCString& aTypeHint = EmptyCString(),
>+                              PRBool aForceType = PR_FALSE,
>+                              PRBool aForceLoad = PR_FALSE);

I think I'd really prefer we didn't use default values here, unless it makes
callers a _lot_ more readable...

>+     * The URI can be null. The type can be null. Both can be null.

How about:  "The URI and type can both be null; if the URI is null a plugin
will be instantiated in the hope that there is a <param> with a useful URI
somewhere around"?

>+     * - The classid attribute, if any.

But only for <object>....

>+     * If no class ID is present and aForceType is true, the plugin given by
>+     * aTypeHint will be instantiated for this content.

Won't that break handling of SVG via <embed>?  In that case we want to create a
document, not a plugin....  Also, document that if aTypeHint is empty then
aForceType is ignored?	Should fix that.

>+     * If the URI is null or not supported, and a type hint is given, the plugin
>+     * corresponding to that type is instantiated. This is also the case if
>+     * |this| is an embed tag.

I'm not sure what the part about embed means here...

>+     * Otherwise a request to that URI is made and its type is used to find a
>+     * suitable plugin.

s/plugin/handler/?  Or something?  And s/its type/type returned by the server/
?
>+    nsresult ObjectURIChanged(nsIURI* aURI,
>+                              PRBool aNotify,
>+                              const nsCString& aTypeHint = EmptyCString(),
>+                              PRBool aForceType = PR_FALSE,
>+                              PRBool aForceLoad = PR_FALSE);

Again, I would prefer to not use default values.

>+    void Fallback(PRBool aNotify = PR_TRUE);

So... you explicitly pass PR_FALSE for aNotify to fallback in all but one case.
 In that one case you pass the aNotify arg of ObjectURIChanged.  Given that,
how about making that arg not have a default value?  Or even removing it
altogether and manually notifying in the one case where we might, and removing
notification stuff from Fallback()?

>+    PRBool                      mInstantiating : 1;
>+    PRBool                      mUserDisabled : 1;
>+    PRBool                      mSuppressed : 1;

Just use PRPackedBool (without the :1) for those three.

>Index: content/html/content/src/nsHTMLAppletElement.cpp

You really need to override both SetAttr signatures to prevent compiler
warnings and such (and just have one call the other as it does in
nsGenericElement).  I mentioned this in my first review...

> nsHTMLAppletElement::DoneAddingChildren()
>+    StartAppletLoad(PR_TRUE);

Hmm... This is suboptimal, since the common case is that we have no frame at
this point and never have.  How about StartAppletLoad(MayHaveFrame()) instead?

>+nsHTMLAppletElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,

Arguably, we should handle changes to the codebase attr too.  Followup bug,
please.

> nsHTMLAppletElement::IntrinsicState() const
>   PRInt32 state = nsGenericHTMLElement::IntrinsicState();

Why bother with the |state| local?  Just:

return nsGenericHTMLElement::IntrinsicState() | ObjectState();

>Index: content/html/content/src/nsHTMLObjectElement.cpp

Again, override both SetAttrs.

> nsHTMLObjectElement::DoneAddingChildren()
>+    StartObjectLoad(PR_TRUE);

Again, MayHaveFrame().

And maybe file a followup bug on passing a boolean indicating whether we've
notified on the piece of content yet to DoneAddingChildren() ?

> nsHTMLObjectElement::IntrinsicState() const

Same comment as for <applet>.

>Index: content/html/content/src/nsHTMLSharedElement.cpp

Again, override both SetAttrs

> nsHTMLSharedElement::nsHTMLSharedElement(nsINodeInfo *aNodeInfo)
>+  : nsGenericHTMLElement(aNodeInfo), nsObjectLoadingContent(eType_Plugin)

Again, for non-<embed>, should we use Null?  Or does it just not matter?

>+nsHTMLSharedElement::BindToTree(nsIDocument* aDocument,
>+    if (rv == NS_CONTENT_ATTR_HAS_VALUE) {

You probably want rv != NS_CONTENT_ATTR_NOT_THERE.

Now that you've removed the silly property stuff I added for bug 11011, you can
probably remove the #includes I added to object/applet/sharedelement in it,
too.

>Index: content/html/document/src/nsPluginDocument.cpp
>+  // This not start the load, because at this point there is no presshell yet.
"This will not start the load because nsObjectLoadingContent checks whether its
document is an nsIPluginDocument", rather?

>Index: layout/base/nsCSSFrameConstructor.cpp
>@@ -5404,13 +5405,30 @@ nsCSSFrameConstructor::ConstructHTMLFram
>+      NS_ASSERTION(objContent,
>+                   "applet, embed and object must implement nsIObjectLoadingContent!");

xbl:extends can actually give us some sort of random XML here, actually... I'd
throw if objContent is null (with the assert in place as it is to catch people
doing stupid things).

>+      objContent->GetDisplayedType(NS_REINTERPRET_CAST(PRUint32*, &type));

If we did define the constants on the interface, we wouldn't need the cast
here.
>Index: layout/generic/nsObjectFrame.cpp
>+  // XXX having to do this sucks. it'd be better to move the code from DidReflow
>+  // to FixupWindow.

File a followup on that?

>Index: modules/plugin/base/public/npupp.h

Don't check this part in!

>Index: modules/plugin/base/src/nsPluginsDirUnix.cpp

Probably also don't check this in.

This looks wonderful!  I'd like to see another patch with these nits addressed
before you land, but then we should land this and start filing all these
followup bugs.	;)

Comment 56

[Christian :Biesinger (don't email me, ping me on IRC)]

>Is there any way we can put the constants on the interface?  And maybe define
>the nsObjectLoadingContent::ObjectType enum in terms of those values

mmm, yeah, that works.

>No need for this XXX comment, imo.  If we ever add some sort of LOADING state,
>we'll deal then.

I was thinking we might need it to show the standby attribute while the object
is loading. but sure... I'll remove it in the meantime, you already asked me to
file a followup bug for standy :)

>>+        // This can go away once plugin loading moves to content (bug 90268)
>Again, file a followup bug on removing this.

Actually I intended to have that done in bug 90268, because to fix that bug, the
code here needs changes anyway.

> Or do we really want to only expose nsIChannelEventSink?

I only want to expose nsIChannelEventSink....

>That said, shouldn't we pass aForceLoad for the relevant arg
>instead of PR_FALSE?

Hm... maybe we should pass aForceLoad? I'll go for that unless you have a reason
against that..

> nsnull, please.  Same elsewhere in this method.

oops :) I think I was also writing some non-mozilla code at the time...

>You also need to check for a pluginurl attr, I think.  The pluginhost code
>does.

as best as I can tell, it only looks for a <param>. compare
http://biesi.damowmow.com/object/033.html last element. (plus, iirc pluginhost
uses GetParameter, and that only checks <param> values)

>How about replacing all that with:
>  if (child->AttrValueIs(kNameSpaceID_None, nsHTMLAtoms::name,

that's newer than my patch :-)

> Take that WRITEME out?	Or just write them?  ;)

well, I wasn't sure whether the list was complete. but ok, will take it out.

>I think I'd really prefer we didn't use default values here, unless it makes
>callers a _lot_ more readable...

Hm... I did that largely so that callers didn't have to pass all the booleans.

>Also, document that if aTypeHint is empty then
>aForceType is ignored?	Should fix that.

You think we should fallback in that case? Hm... that probably makes sense.
Would require a way to differentiate between "no type attr specified" and "empty
type specified" though.

> I'm not sure what the part about embed means here...

hm, not sure either... I'll remove that sentence.

>>+    void Fallback(PRBool aNotify = PR_TRUE);
>So... you explicitly pass PR_FALSE for aNotify to fallback in all but one case.

I provided the default argument for use by subclasses, similar to the
CancelImageRequests function of nsImageLoadingContent. But maybe I should remove
the default argument.

> Just use PRPackedBool (without the :1) for those three.

Why? That takes three times the space...

>You really need to override both SetAttr signatures to prevent compiler
>warnings and such 

I know, I replied to it saying that I didn't get those warnings. Is it not GCC
who warns for that case? (Is "using" portable?)

> Again, for non-<embed>, should we use Null?  Or does it just not matter?

I guess I can make it null. I didn't think it was worth the code. this class
doesn't even QI to nsIObjectLoadingContent for non-embed.

>"This will not start the load because nsObjectLoadingContent checks whether its
>document is an nsIPluginDocument", rather?

I suppose so, now that we always go through the async codepath for the initial
<embed> load.

>If we did define the constants on the interface, we wouldn't need the cast
>here.

leaving it in would make it easier for someone who tries to debug this code
though, because he sees the symbolic name... I suppose they can always view
constants in the debugger.

>>Index: modules/plugin/base/src/nsPluginsDirUnix.cpp
>Probably also don't check this in.

just an indentation fix... can't hurt :)

Comment 57

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: with those comments

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196927 [details] [diff] [review]
with those comments

r+sr=bzbarsky with the minor nits I mentioned on irc

Comment 59

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch for checkin

Comment 60

[Christian :Biesinger (don't email me, ping me on IRC)]

Checked in to trunk, and filed followup bugs:
Bug 309521 fix pluginfinder
Bug 309523 sort out plugin instantiation via extensions
Bug 309524 call shouldProcess
Bug 309525 stream converters
bug 309526 fallback if we already tried to instantiate the plugin
bug 14088  (was already filed) support the standby attribute
Bug 309528 fallback for <embed>/<applet>
Bug 309529 move code from nsObjectFrame::InstantiatePlugin to 
           nsObjectLoadingContent and figure out the baseURI stuff in that 
           function
Bug 309531 remove some frame recreation from nsObjectLoadingContent
Bug 309533 dynamic changes of the codebase attribute
Bug 309534 boolean argument to DoneAddingChildren
Bug 309536 move code from DidReflow to FixupWindow

Comment 61

[Bill Gianopoulos [:WG9s]]

This may have caused bug 309706.

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch left nsObjectFrame::GetMIMEType completely unused.

Comment 63

[Nickolay_Ponomarev]

This may have caused bug 312256.

Comment 64

[Robert Claypool]

If there are open bugs that block this bug, shouldn't it be reopened?

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No.

Comment 66

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 317609 has been marked as a duplicate of this bug. ***

Comment 67

[Christian :Biesinger (don't email me, ping me on IRC)]

as this is fixed on trunk, this is fixed for 1.9a1, so it need not block it anymore. clearing request.

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like this patch merged a little oddly with bug 269125 (a new check should probably have had FireEvent added).  I plan to fix that on bug 321059.

Comment 69

[Christian :Biesinger (don't email me, ping me on IRC)]

dbaron: that second bug number looks wrong to me...

Comment 70

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

oops.  It's bug 321054.

Comment 71

[Eugene Savitsky]

[e-mail] says it caused bug 340262.

Comment 72

[Jeff Walden [:Waldo]]

Attachment: Reftests

This is all of biesi's tests, except for the ones enumerated in the diff to reftest.list; see there for reasons.

I'd like biesi's review of these to make sure the assertions the tests make are correct assertions (particularly for the two tests which failed when I ran them manually).  I'd like dbaron's review to double-check that the tests don't rely overly or underly much on default browser stylesheets (i.e., they're idiomatic).

I have not run these tests in the automated manner, because the high number of negative results I got the one time I tried suggests that there's something wrong/old with the machine where I ran them.  If you wish to try doing so, the five or so binary files added in the patch may be found at the URL from which the tests originally came.

(An aside: the file at http://biesi.damowmow.com/object/pass.svg has a CSS error: a font-size with no units.  The patch adds what seems to have been a 'px' omitted by Inkscape.)

Note that any of Hixie's tests which aren't redundant (I haven't looked) still need to be added to the test suite.

Comment 73

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 249007 [details] [diff] [review]
Reftests

- don't checkin the first hunk of this patch :)
- image-no-useful-extension.html (and the -with-type variant) doesn't test the same thing as my test case did, because I used HTTP and a .htaccess file to map .image to image/png. perhaps all of these should get run over both HTTP and file://...

-
+# XXX I don't know for sure that these next two tests are actually correct;

it turns out that they aren't :-)
probably use an <iframe> for svg-ref

r=biesi with those changes

Comment 74

[Jeff Walden [:Waldo]]

Attachment: Addresses comments

(In reply to comment #73)
> - don't checkin the first hunk of this patch :)

Removed.  (The tests were crashing on my test machine when I wrote it.)

> - image-no-useful-extension.html (and the -with-type variant) doesn't test the
> same thing as my test case did, because I used HTTP and a .htaccess file to
> map .image to image/png. perhaps all of these should get run over both HTTP
> and file://...

Yes, indeed.  We'll need the ability to run tests from the HTTP server; I'll file a bug for that and for doing these tests over HTTP as well.

> +# XXX I don't know for sure that these next two tests are actually correct;
> 
> it turns out that they aren't :-)
> probably use an <iframe> for svg-ref

Well, they're correct if we supported <img src="foo.svg">.  Replaced the <p><img></p> with an iframe as requested, but I haven't checked that the styling matches because I don't have a machine with the libraries to build SVG at the moment.  :-\  I'd appreciate if someone could test them (you'll need to download http://biesi.damowmow.com/object/pass.image and http://biesi.damowmow.com/object/pass.png to layout/reftests/object/data/ to do so).

Still looking for the sr to verify that the tests are stylistically correct and don't make invalid assumptions...

Comment 75

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 253681 [details] [diff] [review]
Addresses comments

It's possible that a few assumptions here could change at some point, but if they do we can always fix the tests.  My main concern is to avoid duplicating orthogonal assumptions across large numbers of tests.  So this looks fine, sr=dbaron.

Comment 76

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Worth noting that a bunch of these tests could share reference renderings.  It's OK as is, though.

Comment 77

[Jeff Walden [:Waldo]]

(In reply to comment #76)
> Worth noting that a bunch of these tests could share reference renderings. 

I figured it was better not to confuse things and make the correlation between a testcase and its reference version painfully obvious.  The cost to doing so is negligible in my opinion.  :-)

Comment 78

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reftest failures on Linux

All the tests passed on Windows, but on Linux I saw these two test failures.

(If you think they could be related to changes in my tree, let me know; I do have a bunch.)

Comment 79

[Jeff Walden [:Waldo]]

The first one is definitely a bug (one of the images is completely blank, probably the reference -- it would be nice if the failure images were labeled as expected/actual, reference/test, or something like that).  The only difference between the test and the reference is:

ref:
<p><iframe style="border: 0px" src="extra/pass.html"></iframe></p>

test:
<p><object data="extra/pass.html">FAIL</object></p>


The second one might be fallout from bug 18333, given that the difference is in the display of an XML error message (could of course be something else, but I doubt it).

Comment 80

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems possible that both failures could be related to things that aren't finished displaying by the time onload fires.

Comment 81

[Boris Zbarsky [:bzbarsky]]

How come onload is firing, then?

Should nsCanvasRenderingContext2D::DrawWindow flush layout on all child frames too perhaps?

Comment 82

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I tried a number of ways to get those tests to fail again, and I couldn't, so it's hard to be sure.

However, I filed bug 369146 on Boris's suggestion in comment 81.

Comment 83

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I did get one additional occurrence of the failure on the malformed-xml.html test with the patch in bug 369146 -- on the test, the scrollbars had no sliders, and in the reference the sliders were properly drawn.

Comment 84

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The test object/image-no-useful-extension-typesniff.html fails on mac (it shows "FAIL").  Is there a known bug on this, is it supposed to fail, or should a bug be filed?

Comment 85

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is it because .image is a meaningful extension on the Mac?  (Apple DiskCopy Image)

Comment 86

[Boris Zbarsky [:bzbarsky]]

If you're loading from a file:// URL, then that would do it, yes.

Comment 87

[Jeff Walden [:Waldo]]

I guess we change the name, then.  If no one else finds time, I'll try to get to this tomorrow night.

Also on the subject of names, malformed-xml.html is failing on linref test (and probably others) now:

http://web.mit.edu/jwalden/Public/malformed-1.png
http://web.mit.edu/jwalden/Public/malformed-2.png

It seems likely this is because the length of the filename affects the size of the scrollbars on the test.  I don't think there's a way to work around this in general for every possible default font size and UI scrollbar size, but in the typical case I think we can address this using absolute positioning and clip.  (My first instinct was to use overflow: hidden, but it's not specified to apply to replaced block elements -- it's possible we might have a CSS extension that does, but I don't know about it.)  I'll try to get a patch for this tomorrow.

Comment 88

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Those images are showing that one of the two doesn't have scrollbar sliders at all -- which suggests some sort of flushing problem.  I saw that a few times as well.  It's possible bug 369319 would help.

Comment 89

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I renamed pass.image to pass_image .

Comment 91

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Word on the street is that this bug's fix caused bug 421217.