Closed Bug 1204581 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files, 1 obsolete file)

Attached image Screenshot
It's time to start warn people to stop using AppCache.
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.
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
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+
https://hg.mozilla.org/mozilla-central/rev/19400ffcf469
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(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)
Blocks: 1237782
Depends on: 1244076
(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)
Blocks: 1619673
You need to log in before you can comment on or make changes to this bug.