Closed Bug 935857 Opened 6 years ago Closed 6 years ago

Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

Bug 831645 added a new protocol to necko, but instead of using the existing infrastructure (NETWORK_PROTOCOLS), it added its own variable.
Attachment #828528 - Flags: review?(mcmanus)
Attachment #828528 - Flags: review?(gps)
Blocks: 935881
Comment on attachment 828528 [details] [diff] [review]
Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

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

::: b2g/installer/Makefile.in
@@ +49,5 @@
>  ifneq (,$(filter WINNT Darwin Android,$(OS_TARGET)))
>  DEFINES += -DMOZ_SHARED_MOZGLUE=1
>  endif
>  
> +ifneq (,$(filter rtsp,$(NECKO_PROTOCOLS)))

Can you do this in moz.build?

::: netwerk/protocol/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +PARALLEL_DIRS += sorted(CONFIG['NECKO_PROTOCOLS'].split())

Is sorted() exposed?
(In reply to :Ms2ger from comment #2)
> > +PARALLEL_DIRS += sorted(CONFIG['NECKO_PROTOCOLS'].split())
> 
> Is sorted() exposed?

yes.
(In reply to :Ms2ger from comment #2)
> > +ifneq (,$(filter rtsp,$(NECKO_PROTOCOLS)))
> 
> Can you do this in moz.build?

Actually, no, because b2g/installer is not a moz.build directory, although there is a moz.build, but it's not recursed into.
https://mxr.mozilla.org/mozilla-central/source/b2g/moz.build#7
Attachment #828528 - Flags: review?(mcmanus) → review?(vchang)
Comment on attachment 828528 [details] [diff] [review]
Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

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

::: netwerk/base/src/Makefile.in
@@ +10,3 @@
>  LOCAL_INCLUDES  += -I$(topsrcdir)/netwerk/protocol/rtsp/controller
>  LOCAL_INCLUDES  += -I$(topsrcdir)/netwerk/protocol/rtsp/rtsp
>  endif

While you're here, can you move these LOCAL_INCLUDES to moz.build?

::: netwerk/streamconv/converters/moz.build
@@ +16,5 @@
>      'nsTXTToHTMLConv.cpp',
>      'nsUnknownDecoder.cpp',
>  ]
>  
> +if 'ftp' in CONFIG['NECKO_PROTOCOLS'].split():

I wonder how much value exposing certain config variables as sets to Python would be. AC_SUBST_SET - Value would be string to make like AC_SUBST, but set to Python.
Attachment #828528 - Flags: review?(gps) → review+
I think this is a good idea and has value. Let's try that.
Attachment #829009 - Flags: review?(gps)
Updated with gps' comments and rebased against the other patch ; only interested in review of the .h/.cpp changes.
Attachment #829011 - Flags: review?(vchang)
Attachment #828528 - Attachment is obsolete: true
Attachment #828528 - Flags: review?(vchang)
Comment on attachment 829009 [details] [diff] [review]
Add AC_SUBST_SET to create a set() of strings in config.status and use it for NECKO_PROTOCOLS

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

::: build/autoconf/config.status.m4
@@ +116,5 @@
>  
>  cat >> $CONFIG_STATUS <<\EOF
>  ] ]
>  
> +substs = [(name[1:-1], value[1:-1] if isinstance(value, types.StringTypes) else value) for name, value in [

All the literals should be str. Why bring in types.StringTypes?

::: python/mozbuild/mozbuild/backend/configenvironment.py
@@ +156,5 @@
> +            if not isinstance(v, StringTypes):
> +                if isinstance(v, Iterable):
> +                    type(v)(decode(i) for i in v)
> +            elif not isinstance(v, text_type):
> +                v = decode(v)

This feels like we're missing some asserts to ensure unexpected conditions are never hit.
Attachment #829009 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #8)
> All the literals should be str. Why bring in types.StringTypes?

Future-proofing, since we want to use unicode literals, in the end.
Comment on attachment 829011 [details] [diff] [review]
Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

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

Thanks for you help to clean this up. It's really cool.
Attachment #829011 - Flags: review?(vchang) → review+
https://hg.mozilla.org/mozilla-central/rev/30a667407079
https://hg.mozilla.org/mozilla-central/rev/260fbed9b3ef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.