Add a deprecation notice for AppCache if service worker fetch interception is enabled

RESOLVED FIXED in Firefox 44

Status

()

Core
Networking: HTTP
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

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

unspecified
mozilla44
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 8660828 [details]
Screenshot

It's time to start warn people to stop using AppCache.
Created attachment 8660829 [details] [diff] [review]
Add a deprecation warning for the usage of AppCache when service worker fetch interception is enabled

Requesting review from Jason on the Necko bits and baku on the rest.
Attachment #8660829 - Flags: review?(jduell.mcbugs)
Attachment #8660829 - Flags: review?(amarchesini)
Comment on attachment 8660829 [details] [diff] [review]
Add a deprecation warning for the usage of AppCache when service worker fetch interception is enabled

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

lgtm!

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7032,5 @@
> +                          true);
> +
> +    // Then, issue a deprecation warning if service worker interception is
> +    // enabled.
> +    if (Preferences::GetBool("dom.serviceWorkers.interception.enabled", false)) {

I have to remember to remove this when patch for bug 1163545 will land. I added a nice:

nsContentUtils::ServiceWorkerInterceptionEnabled()

::: netwerk/protocol/http/nsHttpChannel.h
@@ +397,5 @@
>                                    bool checkingAppCacheEntry);
>  
>      void SetPushedStream(Http2PushedStream *stream);
>  
> +    void MaybeWarnAboutAppCache();

maybe this can be const?
Attachment #8660829 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini (:baku) from comment #2)
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +7032,5 @@
> > +                          true);
> > +
> > +    // Then, issue a deprecation warning if service worker interception is
> > +    // enabled.
> > +    if (Preferences::GetBool("dom.serviceWorkers.interception.enabled", false)) {
> 
> I have to remember to remove this when patch for bug 1163545 will land. I
> added a nice:
> 
> nsContentUtils::ServiceWorkerInterceptionEnabled()

Nice!

> ::: netwerk/protocol/http/nsHttpChannel.h
> @@ +397,5 @@
> >                                    bool checkingAppCacheEntry);
> >  
> >      void SetPushedStream(Http2PushedStream *stream);
> >  
> > +    void MaybeWarnAboutAppCache();
> 
> maybe this can be const?

It can't, because NS_QueryNotificationCallbacks() requires non-const loadgroup and callbacks arguments.
Created attachment 8660854 [details] [diff] [review]
Add a deprecation warning for the usage of AppCache when service worker fetch interception is enabled

The previous patch mistakenly called MaybeWarnAboutAppCache() from OnNormalCacheEntryAvailable() too.
Attachment #8660829 - Attachment is obsolete: true
Attachment #8660829 - Flags: review?(jduell.mcbugs)
Attachment #8660854 - Flags: review?(jduell.mcbugs)
Status: NEW → ASSIGNED
Blocks: 1048291
No longer blocks: 1048291
Comment on attachment 8660854 [details] [diff] [review]
Add a deprecation warning for the usage of AppCache when service worker fetch interception is enabled

Ping?  I was hoping to land this before the uplift and I lost track of it, and now we cannot uplift because of the string changes.  :(
Attachment #8660854 - Flags: review?(mcmanus)
Comment on attachment 8660854 [details] [diff] [review]
Add a deprecation warning for the usage of AppCache when service worker fetch interception is enabled

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

r+ on netwerk
Attachment #8660854 - Flags: review?(mcmanus)
Attachment #8660854 - Flags: review?(jduell.mcbugs)
Attachment #8660854 - Flags: review+

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/19400ffcf469
https://hg.mozilla.org/mozilla-central/rev/19400ffcf469
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/application-cache-api-has-been-deprecated/
Keywords: dev-doc-needed, site-compat
(In reply to Kohei Yoshino [:kohei] from comment #9)
> Added the site compatibility doc:
> https://www.fxsitecompat.com/en-US/docs/2015/application-cache-api-has-been-
> deprecated/

Do you mind fixing the page please?  We have not shipped service workers yet, and our implementation of Cache API without service workers is not quite useful for replacing appcache yet.
Flags: needinfo?(kohei.yoshino)
Gah, I thought at least the Cache API had already been shipped. Fixed the copy accordingly. Thanks for flagging!

https://github.com/fxsitecompat/www.fxsitecompat.com/commit/83e9986
Flags: needinfo?(kohei.yoshino)
I have added a note covering this in appropriate places on 

https://developer.mozilla.org/en-US/Firefox/Releases/44#Service_Workers

https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API/Using_Service_Workers

https://developer.mozilla.org/en-US/docs/Web/HTML/Using_the_application_cache

Let me know if that's ok, or if anything more is needed.
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1237782
Depends on: 1244076

Comment 13

2 years ago
(In reply to :Ehsan Akhgari from comment #10)
> (In reply to Kohei Yoshino [:kohei] from comment #9)
> > Added the site compatibility doc:
> > https://www.fxsitecompat.com/en-US/docs/2015/application-cache-api-has-been-
> > deprecated/
> 
> Do you mind fixing the page please?  We have not shipped service workers
> yet, and our implementation of Cache API without service workers is not
> quite useful for replacing appcache yet.

Should <https://www.fxsitecompat.com/en-CA/docs/2015/application-cache-api-has-been-deprecated/> be updated now to say service workers shipped in Firefox 44, rather than "available on Firefox Developer Edition for testing"?
(In reply to Matt Falkenhagen from comment #13)
> (In reply to :Ehsan Akhgari from comment #10)
> > (In reply to Kohei Yoshino [:kohei] from comment #9)
> > > Added the site compatibility doc:
> > > https://www.fxsitecompat.com/en-US/docs/2015/application-cache-api-has-been-
> > > deprecated/
> > 
> > Do you mind fixing the page please?  We have not shipped service workers
> > yet, and our implementation of Cache API without service workers is not
> > quite useful for replacing appcache yet.
> 
> Should
> <https://www.fxsitecompat.com/en-CA/docs/2015/application-cache-api-has-been-
> deprecated/> be updated now to say service workers shipped in Firefox 44,
> rather than "available on Firefox Developer Edition for testing"?

Yes, good catch!  Kohei, do you mind updating that site, please?  Thanks!
Flags: needinfo?(kohei.yoshino)
Thanks, updated the compat doc.
Flags: needinfo?(kohei.yoshino)
You need to log in before you can comment on or make changes to this bug.