Closed Bug 547707 Opened 14 years ago Closed 6 years ago

Use definitions in nsMimeTypes.h rather than hardcoded mime type strings

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: fredw, Assigned: videetssinghai)

Details

(Whiteboard: [good first bug])

Attachments

(4 files)

Bash script to find the strings
3.89 KB, text/plain
Details
Result of the script
59.63 KB, text/plain
Details
patch - v1
172.51 KB, patch
fredw
: feedback+
Details | Diff | Splinter Review
Bug 547707: replaced hardcoded strings by definitions in nsMimeTypes.h
59 bytes, text/x-review-board-request
mozbugz
: review+
Details 
Mime type definitions are stored in netwerk/mime/public/nsMimeTypes.h. There are various place in the source code where hardcoded mime type strings should be replaced by these definitions. Definitions in widget/public/nsITransferable.idl should as well be moved in nsMimeTypes.h.

I'm going to attach a bash script that looks for these hardcoded string as well as definitions from widget/public/nsITransferable.idl. The script contains the concatenation of definitions from nsMimeTypes.h and nsITransferable.idl (mime type strings may be repeated twice but it doesn't matter).
Attached file Result of the script — 

    
  
Whiteboard: [good first bug]

    
    

    
  
I can take this as my first bug fix in the Mozilla project.

But how should i handle ex.: "application/xhtml+xml; x-view-type=view-source"
Should I strcat with definition, or leave it as a text string?

    
    

    
  
(In reply to comment #2)
> I can take this as my first bug fix in the Mozilla project.
> 
> But how should i handle ex.: "application/xhtml+xml; x-view-type=view-source"
> Should I strcat with definition, or leave it as a text string?

When I reported the bug, I was only thinking about simple strings such that those found in attachment 428185 [details]. I would say that you can leave concatenated strings alone, at least in a first time.

---> cc'ing Boris Zbarsky who may know better what to do.

    
    

    
  
I wouldn't touch the x-view-type stuff.  Or if you really want you can do things like:

  APPLICATION_XHTML_XML "; x-view-type=view-source"

as needed.

    
    

    
  
Should the "nsMimeTypes.h" be included in each file that undergoes this change?

    
    

    
  
(In reply to Koosha Khajeh Moogahi [:koosha] from comment #5)
> Should the "nsMimeTypes.h" be included in each file that undergoes this
> change?

Indeed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch - v1
        — 
          — Splinter Review
      

    


  
Mammoth patch!

Not sure if targeted the review request to the right person.

        Attachment #663008 -
        Flags: review?(fred.wang)

    
  
Assignee: nobody → koosha.khajeh
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 663008 [details] [diff] [review]
patch - v1

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

Thanks for working on this! Indeed, I am not sure I am the most appropriate person to review this patch. Perhaps you should ask to Boris instead. However, below are some feedback.

::: browser/components/shell/src/nsGNOMEShellService.cpp
@@ +66,2 @@
>    { "application/xhtml+xml", "xhtml xht"      }
>  };

Try to preserve alignment. Also, I guess you can use APPLICATION_XHTML_XML here.

::: content/base/public/nsContentUtils.h
@@ +2021,1 @@
>        "application/x-javascript",

You can use APPLICATION_XJAVASCRIPT.

::: content/base/src/nsDOMParser.cpp
@@ +146,3 @@
>        (nsCRT::strcmp(contentType, "application/xml") != 0) &&
>        (nsCRT::strcmp(contentType, "application/xhtml+xml") != 0) &&
>        !svg)

APPLICATION_XML, APPLICATION_XHTML_XML can be used here.
Here and elsewhere, I think you can keep the hardcoded value in the comment.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2014,1 @@
>    "application/ogg"

APPLICATION_OGG

::: editor/composer/src/nsEditingSession.cpp
@@ +268,1 @@
>    "application/x-javascript",  // obsolete type

APPLICATION_XJAVASCRIPT

@@ +268,3 @@
>    "application/x-javascript",  // obsolete type
>    "text/xul",                  // obsolete type
>    "application/vnd.mozilla.xul+xml",

You can replace "application/vnd.mozilla.xul+xml" by TEXT_XUL

::: image/build/nsImageModule.cpp
@@ +87,5 @@
>    { "Gecko-Content-Viewers", "image/bmp", "@mozilla.org/content/document-loader-factory;1" },
>    { "Gecko-Content-Viewers", "image/x-ms-bmp", "@mozilla.org/content/document-loader-factory;1" },
>    { "Gecko-Content-Viewers", "image/icon", "@mozilla.org/content/document-loader-factory;1" },
>    { "Gecko-Content-Viewers", "image/png", "@mozilla.org/content/document-loader-factory;1" },
>    { "Gecko-Content-Viewers", "image/x-png", "@mozilla.org/content/document-loader-factory;1" },

Verify if you can replace more here (bmp, png and x-icon for example)

::: layout/build/nsLayoutModule.cpp
@@ +1137,5 @@
>    { NS_CONTENTSERIALIZER_CONTRACTID_PREFIX "text/plain", &kNS_PLAINTEXTSERIALIZER_CID },
>    { NS_PARSERUTILS_CONTRACTID, &kNS_PARSERUTILS_CID },
>    { NS_SCRIPTABLEUNESCAPEHTML_CONTRACTID, &kNS_SCRIPTABLEUNESCAPEHTML_CID },
>    { NS_CONTENTPOLICY_CONTRACTID, &kNS_CONTENTPOLICY_CID },
>    { NS_DATADOCUMENTCONTENTPOLICY_CONTRACTID, &kNS_DATADOCUMENTCONTENTPOLICY_CID },

Verify here too (application/xml, application/xhtml+xml for example)

::: modules/libjar/zipwriter/src/nsDeflateConverter.cpp
@@ +13,5 @@
>  #include "nsAutoPtr.h"
>  #include "plstr.h"
>  
> +#define ZLIB_TYPE ENCODING_DEFLATE2
> +#define GZIP_TYPE ENCODING_GZIP2

I guess you can directly replace the uses of ZLIB_TYPE and GZIP_TYPE in this file by ENCODING_DEFLATE2 and ENCODING_GZIP2. Perhaps mentioning that they are defined in nsMimeTypes.

::: parser/html/nsHtml5AtomList.h
@@ +191,5 @@
>  HTML5_ATOM(border, "border")
>  HTML5_ATOM(cursor, "cursor")
>  HTML5_ATOM(coords, "coords")
>  HTML5_ATOM(filter, "filter")
> +HTML5_ATOM(format, PARAM_FORMAT)

Not sure this change is relevant. PARAM_FORMAT is supposed to be used in MIME header. Here I think it is a name of an attribute or element in HTML5.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2743,5 @@
>      return Init(nsICryptoHash::SHA1);
>  
>    if (aAlgorithm.LowerCaseEqualsLiteral("sha256"))
>      return Init(nsICryptoHash::SHA256);
>  

Verify other changes here. (sha1 or sha256 for example)

::: testing/tools/screenshot/win32-screenshot.cpp
@@ +95,5 @@
>    const wchar_t* filename = (argc > 1) ? argv[1] : L"screenshot.png";
>    Bitmap* b = Bitmap::FromHBITMAP(mybmp, NULL);
>    CLSID  encoderClsid;
>    Status stat = GenericError;
> +  if (b && GetEncoderClsid(LIMAGE_PNG, &encoderClsid) != -1) {

Not really sure this change will work.

::: widget/android/AndroidJNI.cpp
@@ +959,5 @@
>          jSurfaceBitsCtor = jenv->GetMethodID(jSurfaceBitsClass, "<init>", "()V");
>  
>          jSurfaceBitsWidth = jenv->GetFieldID(jSurfaceBitsClass, "width", "I");
>          jSurfaceBitsHeight = jenv->GetFieldID(jSurfaceBitsClass, "height", "I");
> +        jSurfaceBitsFormat = jenv->GetFieldID(jSurfaceBitsClass, PARAM_FORMAT, "I");

Again, PARAM_FORMAT is probably not relevant here.

::: widget/nsITransferable.idl
@@ +26,1 @@
>  #define kFileMime                   "application/x-moz-file"

I suspect it could be possible to get rid of some duplicate macro definitions here too.

        Attachment #663008 -
        Flags: review?(fred.wang) → feedback+

    
  
Assignee: koosha.khajeh → nobody
Status: ASSIGNED → NEW

    
    

    
  
I want to work on this bug as my first contribution.

A little help on how to proceed is appreciated. I have setup local environment today itself.

    
    

    
  
You would probably want to take the existing attachment on this bug, address the review comments from comment 8, and then ask for review or just let me know via a needinfo request (you can't ask me for review right now; long story).

    
    

    
  
Newbie here, I think the shell script for finding the hardcoded strings is out of date.  The paths for the "excludes" don't seem to have changed, and there are more types now in nsITransferable.idl.  

There are around 600 or 700 places where the replacement needs to be done, so it would be a big patch I think. There might also be some other challenges like the previous patch ran into; can't just replace them all without discretion.

I am interested in taking a crack at it if it's still considered a good-first-bug.

    
    

    
  
Hello, I am working on this bug. Please update me.

There are some strings whose definitions are not included in the nsMimeTypes.h file. Should I include them?
Flags: needinfo?(fred.wang)

    
    

    
  
(In reply to Videet Singhai from comment #12)
> Hello, I am working on this bug. Please update me.
> 
> There are some strings whose definitions are not included in the
> nsMimeTypes.h file. Should I include them?

Thanks! I would recommend trying a first patch without adding new strings into nsMimeTypes.h. However, I'm not really up-to-date on Gecko/Servo's source code so probably you want to ask someone else to mentor you on this bug.
Flags: needinfo?(fred.wang)

    
    

    
  
Yeah, that should be ok, we don't use that code in Servo so just a plain patch to the Gecko's source should be enough. I agree a patch without adding new entries to nsMimeTypes should be better, then adding extra ones should be nice.

    
    

    
  
Whom should I add in review?

    
    

    
  
You can flag me for review if you want :)

    
    

    
  
I had already submitted a patch before, hence I rollbacked the commit and did hg pull. I made some changes and commited. Now I am pushing for review it says- 
submitting 2 changesets for review
abort: Review request is submitted or discarded.
You must reopen the review request before it can be updated.
Review requests should only be reopened if your changes have not landed or have
been backed out - file new bugs for follow-up work.

Please help!

    
    

    
  
(In reply to Videet Singhai from comment #17)
> I had already submitted a patch before, hence I rollbacked the commit and
> did hg pull. I made some changes and commited. Now I am pushing for review
> it says- 
> submitting 2 changesets for review
> abort: Review request is submitted or discarded.
> You must reopen the review request before it can be updated.
> Review requests should only be reopened if your changes have not landed or
> have
> been backed out - file new bugs for follow-up work.
> 
> Please help!

Probably you just need to rebase. I don't use mercurial myself, but 'hg push review -r .' may work as well. Please log into IRC and ask in #introduction if it doesn't and I'm sure someone will know :)

    
    

    
  
I am also familiar with git. We can just push directly to https://github.com/mozilla/gecko-dev ?

    
    

    
  
It's a bit more complicated than that, the canonical repo is mercurial. See: https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8956537 [details]
Bug 547707: replaced hardcoded strings by definitions in nsMimeTypes.h

https://reviewboard.mozilla.org/r/225444/#review231378

There's probably a lot more uses of mimes around the codebase, but this is a good start, let's get the nit fixed and try to get it reviewed again. Given you're mostly touching media stuff, I'd send the review to someone that works on that code, like `gerald`, when the nits are fixed.

Thanks for working on this!

::: dom/media/gtest/TestMediaDataDecoder.cpp:16
(Diff revision 1)
>  #include "MP4Demuxer.h"
>  #include "WebMDecoder.h"
>  #include "WebMDemuxer.h"
>  #include "mozilla/AbstractThread.h"
> -
> +#include "nsMimeTypes.h"
> + 

Ditto re. trailing whitespace.

::: dom/media/gtest/TestVideoUtils.cpp:9
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "gtest/gtest.h"
>  #include "nsString.h"
>  #include "VideoUtils.h"
> -
> +#include "nsMimeTypes.h"

nit: There's trailing whitespace, and there's the convention of sorting includes alphabetically, so this should probably go before `nsString`.

        Attachment #8956537 -
        Flags: review?(emilio)

    
    

    
  
so right now I just need to fix the issues, if possible i will make more replacements. You will tell me the reviewer's email or I just need to push normally and you will assign?

    
    

    
  
Yeah, the reviewer's nickname is literally "gerald" (gsquelart@mozilla.com), but feel free to tag me and I'll redirect to him if you want.

    
    

    
      
  

    
    

    
  
Hey Emilio, did u get the review request?

    
    

    
  
Comment on attachment 8956537 [details]
Bug 547707: replaced hardcoded strings by definitions in nsMimeTypes.h

https://reviewboard.mozilla.org/r/225444/#review231694

::: dom/html/HTMLMediaElement.cpp:111
(Diff revision 2)
>  
>  #include "nsIFrame.h"
>  #include "nsDisplayList.h"
>  #include "SVGObserverUtils.h"
> -
> +#include "nsMimeTypes.h"
> + 

nit: This still has trailing whitespace.

        Attachment #8956537 -
        Flags: review?(emilio)

    
    

    
  
Comment on attachment 8956537 [details]
Bug 547707: replaced hardcoded strings by definitions in nsMimeTypes.h

I did! I think Gerald should probably review this.

        Attachment #8956537 -
        Flags: review?(gsquelart)

    
    

    
  
damn is there a tool to fix these white spaces?

    
    

    
      
  

    
    

    
  
Hey I have pushed the fixes and added Gerald in the reviewers

    
  
Flags: needinfo?(gsquelart)
Assignee: nobody → videetssinghai
Flags: needinfo?(gsquelart)

    
    

    
  
Comment on attachment 8956537 [details]
Bug 547707: replaced hardcoded strings by definitions in nsMimeTypes.h

https://reviewboard.mozilla.org/r/225444/#review231792

Good start, thank you for doing this.

I'm pretty sure there are more strings around the code (see https://searchfox.org/mozilla-central/search?q=MEDIAMIMETYPE&case=true&regexp=false&path= ), but I think this patch is already valuable as-is, so after you've landed this one, please open another bug so we don't forget to change more strings into macros.

        Attachment #8956537 -
        Flags: review?(gsquelart) → review+

    
    

    
  
Okay, I will open another bug after this one is approved.

    
    

    
  
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83875d9859a4
replaced hardcoded strings by definitions in nsMimeTypes.h r=gerald

    
    

    
  
Oh, it's already been approved (by my "r+"), so it just needs to land; I've just done that for you -- it's now in the "autoland" repository, and should soon move to "central" where it will be used in Nightly builds, and this bug will be automatically marked as resolved, so our job should be done now. :-)
Thank you again for your contribution.

For next time, you may want to read https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction for more information about the whole process.

    
    

    
  
Thanks Videet Singhai!

    
    

    
  
Thank you so much ppl. Sorry for the inconvenience.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/83875d9859a4
Status: NEW → RESOLVED
Closed: 6 years ago

          status-firefox60:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

    
    

    
  
hey Gerald can I add u as the reviewer in the bug that I am opening for this issue?

    
    

    
  
(In reply to Videet Singhai from comment #39)
> hey Gerald can I add u as the reviewer in the bug that I am opening for this
> issue?

Sure, go for it.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: