Closed
Bug 1279218
Opened 8 years ago
Closed 7 years ago
Remove <applet> element
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: annevk, Assigned: qdot)
References
Details
(Keywords: dev-doc-complete, site-compat, Whiteboard: Blocked on removing Java support. Revisit after Firefox 52 ESR.)
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
benjamin
:
review+
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jld
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
benjamin
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
https://github.com/whatwg/html/pull/1399 has a good summary of what that entails. Chrome has removed support already and Edge is interested.
https://bugs.webkit.org/show_bug.cgi?id=157926 is the corresponding WebKit issue.
Comment 1•8 years ago
|
||
Benjamin, I think you're tracking this.
Flags: needinfo?(benjamin)
Whiteboard: btpp-followup-2016-07-05
Comment 2•8 years ago
|
||
Once we drop support for Java in FF52, I'm all about this. Before then I'm not willing to take any risks.
Depends on: npapi-eol
Flags: needinfo?(benjamin)
Priority: -- → P3
Whiteboard: btpp-followup-2016-07-05 → Blocked on removing Java support. Revisit for FF52.
Updated•8 years ago
|
Keywords: dev-doc-needed,
site-compat
Updated•8 years ago
|
Whiteboard: Blocked on removing Java support. Revisit for FF52. → Blocked on removing Java support. Revisit for FF52. [btpp-backlog]
Comment 3•8 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/applet-support-will-be-removed/
Comment 4•8 years ago
|
||
Please wait until 53 for this, because we're still supporting applet via a pref on 52 ESR.
Updated•8 years ago
|
Target Milestone: --- → mozilla53
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Comment 5•8 years ago
|
||
qdot, do you want to take it?
Flags: needinfo?(amarchesini) → needinfo?(kyle)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → kyle
Flags: needinfo?(kyle)
Updated•8 years ago
|
Whiteboard: Blocked on removing Java support. Revisit for FF52. [btpp-backlog] → Blocked on removing Java support. Revisit after Firefox 52 ESR.
Comment 6•8 years ago
|
||
Why is ESR relevant to mainline development?
Assignee | ||
Comment 7•8 years ago
|
||
We had to release the 52 ESR with applet/java support before starting on this bug.
Assignee | ||
Comment 10•8 years ago
|
||
Adding webplatform test cleanup to the landing criteria for this bug, since landing this is basically the trigger for html spec removal also. See https://github.com/whatwg/html/pull/1399#issuecomment-303875482
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/applet-support-has-been-dropped/
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8886392 [details]
Bug 1279218 - Remove Applet/Java support from nsObjectLoadingContent
https://reviewboard.mozilla.org/r/155862/#review162908
r=me either way but please file followups for additional cleanup if appropriate.
::: dom/base/nsObjectLoadingContent.h:304
(Diff revision 1)
> eSupportImages = 1u << 0, // Images are supported (imgILoader)
> eSupportPlugins = 1u << 1, // Plugins are supported (nsIPluginHost)
> eSupportDocuments = 1u << 2, // Documents are supported
> // (nsIDocumentLoaderFactory)
> // This flag always includes SVG
> - eSupportClassID = 1u << 3, // The classid attribute is supported
> + eSupportClassID = 1u << 3, // The classid attribute is supported. No
Will there be a followup to remove eSupportClassID completely?
::: dom/base/nsObjectLoadingContent.h:424
(Diff revision 1)
> * - mOriginalURI : The src or data attribute on the element
> * - mURI : The final URI, considering mChannel if
> * mChannelLoaded is set
> * - mContentType : The final content type, considering mChannel if
> * mChannelLoaded is set
> * - mBaseURI : The object's base URI, which may be set by the
Is `codebase` the only way to get a custom base URI for this? If so do we still need `mBaseURI` at all?
Attachment #8886392 -
Flags: review?(benjamin) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8886394 [details]
Bug 1279218 - Remove tests related to the applet tag;
https://reviewboard.mozilla.org/r/155866/#review162914
Does it make sense to/can you remove all of testplugin/javaplugin as part of this?
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8886393 [details]
Bug 1279218 - Remove Applet element support from WebBrowserPersist;
https://reviewboard.mozilla.org/r/155864/#review163002
Attachment #8886393 -
Flags: review?(jld) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8886391 [details]
Bug 1279218 - Remove Applet tag;
https://reviewboard.mozilla.org/r/155860/#review163836
This needs changes to test_interfaces.html, I would think.
::: dom/bindings/Bindings.conf:405
(Diff revision 2)
> 'History': {
> 'headerFile': 'nsHistory.h',
> 'nativeType': 'nsHistory'
> },
>
> 'HTMLAppletElement': {
Why is this needed at all? We're removing this interface, aren't we? So what's the point of this descriptor?
::: dom/html/HTMLSharedObjectElement.h
(Diff revision 2)
>
> NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED_NO_UNLINK(HTMLSharedObjectElement,
> nsGenericHTMLElement)
>
> - // WebIDL API for <applet>
> + // WebIDL <embed> api
> - void GetAlign(DOMString& aValue)
Why are you losing the GetAlign implementation? Please don't.
::: dom/html/HTMLSharedObjectElement.h
(Diff revision 2)
> - // XPCOM GetCodebase is ok; note that it's a URI attribute
> - void SetCodeBase(const nsAString& aValue, ErrorResult& aRv)
> - {
> - SetHTMLAttr(nsGkAtoms::codebase, aValue, aRv);
> - }
> - void GetHeight(DOMString& aValue)
Please don't lose GetHeight either.
::: dom/html/HTMLSharedObjectElement.h
(Diff revision 2)
> - }
> - void SetHspace(uint32_t aValue, ErrorResult& aRv)
> - {
> - SetUnsignedIntAttr(nsGkAtoms::hspace, aValue, 0, aRv);
> - }
> - void GetName(DOMString& aValue)
Or GetName.
::: dom/html/HTMLSharedObjectElement.h
(Diff revision 2)
> - }
> - void SetVspace(uint32_t aValue, ErrorResult& aRv)
> - {
> - SetUnsignedIntAttr(nsGkAtoms::vspace, aValue, 0, aRv);
> - }
> - void GetWidth(DOMString& aValue)
Or GetWidth.
::: dom/html/HTMLSharedObjectElement.h
(Diff revision 2)
> {
> SetHTMLAttr(nsGkAtoms::width, aValue, aRv);
> }
> -
> - // WebIDL <embed> api
> - // XPCOM GetSrc is ok; note that it's a URI attribute
Please don't lose this comment.
::: dom/html/HTMLSharedObjectElement.h
(Diff revision 2)
> - // XPCOM GetSrc is ok; note that it's a URI attribute
> void SetSrc(const nsAString& aValue, ErrorResult& aRv)
> {
> SetHTMLAttr(nsGkAtoms::src, aValue, aRv);
> }
> - void GetType(DOMString& aValue)
And this function should stay too.
::: dom/html/HTMLSharedObjectElement.h:127
(Diff revision 2)
> bool aNotify) override;
>
> private:
> - virtual ~HTMLSharedObjectElement();
> + ~HTMLSharedObjectElement();
>
> nsIAtom *URIAttrName() const
Does this function still need to exist? It only has one consumer; that consumer could compare to nsGkAtoms::src directly.
::: dom/html/HTMLSharedObjectElement.cpp:93
(Diff revision 2)
> nsIImageLoadingContent,
> imgIOnloadBlocker,
> nsIChannelEventSink)
> NS_INTERFACE_TABLE_TO_MAP_SEGUE
> - NS_INTERFACE_MAP_ENTRY_IF_TAG(nsIDOMHTMLAppletElement, applet)
> NS_INTERFACE_MAP_ENTRY_IF_TAG(nsIDOMHTMLEmbedElement, embed)
Does this still need to be IF_TAG? I don't think it does.
::: dom/html/HTMLSharedObjectElement.cpp:401
(Diff revision 2)
> MOZ_ASSERT(mNodeInfo->Equals(nsGkAtoms::embed));
> return nsIContentPolicy::TYPE_INTERNAL_EMBED;
> }
> }
>
> +NS_IMPL_STRING_ATTR(HTMLSharedObjectElement, Align, align)
If we're touching this stuff anyway, might be better to remove it altogether (from here and nsIDOMHTMLEmbedElement.idl). Followup better for that.
Attachment #8886391 -
Flags: review?(bzbarsky) → review+
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8886391 [details]
Bug 1279218 - Remove Applet tag;
https://reviewboard.mozilla.org/r/155860/#review163842
::: dom/html/HTMLObjectElement.cpp:563
(Diff revision 2)
> }
>
> uint32_t
> HTMLObjectElement::GetCapabilities() const
> {
> - return nsObjectLoadingContent::GetCapabilities() | eSupportClassID;
> + return nsObjectLoadingContent::GetCapabilities();
Oh, shouldn't this be in part 2? Seems to me like it should be.
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8886392 [details]
Bug 1279218 - Remove Applet/Java support from nsObjectLoadingContent
https://reviewboard.mozilla.org/r/155862/#review163848
::: dom/base/nsObjectLoadingContent.cpp
(Diff revision 2)
> #include "nsIDocShellLoadInfo.h"
> #include "nsIDocument.h"
> #include "nsIDOMCustomEvent.h"
> #include "nsIDOMDocument.h"
> #include "nsIDOMHTMLObjectElement.h"
> -#include "nsIDOMHTMLAppletElement.h"
This should have been in part 1.
::: dom/base/nsObjectLoadingContent.cpp:892
(Diff revision 2)
> if (name.IsEmpty())
> continue;
>
> nsCOMPtr<nsIContent> parent = element->GetParent();
> nsCOMPtr<nsIDOMHTMLObjectElement> domObject;
> - nsCOMPtr<nsIDOMHTMLAppletElement> domApplet;
> + while (!domObject && parent) {
Followup bug to rewrite this in terms of FromContent?
::: dom/base/nsObjectLoadingContent.cpp
(Diff revision 2)
> - // Java expects a directory as the codebase, or else it will construct
> - // relative URIs incorrectly :(
> - codebaseStr.Assign('.');
> - }
> -
> - if (!codebaseStr.IsEmpty()) {
Hmm. So we used to consider the "codebase" attr on non-applet tags too here? I think we did... Is changing that ok?
::: dom/base/nsObjectLoadingContent.cpp
(Diff revision 2)
> - /// Check if the original (pre-channel) content-type or URI changed, and
> - /// record mOriginal{ContentType,URI}
Why is this comment being removed?
::: dom/plugins/base/nsPluginInstanceOwner.cpp
(Diff revision 2)
> #include "nsIPluginWidget.h"
> #include "nsViewManager.h"
> #include "nsIDocShellTreeOwner.h"
> #include "nsIDOMHTMLObjectElement.h"
> #include "nsIAppShell.h"
> -#include "nsIDOMHTMLAppletElement.h"
This sould have been in part 1, right?
::: dom/plugins/base/nsPluginInstanceOwner.cpp:1307
(Diff revision 2)
> NS_ENSURE_ARG_POINTER(result);
>
> *result = nsPluginTagType_Unknown;
>
> nsCOMPtr<nsIContent> content = do_QueryReferent(mContent);
> - if (content->IsHTMLElement(nsGkAtoms::applet))
> + if (content->IsHTMLElement(nsGkAtoms::embed))
This should have been in part 1, I would think.
Attachment #8886392 -
Flags: review?(bzbarsky) → review+
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8886394 [details]
Bug 1279218 - Remove tests related to the applet tag;
https://reviewboard.mozilla.org/r/155866/#review163854
Attachment #8886394 -
Flags: review?(bzbarsky) → review+
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8886395 [details]
Bug 1279218 - Additional applet tag logic removal;
https://reviewboard.mozilla.org/r/155868/#review163856
Should presumably be folded into part 1 before landing?
Attachment #8886395 -
Flags: review?(bzbarsky) → review+
Comment 31•7 years ago
|
||
OK, so changes that should presumably be happening (based on code search and <https://github.com/whatwg/html/pull/1399#issue-158960327>) that I'm not seeing:
1) Remove applet from nsGenericHTMLElement::ShouldExposeIdAsHTMLDocumentProperty.
2) Remove applet from nsGenericHTMLElement::CanHaveName.
3) Remove applet rom the isXULOrPluginElement bit in Element::GetBindingURL.
4) Remove applet from the special-check in EventStateManager::FireContextClick
5) Remove applet from IsAllNamedElement in HTMLAllCollection.cpp.
6) Changing document.applets to return an empty collection
7) Remove checks for applet in nsXMLContentSink::CloseElement and nsXMLContentSink::IsMonolithicContainer.
8) Remove check for applet in txMozillaXMLOutput::endElement
9) Remove dead checks for applet in nsPluginFrame::GetMinISize and nsPluginFrame::GetDesiredSize.
10) Remove special cases from HTML5 parser for applet (?) Not sure which things should go and which
should stay, here. I don't know whether the HTML5 parser has something like the "monolithic
container" bits from the XML parser above, for example.
11) Remove the DoneAddingChildren bit for applet in nsHtml5TreeBuilder::elementPopped
12) Rename HTMLSharedObjectElement to HTMLEmbedElement.
Flags: needinfo?(kyle)
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8887670 [details]
Bug 1279218 - Remove Java Test Plugin and rest of Java references;
https://reviewboard.mozilla.org/r/158552/#review164352
Attachment #8887670 -
Flags: review?(benjamin) → review+
Reporter | ||
Comment 33•7 years ago
|
||
Regarding point 10 in comment 31, the HTML standard continues to parse applet element start and end tags the same way as those of marquee and object. Anything that's not related to building a tree can go though (and was never in the standard to begin with).
Comment 34•7 years ago
|
||
Right, I'm not talking about changing the parsing model. I'm talking about gecko-internal special handling like preventing parser interrupts inside <applet>, assuming we do that right now.
Assignee | ||
Comment 35•7 years ago
|
||
:wchen, I'm not sure where parser interrupts happen for tags as mentioned in Comment 34, and the parsing for applet looks like it basically follows marquee right now. Do you know if there's else I need to remove in the parser?
Flags: needinfo?(kyle) → needinfo?(wchen)
Assignee | ||
Comment 36•7 years ago
|
||
:bz, what would you recommend for created the null content list in nsHTMLDocument::Applets()? We don't have an nsContentList constructor that can just produce a null list on its own at the moment, and I'm not seeing any straight-forward way to create an nsIHTMLCollection that's not /actually/ a collection of something.
Flags: needinfo?(bzbarsky)
Comment 37•7 years ago
|
||
The HTML5 parser doesn't have any special handing when we create an applet element, the only extra thing we do is call DoneAddingChildren on the element when it gets popped off the stack in tree builder.
Flags: needinfo?(wchen)
Comment 38•7 years ago
|
||
We don't really have a thing for the "nothing in it" content list right now.
You could either do nsContentList with a nsContentListMatchFunc that always returns false, or create a new small subclass of nsBaseContentList that inherits from nsIHTMLCollection, implements nsIDOMHTMLCollection, and returns null and 0 as needed from the various methods involved.
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8891478 [details]
Bug 1279218 - Rename HTMLSharedObjectElement to HTMLEmbedElement;
https://reviewboard.mozilla.org/r/162616/#review167954
It looks like this change just does a delete and add instead of "hg mv". Please use "hg mv", which will not only preserve history but make for a much more reviewable diff...
::: dom/base/nsObjectLoadingContent.cpp:2960
(Diff revision 1)
> if (child->IsHTMLElement(nsGkAtoms::embed)) {
> - HTMLSharedObjectElement* embed = static_cast<HTMLSharedObjectElement*>(child);
> + HTMLEmbedElement* embed = static_cast<HTMLEmbedElement*>(child);
This can become:
if (auto* embed = HTMLEmbedElement::FromContent(child)) {
::: dom/bindings/Bindings.conf:427
(Diff revision 1)
> 'HTMLEmbedElement': {
> - 'nativeType': 'mozilla::dom::HTMLSharedObjectElement'
> + 'nativeType': 'mozilla::dom::HTMLEmbedElement'
> },
This entire block can go away, since it now has the right name.
Attachment #8891478 -
Flags: review?(bzbarsky) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•7 years ago
|
||
Even though I pushed https://reviewboard.mozilla.org/r/155868/diff/4#index_header this morning, it's marked as r+ from a week ago, before the patch even existed. It hasn't actually received a review yet and still needs to be looked at.
Flags: needinfo?(bzbarsky)
Comment 50•7 years ago
|
||
mozreview-review |
Comment on attachment 8886395 [details]
Bug 1279218 - Additional applet tag logic removal;
https://reviewboard.mozilla.org/r/155868/#review167982
r- for the leak bit. The rest looks good.
::: dom/base/nsContentList.h:182
(Diff revision 4)
> +
> +protected:
> + virtual ~nsEmptyContentList() {}
> +private:
> + // This has to be a strong reference, the root might go away before the list.
> + nsCOMPtr<nsINode> mRoot;
This needs to be cycle-collected; otherwise it will leak. Note that nsSimpleContentList cycle-collects stuff.
::: editor/libeditor/HTMLEditUtils.cpp:598
(Diff revision 4)
> ELEM(abbr, true, true, GROUP_PHRASE, GROUP_INLINE_ELEMENT),
> ELEM(acronym, true, true, GROUP_PHRASE, GROUP_INLINE_ELEMENT),
> ELEM(address, true, true, GROUP_BLOCK,
> GROUP_INLINE_ELEMENT | GROUP_P),
> + // While applet is no longer a valid tag, removing it here breaks the editor
> + // in a lot of ways. Just leave it.
Something more specific than "a lot of ways" (e.g. which tests fail or something) would be nice here in case someone does want to take a stab at removing this.
::: layout/generic/nsPluginFrame.cpp:382
(Diff revision 4)
> nsPluginFrame::GetMinISize(gfxContext *aRenderingContext)
> {
> nscoord result = 0;
>
> if (!IsHidden(false)) {
> - if (mContent->IsAnyOfHTMLElements(nsGkAtoms::applet,
> + if (mContent->IsAnyOfHTMLElements(nsGkAtoms::embed)) {
IsHTMLElement(), not IsAnyOfHTMLElements(), since now we just have the one tag.
::: layout/generic/nsPluginFrame.cpp:444
(Diff revision 4)
>
> aMetrics.Width() = aReflowInput.ComputedWidth();
> aMetrics.Height() = aReflowInput.ComputedHeight();
>
> - // for EMBED and APPLET, default to 240x200 for compatibility
> - if (mContent->IsAnyOfHTMLElements(nsGkAtoms::applet,
> + // for EMBED, default to 240x200 for compatibility
> + if (mContent->IsAnyOfHTMLElements(nsGkAtoms::embed)) {
Again, IsHTMLElement.
::: netwerk/streamconv/converters/nsUnknownDecoder.cpp
(Diff revision 4)
> MATCHES_TAG("base") ||
> MATCHES_TAG("style") ||
> MATCHES_TAG("div") ||
> MATCHES_TAG("p") ||
> MATCHES_TAG("font") ||
> - MATCHES_TAG("applet") ||
I'm not sure we should change this part, honestly. This is a heuristic sniffer; it doesn't care whether the content is _valid_; it cares whether the sender wanted it treated as HTML.
It probably doesn't matter too much either way, but I'd prefer this part happen in a separate bug for regression-tracking purposes.
Attachment #8886395 -
Flags: review+ → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 53•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8886395 [details]
Bug 1279218 - Additional applet tag logic removal;
https://reviewboard.mozilla.org/r/155868/#review167982
> I'm not sure we should change this part, honestly. This is a heuristic sniffer; it doesn't care whether the content is _valid_; it cares whether the sender wanted it treated as HTML.
>
> It probably doesn't matter too much either way, but I'd prefer this part happen in a separate bug for regression-tracking purposes.
I just removed this change, instead of possibly causing a regression.
Comment hidden (mozreview-request) |
Comment 55•7 years ago
|
||
mozreview-review |
Comment on attachment 8886395 [details]
Bug 1279218 - Additional applet tag logic removal;
https://reviewboard.mozilla.org/r/155868/#review168064
Attachment #8886395 -
Flags: review?(bzbarsky) → review+
Comment 56•7 years ago
|
||
mozreview-review |
Comment on attachment 8891478 [details]
Bug 1279218 - Rename HTMLSharedObjectElement to HTMLEmbedElement;
https://reviewboard.mozilla.org/r/162616/#review168066
r=me with the nits below. Thank you; this diff was much easier to see changes in!
::: dom/base/nsObjectLoadingContent.cpp:2961
(Diff revision 4)
> nsStyleUtil::IsSignificantChild(child, true, false)) {
> aType = eFallbackAlternate;
> }
> if (thisIsObject) {
> - if (child->IsHTMLElement(nsGkAtoms::embed)) {
> - HTMLSharedObjectElement* embed = static_cast<HTMLSharedObjectElement*>(child);
> + if (auto embed = HTMLEmbedElement::FromContent(child)) {
> + static_cast<HTMLEmbedElement*>(embed)->StartObjectLoad(true, true);
You don't need the static_cast. "embed" is already HTMLEmbedElement*. Just do:
embed->StartObjectLoad(true, true);
::: dom/bindings/BindingUtils.cpp:2312
(Diff revision 4)
>
> JS::Rooted<JSObject*> maybeObjLC(aCx, aObj);
> nsObjectLoadingContent* htmlobject;
> nsresult rv = UNWRAP_OBJECT(HTMLObjectElement, &maybeObjLC, htmlobject);
> if (NS_FAILED(rv)) {
> rv = UnwrapObject<prototypes::id::HTMLEmbedElement,
This can just become:
rv = UNWRAP_OBJECT(HTMLEmbedElement, &maybeObjLC, htmlobject);
since now the interface name and class name match, so you don't have to specify them separately.
::: dom/html/HTMLEmbedElement.cpp:26
(Diff revision 4)
> #include "mozilla/dom/Event.h"
> #endif
> #include "mozilla/dom/HTMLObjectElement.h"
>
>
> -NS_IMPL_NS_NEW_HTML_ELEMENT_CHECK_PARSER(SharedObject)
> +NS_IMPL_NS_NEW_HTML_ELEMENT_CHECK_PARSER(Embed)
This can now be NS_IMPL_NS_NEW_HTML_ELEMENT(Embed), since you no longer need aFromParser in the constructor.
::: dom/html/HTMLEmbedElement.cpp:31
(Diff revision 4)
> -NS_IMPL_NS_NEW_HTML_ELEMENT_CHECK_PARSER(SharedObject)
> +NS_IMPL_NS_NEW_HTML_ELEMENT_CHECK_PARSER(Embed)
>
> namespace mozilla {
> namespace dom {
>
> -HTMLSharedObjectElement::HTMLSharedObjectElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo,
> +HTMLEmbedElement::HTMLEmbedElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo,
And the constructor can drop the aFromParser arg.
::: dom/html/HTMLEmbedElement.cpp:198
(Diff revision 4)
> -HTMLSharedObjectElement::IsHTMLFocusable(bool aWithMouse,
> +HTMLEmbedElement::IsHTMLFocusable(bool aWithMouse,
> bool *aIsFocusable,
> int32_t *aTabIndex)
> {
> - if (mNodeInfo->Equals(nsGkAtoms::embed) || Type() == eType_Plugin) {
> // Has plugin content: let the plugin decide what to do in terms of
This needs to actually be outdented, right? Same for the rest of this function, up to the '}'.
::: dom/html/HTMLEmbedElement.cpp
(Diff revision 4)
> - if (mNodeInfo->Equals(nsGkAtoms::embed)) {
> return &MapAttributesIntoRuleExceptHidden;
> }
Needs to be outdented.
::: dom/tests/mochitest/bugs/test_bug396843.html:20
(Diff revision 4)
> </p>
> <div id="content" style="display: none">
>
> </div>
> <pre id="test">
> -<script class="testbody" type="text/javascript">
> + <script class="testbody" type="text/javascript">
Not sure why all the indentation changes in this file...
Attachment #8891478 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 57•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8891478 [details]
Bug 1279218 - Rename HTMLSharedObjectElement to HTMLEmbedElement;
https://reviewboard.mozilla.org/r/162616/#review168066
> You don't need the static_cast. "embed" is already HTMLEmbedElement*. Just do:
>
> embed->StartObjectLoad(true, true);
For some reason I do seem to need to cast it, otherwise this happens in clang:
/share/code/mozbuild/mc-hg/dom/base/nsObjectLoadingContent.cpp:2961:18: error: no member named 'StartObjectLoad' in 'nsGenericHTMLElement'
embed->StartObjectLoad(true, true);
~~~~~ ^
> And the constructor can drop the aFromParser arg.
What about the call to SetIsNetworkCreated in the constructor? I see that NS_IMPL_NS_NEW_HTML_ELEMENT just drops the boolean, do we assume it to always or never be from the parser?
Comment 58•7 years ago
|
||
> /share/code/mozbuild/mc-hg/dom/base/nsObjectLoadingContent.cpp:2961:18: error: no member named 'StartObjectLoad' in 'nsGenericHTMLElement'
Oh. That's because HTMLEmbedElement doesn't actually declare FromContent, so you're getting the nsGenericHTMLElement one. And then the static_cast is NOT safe: you could have any HTML element!
You want to throw:
NS_IMPL_FROMCONTENT_HTML_WITH_TAG(HTMLEmbedElement, embed)
in the class decl of HTMLEmbedElement to make the FromContent thing work.
> What about the call to SetIsNetworkCreated in the constructor?
Ah, I missed that line. Then you do need aFromParser, so just keep things as they are.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 59•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 62•7 years ago
|
||
mozreview-review |
Comment on attachment 8891478 [details]
Bug 1279218 - Rename HTMLSharedObjectElement to HTMLEmbedElement;
https://reviewboard.mozilla.org/r/162616/#review168146
Assignee | ||
Comment 63•7 years ago
|
||
mozreview-review |
Comment on attachment 8886391 [details]
Bug 1279218 - Remove Applet tag;
https://reviewboard.mozilla.org/r/155860/#review168148
Comment 64•7 years ago
|
||
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6f1f895449d
Remove Applet tag; r=bz
https://hg.mozilla.org/integration/autoland/rev/1d9ee33174d5
Remove Applet/Java support from nsObjectLoadingContent; r=bsmedberg,bz
https://hg.mozilla.org/integration/autoland/rev/e825f2ea34c1
Remove Applet element support from WebBrowserPersist; r=jld
https://hg.mozilla.org/integration/autoland/rev/91274528b1f8
Remove tests related to the applet tag; r=bz
https://hg.mozilla.org/integration/autoland/rev/893ac8dfa0a1
Remove Java Test Plugin and rest of Java references; r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/1c702ea52e96
Additional applet tag logic removal; r=bz
https://hg.mozilla.org/integration/autoland/rev/2ee0b5a0560a
Rename HTMLSharedObjectElement to HTMLEmbedElement; r=bz
Updated•7 years ago
|
Target Milestone: mozilla53 → ---
Comment 65•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e6f1f895449d
https://hg.mozilla.org/mozilla-central/rev/1d9ee33174d5
https://hg.mozilla.org/mozilla-central/rev/e825f2ea34c1
https://hg.mozilla.org/mozilla-central/rev/91274528b1f8
https://hg.mozilla.org/mozilla-central/rev/893ac8dfa0a1
https://hg.mozilla.org/mozilla-central/rev/1c702ea52e96
https://hg.mozilla.org/mozilla-central/rev/2ee0b5a0560a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 66•7 years ago
|
||
I've documented this; I have marked the element page as obsolete, and included details of when it was dropped/removal progress in other browsers:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/applet
I also added a note here to mention how the document.applets property now returns an empty HTMLCollection:
https://developer.mozilla.org/en-US/docs/Web/API/Document/applets
Last, I added a note to
https://developer.mozilla.org/en-US/Firefox/Releases/56#Removals_from_the_web_platform
Let me know if that look OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Comment 67•7 years ago
|
||
when this landed the total number of compiler warnings were reduced:
== Change summary for alert #8435 (as of July 30 2017 18:36 UTC) ==
Improvements:
2% compiler warnings summary linux64 pgo 1,550.08 -> 1,516.92
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8435
Comment 68•7 years ago
|
||
https://github.com/webcompat/web-bugs/issues/10722
It seems to cause some of sites flash video can't play based on this bugfix?
Reporter | ||
Comment 69•7 years ago
|
||
I don't think so. Chrome doesn't support <applet> either per comment 0 and the issue you point to suggests the site works in Chrome.
Comment 70•7 years ago
|
||
Sites could be sniffing and depending on <applet> in Firefox somehow but not in Chrome.
Or it's possible we removed more, or less, stuff than Chrome did...
It would be good to have some sort of minimized testcase for the problem from that webcompat issue.
Updated•7 years ago
|
Comment 71•7 years ago
|
||
Hey!
Looks like something went wrong when implementing this:
https://github.com/webcompat/web-bugs/issues/17004#issuecomment-391903536
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=18718bb58de42a44f461e41e1d8fe7922d83f68d&tochange=5bf13799a1fcb612701222f8f877afc45eb7254f
Comment 72•7 years ago
|
||
The regression range contains bug 1433221, not this bug.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•