Closed Bug 1417463 Opened 7 years ago Closed 5 years ago

consider changing default accept header to */*

Categories

(Core :: Networking: HTTP, defect, P3)

Product:
Core
Component:
Networking: HTTP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox65 - wontfix
firefox66 --- fixed

People

(Reporter: bkelly, Assigned: baku)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [necko-triaged])

Attachments

(3 files, 1 obsolete file)

part 1 - necko
19.06 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
part 2 - default values in the code base
4.11 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
part 3 - tests
5.25 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review 
Currently our default accept header is:

text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#1708

This is an odd default because we should really only be sending something like this for document requests. See step 1.3 here:

https://fetch.spec.whatwg.org/#fetching

Can we change our default accept header to */* and make nsDocShell set the text/html based value as an override?
Hi Patrick, can you handle this?
Flags: needinfo?(mcmanus)
ben - I'm fine with this change. would you make it?
Flags: needinfo?(mcmanus)
Yea, I can make the change now that I have necko team blessing.  I'll try to do it in the FF59 time frame.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [necko-triaged]
Attached patch default_accept.patch (obsolete) — — Splinter Review 
I'm going to write a test in a separate patch.
Assignee: ben → amarchesini
Attachment #9034456 - Flags: review?(honzab.moz)
Attached patch part 1 - necko — — Splinter Review 

        Attachment #9034456 -
        Attachment is obsolete: true

        Attachment #9034456 -
        Flags: review?(honzab.moz)

        Attachment #9034569 -
        Flags: review?(honzab.moz)

    
    

    
  


  
By spec, imglib uses the "wrong" content-type but I want to talk with image peers before changing it. Maybe it would be a follow up.

        Attachment #9034570 -
        Flags: review?(honzab.moz)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 3 - tests
        — 
          — Splinter Review
      

    


  

        Attachment #9034571 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 9034569 [details] [diff] [review]
part 1 - necko

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

thanks for doing this!

        Attachment #9034569 -
        Flags: review?(honzab.moz) → review+

        Attachment #9034570 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Comment on attachment 9034571 [details] [diff] [review]
part 3 - tests

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

::: netwerk/test/mochitests/test_accept_header.html
@@ +5,5 @@
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<body>
> +<script>

please write a comment what this test does (reasonably deep list of steps it takes) - like what the ?get does (how you are getting the accept request header back from the server.)  it took a while to decipher this.

        Attachment #9034571 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e1cead8dcf
Default accept header should follow the fetch spec, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf7342d19f7
Default accept header should follow the fetch spec, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7dce7a42ea
Default accept header should follow the fetch spec - tests, r=mayhemer

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/74e1cead8dcf

https://hg.mozilla.org/mozilla-central/rev/fdf7342d19f7

https://hg.mozilla.org/mozilla-central/rev/2a7dce7a42ea


Status: ASSIGNED → RESOLVED
Closed: 5 years ago

          status-firefox66:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

    
    

    
  
[Tracking Requested - why for this release]:


It would be nice to uplift this; I am seeing comments on Twiter that saving images results in un-openable images (which I verified was fixed by this bug).


https://twitter.com/LifeTimeCooking/status/1094488840067178496


Saving the Indian French Toast with Baked Strawberries image results in a corrupt image.



          status-firefox65:
          --- → affected

          tracking-firefox65:
          --- → ?

    
    

    
  
Oops, missed the URL in the last comment: https://vegeyum.wordpress.com/



    
    

    
  
Sorry, this is an old issue and the patches involved are more risky than we can accept for a dot release. This fix will need to ride with Fx66.



          status-firefox65:
          affected → wontfix

          tracking-firefox65:
          ? → -

    
  
Depends on: 1526731

    
  
Blocks: 1530887
No longer depends on: 1526731
Regressions: 1526731

    
    

    
  
Hello,


Should you have a look here : https://github.com/apache/incubator-pagespeed-mod/issues/1857#issuecomment-480581634 .


Thanks,


Eric



    
    

    
  
Plus :


network.http.accept.default is not handled nor editable anymore


https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation/List_of_default_Accept_values



    
    

    
  
(In reply to webmaster from comment #19)




Hello,


Should you have a look here : https://github.com/apache/incubator-pagespeed-mod/issues/1857#issuecomment-480581634 .


Thanks,


Eric




(In reply to moreeasy from comment #21)




"image/webp" accept header is gone in ff66, despite https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation/List_of_default_Accept_values




Removing image/webp was probably unintentional and should be the subject of a new bug (if it hasn't been filed already).



    
  
Regressions: 1544231

    
    

    
  
:cmills Please see comment 20 and comment 21


(In reply to jscher2000 from comment #22)




Removing image/webp was probably unintentional and should be the subject of a new bug (if it hasn't been filed already).




Bug 1544231


Flags: needinfo?(cmills)
No longer regressions: 1544231

    
    

    
  
(In reply to Gingerbread Man from comment #23)




:cmills Please see comment 20 and comment 21


(In reply to jscher2000 from comment #22)




Removing image/webp was probably unintentional and should be the subject of a new bug (if it hasn't been filed already).




Bug 1544231




Thanks for bringing this to my attention. I just want to verify what I need to change here. Is this just a straight revert of what I did previously, or is there more to it?


Flags: needinfo?(cmills) → needinfo?(gingerbread_man)

    
    

    
  
The needinfo request was for updating the List_of_default_Accept_values article. You never touched that one, so it wasn't a case of reverting your changes. It has since been updated by jscher2000 and appears to be in order now. If you could double-check, I'd appreciate it.


Flags: needinfo?(gingerbread_man)

    
    

    
  
(In reply to Gingerbread Man from comment #25)




The needinfo request was for updating the List_of_default_Accept_values article. You never touched that one, so it wasn't a case of reverting your changes. It has since been updated by jscher2000 and appears to be in order now. If you could double-check, I'd appreciate it.




Ah, ok, thanks! And sorry for the confusion.


I've checked it, and it looks fine.



    
    

    
  
Is it no longer possible to configure the Accept header?


I work a lot with web APIs and being able to prefer JSON over XML was a huge plus for Firefox compared to say Chrome. Especially with Firefoxs built in JSON viewer.



    
    

    
  
Hello,


(In reply to jscher2000 from comment #22)




Removing image/webp was probably unintentional and should be the subject of a new bug (if it hasn't been filed already).




https://bugzilla.mozilla.org/show_bug.cgi?id=1544231


Thanks,


Eric



    
  
Regressions: 1560896

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: