Remove our HTML import implementation

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: Ehsan, Assigned: bkelly)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

AFAIK we have decided that we're not going to ship HTML imports, but we still have the code in the tree that people need to keep working.  If that decision is final, it would be nice to delete the code so that we don't have to keep working on it...
Considering it can be resurrected from hg if necessary, this would be nice.

I'm running into issues with import over in bug 1184607.  It seems html imports has the wrong nsContentPolicyType which will likely confuse our service worker code if it the two interact.
That decision is final. If we do HTML imports again it will likely have a different shape from what it has today.
Of course, gaia uses imports.  :(
(In reply to Ehsan Akhgari (don't ask for review please) from comment #3)
> Of course, gaia uses imports.  :(

Are you sure? Last time I checked (it was a while ago) they used a JS implemented custom implementation instead the native one this bug is about.

Plus imports are hidden behind a flag that is off by default (tests relying on it turns that flag on), so we could just turn off the tests as a start, and stop worrying about breaking them if this blocking people.
Flags: needinfo?(ehsan)
(In reply to Ehsan Akhgari (don't ask for review please) from comment #5)
> I'm relatively sure.  See
> <http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#5879>
> and then
> <https://mxr.mozilla.org/gaia/search?string=moz-extremely-unstable-and-will-
> change-webcomponents>.

I'm not sure what that flag does, but imports are relying on this one: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsStyleLinkElement.cpp#139 and I'm pretty sure it is only turned on for tests.
You're right, good!
I'm going to take this.  With html imports enabled in tests we're actually hitting assertions in a service worker WPT.  We also have other intermittent test failures in the imports tests.  Lets finally de-orbit this to reduce our maintenance load.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Duplicate of this bug: 1369893
Blocks: nukeb2g
Setting straight to dev-doc-complete; I went to the HTML Imports page, and simple removed the mention of being able to turn HMTL Imports on in Fx using the pref.

https://developer.mozilla.org/en-US/docs/Web/Web_Components/HTML_Imports

Since we never shipped this, I'm not going to mention it in the Fx rel notes.
William, this patch attempts to remove our pref'ed off HTML imports implementation.

I basically did this by removing import specific stuff in HTMLLinkElement/nsDocument/ImportManager and then fixing build errors.  I also removed the pref in the test profile and the mochitests themselves.  I updated test expectations for WPT.
Attachment #8874991 - Attachment is obsolete: true
Attachment #8875328 - Flags: review?(wchen)
Comment on attachment 8875328 [details] [diff] [review]
Remove the disabled HTML imports implementation. r=wchen

Review of attachment 8875328 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsIDocument.h
@@ -2854,5 @@
> -  virtual bool HasSubImportLink(nsINode* aLink) = 0;
> -  virtual uint32_t IndexOfSubImportLink(nsINode* aLink) = 0;
> -  virtual void AddSubImportLink(nsINode* aLink) = 0;
> -  virtual nsINode* GetSubImportLink(uint32_t aIdx) = 0;
> -

We should also get rid of the forward declaration. https://dxr.mozilla.org/mozilla-central/rev/e61060be36424240058f8bef4c5597f401bc8b7e/dom/base/nsIDocument.h#144

::: dom/html/nsHTMLDocument.cpp
@@ -1893,5 @@
>    mTooDeepWriteRecursion =
>      (mWriteLevel > NS_MAX_DOCUMENT_WRITE_DEPTH || mTooDeepWriteRecursion);
>    NS_ENSURE_STATE(!mTooDeepWriteRecursion);
>  
> -  if (!IsHTMLDocument() || mDisableDocWrite || !IsMasterDocument()) {

We also need to get rid of a check for import here: https://dxr.mozilla.org/mozilla-central/rev/e61060be36424240058f8bef4c5597f401bc8b7e/dom/html/nsHTMLDocument.cpp#534
Attachment #8875328 - Flags: review?(wchen) → review+
See Also: → 1372049
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c41595a24fbd
Remove the disabled HTML imports implementation. r=wchen
https://hg.mozilla.org/mozilla-central/rev/c41595a24fbd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.