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

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
4 years ago
a year 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)

(Assignee)

Description

4 years ago
Posted image Screenshot
It's time to start warn people to stop using AppCache.
(Assignee)

Comment 1

4 years ago
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+
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
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
(Assignee)

Updated

4 years ago
No longer blocks: 1048291
(Assignee)

Comment 5

4 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/19400ffcf469
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Comment 10

4 years ago
(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)
(Assignee)

Updated

3 years ago
Blocks: 1237782

Updated

3 years ago
Depends on: 1244076

Comment 13

3 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"?
(Assignee)

Comment 14

3 years ago
(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.