Closed Bug 1156 Opened 26 years ago Closed 19 years ago

OBJECTs without type attribute aren't rendered

Categories

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

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: Antti.Nayha, Assigned: Biesinger)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [A2])

Attachments

(3 files, 15 obsolete files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Waldo
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(deleted), text/html; charset=UTF-8
Details
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.
Assignee: michaelp → amusil
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 26 years ago
Resolution: --- → DUPLICATE
*** This bug has been marked as a duplicate of 678 ***
Status: RESOLVED → VERIFIED
Marked verified as duplicate; reporter if you disagree please reopen.
This is not a dup of 678. And it's still buggy, 7 years later...
Status: VERIFIED → REOPENED
QA Contact: ian
Resolution: DUPLICATE → ---
Whiteboard: [A2]
Assignee: amusil → nobody
Status: REOPENED → NEW
OS: Windows NT → All
Priority: P2 → --
QA Contact: ian → plugins
Hardware: PC → All
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.
Blocks: acid2
... 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.
Assignee: nobody → cbiesinger
Target Milestone: --- → mozilla1.9alpha
Attached patch patch v0.5 (obsolete) (deleted) — Splinter Review
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.
Attached patch patch v0.5, merged to trunk (obsolete) (deleted) — Splinter Review
Attachment #188380 - Attachment is obsolete: true
> - 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.
Attachment #188490 - Attachment is obsolete: true
Attached patch patch v0.51 (obsolete) (deleted) — Splinter Review
- fix <embed> for SVG (i.e. fix the crash) - fix linux compilation
Attachment #188599 - Attachment is obsolete: true
Attached patch patch v0.52 (obsolete) (deleted) — Splinter Review
- 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
Attachment #189073 - Attachment is obsolete: true
more problems with the patch: - alternate content for <embed> doesn't work - something seems wrong with painting of <embed> plugins
Blocks: 300659
Attached patch patch v0.6 (obsolete) (deleted) — Splinter Review
- 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) :-/)
Attachment #189122 - Attachment is obsolete: true
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.
oops, actually, the pluginurl check for <object> is wrong - it should look for a <param>, not an attribute...
Attached patch patch v0.65 (obsolete) (deleted) — Splinter Review
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)
Attachment #189365 - Attachment is obsolete: true
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?
zero, way too much risk.
Attached patch patch v0.7 (obsolete) (deleted) — Splinter Review
fix nullplugin for <object> with a pluginurl param I think this is ready for review now.
Attachment #189528 - Attachment is obsolete: true
Attachment #189553 - Flags: review?(bzbarsky)
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.
Attachment #189553 - Flags: review?(bzbarsky)
Attached patch patch v0.8 (obsolete) (deleted) — Splinter Review
Attachment #189553 - Attachment is obsolete: true
Attachment #189777 - Flags: review?(bzbarsky)
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.
Blocks: 303816
Blocks: 265532
Blocks: 192891
Blocks: 305441
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.
> 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. ;)
>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...
>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)
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.
> 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?
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.
> 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?
> But then it gets BindToTree called, no? hmm, yeah, probably... need to look at this more (note to self)
>>+ 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.
> 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...
>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?
Attachment #189777 - Flags: review?(bzbarsky) → review-
>>+ // 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.
> 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?
Attached patch intermediate patch (obsolete) (deleted) — Splinter Review
merged to trunk and many of those changes made; but more to be done. attaching mainly for backup purposes. (this does observe redirects)
Attachment #189777 - Attachment is obsolete: true
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.
>>+ 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.
Attached patch intermediate patch 2 (obsolete) (deleted) — Splinter Review
Attachment #194240 - Attachment is obsolete: true
note to self: file a followup bug to investigate the baseURI business in nsObjectFrame::Instantiate
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.
nsPluginDocument works because when BindToTree is called, there is no presshell yet.
(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)
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.
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.
(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
> nasty warnings about hiding superclass methods... I don't get these warnings, actually...
> nsPluginDocument works because when BindToTree is called, there is no presshell > yet. Please comment this clearly in the patch?
Attached patch final (?) patch (obsolete) (deleted) — Splinter Review
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)
Attachment #194955 - Attachment is obsolete: true
Attachment #195045 - Flags: superreview?(bzbarsky)
Attachment #195045 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Priority: -- → P1
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
Attachment #195045 - Flags: review?(sfraser_bugs)
Comment on attachment 195045 [details] [diff] [review] final (?) patch Yes, it's fine to remove -#if defined(XP_MAC) && !TARGET_CARBON
Attachment #195045 - Flags: review?(sfraser_bugs) → review+
Blocks: 309065
Blocks: 309118
Blocks: 309237
Blocks: 71473
Attachment #195045 - Flags: superreview?(bzbarsky)
Attachment #195045 - Flags: review?(bzbarsky)
Attached patch merged to trunk (obsolete) (deleted) — Splinter Review
merged to trunk, especially (but not only) the bug 11011 changes.
Attachment #195045 - Attachment is obsolete: true
Attachment #196802 - Flags: superreview?(bzbarsky)
Attachment #196802 - Flags: review?(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. ;)
Attachment #196802 - Flags: superreview?(bzbarsky)
Attachment #196802 - Flags: superreview+
Attachment #196802 - Flags: review?(bzbarsky)
Attachment #196802 - Flags: review+
>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 :)
Attached patch with those comments (obsolete) (deleted) — Splinter Review
Attachment #196802 - Attachment is obsolete: true
Attachment #196927 - Flags: superreview?(bzbarsky)
Attachment #196927 - Flags: review?(bzbarsky)
Comment on attachment 196927 [details] [diff] [review] with those comments r+sr=bzbarsky with the minor nits I mentioned on irc
Attachment #196927 - Flags: superreview?(bzbarsky)
Attachment #196927 - Flags: superreview+
Attachment #196927 - Flags: review?(bzbarsky)
Attachment #196927 - Flags: review+
Attached patch patch for checkin (deleted) — Splinter Review
Attachment #196927 - Attachment is obsolete: true
Blocks: 287124
Blocks: 309532
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
Status: ASSIGNED → RESOLVED
Closed: 26 years ago19 years ago
Resolution: --- → FIXED
This may have caused bug 309706.
Depends on: 310515
This patch left nsObjectFrame::GetMIMEType completely unused.
Depends on: 311668
No longer blocks: 306417
Depends on: 306417
Depends on: 311674
This may have caused bug 312256.
Depends on: 312256
Depends on: 312685
If there are open bugs that block this bug, shouldn't it be reopened?
No.
Depends on: 317179
Blocks: 317189
*** Bug 317609 has been marked as a duplicate of this bug. ***
Flags: blocking1.9a1?
as this is fixed on trunk, this is fixed for 1.9a1, so it need not block it anymore. clearing request.
Flags: blocking1.9a1?
Blocks: 305564
Depends on: 322170
Depends on: 329768
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.
dbaron: that second bug number looks wrong to me...
oops. It's bug 321054.
Depends on: 347736
Depends on: 340262
Depends on: 354102
Depends on: 363379
Attached patch Reftests (obsolete) (deleted) — Splinter Review
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.
Attachment #249007 - Flags: superreview?(dbaron)
Attachment #249007 - Flags: review?(cbiesinger)
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
Attachment #249007 - Flags: review?(cbiesinger) → review+
Attached patch Addresses comments (deleted) — Splinter Review
(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...
Attachment #249007 - Attachment is obsolete: true
Attachment #253681 - Flags: superreview?(dbaron)
Attachment #253681 - Flags: review+
Attachment #249007 - Flags: superreview?(dbaron)
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.
Attachment #253681 - Flags: superreview?(dbaron) → superreview+
Worth noting that a bunch of these tests could share reference renderings. It's OK as is, though.
(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. :-)
Attached file reftest failures on Linux (deleted) —
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.)
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).
It seems possible that both failures could be related to things that aren't finished displaying by the time onload fires.
How come onload is firing, then? Should nsCanvasRenderingContext2D::DrawWindow flush layout on all child frames too perhaps?
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.
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.
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?
Is it because .image is a meaningful extension on the Mac? (Apple DiskCopy Image)
If you're loading from a file:// URL, then that would do it, yes.
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.
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.
I renamed pass.image to pass_image .
Depends on: 382378
Depends on: 386599
Depends on: 381512
Depends on: 389677
Depends on: 348813
No longer depends on: 348813
Depends on: 390891
Depends on: 393503
Depends on: 402320
Depends on: 402937
Depends on: 409025
Depends on: 416480
Depends on: 428075
Word on the street is that this bug's fix caused bug 421217.
Depends on: 421217
No longer depends on: 421217
No longer depends on: 428075
Depends on: 431280
Blocks: 563030
Blocks: 1038570
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: